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

[EG] Update the EG updater to allow sideloading #3938

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

TyHolc
Copy link
Contributor

@TyHolc TyHolc commented Aug 6, 2024

Add h5vcc API options to skip checking if package signature matches our key and to set a custom URL to check for package updates.

Implement the above features into the updater for non-gold builds only.

b/325626249

Change-Id: I594d04207d242dfbf69c7b6f734174ccb03a43cd

@TyHolc
Copy link
Contributor Author

TyHolc commented Aug 6, 2024

Here's a draft EG sideloading change, I'm still waiting on Kabuki to ack my bug for adding a user warning but figured we can start a review now and get any comments out of the way.

I tested this and it works, but requires a server with SSL setup. I'm looking into a simple way to set that up in a Flask server but running into connection issues with self-signed certs. An alternative can be to also disable requiring HTTPS with a flag (or alongside one of the new flags I'm adding).

@niranjanyardi
Copy link
Contributor

I'm assuming the whole PR is blocked until webapp implements a dialog/message telling the user they are in debug mode . We could temporarily add another boolean which turns off this whole functionality get the PR submitted and turn it back on when webapp finishes its implementation.

@TyHolc
Copy link
Contributor Author

TyHolc commented Aug 7, 2024

I'm assuming the whole PR is blocked until webapp implements a dialog/message telling the user they are in debug mode . We could temporarily add another boolean which turns off this whole functionality get the PR submitted and turn it back on when webapp finishes its implementation.

Yeah, it'll be blocked until then. I also need to add unit tests for the new h5vcc apis. To get this in so we don't end up with conflicts that's a good idea, to lock it behind a bool and make a new change once things are completed on the webapp. There may be further changes too depending on the webapp team's requirements.

@hlwarriner
Copy link
Contributor

I should have time tomorrow to take a look at this.

@niranjanyardi
Copy link
Contributor

I'm assuming the whole PR is blocked until webapp implements a dialog/message telling the user they are in debug mode . We could temporarily add another boolean which turns off this whole functionality get the PR submitted and turn it back on when webapp finishes its implementation.

Yeah, it'll be blocked until then. I also need to add unit tests for the new h5vcc apis. To get this in so we don't end up with conflicts that's a good idea, to lock it behind a bool and make a new change once things are completed on the webapp. There may be further changes too depending on the webapp team's requirements.

Unit tests are a good idea: to ensure this works for partners we might want to add YTS tests as well.

@yuying-y
Copy link
Contributor

yuying-y commented Aug 7, 2024

I was pulled away by a few P0 issues this week. I'll review this PR Friday or early next week. @TyHolc Please take a look at the failed Evergreen checks first. Thanks.

Copy link
Contributor

@hlwarriner hlwarriner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice - the scope of the changes is smaller than I was expecting.

I left a couple comments but this looks good overall.

cobalt/h5vcc/h5vcc_updater.idl Show resolved Hide resolved
#if defined(COBALT_BUILD_TYPE_GOLD)
bool skipVerifyPublicKeyHash = false;
#else // defined(COBALT_BUILD_TYPE_GOLD)
bool skipVerifyPublicKeyHash = GetAllowSelfSignedBuilds();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the design I thought we were adding a web API to set a public key to use for verification of the self-signed package. Did we decide instead to skip package verification altogether?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, from the security review I go the impression that they don't see that approach as more secure than skipping the signature check, and it was more difficult for users.

If we agree as a team that we should do that instead I can update this, it would pretty much just be changing this to a string and instead of clearing the key list in the updater_module I'd instead add the given key hash. LMK what you think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good to me, I also don't see a significant difference in security between the two approaches.

It may be good to just capture that feedback from the security review in the design doc somewhere, maybe under Design Review Notes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR lgtm. The current approach is good to me, as long as the security review team and EG team decide that skipping verification is okay. I only have some nit comments. Other than that, please take a look at the failed checks and resolve them. Thanks Tyler!

@TyHolc
Copy link
Contributor Author

TyHolc commented Aug 14, 2024

I'm assuming the whole PR is blocked until webapp implements a dialog/message telling the user they are in debug mode . We could temporarily add another boolean which turns off this whole functionality get the PR submitted and turn it back on when webapp finishes its implementation.

Yeah, it'll be blocked until then. I also need to add unit tests for the new h5vcc apis. To get this in so we don't end up with conflicts that's a good idea, to lock it behind a bool and make a new change once things are completed on the webapp. There may be further changes too depending on the webapp team's requirements.

Currently still discussing with the webapp team but so far their guidance is to have Cobalt render this message rather than have the webapp show it. That will make it so it's impossible to skirt around the message being displayed through redirects. I'm looking into how we can do this from our end.

Add h5vcc API options to skip checking if package signature matches our
key and to set a custom URL to check for package updates.

Implement the above features into the updater for non-gold builds only.

b/325626249

Change-Id: I594d04207d242dfbf69c7b6f734174ccb03a43cd
b/325626249

Change-Id: I30353d0559453a62793e3724c0152d2cadf97e64
Add an option to disable the network encryption check when checking for
and downloading Evergreen updates on non-production builds.

b/325626249

Change-Id: I9b5b402a5852bd183296d4406e88562db1ed662f
Add a minimal python server to act as a stub omaha server. It will serve
serve an appropriate response for the file at `file_location` when
queried for an update, then serve that file on the subsequent request.

b/325626249

Change-Id: I2933e99b485d74581fd332763f05204be21f2c93
@TyHolc
Copy link
Contributor Author

TyHolc commented Aug 15, 2024

Made a couple of significant updates. I uploaded the python server I've been using to test and added a toggle for forced network encryption so we can use http. I looked at instead allowing https and ignoring ssl errors so we can have a self-signed cert for the server as well, but that was more difficult to implement and not really more secure.

I also internally uploaded the scripts I made for creating a signed crx package and for rebuilding EG and the loader app if anyone is interested in re-using them.

Add implementations of new virtual methods to test_configurator

b/325626249

Change-Id: I000d8742475829bd396918f17cedb3450d52773a
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.

4 participants