|
|
DescriptionAdd Expect-Staple to preload list
BUG=598021
R=agl@chromium.org, estark@chromium.org, lgarron@chromium.org
Committed: https://chromium.googlesource.com/chromium/src/+/d59a9318491742f487eaea3d10920c063065ec90
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address comments from estark@ #Patch Set 3 : Update test domain names #
Total comments: 1
Patch Set 4 : Initialize all variables #Patch Set 5 : Patch with static.h #
Total comments: 2
Patch Set 6 : Test enable_static_expect_staple_ is followed #
Messages
Total messages: 25 (7 generated)
Description was changed from ========== Add Expect-Staple to preload list BUG=598021 ========== to ========== Add Expect-Staple to preload list BUG=598021 ==========
dadrian@google.com changed reviewers: + estark@chromium.org, lgarron@chromium.org
This corresponds with https://github.com/chromium/hstspreload/pull/78. I regenerated //net/http/transport_security_state_static.h with the Expect-Staple changes, although it looks like the patch wasn't uploaded because the file is too large.
https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... net/http/transport_security_state.cc:379: bool es_include_subdomains; nit: even though it's long my preference would be |expect_staple_include_subdomains|, as the acronym is kind of confusing if seen out of context https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... File net/http/transport_security_state_static.json (right): https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... net/http/transport_security_state_static.json:215: + { "name": "expect-staple-test.badssl.com", "expect_staple": true, "expect_staple_report_uri": "https://report.badssl.com/expect-staple", "include_subdomains_for_expect_staple": true }, If this exists solely for testing include_subdomains, probably should have that in the name. include-subdomains-expect-staple-test.badssl.com? I dunno, Lucas probably has an opinion. https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... net/http/transport_security_state_unittest.cc:1696: &expect_staple_state)); Can you add a test that a subdomain of kExpectStapleStaticHostname is not found?
https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... net/http/transport_security_state.cc:379: bool es_include_subdomains; On 2016/06/02 22:41:51, estark wrote: > nit: even though it's long my preference would be > |expect_staple_include_subdomains|, as the acronym is kind of confusing if seen > out of context Done. https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... File net/http/transport_security_state_static.json (right): https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... net/http/transport_security_state_static.json:215: + { "name": "expect-staple-test.badssl.com", "expect_staple": true, "expect_staple_report_uri": "https://report.badssl.com/expect-staple", "include_subdomains_for_expect_staple": true }, On 2016/06/02 22:41:51, estark wrote: > If this exists solely for testing include_subdomains, probably should have that > in the name. include-subdomains-expect-staple-test.badssl.com? I dunno, Lucas > probably has an opinion. I was basing it off of pinning-test, but in retrospect, that doesn't make a whole lot of sense. https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... net/http/transport_security_state_unittest.cc:1696: &expect_staple_state)); On 2016/06/02 22:41:51, estark wrote: > Can you add a test that a subdomain of kExpectStapleStaticHostname is not found? Done.
As you observed, the transport_security_state_static.h file diff is too large to upload to Rietveld until https://crbug.com/595493 is fixed. You'll probably have to get a committer (me or Emily) to land this for you. https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... File net/http/transport_security_state_static.json (right): https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... net/http/transport_security_state_static.json:215: + { "name": "expect-staple-test.badssl.com", "expect_staple": true, "expect_staple_report_uri": "https://report.badssl.com/expect-staple", "include_subdomains_for_expect_staple": true }, On 2016/06/02 at 23:17:24, dadrian wrote: > On 2016/06/02 22:41:51, estark wrote: > > If this exists solely for testing include_subdomains, probably should have that > > in the name. include-subdomains-expect-staple-test.badssl.com? I dunno, Lucas > > probably has an opinion. > > I was basing it off of pinning-test, but in retrospect, that doesn't make a whole lot of sense. The Cartesian product of all test combinations can get pretty large, and badssl.com is mainly meant to test all possible UI cases, not necessarily all client-side code paths. Would it be sufficient to have just preloaded-expect-staple.badssl.com (without stapling on that domain)? If you want both, I would prefer something containing `preloaded` like preloaded-expect-stable-including-subdomains.badssl.com (`pinning-test.badssl.com` should probably have been `preloaded-hpkp.badssl.com`, except that I was trying to keep it similar to `pinningtest.appspot.com`)
https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... File net/http/transport_security_state_static.json (right): https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... net/http/transport_security_state_static.json:215: + { "name": "expect-staple-test.badssl.com", "expect_staple": true, "expect_staple_report_uri": "https://report.badssl.com/expect-staple", "include_subdomains_for_expect_staple": true }, On 2016/06/03 00:41:42, lgarron wrote: > On 2016/06/02 at 23:17:24, dadrian wrote: > > On 2016/06/02 22:41:51, estark wrote: > > > If this exists solely for testing include_subdomains, probably should have > that > > > in the name. include-subdomains-expect-staple-test.badssl.com? I dunno, > Lucas > > > probably has an opinion. > > > > I was basing it off of pinning-test, but in retrospect, that doesn't make a > whole lot of sense. > > The Cartesian product of all test combinations can get pretty large, and > http://badssl.com is mainly meant to test all possible UI cases, not necessarily all > client-side code paths. Would it be sufficient to have just > http://preloaded-expect-staple.badssl.com (without stapling on that domain)? > > If you want both, I would prefer something containing `preloaded` like > http://preloaded-expect-stable-including-subdomains.badssl.com > (`pinning-test.badssl.com` should probably have been > `preloaded-hpkp.badssl.com`, except that I was trying to keep it similar to > `pinningtest.appspot.com`) I don't think there are any UI cases for Expect-Staple currently. While writing the test cases for include subdomains, this definitely caught bugs, so it'd be good to test the client side code. Do you have any suggestions for how to do test this without modifying the preload list?
On 2016/06/03 18:08:33, dadrian wrote: > https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... > File net/http/transport_security_state_static.json (right): > > https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... > net/http/transport_security_state_static.json:215: + { "name": > "expect-staple-test.badssl.com", "expect_staple": true, > "expect_staple_report_uri": "https://report.badssl.com/expect-staple", > "include_subdomains_for_expect_staple": true }, > On 2016/06/03 00:41:42, lgarron wrote: > > On 2016/06/02 at 23:17:24, dadrian wrote: > > > On 2016/06/02 22:41:51, estark wrote: > > > > If this exists solely for testing include_subdomains, probably should have > > that > > > > in the name. include-subdomains-expect-staple-test.badssl.com? I dunno, > > Lucas > > > > probably has an opinion. > > > > > > I was basing it off of pinning-test, but in retrospect, that doesn't make a > > whole lot of sense. > > > > The Cartesian product of all test combinations can get pretty large, and > > http://badssl.com is mainly meant to test all possible UI cases, not > necessarily all > > client-side code paths. Would it be sufficient to have just > > http://preloaded-expect-staple.badssl.com (without stapling on that domain)? > > > > If you want both, I would prefer something containing `preloaded` like > > http://preloaded-expect-stable-including-subdomains.badssl.com > > (`pinning-test.badssl.com` should probably have been > > `preloaded-hpkp.badssl.com`, except that I was trying to keep it similar to > > `pinningtest.appspot.com`) > > I don't think there are any UI cases for Expect-Staple currently. While writing > the test cases for include subdomains, this definitely caught bugs, so it'd be > good to test the client side code. Do you have any suggestions for how to do > test this without modifying the preload list? We might want to just use ".test" domains in the preload list. That's what we usually tend to use in tests anyway, to avoid confusion and accidentally hitting the network. Lucas, what do you think of having .test domains in the preload list?
On 2016/06/03 at 18:10:18, estark wrote: > On 2016/06/03 18:08:33, dadrian wrote: > > https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... > > File net/http/transport_security_state_static.json (right): > > > > https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... > > net/http/transport_security_state_static.json:215: + { "name": > > "expect-staple-test.badssl.com", "expect_staple": true, > > "expect_staple_report_uri": "https://report.badssl.com/expect-staple", > > "include_subdomains_for_expect_staple": true }, > > On 2016/06/03 00:41:42, lgarron wrote: > > > On 2016/06/02 at 23:17:24, dadrian wrote: > > > > On 2016/06/02 22:41:51, estark wrote: > > > > > If this exists solely for testing include_subdomains, probably should have > > > that > > > > > in the name. include-subdomains-expect-staple-test.badssl.com? I dunno, > > > Lucas > > > > > probably has an opinion. > > > > > > > > I was basing it off of pinning-test, but in retrospect, that doesn't make a > > > whole lot of sense. > > > > > > The Cartesian product of all test combinations can get pretty large, and > > > http://badssl.com is mainly meant to test all possible UI cases, not > > necessarily all > > > client-side code paths. Would it be sufficient to have just > > > http://preloaded-expect-staple.badssl.com (without stapling on that domain)? > > > > > > If you want both, I would prefer something containing `preloaded` like > > > http://preloaded-expect-stable-including-subdomains.badssl.com > > > (`pinning-test.badssl.com` should probably have been > > > `preloaded-hpkp.badssl.com`, except that I was trying to keep it similar to > > > `pinningtest.appspot.com`) > > > > I don't think there are any UI cases for Expect-Staple currently. While writing > > the test cases for include subdomains, this definitely caught bugs, so it'd be > > good to test the client side code. Do you have any suggestions for how to do > > test this without modifying the preload list? > > We might want to just use ".test" domains in the preload list. That's what we usually tend to use in tests anyway, to avoid confusion and accidentally hitting the network. Lucas, what do you think of having .test domains in the preload list? I would prefer not to add .test domains. David informs in person that both domains have been useful for writing tests and catching bugs. Let's just add real ones: - preloaded-expect-staple.badssl.com - preloaded-expect-stable-include-subdomains.badssl.com (I wrote "including" above – "include" is more consistent.) We can always change it if we feel strongly about it later.
On 2016/06/03 at 21:48:59, lgarron wrote: > On 2016/06/03 at 18:10:18, estark wrote: > > On 2016/06/03 18:08:33, dadrian wrote: > > > https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... > > > File net/http/transport_security_state_static.json (right): > > > > > > https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... > > > net/http/transport_security_state_static.json:215: + { "name": > > > "expect-staple-test.badssl.com", "expect_staple": true, > > > "expect_staple_report_uri": "https://report.badssl.com/expect-staple", > > > "include_subdomains_for_expect_staple": true }, > > > On 2016/06/03 00:41:42, lgarron wrote: > > > > On 2016/06/02 at 23:17:24, dadrian wrote: > > > > > On 2016/06/02 22:41:51, estark wrote: > > > > > > If this exists solely for testing include_subdomains, probably should have > > > > that > > > > > > in the name. include-subdomains-expect-staple-test.badssl.com? I dunno, > > > > Lucas > > > > > > probably has an opinion. > > > > > > > > > > I was basing it off of pinning-test, but in retrospect, that doesn't make a > > > > whole lot of sense. > > > > > > > > The Cartesian product of all test combinations can get pretty large, and > > > > http://badssl.com is mainly meant to test all possible UI cases, not > > > necessarily all > > > > client-side code paths. Would it be sufficient to have just > > > > http://preloaded-expect-staple.badssl.com (without stapling on that domain)? > > > > > > > > If you want both, I would prefer something containing `preloaded` like > > > > http://preloaded-expect-stable-including-subdomains.badssl.com > > > > (`pinning-test.badssl.com` should probably have been > > > > `preloaded-hpkp.badssl.com`, except that I was trying to keep it similar to > > > > `pinningtest.appspot.com`) > > > > > > I don't think there are any UI cases for Expect-Staple currently. While writing > > > the test cases for include subdomains, this definitely caught bugs, so it'd be > > > good to test the client side code. Do you have any suggestions for how to do > > > test this without modifying the preload list? > > > > We might want to just use ".test" domains in the preload list. That's what we usually tend to use in tests anyway, to avoid confusion and accidentally hitting the network. Lucas, what do you think of having .test domains in the preload list? > > I would prefer not to add .test domains. David informs in person that both domains have been useful for writing tests and catching bugs. > > Let's just add real ones: > - preloaded-expect-staple.badssl.com > - preloaded-expect-stable-include-subdomains.badssl.com (I wrote "including" above – "include" is more consistent.) > > We can always change it if we feel strongly about it later. *staple, not stable. :-P - preloaded-expect-staple.badssl.com - preloaded-expect-staple-include-subdomains.badssl.com
On 2016/06/03 21:49:25, lgarron wrote: > On 2016/06/03 at 21:48:59, lgarron wrote: > > On 2016/06/03 at 18:10:18, estark wrote: > > > On 2016/06/03 18:08:33, dadrian wrote: > > > > > https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... > > > > File net/http/transport_security_state_static.json (right): > > > > > > > > > https://codereview.chromium.org/2034843003/diff/1/net/http/transport_security... > > > > net/http/transport_security_state_static.json:215: + { "name": > > > > "expect-staple-test.badssl.com", "expect_staple": true, > > > > "expect_staple_report_uri": "https://report.badssl.com/expect-staple", > > > > "include_subdomains_for_expect_staple": true }, > > > > On 2016/06/03 00:41:42, lgarron wrote: > > > > > On 2016/06/02 at 23:17:24, dadrian wrote: > > > > > > On 2016/06/02 22:41:51, estark wrote: > > > > > > > If this exists solely for testing include_subdomains, probably > should have > > > > > that > > > > > > > in the name. include-subdomains-expect-staple-test.badssl.com? I > dunno, > > > > > Lucas > > > > > > > probably has an opinion. > > > > > > > > > > > > I was basing it off of pinning-test, but in retrospect, that doesn't > make a > > > > > whole lot of sense. > > > > > > > > > > The Cartesian product of all test combinations can get pretty large, and > > > > > http://badssl.com is mainly meant to test all possible UI cases, not > > > > necessarily all > > > > > client-side code paths. Would it be sufficient to have just > > > > > http://preloaded-expect-staple.badssl.com (without stapling on that > domain)? > > > > > > > > > > If you want both, I would prefer something containing `preloaded` like > > > > > http://preloaded-expect-stable-including-subdomains.badssl.com > > > > > (`pinning-test.badssl.com` should probably have been > > > > > `preloaded-hpkp.badssl.com`, except that I was trying to keep it similar > to > > > > > `pinningtest.appspot.com`) > > > > > > > > I don't think there are any UI cases for Expect-Staple currently. While > writing > > > > the test cases for include subdomains, this definitely caught bugs, so > it'd be > > > > good to test the client side code. Do you have any suggestions for how to > do > > > > test this without modifying the preload list? > > > > > > We might want to just use ".test" domains in the preload list. That's what > we usually tend to use in tests anyway, to avoid confusion and accidentally > hitting the network. Lucas, what do you think of having .test domains in the > preload list? > > > > I would prefer not to add .test domains. David informs in person that both > domains have been useful for writing tests and catching bugs. > > > > Let's just add real ones: > > - http://preloaded-expect-staple.badssl.com > > - http://preloaded-expect-stable-include-subdomains.badssl.com (I wrote "including" > above – "include" is more consistent.) > > > > We can always change it if we feel strongly about it later. > > *staple, not stable. :-P > > - http://preloaded-expect-staple.badssl.com > - http://preloaded-expect-staple-include-subdomains.badssl.com Done. Any suggestions for who to have review the remaining files?
LGTM for the JSON. It would normally be agl@, but seeing as he is gone I would normally start by asking rsleevi@, but he's out until 6/6. davidben@ might be good, but he has a huge review load already (and lives on the other coast).
agl@chromium.org changed reviewers: + agl@chromium.org
LGTM Rubber stamp for net/
The CQ bit was checked by dadrian@google.com
I pinged agl@; he's willing to rubber-stamp if Emily and I approve.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2034843003/20001
The CQ bit was unchecked by dadrian@google.com
lgtm if you initialize enable_static_expect_staple_ and set it to false in the same place that enable_static_pins_ is set https://codereview.chromium.org/2034843003/diff/20001/net/http/transport_secu... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2034843003/diff/20001/net/http/transport_secu... net/http/transport_security_state.h:494: bool enable_static_expect_staple_; need to initialize this in the TSS constructor
https://codereview.chromium.org/2034843003/diff/40001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2034843003/diff/40001/net/http/transport_secu... net/http/transport_security_state.cc:620: enable_static_expect_staple_(false), I think you can initialize this to true and set it to false after line 627. (Or is there a reason you want to leave it disabled for now?)
https://codereview.chromium.org/2034843003/diff/40001/net/http/transport_secu... File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/2034843003/diff/40001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:1688: TransportSecurityStateTest::EnableStaticExpectStaple(&state); Sorry, I am doing a terrible job getting all my comments in in one round. One more request: could you please add a test that GetExpectStapleState() is false when enable_static_expect_staple_ is false?
Message was sent while issue was closed.
Description was changed from ========== Add Expect-Staple to preload list BUG=598021 ========== to ========== Add Expect-Staple to preload list BUG=598021 R=agl@chromium.org, estark@chromium.org, lgarron@chromium.org Committed: https://crrev.com/d59a9318491742f487eaea3d10920c063065ec90 Cr-Commit-Position: refs/heads/master@{#397844} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d59a9318491742f487eaea3d10920c063065ec90 Cr-Commit-Position: refs/heads/master@{#397844}
Message was sent while issue was closed.
Description was changed from ========== Add Expect-Staple to preload list BUG=598021 R=agl@chromium.org, estark@chromium.org, lgarron@chromium.org Committed: https://crrev.com/d59a9318491742f487eaea3d10920c063065ec90 Cr-Commit-Position: refs/heads/master@{#397844} ========== to ========== Add Expect-Staple to preload list BUG=598021 R=agl@chromium.org, estark@chromium.org, lgarron@chromium.org Committed: https://chromium.googlesource.com/chromium/src/+/d59a9318491742f487eaea3d1092... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:60001) manually as d59a9318491742f487eaea3d10920c063065ec90 (presubmit successful). |