Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2906633003:
Add a build flag to configure bundling of HSTS preload list (Closed)
DescriptionAdd a build flag to configure bundling of HSTS preload list
This CL adds a build flag (INCLUDE_TRANSPORT_SECURITY_STATE_PRELOAD_LIST)
for //net embedders to configure whether to include the preload list
(~258KiB) in their binary.
BUG=603597
Review-Url: https://codereview.chromium.org/2906633003
Cr-Commit-Position: refs/heads/master@{#487486}
Committed: https://chromium.googlesource.com/chromium/src/+/815ad5ba47f22bdfa7ac523048285e873a74aaac
Patch Set 1 #Patch Set 2 : change flag to INCLUDE_TRANSPORT_SECURITY_STATE_PRELOAD_LIST #
Total comments: 8
Patch Set 3 : address sleevi comments #Patch Set 4 : address sleevi comments #
Total comments: 1
Patch Set 5 : self (one typo) #
Total comments: 8
Patch Set 6 : address comments to not compile transport_security_state_static.template #Patch Set 7 : missed one #Patch Set 8 : one more #
Total comments: 9
Patch Set 9 : address Ryan comments #Patch Set 10 : address Ryan comments #
Total comments: 4
Patch Set 11 : add one missing file and address lgarron comments #
Messages
Total messages: 72 (49 generated)
The CQ bit was checked by xunjieli@chromium.org 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...
Description was changed from ========== temp BUG= ========== to ========== Add a build flag to exclude HSTS preload list This CL adds a build flag (EXCLUDE_HSTS_PRELOAD_LIST) for //net embedders who would like to exclude the preload list (~100KB) from their binary. BUG=603597 ==========
Description was changed from ========== Add a build flag to exclude HSTS preload list This CL adds a build flag (EXCLUDE_HSTS_PRELOAD_LIST) for //net embedders who would like to exclude the preload list (~100KB) from their binary. BUG=603597 ========== to ========== Add a build flag to exclude HSTS preload list This CL adds a build flag (EXCLUDE_HSTS_PRELOAD_LIST) for //net embedders who would like to exclude the preload list (~258KiB) from their binary. BUG=603597 ==========
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by xunjieli@chromium.org 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...
Patchset #4 (id:100001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Add a build flag to exclude HSTS preload list This CL adds a build flag (EXCLUDE_HSTS_PRELOAD_LIST) for //net embedders who would like to exclude the preload list (~258KiB) from their binary. BUG=603597 ========== to ========== Add a build flag to exclude HSTS preload list This CL adds a build flag (EXCLUDE_HSTS_PRELOAD_LIST) for //net embedders who would like to exclude the preload list (~258KiB) from their binary. This is based on martijn's patch at https://codereview.chromium.org/2901393005/patch/20001/30003 BUG=603597 ==========
Description was changed from ========== Add a build flag to exclude HSTS preload list This CL adds a build flag (EXCLUDE_HSTS_PRELOAD_LIST) for //net embedders who would like to exclude the preload list (~258KiB) from their binary. This is based on martijn's patch at https://codereview.chromium.org/2901393005/patch/20001/30003 BUG=603597 ========== to ========== Add a build flag to exclude HSTS preload list This CL adds a build flag (EXCLUDE_HSTS_PRELOAD_LIST) for //net embedders who would like to exclude the preload list (~258KiB) from their binary. This is based on martijn's patch at https://codereview.chromium.org/2901393005/patch/20001/30003 BUG=603597 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
xunjieli@chromium.org changed reviewers: + davidben@chromium.org, lgarron@chromium.org
Hi davidben@ and lgarron@: This is based on martijn's CL. I copied out the hsts entries that tests depend on to a different json file. I disabled tests which depend on preload pinsets if the new build flag is on. Not sure if there's a better way. PTAL. Thank you.
cc-ing rsleevi@ and mef@: Please let me know if you have any concerns. Thank you.
On 2017/06/14 at 19:33:44, xunjieli wrote: > cc-ing rsleevi@ and mef@: Please let me know if you have any concerns. Thank you. Negative flags can become confusing. Could we make this INCLUDE_HSTS_PRELOAD_LIST and default to true?
On 2017/06/14 at 19:35:45, lgarron wrote: > On 2017/06/14 at 19:33:44, xunjieli wrote: > > cc-ing rsleevi@ and mef@: Please let me know if you have any concerns. Thank you. > > Negative flags can become confusing. Could we make this INCLUDE_HSTS_PRELOAD_LIST and default to true? Also, we should probably make it clear that this is not just about HSTS. How about one of the following (in order of specificity)? - INCLUDE_TRANSPORT_SECURITY_STATE_PRELOAD_LIST - INCLUDE_DOMAIN_SECURITY_PRELOAD_LIST - INCLUDE_NET_PRELOAD_LIST
On 2017/06/14 19:37:48, lgarron wrote: > On 2017/06/14 at 19:35:45, lgarron wrote: > > On 2017/06/14 at 19:33:44, xunjieli wrote: > > > cc-ing rsleevi@ and mef@: Please let me know if you have any concerns. Thank > you. > > > > Negative flags can become confusing. Could we make this > INCLUDE_HSTS_PRELOAD_LIST and default to true? > > Also, we should probably make it clear that this is not just about HSTS. > > How about one of the following (in order of specificity)? > - INCLUDE_TRANSPORT_SECURITY_STATE_PRELOAD_LIST > - INCLUDE_DOMAIN_SECURITY_PRELOAD_LIST > - INCLUDE_NET_PRELOAD_LIST Done. Thank you. PTAL.
LGTM in principle, but don't own any of this code and I defer to //net owners (and Martijn) about whether this is the best way to do this.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
I didn't see a clear decision on the bug that this was the right thing to do. I stand by my reservations with adding build flags and gutting code - I'm not sure what our principles here, or how we evaluate the cost of added permutations against the binary savings cost.
On 2017/06/15 02:50:11, Ryan Sleevi (slow through 6-27 wrote: > I didn't see a clear decision on the bug that this was the right thing to do. I > stand by my reservations with adding build flags and gutting code - I'm not sure > what our principles here, or how we evaluate the cost of added permutations > against the binary savings cost. Thanks, Ryan. I talked with Chris and Misha. The decision is to proceed with this change. More discussion is at the linked bug. davidben@: PTAL. Thank you.
On 2017/06/19 at 20:58:02, xunjieli wrote: > On 2017/06/15 02:50:11, Ryan Sleevi (slow through 6-27 wrote: > > I didn't see a clear decision on the bug that this was the right thing to do. I > > stand by my reservations with adding build flags and gutting code - I'm not sure > > what our principles here, or how we evaluate the cost of added permutations > > against the binary savings cost. > > Thanks, Ryan. I talked with Chris and Misha. The decision is to proceed with this change. More discussion is at the linked bug. > davidben@: PTAL. Thank you. Which Chris and Misha, and what was the conclusion? I'd actually like to not LGTM this until I have a response to comment #41: https://bugs.chromium.org/p/chromium/issues/detail?id=603597#c41
xunjieli@chromium.org changed reviewers: + cbentzel@chromium.org, mef@chromium.org
On 2017/06/19 21:00:56, lgarron wrote: > On 2017/06/19 at 20:58:02, xunjieli wrote: > > On 2017/06/15 02:50:11, Ryan Sleevi (slow through 6-27 wrote: > > > I didn't see a clear decision on the bug that this was the right thing to > do. I > > > stand by my reservations with adding build flags and gutting code - I'm not > sure > > > what our principles here, or how we evaluate the cost of added permutations > > > against the binary savings cost. > > > > Thanks, Ryan. I talked with Chris and Misha. The decision is to proceed with > this change. More discussion is at the linked bug. > > davidben@: PTAL. Thank you. > > Which Chris and Misha, and what was the conclusion? > > I'd actually like to not LGTM this until I have a response to comment #41: > https://bugs.chromium.org/p/chromium/issues/detail?id=603597#c41 Is comment#41 a separate issue than the decision whether to exclude the list in Cronet? +cbentzel@ and mef@. I will comment on the bug too.
lgarron@ and rsleevi@: PTAL. //net doesn't know about cronet. There isn't a is_cronet_build flag. The only solution that I find is adding a //net feature flag configurable for all //net embedders. I know we talked about limiting this to Cronet to reduce cognitive load. I am happy to take any suggestion. PTAL. Thank you.
https://codereview.chromium.org/2906633003/diff/140001/net/features.gni File net/features.gni (right): https://codereview.chromium.org/2906633003/diff/140001/net/features.gni#newco... net/features.gni:31: # Whether transport security preload list should be included. Can we provide any better documentation here about the risks/tradeoffs and/or general advice not to change this? https://codereview.chromium.org/2906633003/diff/140001/net/http/http_security... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/2906633003/diff/140001/net/http/http_security... net/http/http_security_headers_unittest.cc:658: #endif To better allow the compiler to optimize this out, shouldn't this be an #if / #else - so that it doesn't have to compile this code? Alternatively, using the DISABLED_ conditional macro for the test based on the build flag (which forces compilation, but uses our more idiomatic approach to disabling tests) https://codereview.chromium.org/2906633003/diff/140001/net/tools/transport_se... File net/tools/transport_security_state_generator/pinsets.h (right): https://codereview.chromium.org/2906633003/diff/140001/net/tools/transport_se... net/tools/transport_security_state_generator/pinsets.h:38: void FilterPinsets(const std::set<std::string>& except_these); This API seems pretty heavy weight - both from performance, and from a 'hard to use'. From looking at the overall CL, it seems like it's just meant to ensure the generator can generate a file, but is that something we can fix with the .gn, so that we don't have to introduce this API at all? https://codereview.chromium.org/2906633003/diff/140001/net/tools/transport_se... File net/tools/transport_security_state_generator/transport_security_state_generator.cc (right): https://codereview.chromium.org/2906633003/diff/140001/net/tools/transport_se... net/tools/transport_security_state_generator/transport_security_state_generator.cc:30: // the first entry to allow the generator to generate a valid file. This seems a bit suboptimal - that is, wouldn't it be better not to generate the file / call the generator? Could you explain more why this approach?
The CQ bit was checked by xunjieli@chromium.org 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...
Description was changed from ========== Add a build flag to exclude HSTS preload list This CL adds a build flag (EXCLUDE_HSTS_PRELOAD_LIST) for //net embedders who would like to exclude the preload list (~258KiB) from their binary. This is based on martijn's patch at https://codereview.chromium.org/2901393005/patch/20001/30003 BUG=603597 ========== to ========== Add a build flag to configure bundling of HSTS preload list This CL adds a build flag (INCLUDE_TRANSPORT_SECURITY_STATE_PRELOAD_LIST) for //net embedders to configure whether to include the preload list (~258KiB) in their binary. BUG=603597 ==========
The CQ bit was checked by xunjieli@chromium.org 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...
Patchset #4 (id:180001) has been deleted
Patchset #5 (id:220001) has been deleted
Patchset #4 (id:200001) has been deleted
The CQ bit was checked by xunjieli@chromium.org 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...
Description was changed from ========== Add a build flag to configure bundling of HSTS preload list This CL adds a build flag (INCLUDE_TRANSPORT_SECURITY_STATE_PRELOAD_LIST) for //net embedders to configure whether to include the preload list (~258KiB) in their binary. BUG=603597 ========== to ========== Add a build flag to configure bundling of HSTS preload list This CL adds a build flag (INCLUDE_TRANSPORT_SECURITY_STATE_PRELOAD_LIST) for //net embedders to configure whether to include the preload list (~258KiB) in their binary. BUG=603597 ==========
Thank you. PTAL. https://codereview.chromium.org/2906633003/diff/140001/net/features.gni File net/features.gni (right): https://codereview.chromium.org/2906633003/diff/140001/net/features.gni#newco... net/features.gni:31: # Whether transport security preload list should be included. On 2017/06/29 20:47:40, Ryan Sleevi (slow through 6-27 wrote: > Can we provide any better documentation here about the risks/tradeoffs and/or > general advice not to change this? Done. https://codereview.chromium.org/2906633003/diff/140001/net/http/http_security... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/2906633003/diff/140001/net/http/http_security... net/http/http_security_headers_unittest.cc:658: #endif On 2017/06/29 20:47:40, Ryan Sleevi (slow through 6-27 wrote: > To better allow the compiler to optimize this out, shouldn't this be an #if / > #else - so that it doesn't have to compile this code? > > Alternatively, using the DISABLED_ conditional macro for the test based on the > build flag (which forces compilation, but uses our more idiomatic approach to > disabling tests) Done. https://codereview.chromium.org/2906633003/diff/140001/net/tools/transport_se... File net/tools/transport_security_state_generator/pinsets.h (right): https://codereview.chromium.org/2906633003/diff/140001/net/tools/transport_se... net/tools/transport_security_state_generator/pinsets.h:38: void FilterPinsets(const std::set<std::string>& except_these); On 2017/06/29 20:47:40, Ryan Sleevi (slow through 6-27 wrote: > This API seems pretty heavy weight - both from performance, and from a 'hard to > use'. > > From looking at the overall CL, it seems like it's just meant to ensure the > generator can generate a file, but is that something we can fix with the .gn, so > that we don't have to introduce this API at all? Done. https://codereview.chromium.org/2906633003/diff/140001/net/tools/transport_se... File net/tools/transport_security_state_generator/transport_security_state_generator.cc (right): https://codereview.chromium.org/2906633003/diff/140001/net/tools/transport_se... net/tools/transport_security_state_generator/transport_security_state_generator.cc:30: // the first entry to allow the generator to generate a valid file. On 2017/06/29 20:47:41, Ryan Sleevi (slow through 6-27 wrote: > This seems a bit suboptimal - that is, wouldn't it be better not to generate the > file / call the generator? Could you explain more why this approach? Done. The Huffman tree decoding code expects at least one entry. I have reverted this and modified transport_security_state.cc. https://codereview.chromium.org/2906633003/diff/240001/net/http/transport_sec... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2906633003/diff/240001/net/http/transport_sec... net/http/transport_security_state.cc:643: // false, in which can BitReader.seek(0) will return false. Ryan, I had to update the code to not DCHECK(false) here.
I'm not sure this is the right approach, but I may have missed something, so I added questions and context below. I think our desired state should be that - Unused code is not compiled if the build flag is (not) set - Unnecessary build steps are not run if the build flag is (not) set - We have a clean(ish) API contract and separation. For example, for tests that expect a baked-in preload list, one approach is the MAYBE_ (or variations thereof), but another is actually to make a static method on the TransportSecurityState, such as HasStaticState() (if we see this being all/nothing, and not per-feature), and having the tests check that, without all the #ifdefs Since we're adding this for Cronet, we're effectively making it Supported API, so I suppose we should try to make that as explicit and integrated as possible. https://codereview.chromium.org/2906633003/diff/260001/net/features.gni File net/features.gni (right): https://codereview.chromium.org/2906633003/diff/260001/net/features.gni#newco... net/features.gni:41: include_transport_security_state_preload_list = true In looking at the flags, I see a mix between positive and negative flags. Given we want this to be the 'default' state, do you think it makes sense to rewrite this to be a negative (e.g. "exclude"), which may signal more clearly the assumptions being made? It's not a very strong argument, and if there's existing documentation that says to prefer positive over negative (e.g. to avoid the double-negative of "exclude = false"), do so - but just a thought. https://codereview.chromium.org/2906633003/diff/260001/net/http/http_security... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/2906633003/diff/260001/net/http/http_security... net/http/http_security_headers_unittest.cc:659: #endif Ah, apologies for not providing a clearer example. We generally try to use a MAYBE_ preprocessor macro to handle these tests, to avoid wrapping the TEST_F() within an #ifdef That is #if !BUILDFLAG(...) #define MAYBE_UpdateDynamicPKPOnly DISABLED_... #else #define MAYBE_UpdateDynamicPKPOnly UpdateDynamicPKPOnly #endif TEST_F(HttpSecurityHeadersTest, MAYBE_UpdateDynamicPKPOnly) (Example: https://cs.chromium.org/chromium/src/net/nqe/network_quality_estimator_unitte... or https://cs.chromium.org/chromium/src/net/socket/udp_socket_unittest.cc?rcl=a2... - in general, search for MAYBE_) A lesser used pattern, which it applies to things like OS-wide behaviours or build-flag behaviours, is an extra macro, such as #if !BUILDFLAG(FOO) #define DISABLED_IF_NOT_FOO(test) DISABLED_##test #else #define DISABLED_IF_NOT_FOO(test) test #endif (Example: https://cs.chromium.org/chromium/src/net/websockets/websocket_end_to_end_test... ) Finally, there's one last pattern - one which involves some process startup overhead, but also works, which is to use a static function to hide the various build flags, and then a helper method in tests which involve that. (Example: https://cs.chromium.org/chromium/src/net/url_request/url_request_unittest.cc?... and https://cs.chromium.org/chromium/src/net/url_request/url_request_unittest.cc?... ) https://codereview.chromium.org/2906633003/diff/260001/net/http/transport_sec... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2906633003/diff/260001/net/http/transport_sec... net/http/transport_security_state.cc:643: // false, in which case BitReader.seek(0) will return false. Could you explain the rationale for doing this change, versus using the build flag? That is, one option is #if !BUILDFLAG(...) return false; #else bool found; if (...) { } return found; #endif - that forces the linker to optimize out DecodeHSTSPreloadRaw (and all the supporting code), which it should be capable of doing. Alternatively, you could do #if !BUILDFLAG(...) bool DecodeHSTSPreloadRaw(const std::string& search_hostname, bool* out_found, PreloadResult* out) { *out_found = false; return true; } #else .... #endif Which would explicitly compile that code out. From my reading, this would fully allow you to compile out both the HuffmanDecoder and the BitReader, which this current approach - removing the DCHECK - doesn't do. By removing the DCHECK, you're removing a guard for when INCLUDE_TRANSPORT_SECURITY_STATE_PRELOAD_LIST is true - thus weakening the preconditions. That seems undesirable, but I was curious if there was a reason for that which I missed. https://codereview.chromium.org/2906633003/diff/260001/net/http/transport_sec... File net/http/transport_security_state_static.template (right): https://codereview.chromium.org/2906633003/diff/260001/net/http/transport_sec... net/http/transport_security_state_static.template:61: static const net::TransportSecurityStateSource kHSTSSource = { I'm not sure it makes sense to force the BUILDFLAG into the template - it seems cleaner to remove this dependency entirely if the build flag is not set. Could you explain more the reasoning?
The CQ bit was checked by xunjieli@chromium.org 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 checked by xunjieli@chromium.org 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...
Thanks Ryan for the very helpful review. PTAL.
> - We have a clean(ish) API contract and separation. For example, for tests
that
> expect a baked-in preload list, one approach is the MAYBE_ (or variations
> thereof), but another is actually to make a static method on the
> TransportSecurityState, such as HasStaticState() (if we see this being
> all/nothing, and not per-feature), and having the tests check that, without
all
> the #ifdefs
I ended up using MAYBE_ instead of adding a HasStaticState() method. Some tests
in transport_security_state_unittest.cc can supply a test-only static state
(e.g. *_unittest{1,2,3}.json). The HasStaticState() method doesn't indicate
clearly which static state that it checks. I could have a more specific method,
e.g HasRealStaticState(), but I am not sure if it's clearer than checking the
BUILDFLAG directly.
https://codereview.chromium.org/2906633003/diff/260001/net/features.gni
File net/features.gni (right):
https://codereview.chromium.org/2906633003/diff/260001/net/features.gni#newco...
net/features.gni:41: include_transport_security_state_preload_list = true
On 2017/07/07 16:45:13, Ryan Sleevi wrote:
> In looking at the flags, I see a mix between positive and negative flags.
>
> Given we want this to be the 'default' state, do you think it makes sense to
> rewrite this to be a negative (e.g. "exclude"), which may signal more clearly
> the assumptions being made?
>
> It's not a very strong argument, and if there's existing documentation that
says
> to prefer positive over negative (e.g. to avoid the double-negative of
"exclude
> = false"), do so - but just a thought.
I slightly prefer the exclude_* version because (as you said) it signals that
that including the list is the default state. I used that in the original patch
but changed to include_* as lgarron@ doesn't like double negatives
(https://codereview.chromium.org/2906633003/#msg22).
lgarron@: Do you feel strongly about keeping this as a positive flag?
https://codereview.chromium.org/2906633003/diff/260001/net/http/http_security...
File net/http/http_security_headers_unittest.cc (right):
https://codereview.chromium.org/2906633003/diff/260001/net/http/http_security...
net/http/http_security_headers_unittest.cc:659: #endif
On 2017/07/07 16:45:13, Ryan Sleevi wrote:
> Ah, apologies for not providing a clearer example.
>
> We generally try to use a MAYBE_ preprocessor macro to handle these tests, to
> avoid wrapping the TEST_F() within an #ifdef
>
> That is
>
> #if !BUILDFLAG(...)
> #define MAYBE_UpdateDynamicPKPOnly DISABLED_...
> #else
> #define MAYBE_UpdateDynamicPKPOnly UpdateDynamicPKPOnly
> #endif
>
> TEST_F(HttpSecurityHeadersTest, MAYBE_UpdateDynamicPKPOnly)
>
> (Example:
>
https://cs.chromium.org/chromium/src/net/nqe/network_quality_estimator_unitte...
> or
>
https://cs.chromium.org/chromium/src/net/socket/udp_socket_unittest.cc?rcl=a2...
> - in general, search for MAYBE_)
>
>
> A lesser used pattern, which it applies to things like OS-wide behaviours or
> build-flag behaviours, is an extra macro, such as
>
> #if !BUILDFLAG(FOO)
> #define DISABLED_IF_NOT_FOO(test) DISABLED_##test
> #else
> #define DISABLED_IF_NOT_FOO(test) test
> #endif
>
> (Example:
>
https://cs.chromium.org/chromium/src/net/websockets/websocket_end_to_end_test...
> )
>
> Finally, there's one last pattern - one which involves some process startup
> overhead, but also works, which is to use a static function to hide the
various
> build flags, and then a helper method in tests which involve that.
>
> (Example:
>
https://cs.chromium.org/chromium/src/net/url_request/url_request_unittest.cc?...
> and
>
https://cs.chromium.org/chromium/src/net/url_request/url_request_unittest.cc?...
> )
>
Done. Changed to MAYBE_ prefix. Thank you for the detailed list of examples.
This is very helpful.
https://codereview.chromium.org/2906633003/diff/260001/net/http/transport_sec...
File net/http/transport_security_state.cc (right):
https://codereview.chromium.org/2906633003/diff/260001/net/http/transport_sec...
net/http/transport_security_state.cc:643: // false, in which case
BitReader.seek(0) will return false.
On 2017/07/07 16:45:13, Ryan Sleevi wrote:
> Could you explain the rationale for doing this change, versus using the build
> flag?
>
> That is, one option is
>
> #if !BUILDFLAG(...)
> return false;
> #else
> bool found;
> if (...) {
> }
> return found;
> #endif
>
> - that forces the linker to optimize out DecodeHSTSPreloadRaw (and all the
> supporting code), which it should be capable of doing.
>
> Alternatively, you could do
>
> #if !BUILDFLAG(...)
> bool DecodeHSTSPreloadRaw(const std::string& search_hostname, bool* out_found,
> PreloadResult* out) {
> *out_found = false;
> return true;
> }
> #else
> ....
>
> #endif
>
> Which would explicitly compile that code out.
>
> From my reading, this would fully allow you to compile out both the
> HuffmanDecoder and the BitReader, which this current approach - removing the
> DCHECK - doesn't do.
>
> By removing the DCHECK, you're removing a guard for when
> INCLUDE_TRANSPORT_SECURITY_STATE_PRELOAD_LIST is true - thus weakening the
> preconditions. That seems undesirable, but I was curious if there was a reason
> for that which I missed.
Done. That's very neat. Thank you for the suggestion.
https://codereview.chromium.org/2906633003/diff/260001/net/http/transport_sec...
File net/http/transport_security_state_static.template (right):
https://codereview.chromium.org/2906633003/diff/260001/net/http/transport_sec...
net/http/transport_security_state_static.template:61: static const
net::TransportSecurityStateSource kHSTSSource = {
On 2017/07/07 16:45:13, Ryan Sleevi wrote:
> I'm not sure it makes sense to force the BUILDFLAG into the template - it
seems
> cleaner to remove this dependency entirely if the build flag is not set.
>
> Could you explain more the reasoning?
Done. Not compiling this when the build flag is turned off is definitely much
cleaner. Thanks for the suggestion.
https://codereview.chromium.org/2906633003/diff/320001/net/http/transport_sec...
File net/http/transport_security_state.cc (right):
https://codereview.chromium.org/2906633003/diff/320001/net/http/transport_sec...
net/http/transport_security_state.cc:49: const TransportSecurityStateSource*
g_hsts_source = nullptr;
(This is needed now that |kHSTSSource| is not defined when the flag is turned
off.)
https://codereview.chromium.org/2906633003/diff/320001/net/http/transport_sec...
net/http/transport_security_state.cc:645: if (g_hsts_source == nullptr)
(transport_security_state_unittest.cc can choose to use a test-only static
source, so I returned false only when the test did not provide a |g_hsts_source|
and the flag is false.)
https://codereview.chromium.org/2906633003/diff/320001/net/http/transport_sec...
File net/http/transport_security_state_source.h (right):
https://codereview.chromium.org/2906633003/diff/320001/net/http/transport_sec...
net/http/transport_security_state_source.h:11: static const char kNoReportURI[]
= "";
(I moved this constant from the template file here because this constant is used
in transport_security_state.cc, and the template file is no longer compiled when
the build flag is false)
LGTM % one nit :) Thanks, it looks much cleaner :) https://codereview.chromium.org/2906633003/diff/320001/net/http/transport_sec... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2906633003/diff/320001/net/http/transport_sec... net/http/transport_security_state.cc:49: const TransportSecurityStateSource* g_hsts_source = nullptr; On 2017/07/10 22:20:40, xunjieli wrote: > (This is needed now that |kHSTSSource| is not defined when the flag is turned > off.) So you could probably abstract this a little, and thus avoid the whole need to expose the GetTSSForTesting #if BUILDFLAG(...) #include const TransportSecurityState* const kDefaultHSTSSource = &kHSTSSource; #else const TransportSecurityState* const kDefaultHSTSSource = nullptr; #endif const TransportSecurityState* g_hsts_source = kDefaultHSTSSource; https://codereview.chromium.org/2906633003/diff/320001/net/http/transport_sec... net/http/transport_security_state.cc:746: g_hsts_source = source; And then here g_hsts_source = source ? source : kDefaultHSTSSource; https://codereview.chromium.org/2906633003/diff/320001/net/http/transport_sec... File net/http/transport_security_state_source.h (right): https://codereview.chromium.org/2906633003/diff/320001/net/http/transport_sec... net/http/transport_security_state_source.h:11: static const char kNoReportURI[] = ""; On 2017/07/10 22:20:40, xunjieli wrote: > (I moved this constant from the template file here because this constant is used > in transport_security_state.cc, and the template file is no longer compiled when > the build flag is false) Because this is static, doesn't the actual value need to be moved to the .cc?
Thank you for the review. lgarron@: PTAL. https://codereview.chromium.org/2906633003/diff/320001/net/http/transport_sec... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2906633003/diff/320001/net/http/transport_sec... net/http/transport_security_state.cc:49: const TransportSecurityStateSource* g_hsts_source = nullptr; On 2017/07/11 15:29:40, Ryan Sleevi wrote: > On 2017/07/10 22:20:40, xunjieli wrote: > > (This is needed now that |kHSTSSource| is not defined when the flag is turned > > off.) > > So you could probably abstract this a little, and thus avoid the whole need to > expose the GetTSSForTesting > > #if BUILDFLAG(...) > #include > const TransportSecurityState* const kDefaultHSTSSource = &kHSTSSource; > #else > const TransportSecurityState* const kDefaultHSTSSource = nullptr; > #endif > > const TransportSecurityState* g_hsts_source = kDefaultHSTSSource; Done. Good idea. Thanks! https://codereview.chromium.org/2906633003/diff/320001/net/http/transport_sec... net/http/transport_security_state.cc:746: g_hsts_source = source; On 2017/07/11 15:29:40, Ryan Sleevi wrote: > And then here > > g_hsts_source = source ? source : kDefaultHSTSSource; Done. https://codereview.chromium.org/2906633003/diff/320001/net/http/transport_sec... File net/http/transport_security_state_source.h (right): https://codereview.chromium.org/2906633003/diff/320001/net/http/transport_sec... net/http/transport_security_state_source.h:11: static const char kNoReportURI[] = ""; On 2017/07/11 15:29:40, Ryan Sleevi wrote: > On 2017/07/10 22:20:40, xunjieli wrote: > > (I moved this constant from the template file here because this constant is > used > > in transport_security_state.cc, and the template file is no longer compiled > when > > the build flag is false) > > Because this is static, doesn't the actual value need to be moved to the .cc? Done.
The CQ bit was checked by xunjieli@chromium.org 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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2906633003/diff/360001/net/features.gni File net/features.gni (right): https://codereview.chromium.org/2906633003/diff/360001/net/features.gni#newco... net/features.gni:34: # Whether transport security preload list should be included. This flag should Nit: "Whether the transport security preload list..." I would prefer to be more direct rather than mentioning general "tradeoffs" and a spec. Also, the preload list includes more than HSTS. How about this? ---------------------------- Include the transport security preload list. This list includes mechanisms (e.g. HSTS, HPKP) to enforce trusted connections to a significant set of hardcoded domains. It should not be disabled unless building a subset of the codebase where several hundred KB of binary size are a large burden, and the application is willing to take responsibility to make sure that all important connections use HTTPS. https://codereview.chromium.org/2906633003/diff/360001/net/http/transport_sec... File net/http/transport_security_state_static_unittest0.json (right): https://codereview.chromium.org/2906633003/diff/360001/net/http/transport_sec... net/http/transport_security_state_static_unittest0.json:1: // Copyright 2017 The Chromium Authors. All rights reserved. Could you add a comment about why such a large test is needed? Also, is it possible to shorten it?
Thank you. PTAL. https://codereview.chromium.org/2906633003/diff/360001/net/features.gni File net/features.gni (right): https://codereview.chromium.org/2906633003/diff/360001/net/features.gni#newco... net/features.gni:34: # Whether transport security preload list should be included. This flag should On 2017/07/11 19:29:04, lgarron wrote: > Nit: "Whether the transport security preload list..." > > I would prefer to be more direct rather than mentioning general "tradeoffs" and > a spec. Also, the preload list includes more than HSTS. > > How about this? > > ---------------------------- > > Include the transport security preload list. This list includes mechanisms (e.g. > HSTS, HPKP) to enforce trusted connections to a significant set of hardcoded > domains. It should not be disabled unless building a subset of the codebase > where several hundred KB of binary size are a large burden, and the application > is willing to take responsibility to make sure that all important connections > use HTTPS. Done. https://codereview.chromium.org/2906633003/diff/360001/net/http/transport_sec... File net/http/transport_security_state_static_unittest0.json (right): https://codereview.chromium.org/2906633003/diff/360001/net/http/transport_sec... net/http/transport_security_state_static_unittest0.json:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/07/11 19:29:04, lgarron wrote: > Could you add a comment about why such a large test is needed? Done. These are mostly for tests in transport_security_state_unittests.cc which depend on the default preload list being available. Some tests pick entries rather arbitrarily (see TransportSecurityStateTest.Preloaded). I think relying on the real preload list in unit tests is not a good idea. There are also test-only domains (e.g. first 5 of the hsts entries) in the real preload list. Ideally, we should move off these unit tests to not test arbitrary domains. > > Also, is it possible to shorten it? These are needed by tests in transport_security_state_unittest.cc and 3 CT/HSTS tests in url_request_unittest.cc. I have tried it to shorten it. I think this is the shortest list.
The CQ bit was checked by xunjieli@chromium.org 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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
xunjieli@chromium.org changed reviewers: - cbentzel@chromium.org, davidben@chromium.org, mef@chromium.org
lgtm
(I have moved everyone else to cc. Let me know if you have any concern. Happy to do follow-ups. Thank you.)
The CQ bit was checked by xunjieli@chromium.org
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/2906633003/#ps380001 (title: "add one missing file and address lgarron comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 380001, "attempt_start_ts": 1500383148352610,
"parent_rev": "fcfcaa9965c21f1652f8616cc51ec1caa4603961", "commit_rev":
"815ad5ba47f22bdfa7ac523048285e873a74aaac"}
Message was sent while issue was closed.
Description was changed from ========== Add a build flag to configure bundling of HSTS preload list This CL adds a build flag (INCLUDE_TRANSPORT_SECURITY_STATE_PRELOAD_LIST) for //net embedders to configure whether to include the preload list (~258KiB) in their binary. BUG=603597 ========== to ========== Add a build flag to configure bundling of HSTS preload list This CL adds a build flag (INCLUDE_TRANSPORT_SECURITY_STATE_PRELOAD_LIST) for //net embedders to configure whether to include the preload list (~258KiB) in their binary. BUG=603597 Review-Url: https://codereview.chromium.org/2906633003 Cr-Commit-Position: refs/heads/master@{#487486} Committed: https://chromium.googlesource.com/chromium/src/+/815ad5ba47f22bdfa7ac52304828... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:380001) as https://chromium.googlesource.com/chromium/src/+/815ad5ba47f22bdfa7ac52304828... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
