Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2214793002:
Remove unnecessary SSLRequestInfo class (Closed)
DescriptionRemove unnecessary SSLRequestInfo class
The SSLRequestInfo class was serving only to pass a few pieces of
information into one function in SSLPolicy and does not seem at all
necessary. (I'm guessing it used to serve some purpose, but no
longer.) This CL removes SSLRequestInfo and changes the
SSLPolicy::OnRequestStarted() signature to simply take the few details
it needs about the request (URL, cert id, and cert status).
BUG=558491
Committed: https://crrev.com/23f4b974f96fa3cd8abc0a5da1fc3ac3a3e2359d
Cr-Commit-Position: refs/heads/master@{#409836}
Patch Set 1 #
Total comments: 5
Messages
Total messages: 23 (10 generated)
Description was changed from ========== Remove unnecessary SSLRequestInfo class The SSLRequestInfo class was serving only to pass a few pieces of information into one function in SSLPolicy and does not seem at all necessary. (I'm guessing it used to serve some purpose, but no longer). This CL removes SSLRequestInfo and changes the SSLPolicy::OnRequestStarted() signature to simply take the few details it needs about the request (URL, cert id, and cert status). BUG=558491 ========== to ========== Remove unnecessary SSLRequestInfo class The SSLRequestInfo class was serving only to pass a few pieces of information into one function in SSLPolicy and does not seem at all necessary. (I'm guessing it used to serve some purpose, but no longer.) This CL removes SSLRequestInfo and changes the SSLPolicy::OnRequestStarted() signature to simply take the few details it needs about the request (URL, cert id, and cert status). BUG=558491 ==========
The CQ bit was checked by estark@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...
estark@chromium.org changed reviewers: + felt@chromium.org
felt, PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
+jww with a BONUS QUESTION! https://codereview.chromium.org/2214793002/diff/1/content/browser/ssl/ssl_man... File content/browser/ssl/ssl_manager.cc (left): https://codereview.chromium.org/2214793002/diff/1/content/browser/ssl/ssl_man... content/browser/ssl/ssl_manager.cc:150: // resouces aren't cachable. I'm puzzled by this part. Someone, at some point, thought it was important to note that the resource type matters for the policy. But, obviously we are not checking the resource type now. I'd like to be sure that isn't a bug. I can't find anything in recent history where it looks like we would need to check the resource type though... so it seems OK. Did you look into this? https://codereview.chromium.org/2214793002/diff/1/content/browser/ssl/ssl_man... content/browser/ssl/ssl_manager.cc:158: policy()->OnRequestStarted(info.get()); jww: Is it correct that DidLoadFromMemoryCache is triggering the logic in OnRequestStarted? Let's say the following happens: Monday: cert is valid. subresource is cached. Tuesday: cert expires. user sees and clicks through error. Wednesday: user goes to a page that loads the cached subresource. That will remove the exception from Tuesday based on the cached subresource, which seems kind of weird.
https://codereview.chromium.org/2214793002/diff/1/content/browser/ssl/ssl_man... File content/browser/ssl/ssl_manager.cc (left): https://codereview.chromium.org/2214793002/diff/1/content/browser/ssl/ssl_man... content/browser/ssl/ssl_manager.cc:150: // resouces aren't cachable. On 2016/08/04 17:16:09, felt wrote: > I'm puzzled by this part. Someone, at some point, thought it was important to > note that the resource type matters for the policy. > > But, obviously we are not checking the resource type now. I'd like to be sure > that isn't a bug. I can't find anything in recent history where it looks like we > would need to check the resource type though... so it seems OK. > > Did you look into this? My guess is that this dates back to, oh, 2009 or so, when this class did a check for mixed content. https://codereview.chromium.org/113391/ is what I looked at -- in that CL, SSLPolicy checks for mixed content using the resource type. In the history of Chrome that I've fabricated in my mind, at some point the mixed content check moved out of here and into Blink (where it lives now) because more context about the resource load was needed as the definition of mixed content evolved.
lgtm (although jww, can you still take a look at the question I left? it's orthogonal to this CL but would be good to get your opinion) https://codereview.chromium.org/2214793002/diff/1/content/browser/ssl/ssl_man... File content/browser/ssl/ssl_manager.cc (left): https://codereview.chromium.org/2214793002/diff/1/content/browser/ssl/ssl_man... content/browser/ssl/ssl_manager.cc:150: // resouces aren't cachable. On 2016/08/04 17:25:04, estark wrote: > On 2016/08/04 17:16:09, felt wrote: > > I'm puzzled by this part. Someone, at some point, thought it was important to > > note that the resource type matters for the policy. > > > > But, obviously we are not checking the resource type now. I'd like to be sure > > that isn't a bug. I can't find anything in recent history where it looks like > we > > would need to check the resource type though... so it seems OK. > > > > Did you look into this? > > My guess is that this dates back to, oh, 2009 or so, when this class did a check > for mixed content. https://codereview.chromium.org/113391/ is what I looked at > -- in that CL, SSLPolicy checks for mixed content using the resource type. In > the history of Chrome that I've fabricated in my mind, at some point the mixed > content check moved out of here and into Blink (where it lives now) because more > context about the resource load was needed as the definition of mixed content > evolved. AHhh yes, that history makes sense.
Thanks felt. I'm going to submit this now but will look out for jww's answer to the bonus question. (As you suggested, if we do want to do something differently there, it should probably be in a different CL anyway.)
The CQ bit was checked by estark@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Remove unnecessary SSLRequestInfo class The SSLRequestInfo class was serving only to pass a few pieces of information into one function in SSLPolicy and does not seem at all necessary. (I'm guessing it used to serve some purpose, but no longer.) This CL removes SSLRequestInfo and changes the SSLPolicy::OnRequestStarted() signature to simply take the few details it needs about the request (URL, cert id, and cert status). BUG=558491 ========== to ========== Remove unnecessary SSLRequestInfo class The SSLRequestInfo class was serving only to pass a few pieces of information into one function in SSLPolicy and does not seem at all necessary. (I'm guessing it used to serve some purpose, but no longer.) This CL removes SSLRequestInfo and changes the SSLPolicy::OnRequestStarted() signature to simply take the few details it needs about the request (URL, cert id, and cert status). BUG=558491 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Remove unnecessary SSLRequestInfo class The SSLRequestInfo class was serving only to pass a few pieces of information into one function in SSLPolicy and does not seem at all necessary. (I'm guessing it used to serve some purpose, but no longer.) This CL removes SSLRequestInfo and changes the SSLPolicy::OnRequestStarted() signature to simply take the few details it needs about the request (URL, cert id, and cert status). BUG=558491 ========== to ========== Remove unnecessary SSLRequestInfo class The SSLRequestInfo class was serving only to pass a few pieces of information into one function in SSLPolicy and does not seem at all necessary. (I'm guessing it used to serve some purpose, but no longer.) This CL removes SSLRequestInfo and changes the SSLPolicy::OnRequestStarted() signature to simply take the few details it needs about the request (URL, cert id, and cert status). BUG=558491 Committed: https://crrev.com/23f4b974f96fa3cd8abc0a5da1fc3ac3a3e2359d Cr-Commit-Position: refs/heads/master@{#409836} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/23f4b974f96fa3cd8abc0a5da1fc3ac3a3e2359d Cr-Commit-Position: refs/heads/master@{#409836}
Message was sent while issue was closed.
jww@chromium.org changed reviewers: + jww@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2214793002/diff/1/content/browser/ssl/ssl_man... File content/browser/ssl/ssl_manager.cc (left): https://codereview.chromium.org/2214793002/diff/1/content/browser/ssl/ssl_man... content/browser/ssl/ssl_manager.cc:158: policy()->OnRequestStarted(info.get()); On 2016/08/04 17:16:09, felt wrote: > jww: Is it correct that DidLoadFromMemoryCache is triggering the logic in > OnRequestStarted? Let's say the following happens: > > Monday: cert is valid. subresource is cached. > Tuesday: cert expires. user sees and clicks through error. > Wednesday: user goes to a page that loads the cached subresource. > > That will remove the exception from Tuesday based on the cached subresource, > which seems kind of weird. It seems very possible that this would happen. We should probably have a browser test to verify? Not sure how to write it, though. I think it would be totally reasonable to have an explicit exemption of the "revoke on good cert seen" for memory cache loaded resources, since the "good" cert is old, and its not that we've seen the *server* issue a valid cert. Maybe we should add a "bool cached_" member to SSLRequestInfo to track this?
Message was sent while issue was closed.
On 2016/08/04 18:18:10, jww wrote: > https://codereview.chromium.org/2214793002/diff/1/content/browser/ssl/ssl_man... > File content/browser/ssl/ssl_manager.cc (left): > > https://codereview.chromium.org/2214793002/diff/1/content/browser/ssl/ssl_man... > content/browser/ssl/ssl_manager.cc:158: policy()->OnRequestStarted(info.get()); > On 2016/08/04 17:16:09, felt wrote: > > jww: Is it correct that DidLoadFromMemoryCache is triggering the logic in > > OnRequestStarted? Let's say the following happens: > > > > Monday: cert is valid. subresource is cached. > > Tuesday: cert expires. user sees and clicks through error. > > Wednesday: user goes to a page that loads the cached subresource. > > > > That will remove the exception from Tuesday based on the cached subresource, > > which seems kind of weird. > > It seems very possible that this would happen. We should probably have a browser > test to verify? Not sure how to write it, though. > > I think it would be totally reasonable to have an explicit exemption of the > "revoke on good cert seen" for memory cache loaded resources, since the "good" > cert is old, and its not that we've seen the *server* issue a valid cert. Maybe > we should add a "bool cached_" member to SSLRequestInfo to track this? If we don't want to revoke decisions on good certs seen on memory cached resources, can we just delete SSLPolicy::DidLoadFromMemoryCache() all together? It looks like the only purpose that method serves is to revoke the decisions if the resource loaded from the cache has a good cert.
Message was sent while issue was closed.
On 2016/08/04 18:21:15, estark wrote: > On 2016/08/04 18:18:10, jww wrote: > > > https://codereview.chromium.org/2214793002/diff/1/content/browser/ssl/ssl_man... > > File content/browser/ssl/ssl_manager.cc (left): > > > > > https://codereview.chromium.org/2214793002/diff/1/content/browser/ssl/ssl_man... > > content/browser/ssl/ssl_manager.cc:158: > policy()->OnRequestStarted(info.get()); > > On 2016/08/04 17:16:09, felt wrote: > > > jww: Is it correct that DidLoadFromMemoryCache is triggering the logic in > > > OnRequestStarted? Let's say the following happens: > > > > > > Monday: cert is valid. subresource is cached. > > > Tuesday: cert expires. user sees and clicks through error. > > > Wednesday: user goes to a page that loads the cached subresource. > > > > > > That will remove the exception from Tuesday based on the cached subresource, > > > which seems kind of weird. > > > > It seems very possible that this would happen. We should probably have a > browser > > test to verify? Not sure how to write it, though. > > > > I think it would be totally reasonable to have an explicit exemption of the > > "revoke on good cert seen" for memory cache loaded resources, since the "good" > > cert is old, and its not that we've seen the *server* issue a valid cert. > Maybe > > we should add a "bool cached_" member to SSLRequestInfo to track this? > > If we don't want to revoke decisions on good certs seen on memory cached > resources, can we just delete SSLPolicy::DidLoadFromMemoryCache() all together? > It looks like the only purpose that method serves is to revoke the decisions if > the resource loaded from the cache has a good cert. Offhand, that makes sense to me.
Message was sent while issue was closed.
On 2016/08/04 19:00:42, jww wrote: > On 2016/08/04 18:21:15, estark wrote: > > On 2016/08/04 18:18:10, jww wrote: > > > > > > https://codereview.chromium.org/2214793002/diff/1/content/browser/ssl/ssl_man... > > > File content/browser/ssl/ssl_manager.cc (left): > > > > > > > > > https://codereview.chromium.org/2214793002/diff/1/content/browser/ssl/ssl_man... > > > content/browser/ssl/ssl_manager.cc:158: > > policy()->OnRequestStarted(info.get()); > > > On 2016/08/04 17:16:09, felt wrote: > > > > jww: Is it correct that DidLoadFromMemoryCache is triggering the logic in > > > > OnRequestStarted? Let's say the following happens: > > > > > > > > Monday: cert is valid. subresource is cached. > > > > Tuesday: cert expires. user sees and clicks through error. > > > > Wednesday: user goes to a page that loads the cached subresource. > > > > > > > > That will remove the exception from Tuesday based on the cached > subresource, > > > > which seems kind of weird. > > > > > > It seems very possible that this would happen. We should probably have a > > browser > > > test to verify? Not sure how to write it, though. > > > > > > I think it would be totally reasonable to have an explicit exemption of the > > > "revoke on good cert seen" for memory cache loaded resources, since the > "good" > > > cert is old, and its not that we've seen the *server* issue a valid cert. > > Maybe > > > we should add a "bool cached_" member to SSLRequestInfo to track this? > > > > If we don't want to revoke decisions on good certs seen on memory cached > > resources, can we just delete SSLPolicy::DidLoadFromMemoryCache() all > together? > > It looks like the only purpose that method serves is to revoke the decisions > if > > the resource loaded from the cache has a good cert. > > Offhand, that makes sense to me. Yes, that is what I was suggesting also (delete DidLoadFrommemoryCache if we don't actually want that behavior).
Message was sent while issue was closed.
On 2016/08/04 19:34:14, felt wrote: > On 2016/08/04 19:00:42, jww wrote: > > On 2016/08/04 18:21:15, estark wrote: > > > On 2016/08/04 18:18:10, jww wrote: > > > > > > > > > > https://codereview.chromium.org/2214793002/diff/1/content/browser/ssl/ssl_man... > > > > File content/browser/ssl/ssl_manager.cc (left): > > > > > > > > > > > > > > https://codereview.chromium.org/2214793002/diff/1/content/browser/ssl/ssl_man... > > > > content/browser/ssl/ssl_manager.cc:158: > > > policy()->OnRequestStarted(info.get()); > > > > On 2016/08/04 17:16:09, felt wrote: > > > > > jww: Is it correct that DidLoadFromMemoryCache is triggering the logic > in > > > > > OnRequestStarted? Let's say the following happens: > > > > > > > > > > Monday: cert is valid. subresource is cached. > > > > > Tuesday: cert expires. user sees and clicks through error. > > > > > Wednesday: user goes to a page that loads the cached subresource. > > > > > > > > > > That will remove the exception from Tuesday based on the cached > > subresource, > > > > > which seems kind of weird. > > > > > > > > It seems very possible that this would happen. We should probably have a > > > browser > > > > test to verify? Not sure how to write it, though. > > > > > > > > I think it would be totally reasonable to have an explicit exemption of > the > > > > "revoke on good cert seen" for memory cache loaded resources, since the > > "good" > > > > cert is old, and its not that we've seen the *server* issue a valid cert. > > > Maybe > > > > we should add a "bool cached_" member to SSLRequestInfo to track this? > > > > > > If we don't want to revoke decisions on good certs seen on memory cached > > > resources, can we just delete SSLPolicy::DidLoadFromMemoryCache() all > > together? > > > It looks like the only purpose that method serves is to revoke the decisions > > if > > > the resource loaded from the cache has a good cert. > > > > Offhand, that makes sense to me. > > Yes, that is what I was suggesting also (delete DidLoadFrommemoryCache if we > don't actually want that behavior). Ok cool. Filed https://crbug.com/634553 to track it. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
