This is a tracking bug for work on deprecating and removing key pinning. Use it for related bugs and CLs, such as any developer console warnings, additional metrics, removing the support code, et c.
FWIW, I'm still accepting new preloaded pinning requests for the time being (one every few months) until we have a concrete decision otherwise. This bug looks like a good place for such a decision, in case anyone feels we should stop already.
Redirecting to Matt to see if he can find a better owner.
The remaining steps were blocked on ensuring functional integration tests in the presence of the Network Service, as the current browser_tests rely on using Dynamic HPKP to test behaviours such as sending reports. This potentially involves adding test-only interfaces (NetworkServiceTest?) to configure which static source is used (and to enable the static data, which is default-disabled for Chromium builds), so that it can use test-only data to make sure the features continue to work.
I'm confused. We need tests for a feature before we remove that feature? Is this bug only for deprecating the dynamic list, and not the static one, and you want tests for the static lists?
@mmenke:
Correct. The removal first involves removing dynamic HPKP. Currently, the tests for HPKP reporting (usable with both static and dynamic PKP) use the TestServer w/ dynamic PKP in order to test that reports are sent.
By removing dynamic PKP support, there's no way to test the PKP reporting from the browser_tests side dynamically. The way this is being done w/in the NetworkService & friends is to use some test-only methods on the TransportSecurityState (TSS::EnableStaticPinsForTesting, SetTransportSecurityStateSourceForTesting) to set up a test-only 'static' configuration that configures both domains to report from (when accessed) and the domains to report to.
Palmer's first CL ("full size") had everything working except for those browser_test migrations.
There are currently tests to check that behaviors like sending reports (a feature of PKP that exists for both dynamic and static entries) work correctly. These tests are currently implemented using the dynamic code, and it's nontrivial to reimplement them using only a test static PKP list. (I helped Palmer rewrite some of them, but browser_tests is the largest remaining one to get working with the static list instead of dynamic entries.)
Think you're going to need an cert expert figure out how to implement this, cert-side, before anything can be hooked up to a test API. I assume we'd want to inject cert results at a lower layer than the cert verifier for the built-in certs.
Then what's missing? Is this just a port issue, with the built-in values pointing reports to the wrong port (Since we can intercept DNS lookups, and mock cert verification results, but not mock the port used for the requests), or is the issue that they are only enabled in cert configurations? (Or both)
I *think* the port issue (the report_uri entry in the unittest default pinset is for the default port, port 80, whereas the embedded_test_server needs to receive the request on the test server's ephemeral port) is the main thing that was blocking me getting the browser test working. But in addition to that, it sounds like there's something that would need to get wired thru the network service as well to make that test work when the network service is enabled (I'm fuzzy on what exactly that bit is).
So the options are to either inject a "static" HSTS entry (Do we have code for that?) or make things work with a real entry (Which would require either intercepting the report at the net/ layer, or I guess with a proxy - the proxy solution might be handy, as it could be reused in similar situations, and probably wouldn't be too much code, but does seem a little hairy. We'd need to have minimal bogus proxy (SOCKS5 / HTTP) handshake code above the SSL layer for the embedded test server). The most sane way is presumably injecting the static entry.
Either way, we'd need to enable the static HSTS list for a non-Chrome build as well, and given concerns over it breaking third parties, we want a test-only interface there, instead of just enabling it for content-shell.
This doesn't really seem to depend much on knowledge of the NetworkService, but more knowledge of how to muck with TransportSecurityState (Assuming we go with static HSTS injection - if we go with the other options, HttpJob interception or embedded test server faking a proxy, things are a bit different)...Or am I missing something?
> So the options are to either inject a "static" HSTS entry (Do we have code for that?)
Yes. Noted in the CL in Comment #17 and mentioned in Comment #14.
> we want a test-only interface there
Yes. //net has that (mentioned in Comment #14) and is what the existing lower layer tests are using - but that code runs in the Network Service.
Basically, we have the same tests running at different layers - //net, the Network Service, and browser_tests - functionally acting as unit tests, feature tests, and integration tests. For the former two, it's easier to make sure things 'work', because they run in process and the existing test interfaces work. For the latter - browser_tests - there's at minimum two methods to expose on the Network Service (enable static pins, set the pin source), and possibly a third (which is setup a URL interceptor or the like to respond to the request on the 'wrong' port with a redirect to the 'right' port, or a proxy, or simply dynamically compute the right pin source).
One of the stopping points was NetworkServiceTest not working on MacOS, thus presenting some challenges, but that's now resolved. The rest is just someone to drive the (presumably, NetworkServiceTest) changes through in a way they're OK with.
mattm@: This is not expected, thanks for pointing this out!
Is entire PKP support getting deprecated or just dynamic PKP?
We have a customer that uses PKP on Android and iOS, so we need to figure out a good path forward.
Although Cronet uses the dynamic PKP code, it doesn't use any of the storage bits or whatever. It just wants to register them in memory. So that would still allow the bulk of the code to go. And then once the interfaces are only for Cronet's use case, we can probably simplify that.
Labels: -M-69 M-72 Status: Fixed (was: Started) Summary: Deprecate And Remove header-based HTTP Public Key Pinning (HPKP) (was: Deprecate And Remove Public Key Pinning)
Discussed with palmer, we are going to use this bug just for the header-based public key pinning deprecation, as there are no immediate plans for static PKP.
One thing to note is the base dynamic key pinning code was left in place, with cronet as the only consumer. We can use issue 890409 to discuss if that is a good long term solution for cronet.
See also issue 893055 about removing "Public-Key-Pins" from the list of non-cachable headers once we're sure the removal sticks.
Comment 1 by lgar...@chromium.org
, Oct 27 2017