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

Issue 518: Enabling CRLSets #652

Merged
merged 1 commit into from
Nov 13, 2018
Merged

Issue 518: Enabling CRLSets #652

merged 1 commit into from
Nov 13, 2018

Conversation

jumde
Copy link
Contributor

@jumde jumde commented Oct 16, 2018

fixes brave/brave-browser#518

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

On Windows and Linux:

  1. Open Brave Browser
  2. Verify the crl-sets exist in the data directory - <DATA-DIR>/CertificateRevocation/<ver>/crl-set
  3. Navigate to revoked.badssl.com - Should show a certificate error.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

@bbondy
Copy link
Member

bbondy commented Oct 17, 2018

Forwarding review here to @bridiver

#undef RegisterCRLSetComponent

#include "brave/browser/brave_browser_process_impl.h"
#include "brave/browser/extensions/brave_component_extension.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't seem like it is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed for demand_updater:

#include "../../../../../chrome/browser/component_updater/crl_set_component_installer.cc"
#undef RegisterCRLSetComponent

#include "brave/browser/brave_browser_process_impl.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

#include "chrome/browser/browser_process.h" for g_browser_process
we only need the brave version to access brave-specific methods

@jumde jumde force-pushed the crlsets branch 2 times, most recently from 75d0ee0 to fcadbb7 Compare October 26, 2018 12:52
bridiver
bridiver previously approved these changes Oct 26, 2018
@kjozwiak
Copy link
Member

kjozwiak commented Nov 6, 2018

Denying approval for uplift to 0.56.x after deliberating with @srirambv and @rebron. Reasoning:

We've had several issues in the past of security fixes landing 1-2 days before a release which ended up causing a lot of problems which required prompt hotfixes. If we end up uplifting this, we'll have ~a day or less of bake time which in my opinion, isn't enough time for a change that can have serious implications if something goes wrong. If we did uplift this and required a prompt hotfix, it would also put the muon migration schedule at risk.

@kjozwiak
Copy link
Member

Clearing the uplift-request/0.57.x-Beta flag. As per process, please re-submit for approval once this has landed in master and requires uplift into 0.57.x.

@jumde jumde merged commit 92e9424 into master Nov 13, 2018
@diracdeltas
Copy link
Member

@kjozwiak this has landed in master now so i'm re-adding the uplift request

@kjozwiak
Copy link
Member

Because we're planning on migrating muon users during the 0.57.x release and security issues like these tend to be a bit more riskier as any regression(s) that might spawn from this will most likely require an immediate hotfix. This might complicate things during the muon migration. I proposed skipping 0.57.x and getting this into 0.58.x which will be released on Dec 20th. I double checked and made sure @diracdeltas was okay with this as well.

@jumde can you merge this into 0.58.x before Dec 4th so it gets included when 0.58.x moves into beta? BTW, there's no approval needed to land things into the dev channel for now 👍.

jumde added a commit that referenced this pull request Nov 28, 2018
Issue 518: Enabling CRLSets
@jumde
Copy link
Contributor Author

jumde commented Nov 28, 2018

0.58.x - 4a1a3b6

@bbondy
Copy link
Member

bbondy commented Nov 28, 2018

FYI only smallest version it is merged inside of should be there for labels. Thanks.

@bbondy
Copy link
Member

bbondy commented Nov 28, 2018

master: 92e9424

jumde added a commit that referenced this pull request Nov 29, 2018
This reverts commit 92e9424, reversing
changes made to 1698b31.
@jumde
Copy link
Contributor Author

jumde commented Nov 29, 2018

Reverted: #986 (master and 0.58.x)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enable certificate revocation
5 participants