Enabling CRLSets; Capping the maximum component updates per request to go-updater to 1 #1581
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
48 tasks
jumde
force-pushed
the
crlsets
branch
4 times, most recently
from
23cc4ea
to
773fd94
Compare
NejcZdovc
reviewed
Feb 11, 2019
| URLPattern(URLPattern::SCHEME_HTTPS, "https://crxdownload.brave.com/*"), | ||
|
|
||
| // Will be removed when https://github.com/brave/brave-browser/issues/663 | ||
| // is fixed | ||
| URLPattern(URLPattern::SCHEME_HTTPS, "https://www.gstatic.com/*"), |
There was a problem hiding this comment.
can we remove this line as brave/brave-browser#663 was fixed?
diracdeltas
reviewed
Feb 11, 2019
| /* This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
| * You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
| // Copyright (c) 2019 The Brave Authors. All rights reserved. |
There was a problem hiding this comment.
i think the MPL header is still needed. at least in the reviewer checklist it says New files have MPL-2.0 license header.
There was a problem hiding this comment.
cc: @bridiver -
Should we remove the New files have MPL-2.0 license header. item from the reviewer checklist. Or use this header:
/* Copyright (c) 2019 The Brave Authors. All rights reserved.
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
8e374c2
to
8db7b30
Compare
jumde
force-pushed
the
crlsets
branch
7 times, most recently
from
9ddc7fa
to
72e1cba
Compare
jumde
force-pushed
the
crlsets
branch
6 times, most recently
from
7299ee3
to
e1903f9
Compare
|
PR builder passed: https://staging.ci.brave.com/job/brave-browser-build-pr/job/PR-3452/ |
pilgrim-brave
previously approved these changes
Mar 12, 2019
jumde
force-pushed
the
crlsets
branch
6 times, most recently
from
a46abea
to
f6faf18
Compare
jumde
changed the title
Revoked certificates don't show certificate error on all platforms. This PR enables CRLSets, a component managed by Google to show certificate errors for domains with revoked certificates. Since, CRLSets is maintained by Google we will be proxying requests for CRLSets through crlsets[n].brave.com, crxdownload.brave.com (resources) and componentupdater.brave.com (component updates) This change: 1. Enables CRLSets 2. Proxies requests for CRLSet resources through brave proxies 3. Removes braveRedirect from brave_common_static_redirect_network_delegate 4. Extension endpoint is set to dev server. Will be reverted after QA-signoff to update go-updater auditors: @bbondy, @bsclifton, @diracdeltas
2. Adding Local Data Updater to the Vetted list
bsclifton
approved these changes
Mar 18, 2019
|
PR builder successful run: https://staging.ci.brave.com/view/all/job/brave-browser-build-pr/job/crlsets/3/ |
|
Congratulations on landing this, great to see it closed @jumde ! |
|
🙌 |
|
Thank you! @bbondy @bsclifton |
27 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix brave/brave-browser#518
fix brave/brave-browser#2160
fix brave/brave-browser#3615
Description:
Revoked certificates don't show certificate error on all platforms. This PR enables CRLSets, a component managed by Google to show certificate errors for domains with revoked certificates.
Since, CRLSets is maintained by Google we will be proxying requests for CRLSets through crlsets[n].brave.com, crxdownload.brave.com (resources) and componentupdater.brave.com (component updates)
This PR also caps the maximum component updates per request to
go-updaterto 1, sincego-updatercannot handle an update request with both Google and Brave components.Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests) ongit rebase master(if needed).git rebase -ito squash commits (if needed).Test Plan:
For CRLSets
<Data-Dir>/CertificateRevocation/<CRLSET_ID>For Component Updates
Update ErrorMEI Preload|Brave Local Data Updater|Brave Ad Block Updater|Brave Tor Client Updater (OS)|CRLSet|PDF Viewer (PDF.js)andBrave HTTPS Everywhere Updaterhave non-zero version numbers by defaultFor QA
On
macOSthe system uses the system CRLSets, so to verify this is working verify that theCertificateRevocationdirectory has the correct CRLSet version.Please verify that
revoked.badssl.comdoes not show error on Windows/Linux.For devs:
Please use: brave/brave-browser#3452 to verify the network audit passes. The changes to the go-updater are still in dev. Will be transitioned to prod once QA sign-off is received.
Reviewer Checklist: