Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1131963003:
Switch //net functions to use SchemeIsCryptographic() instead of SchemeIsSecure(). (Closed)
DescriptionSwitch //net functions to use SchemeIsCryptographic() instead of SchemeIsSecure().
SchemeIsCryptographic more appropriately reflects
the intent (that a cryptographically secure
transport was used - e.g. https:// and wss:// schemes),
whereas SchemeIsSecure also considers situations where
the scheme is locally trusted (e.g. filesystem URLs)
BUG=362214
Committed: https://crrev.com/a774b92283e458a9bc77ba0ce49777b74cde493a
Cr-Commit-Position: refs/heads/master@{#329966}
Patch Set 1 #Patch Set 2 : Rebasing. #
Total comments: 11
Messages
Total messages: 30 (8 generated)
lgarron@chromium.org changed reviewers: + rsleevi@chromium.org
Ryan, would you please take a look at this switch?
rsleevi@chromium.org changed reviewers: + asanka@chromium.org, rch@chromium.org
The description is not accurate - this contains with it a functional change (file URLs no longer treated as secure). That's a functional, observable behaviour change that needs to be carefully evaluated. At the least, I think the description should capture that there is a functional change, but that it's expected to be benign. I'm adding Ryan and Asanka to check my evaluation - I think this is OK, but I'd like their opinions as well.
lgarron@chromium.org changed reviewers: - asanka@chromium.org, rch@chromium.org
> The description is not accurate - this contains with it a functional change > (file URLs no longer treated as secure). That's a functional, observable > behaviour change that needs to be carefully evaluated. Typo, I think: you mean filesystem URLs no longer treated as secure. > At the least, I think the description should capture that there is a functional > change, but that it's expected to be benign. Agree. I think we should let unit tests be the guide — if the callers were depending on filesystem URLs being matched by the SchemeIsSecure function, they should/would test for that. So, let's see how the tests shake out.
On 2015/05/07 at 19:10:34, palmer wrote: > > The description is not accurate - this contains with it a functional change > > (file URLs no longer treated as secure). That's a functional, observable > > behaviour change that needs to be carefully evaluated. > > Typo, I think: you mean filesystem URLs no longer treated as secure. > > > At the least, I think the description should capture that there is a functional > > change, but that it's expected to be benign. > > Agree. I think we should let unit tests be the guide — if the callers were depending on filesystem URLs being matched by the SchemeIsSecure function, they should/would test for that. So, let's see how the tests shake out. Based on an earlier CL, all the tests should still pass. I just started `git cl try` on this to double-check.
On 2015/05/07 19:10:34, palmer wrote: > Agree. I think we should let unit tests be the guide — if the callers were > depending on filesystem URLs being matched by the SchemeIsSecure function, they > should/would test for that. So, let's see how the tests shake out. Well, no, that's not quite true. It's perfectly reasonable for a unit test to assume the API it's using is guaranteed to have the same behaviour. I mean, I'm totally sympathetic to your point, but I don't think it holds as a general adage that unit tests in component X should test all the permutations of component Y as part of X, as that quickly gets to testing "implementation rather than interface". Again, I think it's benign, but it's worth calling out, and worth making sure there wasn't any dependence on the API contract when changing it.
rch@chromium.org changed reviewers: + rch@chromium.org
A few comments. * The changes to net/quic/ and net/http/http_stream_parser.cc look fine to me. * I have a question about the change to http_network_transaction.cc * I find SchemeIsCryptographic hard to read :> I suspect the ship has sailed, but I would *MUCH* prefer SchemeIsCryptographicallySecure or SchemeUsesCryptography, but "Is Cryptographic" just doesn't read well to me. (Maybe I just don't encounter that word enough? If so, I'm sure I read SchemeIsCryptographic enough to solve that problem!) https://codereview.chromium.org/1131963003/diff/20001/net/http/http_network_t... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/1131963003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:593: return request_->url.SchemeIsCryptographic(); I'm not sure about this change. Has someone walked up the stack to see if IsSecureRequest is expected to return true for file:// URLs?
I suppose the ship hasn't sailed on naming SchemeIsCryptographic(), but a refactor would cause a bit of annoying blame churn. "Cryptographic" is a sufficiently common adjective (http://en.wikipedia.org/wiki/Cryptographic_protocol) that I didn't find it objectionable. https://codereview.chromium.org/1131963003/diff/20001/net/http/http_network_t... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/1131963003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:593: return request_->url.SchemeIsCryptographic(); On 2015/05/07 23:12:03, Ryan Hamilton wrote: > I'm not sure about this change. Has someone walked up the stack to see if > IsSecureRequest is expected to return true for file:// URLs? It's used in two places in the same function, HttpNetworkTransaction::DoReadHeadersComplete(): - As a DCHECK when client certs are needed [1]. - As a check before getting SSL info [2]. I think it's safe to say that this this is only relevant to HTTPS. [1] https://chromium.googlesource.com/chromium/src/+/ca9d6916c5fedf6f0ee73dfc397c... [2] https://chromium.googlesource.com/chromium/src/+/ca9d6916c5fedf6f0ee73dfc397c...
rsleevi@chromium.org changed reviewers: + asanka@chromium.org
Somehow I forgot to add Asanka in my original request for comments. I've significantly reworded your description to try to concisely explain the motivation and change. I appreciate the thoroughness you had, but that was far more overkill than needed :) Is there a BUG tracking this work? That can further capture the motivations and thinking? Even for cleanup bugs, we prefer to capture such tasks for future reference, since it can be quite a bit to wade through CLs to figure out each of the reasons discussed.
On 2015/05/08 23:05:17, Ryan Sleevi wrote: > Somehow I forgot to add Asanka in my original request for comments. > > I've significantly reworded your description to try to concisely explain the > motivation and change. I appreciate the thoroughness you had, but that was far > more overkill than needed :) > > Is there a BUG tracking this work? That can further capture the motivations and > thinking? Even for cleanup bugs, we prefer to capture such tasks for future > reference, since it can be quite a bit to wade through CLs to figure out each of > the reasons discussed. The SchemeIsCryptographic change was attached to 362214, so I've been using that one.
My comments are nits about naming and whether this change captures the intent in SDCH code (directed at rdsmith@). Since comment #4 wasn't clear, change from GURL::SchemeIsSecure() -> GURL::SchemeIsCryptographic() shouldn't affect how we treat file: URLs. The functional difference only affects filesystem: URLs whose inner URLs are either 'https' or 'wss', which are now considered not secure by //net code. https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.c... net/base/sdch_manager.cc:189: if (!secure_scheme_supported() && url.SchemeIsCryptographic()) This is a bit confusing, but the intent here appears to be to disable SDCH for everything other than HTTP. If so, I think we should make that explicit. I.e.: - rename secure_scheme_supported() to something like support_http_only() - change the condition to url.SchemeIs(url::kHttpScheme) Here and elsewhere. +rdsmith to verify. https://codereview.chromium.org/1131963003/diff/20001/net/http/http_network_t... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/1131963003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:593: return request_->url.SchemeIsCryptographic(); On 2015/05/08 at 01:53:30, lgarron wrote: > On 2015/05/07 23:12:03, Ryan Hamilton wrote: > > I'm not sure about this change. Has someone walked up the stack to see if > > IsSecureRequest is expected to return true for file:// URLs? > > It's used in two places in the same function, HttpNetworkTransaction::DoReadHeadersComplete(): > > - As a DCHECK when client certs are needed [1]. > - As a check before getting SSL info [2]. > > I think it's safe to say that this this is only relevant to HTTPS. > > [1] https://chromium.googlesource.com/chromium/src/+/ca9d6916c5fedf6f0ee73dfc397c... > [2] https://chromium.googlesource.com/chromium/src/+/ca9d6916c5fedf6f0ee73dfc397c... I'm tempted to say we should rename this fairly visible method to match the test being performed. E.g. IsSecureRequest -> IsRequestedOverEncryptedChannel(). I suck at naming things.
https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.c... net/base/sdch_manager.cc:189: if (!secure_scheme_supported() && url.SchemeIsCryptographic()) On 2015/05/11 22:00:46, asanka wrote: > This is a bit confusing, but the intent here appears to be to disable SDCH for > everything other than HTTP. If so, I think we should make that explicit. I.e.: > > - rename secure_scheme_supported() to something like support_http_only() > - change the condition to url.SchemeIs(url::kHttpScheme) > > Here and elsewhere. > > +rdsmith to verify. I started splitting this into a separate CL, but I wasn't sure how to deal with replacing secure_scheme_supported with its negative. static void EnableSecureSchemeSupport(bool enabled); [1] is called in several places. What would you think of preserving EnableSecureSchemeSupport but replacing secure_scheme_supported with support_http_only. [1] https://code.google.com/p/chromium/codesearch#search/&q=EnableSecureSchemeSup... https://codereview.chromium.org/1131963003/diff/20001/net/http/http_network_t... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/1131963003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:593: return request_->url.SchemeIsCryptographic(); On 2015/05/11 22:00:46, asanka wrote: > On 2015/05/08 at 01:53:30, lgarron wrote: > > On 2015/05/07 23:12:03, Ryan Hamilton wrote: > > > I'm not sure about this change. Has someone walked up the stack to see if > > > IsSecureRequest is expected to return true for file:// URLs? > > > > It's used in two places in the same function, > HttpNetworkTransaction::DoReadHeadersComplete(): > > > > - As a DCHECK when client certs are needed [1]. > > - As a check before getting SSL info [2]. > > > > I think it's safe to say that this this is only relevant to HTTPS. > > > > [1] > https://chromium.googlesource.com/chromium/src/+/ca9d6916c5fedf6f0ee73dfc397c... > > [2] > https://chromium.googlesource.com/chromium/src/+/ca9d6916c5fedf6f0ee73dfc397c... > > I'm tempted to say we should rename this fairly visible method to match the test > being performed. E.g. IsSecureRequest -> IsRequestedOverEncryptedChannel(). I > suck at naming things. IsEncryptedRequest()?
rdsmith@chromium.org changed reviewers: + rdsmith@chromium.org
My opinion below, but Elly's the right person to be asking for authoritative SDCH answers at this point. CCing her. https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.c... net/base/sdch_manager.cc:189: if (!secure_scheme_supported() && url.SchemeIsCryptographic()) On 2015/05/11 22:00:46, asanka wrote: > This is a bit confusing, but the intent here appears to be to disable SDCH for > everything other than HTTP. If so, I think we should make that explicit. I.e.: > > - rename secure_scheme_supported() to something like support_http_only() > - change the condition to url.SchemeIs(url::kHttpScheme) > > Here and elsewhere. > > +rdsmith to verify. Yes, that's the intent. I'm not sure the conditioning is needed anymore, though; that condition was in place while we were tentatively rolling SDCH out first for HTTP then for HTTPS; it might be reasonable to eliminate the ability to distinguish between the two schemes in the context of SDCH at this point.
ellyjones@chromium.org changed reviewers: + ellyjones@chromium.org
https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.c... net/base/sdch_manager.cc:189: if (!secure_scheme_supported() && url.SchemeIsCryptographic()) On 2015/05/12 14:08:04, rdsmith wrote: > On 2015/05/11 22:00:46, asanka wrote: > > This is a bit confusing, but the intent here appears to be to disable SDCH for > > everything other than HTTP. If so, I think we should make that explicit. I.e.: > > > > - rename secure_scheme_supported() to something like support_http_only() > > - change the condition to url.SchemeIs(url::kHttpScheme) > > > > Here and elsewhere. > > > > +rdsmith to verify. > > Yes, that's the intent. I'm not sure the conditioning is needed anymore, > though; that condition was in place while we were tentatively rolling SDCH out > first for HTTP then for HTTPS; it might be reasonable to eliminate the ability > to distinguish between the two schemes in the context of SDCH at this point. I would drop support for disabling SDCH for secure schemes (ie, remove SDCH's awareness of whether the protocol is secure/cryptographic or not).
https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.c... net/base/sdch_manager.cc:189: if (!secure_scheme_supported() && url.SchemeIsCryptographic()) On 2015/05/11 at 23:23:47, lgarron wrote: > On 2015/05/11 22:00:46, asanka wrote: > > This is a bit confusing, but the intent here appears to be to disable SDCH for > > everything other than HTTP. If so, I think we should make that explicit. I.e.: > > > > - rename secure_scheme_supported() to something like support_http_only() > > - change the condition to url.SchemeIs(url::kHttpScheme) > > > > Here and elsewhere. > > > > +rdsmith to verify. > > I started splitting this into a separate CL, but I wasn't sure how to deal with replacing secure_scheme_supported with its negative. > > static void EnableSecureSchemeSupport(bool enabled); [1] is called in several places. What would you think of preserving EnableSecureSchemeSupport but replacing secure_scheme_supported with support_http_only. > > [1] https://code.google.com/p/chromium/codesearch#search/&q=EnableSecureSchemeSup... I'd wait till rdsmith confirms whether the suggested change matches the intent. Regarding the callers: The only non-test caller is io_thread.cc which calls it to disable secure scheme support when the SDCH Finch trial is "EnabledHttpOnly*". I think changing the semantics of the call to enable SDCH over HTTP only is consistent with the intent of the callsite. https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.c... net/base/sdch_manager.cc:189: if (!secure_scheme_supported() && url.SchemeIsCryptographic()) On 2015/05/12 at 15:26:31, Elly Jones wrote: > On 2015/05/12 14:08:04, rdsmith wrote: > > On 2015/05/11 22:00:46, asanka wrote: > > > This is a bit confusing, but the intent here appears to be to disable SDCH for > > > everything other than HTTP. If so, I think we should make that explicit. I.e.: > > > > > > - rename secure_scheme_supported() to something like support_http_only() > > > - change the condition to url.SchemeIs(url::kHttpScheme) > > > > > > Here and elsewhere. > > > > > > +rdsmith to verify. > > > > Yes, that's the intent. I'm not sure the conditioning is needed anymore, > > though; that condition was in place while we were tentatively rolling SDCH out > > first for HTTP then for HTTPS; it might be reasonable to eliminate the ability > > to distinguish between the two schemes in the context of SDCH at this point. > > I would drop support for disabling SDCH for secure schemes (ie, remove SDCH's awareness of whether the protocol is secure/cryptographic or not). ellyjones, rdsmith: Cool. Thanks for confirming! I feel that a change to make SDCH support unaware of the properties of the transport is likely a bit involved, and should probably be handled by //net folks. lgarron is welcome to take a whack at it, but I don't feel right about pulling him into doing the work. lgarron: SchemeIsSecure() and SchemeIsCryptographic() are semantically equivalent in the proposed changes to SDCH. If that unblocks your SchemeIsSecure() cleanup, then AFAIAC you can land the changes as proposed and then a follow-up CL (either by you or someone else) can remove the checks entirely along with associated cleanup. ... this is assuming ellyjones is okay with this plan. https://codereview.chromium.org/1131963003/diff/20001/net/http/http_network_t... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/1131963003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:593: return request_->url.SchemeIsCryptographic(); On 2015/05/11 at 23:23:47, lgarron wrote: > On 2015/05/11 22:00:46, asanka wrote: > > On 2015/05/08 at 01:53:30, lgarron wrote: > > > On 2015/05/07 23:12:03, Ryan Hamilton wrote: > > > > I'm not sure about this change. Has someone walked up the stack to see if > > > > IsSecureRequest is expected to return true for file:// URLs? > > > > > > It's used in two places in the same function, > > HttpNetworkTransaction::DoReadHeadersComplete(): > > > > > > - As a DCHECK when client certs are needed [1]. > > > - As a check before getting SSL info [2]. > > > > > > I think it's safe to say that this this is only relevant to HTTPS. > > > > > > [1] > > https://chromium.googlesource.com/chromium/src/+/ca9d6916c5fedf6f0ee73dfc397c... > > > [2] > > https://chromium.googlesource.com/chromium/src/+/ca9d6916c5fedf6f0ee73dfc397c... > > > > I'm tempted to say we should rename this fairly visible method to match the test > > being performed. E.g. IsSecureRequest -> IsRequestedOverEncryptedChannel(). I > > suck at naming things. > > IsEncryptedRequest()? Sure.
On 2015/05/12 17:00:17, asanka wrote: > https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.cc > File net/base/sdch_manager.cc (right): > > https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.c... > net/base/sdch_manager.cc:189: if (!secure_scheme_supported() && > url.SchemeIsCryptographic()) > On 2015/05/11 at 23:23:47, lgarron wrote: > > On 2015/05/11 22:00:46, asanka wrote: > > > This is a bit confusing, but the intent here appears to be to disable SDCH > for > > > everything other than HTTP. If so, I think we should make that explicit. > I.e.: > > > > > > - rename secure_scheme_supported() to something like support_http_only() > > > - change the condition to url.SchemeIs(url::kHttpScheme) > > > > > > Here and elsewhere. > > > > > > +rdsmith to verify. > > > > I started splitting this into a separate CL, but I wasn't sure how to deal > with replacing secure_scheme_supported with its negative. > > > > static void EnableSecureSchemeSupport(bool enabled); [1] is called in several > places. What would you think of preserving EnableSecureSchemeSupport but > replacing secure_scheme_supported with support_http_only. > > > > [1] > https://code.google.com/p/chromium/codesearch#search/&q=EnableSecureSchemeSup... > > I'd wait till rdsmith confirms whether the suggested change matches the intent. s/rdsmith/ellyjones/. > > Regarding the callers: The only non-test caller is io_thread.cc which calls it > to disable secure scheme support when the SDCH Finch trial is > "EnabledHttpOnly*". I think changing the semantics of the call to enable SDCH > over HTTP only is consistent with the intent of the callsite. > > https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.c... > net/base/sdch_manager.cc:189: if (!secure_scheme_supported() && > url.SchemeIsCryptographic()) > On 2015/05/12 at 15:26:31, Elly Jones wrote: > > On 2015/05/12 14:08:04, rdsmith wrote: > > > On 2015/05/11 22:00:46, asanka wrote: > > > > This is a bit confusing, but the intent here appears to be to disable SDCH > for > > > > everything other than HTTP. If so, I think we should make that explicit. > I.e.: > > > > > > > > - rename secure_scheme_supported() to something like support_http_only() > > > > - change the condition to url.SchemeIs(url::kHttpScheme) > > > > > > > > Here and elsewhere. > > > > > > > > +rdsmith to verify. > > > > > > Yes, that's the intent. I'm not sure the conditioning is needed anymore, > > > though; that condition was in place while we were tentatively rolling SDCH > out > > > first for HTTP then for HTTPS; it might be reasonable to eliminate the > ability > > > to distinguish between the two schemes in the context of SDCH at this point. > > > > I would drop support for disabling SDCH for secure schemes (ie, remove SDCH's > awareness of whether the protocol is secure/cryptographic or not). > > ellyjones, rdsmith: Cool. Thanks for confirming! > > I feel that a change to make SDCH support unaware of the properties of the > transport is likely a bit involved, and should probably be handled by //net > folks. lgarron is welcome to take a whack at it, but I don't feel right about > pulling him into doing the work. > > lgarron: SchemeIsSecure() and SchemeIsCryptographic() are semantically > equivalent in the proposed changes to SDCH. If that unblocks your > SchemeIsSecure() cleanup, then AFAIAC you can land the changes as proposed and > then a follow-up CL (either by you or someone else) can remove the checks > entirely along with associated cleanup. > > ... this is assuming ellyjones is okay with this plan. > > https://codereview.chromium.org/1131963003/diff/20001/net/http/http_network_t... > File net/http/http_network_transaction.cc (right): > > https://codereview.chromium.org/1131963003/diff/20001/net/http/http_network_t... > net/http/http_network_transaction.cc:593: return > request_->url.SchemeIsCryptographic(); > On 2015/05/11 at 23:23:47, lgarron wrote: > > On 2015/05/11 22:00:46, asanka wrote: > > > On 2015/05/08 at 01:53:30, lgarron wrote: > > > > On 2015/05/07 23:12:03, Ryan Hamilton wrote: > > > > > I'm not sure about this change. Has someone walked up the stack to see > if > > > > > IsSecureRequest is expected to return true for file:// URLs? > > > > > > > > It's used in two places in the same function, > > > HttpNetworkTransaction::DoReadHeadersComplete(): > > > > > > > > - As a DCHECK when client certs are needed [1]. > > > > - As a check before getting SSL info [2]. > > > > > > > > I think it's safe to say that this this is only relevant to HTTPS. > > > > > > > > [1] > > > > https://chromium.googlesource.com/chromium/src/+/ca9d6916c5fedf6f0ee73dfc397c... > > > > [2] > > > > https://chromium.googlesource.com/chromium/src/+/ca9d6916c5fedf6f0ee73dfc397c... > > > > > > I'm tempted to say we should rename this fairly visible method to match the > test > > > being performed. E.g. IsSecureRequest -> IsRequestedOverEncryptedChannel(). > I > > > suck at naming things. > > > > IsEncryptedRequest()? > > Sure.
On 2015/05/12 17:38:09, rdsmith wrote: > On 2015/05/12 17:00:17, asanka wrote: > > https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.cc > > File net/base/sdch_manager.cc (right): > > > > > https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.c... > > net/base/sdch_manager.cc:189: if (!secure_scheme_supported() && > > url.SchemeIsCryptographic()) > > On 2015/05/11 at 23:23:47, lgarron wrote: > > > On 2015/05/11 22:00:46, asanka wrote: > > > > This is a bit confusing, but the intent here appears to be to disable SDCH > > for > > > > everything other than HTTP. If so, I think we should make that explicit. > > I.e.: > > > > > > > > - rename secure_scheme_supported() to something like support_http_only() > > > > - change the condition to url.SchemeIs(url::kHttpScheme) > > > > > > > > Here and elsewhere. > > > > > > > > +rdsmith to verify. > > > > > > I started splitting this into a separate CL, but I wasn't sure how to deal > > with replacing secure_scheme_supported with its negative. > > > > > > static void EnableSecureSchemeSupport(bool enabled); [1] is called in > several > > places. What would you think of preserving EnableSecureSchemeSupport but > > replacing secure_scheme_supported with support_http_only. > > > > > > [1] > > > https://code.google.com/p/chromium/codesearch#search/&q=EnableSecureSchemeSup... > > > > I'd wait till rdsmith confirms whether the suggested change matches the > intent. > > s/rdsmith/ellyjones/. > > > > > > Regarding the callers: The only non-test caller is io_thread.cc which calls it > > to disable secure scheme support when the SDCH Finch trial is > > "EnabledHttpOnly*". I think changing the semantics of the call to enable SDCH > > over HTTP only is consistent with the intent of the callsite. > > > > > https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.c... > > net/base/sdch_manager.cc:189: if (!secure_scheme_supported() && > > url.SchemeIsCryptographic()) > > On 2015/05/12 at 15:26:31, Elly Jones wrote: > > > On 2015/05/12 14:08:04, rdsmith wrote: > > > > On 2015/05/11 22:00:46, asanka wrote: > > > > > This is a bit confusing, but the intent here appears to be to disable > SDCH > > for > > > > > everything other than HTTP. If so, I think we should make that explicit. > > I.e.: > > > > > > > > > > - rename secure_scheme_supported() to something like > support_http_only() > > > > > - change the condition to url.SchemeIs(url::kHttpScheme) > > > > > > > > > > Here and elsewhere. > > > > > > > > > > +rdsmith to verify. > > > > > > > > Yes, that's the intent. I'm not sure the conditioning is needed anymore, > > > > though; that condition was in place while we were tentatively rolling SDCH > > out > > > > first for HTTP then for HTTPS; it might be reasonable to eliminate the > > ability > > > > to distinguish between the two schemes in the context of SDCH at this > point. > > > > > > I would drop support for disabling SDCH for secure schemes (ie, remove > SDCH's > > awareness of whether the protocol is secure/cryptographic or not). > > > > ellyjones, rdsmith: Cool. Thanks for confirming! > > > > I feel that a change to make SDCH support unaware of the properties of the > > transport is likely a bit involved, and should probably be handled by //net > > folks. lgarron is welcome to take a whack at it, but I don't feel right about > > pulling him into doing the work. > > > > lgarron: SchemeIsSecure() and SchemeIsCryptographic() are semantically > > equivalent in the proposed changes to SDCH. If that unblocks your > > SchemeIsSecure() cleanup, then AFAIAC you can land the changes as proposed and > > then a follow-up CL (either by you or someone else) can remove the checks > > entirely along with associated cleanup. > > > > ... this is assuming ellyjones is okay with this plan. > > > > > https://codereview.chromium.org/1131963003/diff/20001/net/http/http_network_t... > > File net/http/http_network_transaction.cc (right): > > > > > https://codereview.chromium.org/1131963003/diff/20001/net/http/http_network_t... > > net/http/http_network_transaction.cc:593: return > > request_->url.SchemeIsCryptographic(); > > On 2015/05/11 at 23:23:47, lgarron wrote: > > > On 2015/05/11 22:00:46, asanka wrote: > > > > On 2015/05/08 at 01:53:30, lgarron wrote: > > > > > On 2015/05/07 23:12:03, Ryan Hamilton wrote: > > > > > > I'm not sure about this change. Has someone walked up the stack to see > > if > > > > > > IsSecureRequest is expected to return true for file:// URLs? > > > > > > > > > > It's used in two places in the same function, > > > > HttpNetworkTransaction::DoReadHeadersComplete(): > > > > > > > > > > - As a DCHECK when client certs are needed [1]. > > > > > - As a check before getting SSL info [2]. > > > > > > > > > > I think it's safe to say that this this is only relevant to HTTPS. > > > > > > > > > > [1] > > > > > > > https://chromium.googlesource.com/chromium/src/+/ca9d6916c5fedf6f0ee73dfc397c... > > > > > [2] > > > > > > > https://chromium.googlesource.com/chromium/src/+/ca9d6916c5fedf6f0ee73dfc397c... > > > > > > > > I'm tempted to say we should rename this fairly visible method to match > the > > test > > > > being performed. E.g. IsSecureRequest -> > IsRequestedOverEncryptedChannel(). > > I > > > > suck at naming things. > > > > > > IsEncryptedRequest()? > > > > Sure. From what I can tell, there are a few possible followups, but nothing in particular left to do in this CL. Is that right?
On 2015/05/14 18:43:12, lgarron wrote: > On 2015/05/12 17:38:09, rdsmith wrote: > > On 2015/05/12 17:00:17, asanka wrote: > > > > https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.cc > > > File net/base/sdch_manager.cc (right): > > > > > > > > > https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.c... > > > net/base/sdch_manager.cc:189: if (!secure_scheme_supported() && > > > url.SchemeIsCryptographic()) > > > On 2015/05/11 at 23:23:47, lgarron wrote: > > > > On 2015/05/11 22:00:46, asanka wrote: > > > > > This is a bit confusing, but the intent here appears to be to disable > SDCH > > > for > > > > > everything other than HTTP. If so, I think we should make that explicit. > > > I.e.: > > > > > > > > > > - rename secure_scheme_supported() to something like > support_http_only() > > > > > - change the condition to url.SchemeIs(url::kHttpScheme) > > > > > > > > > > Here and elsewhere. > > > > > > > > > > +rdsmith to verify. > > > > > > > > I started splitting this into a separate CL, but I wasn't sure how to deal > > > with replacing secure_scheme_supported with its negative. > > > > > > > > static void EnableSecureSchemeSupport(bool enabled); [1] is called in > > several > > > places. What would you think of preserving EnableSecureSchemeSupport but > > > replacing secure_scheme_supported with support_http_only. > > > > > > > > [1] > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=EnableSecureSchemeSup... > > > > > > I'd wait till rdsmith confirms whether the suggested change matches the > > intent. > > > > s/rdsmith/ellyjones/. > > > > > > > > > > Regarding the callers: The only non-test caller is io_thread.cc which calls > it > > > to disable secure scheme support when the SDCH Finch trial is > > > "EnabledHttpOnly*". I think changing the semantics of the call to enable > SDCH > > > over HTTP only is consistent with the intent of the callsite. > > > > > > > > > https://codereview.chromium.org/1131963003/diff/20001/net/base/sdch_manager.c... > > > net/base/sdch_manager.cc:189: if (!secure_scheme_supported() && > > > url.SchemeIsCryptographic()) > > > On 2015/05/12 at 15:26:31, Elly Jones wrote: > > > > On 2015/05/12 14:08:04, rdsmith wrote: > > > > > On 2015/05/11 22:00:46, asanka wrote: > > > > > > This is a bit confusing, but the intent here appears to be to disable > > SDCH > > > for > > > > > > everything other than HTTP. If so, I think we should make that > explicit. > > > I.e.: > > > > > > > > > > > > - rename secure_scheme_supported() to something like > > support_http_only() > > > > > > - change the condition to url.SchemeIs(url::kHttpScheme) > > > > > > > > > > > > Here and elsewhere. > > > > > > > > > > > > +rdsmith to verify. > > > > > > > > > > Yes, that's the intent. I'm not sure the conditioning is needed > anymore, > > > > > though; that condition was in place while we were tentatively rolling > SDCH > > > out > > > > > first for HTTP then for HTTPS; it might be reasonable to eliminate the > > > ability > > > > > to distinguish between the two schemes in the context of SDCH at this > > point. > > > > > > > > I would drop support for disabling SDCH for secure schemes (ie, remove > > SDCH's > > > awareness of whether the protocol is secure/cryptographic or not). > > > > > > ellyjones, rdsmith: Cool. Thanks for confirming! > > > > > > I feel that a change to make SDCH support unaware of the properties of the > > > transport is likely a bit involved, and should probably be handled by //net > > > folks. lgarron is welcome to take a whack at it, but I don't feel right > about > > > pulling him into doing the work. > > > > > > lgarron: SchemeIsSecure() and SchemeIsCryptographic() are semantically > > > equivalent in the proposed changes to SDCH. If that unblocks your > > > SchemeIsSecure() cleanup, then AFAIAC you can land the changes as proposed > and > > > then a follow-up CL (either by you or someone else) can remove the checks > > > entirely along with associated cleanup. > > > > > > ... this is assuming ellyjones is okay with this plan. > > > > > > > > > https://codereview.chromium.org/1131963003/diff/20001/net/http/http_network_t... > > > File net/http/http_network_transaction.cc (right): > > > > > > > > > https://codereview.chromium.org/1131963003/diff/20001/net/http/http_network_t... > > > net/http/http_network_transaction.cc:593: return > > > request_->url.SchemeIsCryptographic(); > > > On 2015/05/11 at 23:23:47, lgarron wrote: > > > > On 2015/05/11 22:00:46, asanka wrote: > > > > > On 2015/05/08 at 01:53:30, lgarron wrote: > > > > > > On 2015/05/07 23:12:03, Ryan Hamilton wrote: > > > > > > > I'm not sure about this change. Has someone walked up the stack to > see > > > if > > > > > > > IsSecureRequest is expected to return true for file:// URLs? > > > > > > > > > > > > It's used in two places in the same function, > > > > > HttpNetworkTransaction::DoReadHeadersComplete(): > > > > > > > > > > > > - As a DCHECK when client certs are needed [1]. > > > > > > - As a check before getting SSL info [2]. > > > > > > > > > > > > I think it's safe to say that this this is only relevant to HTTPS. > > > > > > > > > > > > [1] > > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/ca9d6916c5fedf6f0ee73dfc397c... > > > > > > [2] > > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/ca9d6916c5fedf6f0ee73dfc397c... > > > > > > > > > > I'm tempted to say we should rename this fairly visible method to match > > the > > > test > > > > > being performed. E.g. IsSecureRequest -> > > IsRequestedOverEncryptedChannel(). > > > I > > > > > suck at naming things. > > > > > > > > IsEncryptedRequest()? > > > > > > Sure. > > From what I can tell, there are a few possible followups, but nothing in > particular left to do in this CL. Is that right? Looks right to me.
Consensus is LGTM
The CQ bit was checked by lgarron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1131963003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a774b92283e458a9bc77ba0ce49777b74cde493a Cr-Commit-Position: refs/heads/master@{#329966} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
