Issue 12190 - Enabling certificate pinning for Brave domains #401
Conversation
jumde
changed the title
|
Updated! |
patches/master_patch.patch
Outdated
| { "name": "preloaded-expect-staple-include-subdomains.badssl.com", "expect_staple": true, "expect_staple_report_uri": "https://report.badssl.com/expect-staple", "include_subdomains_for_expect_staple": true }, | ||
|
|
||
| + //Brave | ||
| + { "name": "ledger.mercury.basicattentiontoken.org", "mode": "force-https", "pins": "brave"}, |
There was a problem hiding this comment.
the ledger client also reaches out to balance.mercury.basicattentiontoken.org
There was a problem hiding this comment.
there are also -staging equivalents
There was a problem hiding this comment.
Will update, what about publishers.basicattentiontoken.org?
There was a problem hiding this comment.
i don't believe that domain is used in browser-laptop
There was a problem hiding this comment.
I think we need to find a better way to handle this because this patch is way too big. If we're going to make this many changes we would be better off patching net/http/transport_security_state.h to use our own kDefaultHSTSSource
|
follow ups from ryan sleevi:
|
patches/master_patch.patch
Outdated
| - { "name": "youtu.be", "include_subdomains": true, "pins": "google" }, | ||
| - { "name": "youtube-nocookie.com", "include_subdomains": true, "pins": "google" }, | ||
| - { "name": "ytimg.com", "include_subdomains": true, "pins": "google" }, | ||
| + { "name": "2mdn.net", "include_subdomains": true }, |
There was a problem hiding this comment.
please verify if this is true, but I think all the entries without a "mode" or "pins" property would do nothing and thus can be deleted
patches/master_patch.patch
Outdated
| + { "name": "sync.brave.com", "mode": "force-https", "pins": "brave"}, | ||
| + { "name": "sync-staging.brave.com", "mode": "force-https", "pins": "brave"}, | ||
| + { "name": "brave-laptop-updates.global.ssl.fastly.net", "mode": "force-https", "pins": "brave"}, | ||
| + { "name": "brave-download.global.ssl.fastly.net", "mode": "force-https", "pins": "brave"}, |
There was a problem hiding this comment.
we might want to wait on brave/browser-laptop#12301 so we can pin brave.com domains instead of fastly.net domains
patches/master_patch.patch
Outdated
| - enable_static_pins_ = false; | ||
| enable_static_expect_ct_ = false; | ||
| #endif | ||
| + enable_static_pins_ = false; |
There was a problem hiding this comment.
you can use
#if !defined(MUON_CHROMIUM_BUILD)
enable_static_pins_ = true;
#endif
| # Inputs in order expected by the command line of the tool. | ||
| inputs = [ | ||
| - "transport_security_state_static.json", | ||
| + "//electron/atom/browser/resources/transport_security_state_static.json", |
There was a problem hiding this comment.
this file should be updated with every Chromium rebase using transport_security_state_static.json what's the best process for doing that? maybe some kind of post-commit script that runs after every rebase?
There was a problem hiding this comment.
the tricky parts seems to be that we both add and remove entries. If we were only adding them it would be much easier because we could just merge the two files in another gn task. Is it necessary to remove entries?
There was a problem hiding this comment.
or maybe we could minimize the diff by removing the entries from chrome transport_security_state_static.json as a patch and then merge?
There was a problem hiding this comment.
I did that before moving transport_security_state_static.json to the electron code, it'll be hard to maintain if we do that. I think a separate file makes gives us more control over the pinned domains.
There was a problem hiding this comment.
@bridiver it's necessary to remove entries because Google told us we shouldn't do certificate pinning for sites we don't have a point-of-contact for in case they break. however, it's probably safe for us to do force-https on any site. so @jumde's list removes the pins property for non-Brave entries that have it but keeps the force-https property
There was a problem hiding this comment.
@diracdeltas are there any changes that still need to be made here then?
There was a problem hiding this comment.
@bridiver i think this PR is fine as-is
There was a problem hiding this comment.
but as-mentioned above, there should be a follow up issue for downstream list maintenance (making sure it happens every browser-laptop release)
There was a problem hiding this comment.
lgtm. please open a follow-up for https://github.com/brave/muon/pull/401/files#r157640158
There was a problem hiding this comment.
regarding to https://github.com/brave/muon/pull/401/files#r157640158
I think the cost of manual applying changes is too high, see upcoming changes https://chromium.googlesource.com/chromium/src.git/+/63.0.3239.108..64.0.3282.24/net/http/transport_security_state_static.json and it is error-prone
we should generated a our own transport_security_state_static.h for brave pinsets and include it in net/http/transport_security_state.cc
There was a problem hiding this comment.
per discussion on slack, @jumde will be in charged of maintaining JSON file. And @diracdeltas suggest we do upgrade every Brave release instead of chromium upgrade
Issue 12190 - Enabling certificate pinning for Brave domains
…mains