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

Enable SCT enforcement #17944

Merged
merged 2 commits into from
May 19, 2023
Merged

Enable SCT enforcement #17944

merged 2 commits into from
May 19, 2023

Conversation

fmarier
Copy link
Member

@fmarier fmarier commented Apr 6, 2023

This makes Brave follow the same Certificate Transparency policy as Chrome for TLS certificates.

It also excludes Brave hostnames which are involved with browser updates in order to ensure that updates always work even if the certificate transparency code breaks in the future.

Resolves brave/brave-browser#22482

Security review: https://github.com/brave/security/issues/1245

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Follow these steps on desktop and Android:

  1. Open https://no-sct.badssl.com/ in Brave.
  2. Confirm that there's a TLS error page (NET::ERR_CERTIFICATE_TRANSPARENCY_REQUIRED).
  3. Open https://sct-exempted.bravesoftware.com in Chrome.
  4. Confirm that there's a TLS error page (NET::ERR_CERTIFICATE_TRANSPARENCY_REQUIRED).
  5. Open https://sct-exempted.bravesoftware.com in Brave.
  6. Confirm that the page loads fine.

@fmarier fmarier self-assigned this Apr 6, 2023
@fmarier fmarier force-pushed the enable-sct-enforcement-22482 branch 2 times, most recently from be0d8b1 to 63012dd Compare April 6, 2023 04:02
@fmarier fmarier added the CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) label Apr 6, 2023
@fmarier fmarier force-pushed the enable-sct-enforcement-22482 branch 5 times, most recently from f329e35 to 2754161 Compare April 13, 2023 23:17
@fmarier fmarier force-pushed the enable-sct-enforcement-22482 branch from 2754161 to 4838bb9 Compare April 17, 2023 19:01
@fmarier fmarier removed the CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) label Apr 17, 2023
@fmarier fmarier marked this pull request as ready for review April 18, 2023 05:19
@fmarier fmarier requested review from a team as code owners April 18, 2023 05:19
@fmarier
Copy link
Member Author

fmarier commented Apr 18, 2023

Note: the test page is not yet live (https://github.com/brave/devops/issues/9497), but the approach is ready for review. I will wait for that test page to be live before merging.

Also, CI is not passing yet, but from what I can see it's not caused by changes in this PR. I will investigate more once CI is more stable. It sounds like many PRs are affected at the moment.

@fmarier fmarier force-pushed the enable-sct-enforcement-22482 branch from 4838bb9 to 726310f Compare April 18, 2023 07:08
@fmarier fmarier force-pushed the enable-sct-enforcement-22482 branch 3 times, most recently from 60e94b6 to 4b2f9de Compare May 5, 2023 19:14
@fmarier fmarier requested a review from mkarolin May 5, 2023 19:16
@fmarier fmarier force-pushed the enable-sct-enforcement-22482 branch from 4b2f9de to e92356c Compare May 5, 2023 23:17
@fmarier fmarier requested a review from goodov May 8, 2023 23:51
@fmarier fmarier force-pushed the enable-sct-enforcement-22482 branch from e92356c to 467e07e Compare May 8, 2023 23:51
@fmarier fmarier force-pushed the enable-sct-enforcement-22482 branch 3 times, most recently from 85a3ffa to 2b13f4b Compare May 19, 2023 04:00
This makes Brave follow the same Certificate Transparency policy
as Chrome for TLS certificates.

It also excludes Brave hostnames which are involved with browser
updates in order to ensure that updates always work even if the
certificate transparency code breaks in the future.
@fmarier fmarier force-pushed the enable-sct-enforcement-22482 branch from 2b13f4b to cf63b4a Compare May 19, 2023 18:22
@fmarier fmarier merged commit 5db4320 into master May 19, 2023
@fmarier fmarier deleted the enable-sct-enforcement-22482 branch May 19, 2023 21:06
@github-actions github-actions bot added this to the 1.53.x - Nightly milestone May 19, 2023
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.

[Security] Add support for Certificate Transparency in Brave
3 participants