Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2726873003:
Make transport security state data source configurable. (Closed)
|
Created:
3 years, 9 months ago by martijnc Modified:
3 years, 9 months ago Reviewers:
Ryan Sleevi CC:
cbentzel+watch_chromium.org, chromium-reviews, lgarron, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake transport security state data source configurable.
The static transport security state data is currently harcoded to the
data in transport_security_state_static.h. In order to test the whole
process end-to-end, this CL makes the data source configurable so that
unittests can specicfy their own data source.
These data sources are generated by the generator during the build from
several json and pin files. The actual tests will be landed in a follow up
CL but [1] gives a general overview of this approach (which is based on
the registry_controlled_domains DAFSA tests) and is split off from [2].
[1] https://codereview.chromium.org/2680933009
[2] https://codereview.chromium.org/2660793002
BUG=595493
Review-Url: https://codereview.chromium.org/2726873003
Cr-Commit-Position: refs/heads/master@{#457971}
Committed: https://chromium.googlesource.com/chromium/src/+/9b806ab221956979391d7d1974c6ea434c7f1752
Patch Set 1 #Patch Set 2 : Add TransportSecurityStateSource struct. #
Total comments: 15
Patch Set 3 : comments rsleevi #Patch Set 4 : newline #Patch Set 5 : -struct #
Total comments: 2
Patch Set 6 : ternary #
Messages
Total messages: 40 (30 generated)
Description was changed from ========== Make transport security state data source configurable. The static transport security state data is currently harcoded to the data in transport_security_state_static.h. In order to test the whole process end-to-end, this CL makes the data source configurable so that unittests can specicfy their own data source. These data sources are generated by the generator during the build from several json and pin files. The actual tests will be landed in a follow up CL but [1] gives a general overview of this approach (which is based on the DAFSA tests) and is split off from [2]. [1] https://codereview.chromium.org/2680933009 [2] https://codereview.chromium.org/2660793002 BUG=595493 ========== to ========== Make transport security state data source configurable. The static transport security state data is currently harcoded to the data in transport_security_state_static.h. In order to test the whole process end-to-end, this CL makes the data source configurable so that unittests can specicfy their own data source. These data sources are generated by the generator during the build from several json and pin files. The actual tests will be landed in a follow up CL but [1] gives a general overview of this approach (which is based on the registry_controlled_domains DAFSA tests) and is split off from [2]. [1] https://codereview.chromium.org/2680933009 [2] https://codereview.chromium.org/2660793002 BUG=595493 ==========
martijn@martijnc.be changed reviewers: + rsleevi@chromium.org
The CQ bit was checked by martijn@martijnc.be to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by martijn@martijnc.be to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This is the next part I split off from https://codereview.chromium.org/2660793002. It makes the HSTS data source configurable/overrideable so that tests can specify their own. Can you take a look? Thanks! https://codereview.chromium.org/2726873003/diff/20001/net/tools/transport_sec... File net/tools/transport_security_state_generator/resources/transport_security_state_static.template (left): https://codereview.chromium.org/2726873003/diff/20001/net/tools/transport_sec... net/tools/transport_security_state_generator/resources/transport_security_state_static.template:32: struct Pinset { This definition will be required in multiple files (in transport_security_state_static.h and in the generated unittest files) and therefore moved to another file.
friendly ping :)
Thanks for the ping! Sorry about the delays, don't hesitate to make such pings in the future :) https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... net/http/transport_security_state.h:33: SetTransportSecurityStateSource(const TransportSecurityStateSource* source); When is this used? Testing only? SetTransportSecurityStateSourceForTesting()? https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... net/http/transport_security_state.h:37: void NET_EXPORT_PRIVATE SetDefaultTransportSecurityStateSource(); Why is this needed? Our normal pattern for overriding defaults let's a nullptr be seen as the 'reset to default' equivalent. https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... File net/http/transport_security_state_static.h (right): https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... net/http/transport_security_state_static.h:584: + "https://log.getdropbox.com/log/expectct", NULL, Doesn't this require a generator update? Is this dependent on that CL? If so, can you make sure these nullptr https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... File net/http/transport_security_state_structs.h (right): https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... net/http/transport_security_state_structs.h:5: #ifndef NET_HTTP_TRANSPORT_SECURITY_STATE_STRUCTS_H_ So we generally try to avoid naming 'catchall cloaca' files ;) Does this make sense as TransportSecurityStateSource, of which Pinset is an implementation detail of a TSSSource? https://codereview.chromium.org/2726873003/diff/20001/net/tools/transport_sec... File net/tools/transport_security_state_generator/resources/transport_security_state_static.template (right): https://codereview.chromium.org/2726873003/diff/20001/net/tools/transport_sec... net/tools/transport_security_state_generator/resources/transport_security_state_static.template:26: NULL, nullptr
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:50001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:70001) has been deleted
The CQ bit was checked by martijn@martijnc.be to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:90001) has been deleted
The CQ bit was checked by martijn@martijnc.be to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... net/http/transport_security_state.h:33: SetTransportSecurityStateSource(const TransportSecurityStateSource* source); On 2017/03/13 at 18:31:18, Ryan Sleevi wrote: > When is this used? Testing only? > > SetTransportSecurityStateSourceForTesting()? Yes, this is only used in tests. Renamed. https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... net/http/transport_security_state.h:37: void NET_EXPORT_PRIVATE SetDefaultTransportSecurityStateSource(); On 2017/03/13 at 18:31:18, Ryan Sleevi wrote: > Why is this needed? Our normal pattern for overriding defaults let's a nullptr be seen as the 'reset to default' equivalent. This was used to set the source back to the default so that other tests weren't affected. Didn't know that, thanks. Removed. https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... File net/http/transport_security_state_static.h (right): https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... net/http/transport_security_state_static.h:584: + "https://log.getdropbox.com/log/expectct", NULL, On 2017/03/13 at 18:31:18, Ryan Sleevi wrote: > Doesn't this require a generator update? Is this dependent on that CL? Yes, but that change (including the NULLs) already landed. Not sure why this showed up here. > If so, can you make sure these nullptr I'll do the NULL -> nullptr change in a separate CL, there are other NULL's that can be replaced in this file (and in the generator). https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... File net/http/transport_security_state_structs.h (right): https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... net/http/transport_security_state_structs.h:5: #ifndef NET_HTTP_TRANSPORT_SECURITY_STATE_STRUCTS_H_ On 2017/03/13 at 18:31:18, Ryan Sleevi wrote: > So we generally try to avoid naming 'catchall cloaca' files ;) > > Does this make sense as TransportSecurityStateSource, of which Pinset is an implementation detail of a TSSSource? Not really sure what you mean but I'be made Pinset part of TransportSecurityStateSource although it's not really an implementation detail now. It's still exposed because it is needed in other places. https://codereview.chromium.org/2726873003/diff/20001/net/tools/transport_sec... File net/tools/transport_security_state_generator/resources/transport_security_state_static.template (right): https://codereview.chromium.org/2726873003/diff/20001/net/tools/transport_sec... net/tools/transport_security_state_generator/resources/transport_security_state_static.template:26: NULL, On 2017/03/13 at 18:31:18, Ryan Sleevi wrote: > nullptr Will do this in a separate CL.
https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... File net/http/transport_security_state_structs.h (right): https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... net/http/transport_security_state_structs.h:5: #ifndef NET_HTTP_TRANSPORT_SECURITY_STATE_STRUCTS_H_ On 2017/03/13 21:51:40, martijnc wrote: > Not really sure what you mean but I'be made Pinset part of > TransportSecurityStateSource although it's not really an implementation detail > now. It's still exposed because it is needed in other places. Well, mostly because "_structs" is a catchall, like _util or _misc. It doesn't really provide clear guidance as to what it contains (if reading the header) or what it should contain (if considering additions). As a consequence, it often becomes a catchall for a variety of decisions, rather than trying to think through the design. I set "an implementation detail" mostly because the only way you get a Pinset is in conjunction with a TransportSecurityStateSource - right? Not that I'm suggesting this level of class design (there's pros and cons), but I'm curious from an API direction if struct TransportSecurityStateSource { struct Pinset { }; ... const struct Pinset* pinsets; }; is conceptually equivalent? (If it's answered already in the 'master' CL, feel free to just link there directly and I can read/see its usage) https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... net/http/transport_security_state_structs.h:24: const struct Pinset* pinsets; No need to "struct pinset" this, since you define it on line 10-14
Thanks for the feedback! https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... File net/http/transport_security_state_structs.h (right): https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... net/http/transport_security_state_structs.h:5: #ifndef NET_HTTP_TRANSPORT_SECURITY_STATE_STRUCTS_H_ On 2017/03/13 at 21:59:40, Ryan Sleevi wrote: > I set "an implementation detail" mostly because the only way you get a Pinset is in conjunction with a TransportSecurityStateSource - right? > > Not that I'm suggesting this level of class design (there's pros and cons), but I'm curious from an API direction if > > struct TransportSecurityStateSource { > struct Pinset { > > }; > > ... > const struct Pinset* pinsets; > }; > > is conceptually equivalent? Ah, I was a bit confused by what you meant by "implementation detail", but yes, a Pinset always comes as part of a TransportSecurityStateSource. If there is a Pinset, it is part of some TransportSecurityStateSource so making Pinset an implementation detail of TransportSecurityStateSource makes sense (and signals this relationship in the code better). https://codereview.chromium.org/2726873003/diff/20001/net/http/transport_secu... net/http/transport_security_state_structs.h:24: const struct Pinset* pinsets; On 2017/03/13 at 21:59:40, Ryan Sleevi wrote: > No need to "struct pinset" this, since you define it on line 10-14 Done.
LGTM (Also, if it's ever > 24 hours and I haven't replied, please ping :D) https://codereview.chromium.org/2726873003/diff/150001/net/http/transport_sec... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2726873003/diff/150001/net/http/transport_sec... net/http/transport_security_state.cc:735: } extremely pedantic nit: g_hsts_source = source ? source : &kHSTSSource; Ternaries for ease ;)
The CQ bit was checked by martijn@martijnc.be to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by martijn@martijnc.be
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/2726873003/#ps170001 (title: "ternary")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2726873003/diff/150001/net/http/transport_sec... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2726873003/diff/150001/net/http/transport_sec... net/http/transport_security_state.cc:735: } On 2017/03/17 at 22:07:16, Ryan Sleevi wrote: > extremely pedantic nit: > > g_hsts_source = source ? source : &kHSTSSource; > > Ternaries for ease ;) Done.
CQ is committing da patch.
Bot data: {"patchset_id": 170001, "attempt_start_ts": 1489853391189200,
"parent_rev": "d0ff3d34a5798ddc3db10815d49dc1687e4f866e", "commit_rev":
"9b806ab221956979391d7d1974c6ea434c7f1752"}
Message was sent while issue was closed.
Description was changed from ========== Make transport security state data source configurable. The static transport security state data is currently harcoded to the data in transport_security_state_static.h. In order to test the whole process end-to-end, this CL makes the data source configurable so that unittests can specicfy their own data source. These data sources are generated by the generator during the build from several json and pin files. The actual tests will be landed in a follow up CL but [1] gives a general overview of this approach (which is based on the registry_controlled_domains DAFSA tests) and is split off from [2]. [1] https://codereview.chromium.org/2680933009 [2] https://codereview.chromium.org/2660793002 BUG=595493 ========== to ========== Make transport security state data source configurable. The static transport security state data is currently harcoded to the data in transport_security_state_static.h. In order to test the whole process end-to-end, this CL makes the data source configurable so that unittests can specicfy their own data source. These data sources are generated by the generator during the build from several json and pin files. The actual tests will be landed in a follow up CL but [1] gives a general overview of this approach (which is based on the registry_controlled_domains DAFSA tests) and is split off from [2]. [1] https://codereview.chromium.org/2680933009 [2] https://codereview.chromium.org/2660793002 BUG=595493 Review-Url: https://codereview.chromium.org/2726873003 Cr-Commit-Position: refs/heads/master@{#457971} Committed: https://chromium.googlesource.com/chromium/src/+/9b806ab221956979391d7d1974c6... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:170001) as https://chromium.googlesource.com/chromium/src/+/9b806ab221956979391d7d1974c6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
