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 HPKP/expect-CT for brave domains #767

Closed
jumde opened this issue Aug 17, 2018 · 12 comments · Fixed by brave/brave-core#352
Closed

Enable HPKP/expect-CT for brave domains #767

jumde opened this issue Aug 17, 2018 · 12 comments · Fixed by brave/brave-core#352

Comments

@jumde
Copy link
Contributor

jumde commented Aug 17, 2018

From: #13 (comment)

Check: enable_static_pins_ and enable_static_expect_ct_ in net/http/transport_security_state.cc

@bbondy
Copy link
Member

bbondy commented Aug 18, 2018

Can you describe a test case to verify this is working correctly once it is done?

@diracdeltas
Copy link
Member

https://pinning-test.badssl.com/ should show cert error

there are 2 subissues here:

  1. pin sites that chrome pins by default
  2. pin brave domains like the updater, ledger services, etc.

(2) is arguably more important, so we do it in browser-laptop. we don't do (1) because Chrome expressed that embedders should be careful with it.

i would prefer we do both in brave-core, or at least (2)

@diracdeltas
Copy link
Member

for beta, we can just make sure the machinery is in place to enable pinning/expect-CT (which might involve just doing #1) and then actually do the pinning in the release milestone when we know the final hostnames of brave-important services.

@jumde
Copy link
Contributor Author

jumde commented Aug 20, 2018

I am actually a bit concerned about pinning sites that chrome pins by default. If the certs or pins change for any chrome pinned domains we will have to release a hot-fix for the domains to work.

@jumde jumde changed the title Enable HPKP/expect-CT Enable HPKP/expect-CT for brave domains Aug 20, 2018
@bridiver bridiver self-assigned this Aug 21, 2018
bridiver added a commit to brave/brave-core that referenced this issue Aug 22, 2018
@bbondy bbondy added the QA/Yes label Aug 25, 2018
@btlechowski
Copy link

https://pinning-test.badssl.com/ test fails. Per above comment this is not expected. I have reported a new issue #1214.

@jumde
Copy link
Contributor Author

jumde commented Sep 20, 2018

@btlechowski pinning is only enabled for brave domains. See here: https://github.com/brave/brave-core/blob/74fb2a164cdd780a8f074597aabae3eecaf45a24/chromium_src/net/tools/transport_security_state_generator/input_file_parsers.cc#L311

We decided against enabling pinning for all domains due to web-compat reasons: #767 (comment)

@btlechowski
Copy link

@jumde thanks for the response. I was referring to the testcase from #767 (comment). Since https://pinning-test.badssl.com/ is not a valid test case how should we test this feature?

@jumde
Copy link
Contributor Author

jumde commented Sep 21, 2018

Test Plan

  1. All the domains listed here - https://github.com/brave/brave-core/blob/74fb2a164cdd780a8f074597aabae3eecaf45a24/chromium_src/net/tools/transport_security_state_generator/input_file_parsers.cc#L311 should work as expected.

For the negative test case, I think it makes sense to spin up a dummy brave domain with SSL cert not part of the brave pins. Thoughts? cc: @diracdeltas @tomlowenthal @w0ts0n

@srirambv
Copy link
Contributor

srirambv commented Sep 24, 2018

Verification Passed on

Brave 0.55.6 Chromium: 70.0.3538.16 (Official Build) dev (64-bit)
Revision 16ed95b41bb05e565b11fb66ac33c660b721f778-refs/branch-heads/3538@{#306}
OS Linux

Verified passed with

Brave 0.55.14 Chromium: 70.0.3538.54 (Official Build) beta(64-bit)
Revision 4f8e578b6680574714e9ed3bb9f02922b4dde40d-refs/branch-heads/3538@{#937}
OS Mac OS X
Brave 0.55.14 Chromium: 70.0.3538.54 (Official Build) beta(64-bit)
Revision 4f8e578b6680574714e9ed3bb9f02922b4dde40d-refs/branch-heads/3538@{#937}
OS Windows 7

@diracdeltas
Copy link
Member

@jumde good idea, please set that up

@LaurenWags
Copy link
Member

@jumde is there a negative test case for QA? cc @kjozwiak

@jumde
Copy link
Contributor Author

jumde commented Oct 2, 2018

Just created an issue for devops. cc: @w0ts0n @RyanJarv https://github.com/brave/devops/issues/313

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