Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rebase on top of v1.3.36.111 #36

Closed
wants to merge 164 commits into from
Closed

Rebase on top of v1.3.36.111 #36

wants to merge 164 commits into from

Conversation

mherrmann
Copy link
Collaborator

@mherrmann mherrmann commented Nov 3, 2021

Tested to compile with:

  • Visual Studio 2017
  • Windows SDK 10.0.18362.0
  • WTL 10.0.10320
  • Python 2.7
  • pywin32 224
  • SCons 1.3.1
  • Google Software Construction Toolkit 0.9.1
  • Go 1.14.6 (64 bit)
  • Google Protocol Buffers 3.17.3

The remaining dependencies are now included via a DEPS file.

The following 43 tests fail. These tests already failed before the rebase, with the exception of UnitTestHelpersTest.ClearGroupPolicies, which also fails upstream.

See failing tests
  1. ExtractorTest.EmbedExtract
  2. ExtractorTest.EmbedAppendExtract
  3. ExtractorTest.AlreadyTaggedError
  4. TimeTest.RFC822TimeParsing
  5. UserRightsTest.UserIsLoggedOnInteractively
  6. UtilsTest.ReadEntireFile
  7. UtilsTest.IsUserLoggedOn
  8. UtilsTest.AddAllowedAce
  9. PingTest.BuildOmahaPing
  10. PingTest.SendInProcess
  11. PingTest.PersistAndSendPersistedPings
  12. ScheduledTaskUtilsTest.GoopdateTasks
  13. WebServicesClientTest.Send
  14. WebServicesClientTest.SendForcingHttps
  15. WebServicesClientTest.SendWithCustomHeader
  16. WebServicesClientTest.SendStringWithCustomHeader
  17. DownloadManagerUserTest.DownloadApp_MultiplePackagesInOneApp
  18. InstallManagerInstallAppMachineTest.InstallApp_MsiInstallerSucceeds
  19. InstallManagerInstallAppMachineTest.InstallApp_MsiInstallerWithArgumentSucceeds
  20. InstallerWrapperMachineTest.InstallApp_MsiInstallerSucceeds
  21. InstallerWrapperMachineTest.InstallApp_MsiInstallerWithArgumentSucceeds
  22. WorkerWithTwoAppsTest.CheckForUpdateAsync_Large
  23. WorkerWithTwoAppsTest.DownloadAsyncThenDownloadAndInstallAsync_Large
  24. CupEcdsaRequestTest.PostSimpleRequest
  25. CupEcdsaRequestTest.PostSimpleRequestHttps
  26. NetworkRequestTest.MultipleRequests
  27. GoogleUpdateRecoveryTest.FixGoogleUpdate_FileReturned_Machine
  28. GoogleUpdateRecoveryTest.FixGoogleUpdate_FileReturned_User
  29. GoogleUpdateRecoveryTest.FixGoogleUpdate_FileCollision
  30. GoogleUpdateRecoveryTest.VerifyFileSignature_NotSigned
  31. GoogleUpdateRecoveryTest.VerifyRepairFileMarkup_InvalidMarkups
  32. GoogleUpdateRecoveryTest.ProductionServerResponseTest
  33. SetupUserTest.TerminateCoreProcesses_BothTypesRunningAndSimilarArgsProcess
  34. SetupMachineTest.StopGoogleUpdateAndWait_ProcessesDoNotStop
  35. SetupMachineTest.StopGoogleUpdateAndWait_UserHandoffWorkerRunningAsSystem
  36. SetupMachineTest.StopGoogleUpdateAndWait_UserLegacyInstallGoogleUpdateWorkerRunningAsSystem
  37. SetupMachineTest.TerminateCoreProcesses_BothTypesRunningAndSimilarArgsProcess
  38. UnitTestHelpersTest.ClearGroupPolicies
  39. IsForeground/WebServicesClientTest.SendUsingCup/0
  40. IsForeground/WebServicesClientTest.SendUsingCup/1
  41. IsForeground/WebServicesClientTest.SendString/0
  42. IsForeground/WebServicesClientTest.SendString/1
  43. IsMachine/InstallManagerTest.InstallDir_ReadOnlyFiles/1

jpawlicki and others added 30 commits September 23, 2020 06:05
dir/ matches the directory and the paths underneath it.
The build passes with the latest version of
the compiler (16.7.3), on /std:c++latest (even though the
build file declares c++17 as std), protocol buffers
version 3.13.0, and googletest version 1.10.0.
Version 1.7.3 needs an additional generated file zip_err_str.c. This
file is missing in the distribution itself and has to be generated.
To make it easier for people to build open source without having to
invoke CMake and generate this file, this file is being included
in this CL.
This CL changes from passing in a more permissive security descriptor
to NULL. The ACLs in the default security descriptor for a named pipe
grant full control to the LocalSystem account, administrators, and the
creator owner. This is sufficient for now, since Omaha is the only
client for the corresponding Breakpad server.

The drawback is that we will not be able to capture crashes that
happen in Medium Integrity Machine Omaha processes, which is a corner
case, and hence is ok.
This change includes the following:
* Group Policy and DM.
* XML protocol.
* Policy template.
* COM interface.

Some DM code is commented out until another CL lands.
It is useful to surface the currently running version of the Updater as
well as the last successful check for updates in chrome://policy.
This change allows MDM enrolled machines that are running a non-personal version of Windows to be considered domain-joined machines. Note: MDM is not the same as Azure Active Directory, which will be a separate change.

Also fixed an issue where a couple of policies were not falling back to DM policies.
This is a needed fix for customers with Azure AD joined machines who are not able to enforce Group Policy for Chrome and other Google apps administered by GoogleUpdate.
The idea is to send some extra context even in cases where the install is a success.
Google Update may get its policies from CBCM or from GPO. This isn't reflected in the "Source" column of chrome://policy.

This makes diagnosing policy behavior very difficult. Fixing this would be a great help for admins.
* Allow for PolicyStatus to run at High Integrity under the Medium Service.
* Support for OnDemand refresh of DM Policies, enabled by running at High Integrity.
* Add PolicyStatus for User Omaha. This is needed to allow chrome://policy to show policy status for per-user installations.
If ::NetGetJoinInformation() returns 'NetSetupUnknownStatus' and the Windows version is an Enterprise SKU, we will consider it domain-joined.
There is an upper limit for how large the payload files
can be for hash verification purposes. Currently, the
limit is 512MB. This CL increases the limit to 1GB.

A new error for this case is defined: SIGS_E_FILE_SIZE_TOO_BIG.
This change increases the size of the read buffer from 128KB to
1MB to assess the impact of reducing the number of IO calls
during the verification of payload hashes.
This change contains two work arounds to slightly improve the user experience due to long times taken by hash verification of large files (>512MB).

First, it changes the text displayed when the download is complete from "Download complete." to "Download complete. Please wait while the installer is verified.". The change is made for en-us only. GRIT in Omaha is broken at the moment. i18n must be dealt with in a follow up CL.

Second, checking of hashes occurs at several times during /install execution.
In particular, it happens once right before running the installer. Unfortunately, during the hash check, the model lock in Omaha is taken, so Omaha can't respond to polling for state changes. That means that the UI keeps displaying "Download Complete." instead of displaying "Waiting to install...". There is no easy workaround for this. A delay is introduced in the WaitingToInstall state to allow the UI client to catch up with the state transition, and show the "Waiting to install..." text while the hash is checked.
…backs.

This CL has the following changes:
* Adds a new IPolicyStatus2 interface which exposes policies from multiple sources. For instance, if both Group Policy and DM have a policy set and Group Policy has precedence, then the policy values will supply the conflicting DM value as well. The IPolicyStatus interface is deprecated.
* Changes from using a single policy source for all Omaha policies, to one where all sources are considered. This brings the behavior on par with Chrome, and lessens the intellectual overhead for customers and support teams. This also makes it easier to support a gradual migration from platform policies to CBCM.
The prod is not responding with Etag any longer. We are using
the kHeaderXETag exclusively now.

Handling this header is encapsulated in the CupEcdsaRequest
and not visible at all in the WebServiceClient class.

The test determines that sending with CUP succeeds solely
by instantiating the WebServiceClient with `use_cup=true`.

There is no need to check for the presence of CUP headers,
which were not tested in the past either.
For Google builds, the function UploadMetrics() is going to return early, before the network call is made, and return S_FALSE.

The idea here is that we want to stop sending usage stats, and start
deprecating the feature, without making intrusive changes to the code, or
creating stability issues.
Version 1.3.36.21 signed on 09/21/2020.
This CL has the following changes:
* refactors the proxy detection policies to go through the Config Manager, thereby unifying all policies under the ConfigManager class.
* exposes the proxy detection policies via the IPolicyStatus2 COM interface, thereby allowing chrome://policy/ to display these policies.
Toolchain and libraries update
Limit the path where LoadLibray should be looking for dlls to
%windir%\System32 only.
@mherrmann
Copy link
Collaborator Author

mherrmann commented Nov 3, 2021

(Edit: @mihaiplesa said he's looking into setting up the different build VM so no need for you to do this @linhkikuchi.)

@linhkikuchi compiling this PR requires a slightly different build environment from the current Omaha build. Specifically, instead of C:\protobuf-2020-sep-rebase, we need C:\protobuf-3.17.3 with contents bin/, include/ and src/ as explained under "Google Protocol Buffers" here. Can you help with this?

The repository also now needs to be checked out with the --recursive flag to git clone. Is there something special I need to do to make this happen @linhkikuchi?

@mherrmann
Copy link
Collaborator Author

mherrmann commented Nov 3, 2021

@simonhong thank you again for the explanations on Slack. I have another question please: Why are the UUIDs for IPolicyStatus and PolicyStatusClass in omaha3_idl.idl still at Google's defaults? I generated new values now because CustomizingOmaha says to "generate new GUIDs for every interface and coclass". But maybe there is a reason why they were left unchanged?

@mherrmann mherrmann self-assigned this Nov 3, 2021
@mherrmann
Copy link
Collaborator Author

mherrmann commented Nov 3, 2021

For reviewers: The diff between this PR and main is huge because it contains Brave's customizations, some of my changes and all upstream changes since the last rebase. I created this PR by manually re-applying Brave's changes on top of v1.3.36.111. The following script illustrates this:

git clone [email protected]:brave/omaha
cd omaha
git remote add upstream [email protected]:google/omaha
git fetch upstream
git checkout -b rebase-v1.3.36.111 v1.3.36.111

set .gitattributes to:

    *.reg diff
    *.rc diff
    *.adm diff

git diff f3558c7a..main | git apply --reject

So I computed the diff between f3558c7a, which is the upstream revision on which the last rebase was based, and Brave's current main. Then I applied this diff on top of upstream v1.3.36.111. This left ~10 merge failures, which I applied by hand. I then read through all code changes to make sure they are still sound, making some improvements as necessary. I also read through Google's upstream commits since our last rebase, to see if there is anything that is likely to break for us. The most direct interaction point I could find is that Omaha now includes some changes that are shown in Chrome on chrome://policy.

Instead of "merging" this PR in the classical sense, it might be easiest to simply make it the new main branch. This is what @mihaiplesa did for the last rebase.

@mherrmann
Copy link
Collaborator Author

mherrmann commented Nov 3, 2021

TODO purely from a code perspective:

Once CI can be run for this PR, it should be relatively trivial to remove the files and see whether everything still works. For now, I'm blocked on this CI integration and am waiting for answers to my questions above.

@simonhong
Copy link
Member

simonhong commented Nov 4, 2021

@simonhong thank you again for the explanations on Slack. I have another question please: Why are the UUIDs for IPolicyStatus and PolicyStatusClass in omaha3_idl.idl still at Google's defaults? I generated new values now because CustomizingOmaha says to "generate new GUIDs for every interface and coclass". But maybe there is a reason why they were left unchanged?

I assume above two didn't exist when we forked omaha at first time.
And, I applied previous commits when I did previous rebase w/o checking newly added UUIDs.
Thanks for catching!

and I found that we are using outdated https://github.com/brave/brave-core/blob/master/chromium_src/google_update/google_update_idl.idl.
https://github.com/chromium/chromium/blob/master/google_update/google_update_idl.idl is upstream's latest one and
it includes above new interfaces that our one doesn't have. We should update this idl file also in brave-core by copying generated idl from omaha.

@mherrmann
Copy link
Collaborator Author

We should update this idl file also in brave-core by copying generated idl from omaha.

Good catch. I remember copying the .idl from Omaha into Chromium's source tree gives a warning "idl file has changed. Here's how to re-generate some sources that depend on it: ...". I'll look into this.

@simonhong
Copy link
Member

simonhong commented Nov 4, 2021

We should update this idl file also in brave-core by copying generated idl from omaha.

Good catch. I remember copying the .idl from Omaha into Chromium's source tree gives a warning "idl file has changed. Here's how to re-generate some sources that depend on it: ...". I'll look into this.

I wrote about it when I did porting first time to https://github.com/brave/brave-browser/wiki/Chromium-rebases at MIDL issue on Windows section.
During the build, it'll catch idl file changes by comparing pre-generated files.
Then, we can copy newly generated files to brave/win_build_output/midl/google_update/

@linhkikuchi
Copy link

protobuf

(Edit: @mihaiplesa said he's looking into setting up the different build VM so no need for you to do this @linhkikuchi.)

@linhkikuchi compiling this PR requires a slightly different build environment from the current Omaha build. Specifically, instead of C:\protobuf-2020-sep-rebase, we need C:\protobuf-3.17.3 with contents bin/, include/ and src/ as explained under "Google Protocol Buffers" here. Can you help with this?

The repository also now needs to be checked out with the --recursive flag to git clone. Is there something special I need to do to make this happen @linhkikuchi?

@mherrmann so you need me to update the windows build node to have C:\protobuf-3.17.3, right?

@mherrmann
Copy link
Collaborator Author

@linhkikuchi yes, thank you :-)

@mherrmann
Copy link
Collaborator Author

(PS @linhkikuchi as I mentioned Mihai told me yesterday he's looking into this but either he or you may do it; Also, we probably still need the C:\protobuf-2020-sep-rebase directory until the new Omaha build is on all channels.)

@linhkikuchi
Copy link

linhkikuchi commented Nov 4, 2021

@mherrmann I downloaded protoc-3.17.3-win64.zip from here https://github.com/protocolbuffers/protobuf/releases/tag/v3.17.3, but it only has bin and include folder. Is this the correct one?

PS C:\protobuf-3.17.3> ls                                                                                                             
                                                                                                                                      
                                                                                                                                      
    Directory: C:\protobuf-3.17.3                                                                                                     
                                                                                                                                      
                                                                                                                                      
Mode                LastWriteTime         Length Name                                                                                 
----                -------------         ------ ----                                                                                 
d-----        11/4/2021  11:16 AM                bin                                                                                  
d-----        11/4/2021  11:16 AM                include                                                                              
-a----         6/8/2021   2:11 PM            724 readme.txt
PS C:\protobuf-3.17.3> .\bin\protoc.exe --version                                                                                     
libprotoc 3.17.3

@mherrmann
Copy link
Collaborator Author

@linhkikuchi as I wrote above - we need C:\protobuf-3.17.3 with contents bin/, include/ and src/ as explained under "Google Protocol Buffers" here. Thanks!

@mihaiplesa
Copy link
Collaborator

@linhkikuchi
Copy link

@linhkikuchi as I wrote above - we need C:\protobuf-3.17.3 with contents bin/, include/ and src/ as explained under "Google Protocol Buffers" here. Thanks!

@mherrmann this has been done https://github.com/brave/devops/pull/6210

@mherrmann
Copy link
Collaborator Author

Thank you @linhkikuchi! I kicked off a build in brave/brave-core#10883 now.

gclient does not support submodules. The cleanest way to deal with this
repository's dependencies when included from another repo via a DEPS file is to
add a DEPS file of our own.
It was removed upstream in 2f4139 and was likely still in our tree
by accident.
It seems it is no longer being used.
Apparently, it needs to be odd.
@mihaiplesa
Copy link
Collaborator

Closing as this PR is not needed anymore since it became the default branch.

@mihaiplesa mihaiplesa closed this Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.