Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2040513003:
Implement Expect-Staple (Closed)
DescriptionImplement Expect-Staple
This adds the ability to send Expect-Staple reports for hosts in the Expect-Staple preload list. OCSP staples are only parsed for hosts on this list. Reporting is currently disabled, and only enabled in tests.
BUG=598021
Patch Set 1 #
Total comments: 3
Patch Set 2 : Move into TransportSecurityState #Patch Set 3 : Start writing tests #
Total comments: 45
Patch Set 4 : Make everything less ugly. #Patch Set 5 : Remove call to GetSSLInfo #
Total comments: 20
Patch Set 6 : Address comments from estark@ #
Total comments: 2
Patch Set 7 : Don't report private certificates #
Total comments: 31
Patch Set 8 : Cleanup imports #
Total comments: 30
Patch Set 9 : Address comments from svaldez@ #Patch Set 10 : Use new der::GeneralizedTime operators #
Total comments: 38
Patch Set 11 : Move OCSP into cert_verify_proc #
Total comments: 17
Messages
Total messages: 36 (6 generated)
Description was changed from ========== Scaffolding for sending Expect-Staple reports BUG=598021 ========== to ========== Scaffolding for sending Expect-Staple reports BUG=598021 ==========
dadrian@google.com changed reviewers: + estark@chromium.org
This isn't intended to be merged, just looking for some thoughts. Also, I might have screwed up the dependent CL, this is assuming https://codereview.chromium.org/2034843003/ lands. https://codereview.chromium.org/2040513003/diff/1/chrome/browser/ssl/chrome_e... File chrome/browser/ssl/chrome_expect_staple_reporter.cc (right): https://codereview.chromium.org/2040513003/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_staple_reporter.cc:33: DCHECK(report_sender_); So, a lot of this function is similar to ChromeExpectCTReport::OnExpectCTFailed. In fact, it would use pretty much every helper function defined in the anonymous namespace inside of chrome_expect_ct_reporter.cc. This raises the question of how to share those implementation details. Do we: 1. Merge the ExpectStapleReporter and ExpectCTReporter interfaces? 2. Have ChromeExpectCTReporter implement both interfaces? 3. Share a base class that implements both interfaces? (ew) 4. Pull out all the helper functions into some utility file? 5. Decide everything is wrong, and I go home and hold my head in shame?
https://codereview.chromium.org/2040513003/diff/1/chrome/browser/ssl/chrome_e... File chrome/browser/ssl/chrome_expect_staple_reporter.cc (right): https://codereview.chromium.org/2040513003/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_staple_reporter.cc:33: DCHECK(report_sender_); On 2016/06/03 19:05:18, dadrian wrote: > So, a lot of this function is similar to ChromeExpectCTReport::OnExpectCTFailed. > In fact, it would use pretty much every helper function defined in the anonymous > namespace inside of chrome_expect_ct_reporter.cc. This raises the question of > how to share those implementation details. Do we: > > 1. Merge the ExpectStapleReporter and ExpectCTReporter interfaces? > 2. Have ChromeExpectCTReporter implement both interfaces? > 3. Share a base class that implements both interfaces? (ew) > 4. Pull out all the helper functions into some utility file? > 5. Decide everything is wrong, and I go home and hold my head in shame? Hmmm. If we do things this way, my vote would be for #4. Though, I was imagining that we would do all the Expect-Staple reporting in //net, probably right in TransportSecurityState itself, similar to how HPKP reporting works in TransportSecurityState::CheckPinsAndMaybeSendReport(). The reason that the Expect-CT implementation is in //chrome is that Expect-CT is pretty Googley. This was my reasoning: "I did it this way (ChromeExpectCTReporter instead of all the logic in //net) because, initially, Expect CT is going to be a pretty Googley feature while it's in a sort of prototype phase. We're not yet specifying or standardizing it any remotely formal way, and initially, we're only going to allow reports to be sent to Google servers (so that we can, for example, tweak the policies and reporting logic without worrying about DoSing other site owners). I was thinking that other embedders would probably not want this feature in its current immature state, and the more report-building and -sending logic we include in //net, the more wasteful this feature will be for them." So, while Expect-Staple is pretty experimental, it's not as tied to Google in that site owners will be sending reports to their own endpoints, not to Google servers. (Btw, "other embedders" in this context means other users of Chrome's net stack -- Opera, for example.)
https://codereview.chromium.org/2040513003/diff/1/chrome/browser/ssl/chrome_e... File chrome/browser/ssl/chrome_expect_staple_reporter.cc (right): https://codereview.chromium.org/2040513003/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_staple_reporter.cc:33: DCHECK(report_sender_); On 2016/06/04 00:39:16, estark wrote: > On 2016/06/03 19:05:18, dadrian wrote: > > So, a lot of this function is similar to > ChromeExpectCTReport::OnExpectCTFailed. > > In fact, it would use pretty much every helper function defined in the > anonymous > > namespace inside of chrome_expect_ct_reporter.cc. This raises the question of > > how to share those implementation details. Do we: > > > > 1. Merge the ExpectStapleReporter and ExpectCTReporter interfaces? > > 2. Have ChromeExpectCTReporter implement both interfaces? > > 3. Share a base class that implements both interfaces? (ew) > > 4. Pull out all the helper functions into some utility file? > > 5. Decide everything is wrong, and I go home and hold my head in shame? > > Hmmm. If we do things this way, my vote would be for #4. Though, I was imagining > that we would do all the Expect-Staple reporting in //net, probably right in > TransportSecurityState itself, similar to how HPKP reporting works in > TransportSecurityState::CheckPinsAndMaybeSendReport(). > > The reason that the Expect-CT implementation is in //chrome is that Expect-CT is > pretty Googley. This was my reasoning: "I did it this way > (ChromeExpectCTReporter instead of all the logic in //net) because, initially, > Expect CT is going to be a pretty Googley feature while it's in a sort of > prototype phase. We're not yet specifying or standardizing it any remotely > formal way, and initially, we're only going to allow reports to be sent to > Google servers (so that we can, for example, tweak the policies and reporting > logic without worrying about DoSing other site owners). I was thinking that > other embedders would probably not want this feature in its current immature > state, and the more report-building and -sending logic we include in //net, the > more wasteful this feature will be for them." > > So, while Expect-Staple is pretty experimental, it's not as tied to Google in > that site owners will be sending reports to their own endpoints, not to Google > servers. > > (Btw, "other embedders" in this context means other users of Chrome's net stack > -- Opera, for example.) Oh, I suppose my alternative vision of the universe, in which we do all the reporting in TSS, doesn't answer your original question about duplicating code. IIRC most of the utility functions you'd need are already duplicates themselves of code in transport_security_state.cc and used for HPKP reporting, so you could use those if you do Expect-Staple in transport_security_state.cc. I might be misremembering though.
On 2016/06/04 00:40:43, estark wrote: > https://codereview.chromium.org/2040513003/diff/1/chrome/browser/ssl/chrome_e... > File chrome/browser/ssl/chrome_expect_staple_reporter.cc (right): > > https://codereview.chromium.org/2040513003/diff/1/chrome/browser/ssl/chrome_e... > chrome/browser/ssl/chrome_expect_staple_reporter.cc:33: DCHECK(report_sender_); > On 2016/06/04 00:39:16, estark wrote: > > On 2016/06/03 19:05:18, dadrian wrote: > > > So, a lot of this function is similar to > > ChromeExpectCTReport::OnExpectCTFailed. > > > In fact, it would use pretty much every helper function defined in the > > anonymous > > > namespace inside of chrome_expect_ct_reporter.cc. This raises the question > of > > > how to share those implementation details. Do we: > > > > > > 1. Merge the ExpectStapleReporter and ExpectCTReporter interfaces? > > > 2. Have ChromeExpectCTReporter implement both interfaces? > > > 3. Share a base class that implements both interfaces? (ew) > > > 4. Pull out all the helper functions into some utility file? > > > 5. Decide everything is wrong, and I go home and hold my head in shame? > > > > Hmmm. If we do things this way, my vote would be for #4. Though, I was > imagining > > that we would do all the Expect-Staple reporting in //net, probably right in > > TransportSecurityState itself, similar to how HPKP reporting works in > > TransportSecurityState::CheckPinsAndMaybeSendReport(). > > > > The reason that the Expect-CT implementation is in //chrome is that Expect-CT > is > > pretty Googley. This was my reasoning: "I did it this way > > (ChromeExpectCTReporter instead of all the logic in //net) because, initially, > > Expect CT is going to be a pretty Googley feature while it's in a sort of > > prototype phase. We're not yet specifying or standardizing it any remotely > > formal way, and initially, we're only going to allow reports to be sent to > > Google servers (so that we can, for example, tweak the policies and reporting > > logic without worrying about DoSing other site owners). I was thinking that > > other embedders would probably not want this feature in its current immature > > state, and the more report-building and -sending logic we include in //net, > the > > more wasteful this feature will be for them." > > > > So, while Expect-Staple is pretty experimental, it's not as tied to Google in > > that site owners will be sending reports to their own endpoints, not to Google > > servers. > > > > (Btw, "other embedders" in this context means other users of Chrome's net > stack > > -- Opera, for example.) > > Oh, I suppose my alternative vision of the universe, in which we do all the > reporting in TSS, doesn't answer your original question about duplicating code. > IIRC most of the utility functions you'd need are already duplicates themselves > of code in transport_security_state.cc and used for HPKP reporting, so you could > use those if you do Expect-Staple in transport_security_state.cc. I might be > misremembering though. Tossing this up here just to record progress. Moving into TransportSecurityState solved most of the problems.
Description was changed from ========== Scaffolding for sending Expect-Staple reports BUG=598021 ========== to ========== This adds the ability to send Expect-Staple reports for hosts in the Expect-Staple preload list. OCSP staples are only parsed for hosts on this list. Reporting is currently disabled, and only enabled in tests. BUG=598021 ==========
dadrian@google.com changed reviewers: + svaldez@chromium.org
Looking for a first round of feedback. This definitely needs more tests, and will have some refactoring to better support the not-yet-written tests, but from what I can tell this is pretty much everything we need for Expect-Staple.
https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:45: struct OCSPStaple { could probably use some comments (in particular, OCSPStaple is the stuff about a stapled response that Expect-Staple cares about, right? at first I was thinking that it was going to hold the actual response. maybe it should be named ExpectStapleResult or something) https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:87: (!has_next_update || check_time_der < next_update); Wait, so it's considered valid if there's no next update? Why is that? https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:92: // TODO: Verify name and key hashes todo format: // TODO(dadrian): ... https://crbug.com/XXXXXX https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:215: NOTREACHED(); nit: I like to write these with no default and just a `return ""` at the end of the function so that if someone adds a new enum value without updating the switch statement, it won't compile. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:220: static bool ParseAndCheckOCSPResponse(const std::string& raw_response, the 'static' shouldn't be necessary https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:232: return false; Hrm. Should we be somehow reporting these error conditions (doesn't parse, or has a status other than successful)? We can modify the report format however we want, it's not sent in stone or anything. If I'm reading correctly a site operator wouldn't be able to distinguish no-staples from any of these other error conditions, which seems odd. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:1245: if (!enable_static_expect_staple_ || !report_sender_) might as well check for an empty report_uri here as well https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:1258: check_time, ocsp_staples, &serialized_report)) { nit: curly braces inconsistent with line 1251 https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.h:387: void CheckExpectStaple(const HostPortPair& host_port_pair, prob needs some documentation Also (having not read anything else but this header file so far), why does this method take an ExpectStapleState as the argument? That seems different than the other similar functions, like CheckPublicKeyPins(0. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.h:396: bool GetStaticExpectStapleState( Why's this public when its compatriots (GetStaticExpectCTState, etc.) are private? https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.h:499: any reason for this change? https://codereview.chromium.org/2040513003/diff/30001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2040513003/diff/30001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:1362: CheckOCSP(); Could you pass in |server_cert_| as an argument so you don't have to call GetSSLInfo() inside CheckOCSP() to get at the cert? https://codereview.chromium.org/2040513003/diff/30001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:1467: host_and_port_, expect_staple_state, *ssl_info.cert, ocsp_response_); Is |ocsp_response_| already always populated? I thought it is only populated if we have a CT verifier or something like that. (Which we probably always do in Chrome, but I'm not sure, might be worth checking.) https://codereview.chromium.org/2040513003/diff/30001/net/ssl/ssl_info.h File net/ssl/ssl_info.h (right): https://codereview.chromium.org/2040513003/diff/30001/net/ssl/ssl_info.h#newc... net/ssl/ssl_info.h:14: #include "net/cert/internal/parse_ocsp.h" unnecessary
https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:45: struct OCSPStaple { On 2016/06/09 21:24:14, estark wrote: > could probably use some comments > > (in particular, OCSPStaple is the stuff about a stapled response that > Expect-Staple cares about, right? at first I was thinking that it was going to > hold the actual response. maybe it should be named ExpectStapleResult or > something) In retrospect, I think I'm going to move this and related functions to a separate file to make everything easier to test. It can handle going from the representations in parse_ocsp.h to a representation not encumbered by der::Input, etc. I've tentatively started filling out net/cert/ocsp_staple.h with this. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:87: (!has_next_update || check_time_der < next_update); On 2016/06/09 21:24:14, estark wrote: > Wait, so it's considered valid if there's no next update? Why is that? This part is still kind of early. nextUpdate is optional because CA's may choose to always provide a fresh response (i.e. an optional nextUpdate implies nextUpdate == thisUpdate). We should probably set some hard limit on the delta between both thisUpdate and now(), and thisUpdate and nextUpdate. This will prevent sites from constantly replaying the same result for the lifetime of the certificate. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:92: // TODO: Verify name and key hashes On 2016/06/09 21:24:14, estark wrote: > todo format: > > // TODO(dadrian): ... https://crbug.com/XXXXXX Acknowledged. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:215: NOTREACHED(); On 2016/06/09 21:24:14, estark wrote: > nit: I like to write these with no default and just a `return ""` at the end of > the function so that if someone adds a new enum value without updating the > switch statement, it won't compile. Oh, that causes compile failure? Nice. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:220: static bool ParseAndCheckOCSPResponse(const std::string& raw_response, On 2016/06/09 21:24:14, estark wrote: > the 'static' shouldn't be necessary Done. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:232: return false; On 2016/06/09 21:24:14, estark wrote: > Hrm. Should we be somehow reporting these error conditions (doesn't parse, or > has a status other than successful)? We can modify the report format however we > want, it's not sent in stone or anything. If I'm reading correctly a site > operator wouldn't be able to distinguish no-staples from any of these other > error conditions, which seems odd. I was contemplating this as well. An extra top-level enum for failure type might be useful, something along the lines of { MISSING, PARSE_RESPONSE, PARSE_DATA, PARSE_SINGLE_RESPONSE, INVALID }. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:1245: if (!enable_static_expect_staple_ || !report_sender_) On 2016/06/09 21:24:14, estark wrote: > might as well check for an empty report_uri here as well Done. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:1258: check_time, ocsp_staples, &serialized_report)) { On 2016/06/09 21:24:14, estark wrote: > nit: curly braces inconsistent with line 1251 Done. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.h:387: void CheckExpectStaple(const HostPortPair& host_port_pair, On 2016/06/09 21:24:14, estark wrote: > prob needs some documentation > > Also (having not read anything else but this header file so far), why does this > method take an ExpectStapleState as the argument? That seems different than the > other similar functions, like CheckPublicKeyPins(0. It could probably just take the report URI. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.h:396: bool GetStaticExpectStapleState( On 2016/06/09 21:24:15, estark wrote: > Why's this public when its compatriots (GetStaticExpectCTState, etc.) are > private? Because we only parse OCSP responses if the host is in the preload list. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.h:499: On 2016/06/09 21:24:15, estark wrote: > any reason for this change? No idea. I'll try to figure out where this came from. https://codereview.chromium.org/2040513003/diff/30001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2040513003/diff/30001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:1362: CheckOCSP(); On 2016/06/09 21:24:15, estark wrote: > Could you pass in |server_cert_| as an argument so you don't have to call > GetSSLInfo() inside CheckOCSP() to get at the cert? Yes, though it will eventually need both the certificate and the unverified certificate. https://codereview.chromium.org/2040513003/diff/30001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:1467: host_and_port_, expect_staple_state, *ssl_info.cert, ocsp_response_); On 2016/06/09 21:24:15, estark wrote: > Is |ocsp_response_| already always populated? I thought it is only populated if > we have a CT verifier or something like that. (Which we probably always do in > Chrome, but I'm not sure, might be worth checking.) I'll make sure it gets populated if enable_static_expect_staple_ is true, although this might already be the case, I might have added that in the preload list change. https://codereview.chromium.org/2040513003/diff/30001/net/ssl/ssl_info.h File net/ssl/ssl_info.h (right): https://codereview.chromium.org/2040513003/diff/30001/net/ssl/ssl_info.h#newc... net/ssl/ssl_info.h:14: #include "net/cert/internal/parse_ocsp.h" On 2016/06/09 21:24:15, estark wrote: > unnecessary Done.
https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:77: bool CheckOCSPDateValid(bool has_next_update, Add a comment to the RFC referencing the this_update/next_update validity periods. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:86: return (this_update < check_time_der) && You possibly need a fudge factor in case of clock skew? Might only matter for super special servers that generate the stapled OCSP live. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:215: NOTREACHED(); On 2016/06/10 01:05:52, dadrian wrote: > On 2016/06/09 21:24:14, estark wrote: > > nit: I like to write these with no default and just a `return ""` at the end > of > > the function so that if someone adds a new enum value without updating the > > switch statement, it won't compile. > > Oh, that causes compile failure? Nice. Might even make sense to move this into the parse_ocsp file. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:232: return false; On 2016/06/10 01:05:52, dadrian wrote: > On 2016/06/09 21:24:14, estark wrote: > > Hrm. Should we be somehow reporting these error conditions (doesn't parse, or > > has a status other than successful)? We can modify the report format however > we > > want, it's not sent in stone or anything. If I'm reading correctly a site > > operator wouldn't be able to distinguish no-staples from any of these other > > error conditions, which seems odd. > > I was contemplating this as well. An extra top-level enum for failure type might > be useful, something along the lines of { MISSING, PARSE_RESPONSE, PARSE_DATA, > PARSE_SINGLE_RESPONSE, INVALID }. ++, we should consider adding reporting to get a better idea of the state of the world for OCSP responses. Though we can already get a fairly good idea from checking commonly visited site. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:758: expect_ct_reporter_(nullptr), Extra change? https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.h:396: bool GetStaticExpectStapleState( On 2016/06/10 01:05:53, dadrian wrote: > On 2016/06/09 21:24:15, estark wrote: > > Why's this public when its compatriots (GetStaticExpectCTState, etc.) are > > private? > > Because we only parse OCSP responses if the host is in the preload list. Would it make more sense to have CheckExpectStaple wrap a call to GetStaticExpectStapleState, or have another wrapper around the combined call? https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:1855: static const char kOCSPPathPrefix[] = "net/data/parse_ocsp_unittest/"; Doesn't need to be static? https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:1899: state.domain = "example.com"; // Doesn't matter Can you use kHost? https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:1900: state.report_uri = GURL("reports.example.com/expect-staple"); Use constant.
https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:758: expect_ct_reporter_(nullptr), On 2016/06/13 14:03:03, svaldez wrote: > Extra change? Formatting error. Fixed. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.h:396: bool GetStaticExpectStapleState( On 2016/06/13 14:03:04, svaldez wrote: > On 2016/06/10 01:05:53, dadrian wrote: > > On 2016/06/09 21:24:15, estark wrote: > > > Why's this public when its compatriots (GetStaticExpectCTState, etc.) are > > > private? > > > > Because we only parse OCSP responses if the host is in the preload list. > > Would it make more sense to have CheckExpectStaple wrap a call to > GetStaticExpectStapleState, or have another wrapper around the combined call? Done. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.h:499: On 2016/06/10 01:05:52, dadrian wrote: > On 2016/06/09 21:24:15, estark wrote: > > any reason for this change? > > No idea. I'll try to figure out where this came from. Just a formatting snafu. Reverted. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:1855: static const char kOCSPPathPrefix[] = "net/data/parse_ocsp_unittest/"; On 2016/06/13 14:03:04, svaldez wrote: > Doesn't need to be static? Done. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:1899: state.domain = "example.com"; // Doesn't matter On 2016/06/13 14:03:04, svaldez wrote: > Can you use kHost? Done. https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:1900: state.report_uri = GURL("reports.example.com/expect-staple"); On 2016/06/13 14:03:04, svaldez wrote: > Use constant. Done.
Mostly nits, but I'm still a tad confused by the missing-nextUpdate/max-age thingy https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:87: (!has_next_update || check_time_der < next_update); On 2016/06/10 01:05:52, dadrian wrote: > On 2016/06/09 21:24:14, estark wrote: > > Wait, so it's considered valid if there's no next update? Why is that? > > This part is still kind of early. nextUpdate is optional because CA's may choose > to always provide a fresh response (i.e. an optional nextUpdate implies > nextUpdate == thisUpdate). We should probably set some hard limit on the delta > between both thisUpdate and now(), and thisUpdate and nextUpdate. This will > prevent sites from constantly replaying the same result for the lifetime of the > certificate. Is this what the new |max_age| parameter addresses, or is that something else? The optional |nextUpdate| thing still kinda makes my brain hurt. If missing nextUpdate implies that nextUpdate == thisUpdate, doesn't that mean that the client should always *reject* responses with a missing nextUpdate? (i.e. if they are received after thisUpdate == nextUpdate, then they are received after nextUpdate, so they should be rejected as expired?) And is the client free to choose whatever maximum validity period it wants, or are there any limits on how long a response can be valid? https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.cc File net/cert/ocsp_staple.cc (right): https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.cc... net/cert/ocsp_staple.cc:25: const base::Time verify_time, nit: pass by reference (and the next arg too) https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.cc... net/cert/ocsp_staple.cc:31: // Place verify_time in the bounds nits: period and pipes around verify_time https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.cc... net/cert/ocsp_staple.cc:38: // Enforce max age nit: period https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.h File net/cert/ocsp_staple.h (right): https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.h#... net/cert/ocsp_staple.h:18: class NET_EXPORT ExpectStapleReport { Probably should document this class with a comment above it. Also, typically the file name would match the class name (expect_staple_report.h). https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.h#... net/cert/ocsp_staple.h:30: // Represents where the staple verification an error occured. typo; extra "an"? https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.h#... net/cert/ocsp_staple.h:45: // TODO(dadrian): Check issuer and signatures add a bug number please https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.h#... net/cert/ocsp_staple.h:47: const std::string raw_response, pass by reference here https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple_un... File net/cert/ocsp_staple_unittest.cc (right): https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple_un... net/cert/ocsp_staple_unittest.cc:4: #include "net/cert/ocsp_staple.h" missing newline above https://codereview.chromium.org/2040513003/diff/70001/net/http/transport_secu... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2040513003/diff/70001/net/http/transport_secu... net/http/transport_security_state.h:421: // Helper method for serilizing an ExpectStaple report. typo: serializing https://codereview.chromium.org/2040513003/diff/70001/net/http/transport_secu... net/http/transport_security_state.h:492: // always unintentional?
https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.cc File net/cert/ocsp_staple.cc (right): https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.cc... net/cert/ocsp_staple.cc:25: const base::Time verify_time, On 2016/06/14 02:10:27, estark wrote: > nit: pass by reference (and the next arg too) Done. (Whoops) https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.cc... net/cert/ocsp_staple.cc:31: // Place verify_time in the bounds On 2016/06/14 02:10:27, estark wrote: > nits: period and pipes around verify_time Done. https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.cc... net/cert/ocsp_staple.cc:38: // Enforce max age On 2016/06/14 02:10:27, estark wrote: > nit: period Done. https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.h File net/cert/ocsp_staple.h (right): https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.h#... net/cert/ocsp_staple.h:18: class NET_EXPORT ExpectStapleReport { On 2016/06/14 02:10:28, estark wrote: > Probably should document this class with a comment above it. Also, typically the > file name would match the class name (expect_staple_report.h). Done. https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.h#... net/cert/ocsp_staple.h:30: // Represents where the staple verification an error occured. On 2016/06/14 02:10:28, estark wrote: > typo; extra "an"? Done. https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.h#... net/cert/ocsp_staple.h:45: // TODO(dadrian): Check issuer and signatures On 2016/06/14 02:10:28, estark wrote: > add a bug number please Done. https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple.h#... net/cert/ocsp_staple.h:47: const std::string raw_response, On 2016/06/14 02:10:28, estark wrote: > pass by reference here Done. https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple_un... File net/cert/ocsp_staple_unittest.cc (right): https://codereview.chromium.org/2040513003/diff/70001/net/cert/ocsp_staple_un... net/cert/ocsp_staple_unittest.cc:4: #include "net/cert/ocsp_staple.h" On 2016/06/14 02:10:28, estark wrote: > missing newline above Done. https://codereview.chromium.org/2040513003/diff/70001/net/http/transport_secu... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2040513003/diff/70001/net/http/transport_secu... net/http/transport_security_state.h:421: // Helper method for serilizing an ExpectStaple report. On 2016/06/14 02:10:28, estark wrote: > typo: serializing Done. https://codereview.chromium.org/2040513003/diff/70001/net/http/transport_secu... net/http/transport_security_state.h:492: // always On 2016/06/14 02:10:28, estark wrote: > unintentional? Done.
https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... net/http/transport_security_state.cc:87: (!has_next_update || check_time_der < next_update); On 2016/06/14 02:10:27, estark wrote: > On 2016/06/10 01:05:52, dadrian wrote: > > On 2016/06/09 21:24:14, estark wrote: > > > Wait, so it's considered valid if there's no next update? Why is that? > > > > This part is still kind of early. nextUpdate is optional because CA's may > choose > > to always provide a fresh response (i.e. an optional nextUpdate implies > > nextUpdate == thisUpdate). We should probably set some hard limit on the delta > > between both thisUpdate and now(), and thisUpdate and nextUpdate. This will > > prevent sites from constantly replaying the same result for the lifetime of > the > > certificate. > > Is this what the new |max_age| parameter addresses, or is that something else? > > The optional |nextUpdate| thing still kinda makes my brain hurt. If missing > nextUpdate implies that nextUpdate == thisUpdate, doesn't that mean that the > client should always *reject* responses with a missing nextUpdate? (i.e. if they > are received after thisUpdate == nextUpdate, then they are received after > nextUpdate, so they should be rejected as expired?) > > And is the client free to choose whatever maximum validity period it wants, or > are there any limits on how long a response can be valid? The validity period is set by the OCSP responder. Basically according to the RFC there are two validity cases here. If nextUpdate is set, then we require the current time to be in (thisUpdate, nextUpdate), otherwise we require the current time to be in (thisUpdate, thisUpdate+max_age). I think what we actually want is to check that the current time is in the range (thisUpdate, max(thisUpdate+max_age, nextUpdate)), with necessary clock skew fudge factors. Since otherwise we have edge conditions of OCSP responders that decide that nextUpdate should be a second from now, making the responses validity period 1 second. Additionally you might want a check that producedAt isn't too far in the past, though I don't think that gives security benefits, other than revealing OCSP responders that have decided to pre-sign a bunch of validity responses.
On 2016/06/14 18:56:13, svaldez wrote: > https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... > File net/http/transport_security_state.cc (right): > > https://codereview.chromium.org/2040513003/diff/30001/net/http/transport_secu... > net/http/transport_security_state.cc:87: (!has_next_update || check_time_der < > next_update); > On 2016/06/14 02:10:27, estark wrote: > > On 2016/06/10 01:05:52, dadrian wrote: > > > On 2016/06/09 21:24:14, estark wrote: > > > > Wait, so it's considered valid if there's no next update? Why is that? > > > > > > This part is still kind of early. nextUpdate is optional because CA's may > > choose > > > to always provide a fresh response (i.e. an optional nextUpdate implies > > > nextUpdate == thisUpdate). We should probably set some hard limit on the > delta > > > between both thisUpdate and now(), and thisUpdate and nextUpdate. This will > > > prevent sites from constantly replaying the same result for the lifetime of > > the > > > certificate. > > > > Is this what the new |max_age| parameter addresses, or is that something else? > > > > The optional |nextUpdate| thing still kinda makes my brain hurt. If missing > > nextUpdate implies that nextUpdate == thisUpdate, doesn't that mean that the > > client should always *reject* responses with a missing nextUpdate? (i.e. if > they > > are received after thisUpdate == nextUpdate, then they are received after > > nextUpdate, so they should be rejected as expired?) > > > > And is the client free to choose whatever maximum validity period it wants, or > > are there any limits on how long a response can be valid? > > The validity period is set by the OCSP responder. Basically according to the RFC > there are two validity cases here. If nextUpdate is set, then we require the > current time to be in (thisUpdate, nextUpdate), otherwise we require the current > time to be in (thisUpdate, thisUpdate+max_age). > > I think what we actually want is to check that the current time is in the range > (thisUpdate, max(thisUpdate+max_age, nextUpdate)), with necessary clock skew > fudge factors. Since otherwise we have edge conditions of OCSP responders that > decide that nextUpdate should be a second from now, making the responses > validity period 1 second. > > Additionally you might want a check that producedAt isn't too far in the past, > though I don't think that gives security benefits, other than revealing OCSP > responders that have decided to pre-sign a bunch of validity responses. If nextUpdate is present, I'm currently checking the current time is in range (thisUpdate, min(thisUpdate+maxAge, nextUpdate)) with no fudge factors (yet).
https://codereview.chromium.org/2040513003/diff/90001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/90001/net/http/transport_secu... net/http/transport_security_state.cc:1217: report_dict.Set("served-certificate-chain", important note: only include this if |is_issued_by_known_root| is true (and please add a unit test for that as well)
https://codereview.chromium.org/2040513003/diff/90001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/90001/net/http/transport_secu... net/http/transport_security_state.cc:1217: report_dict.Set("served-certificate-chain", On 2016/06/15 02:38:37, estark wrote: > important note: only include this if |is_issued_by_known_root| is true (and > please add a unit test for that as well) Done.
Description was changed from ========== This adds the ability to send Expect-Staple reports for hosts in the Expect-Staple preload list. OCSP staples are only parsed for hosts on this list. Reporting is currently disabled, and only enabled in tests. BUG=598021 ========== to ========== Implement Expect-Staple This adds the ability to send Expect-Staple reports for hosts in the Expect-Staple preload list. OCSP staples are only parsed for hosts on this list. Reporting is currently disabled, and only enabled in tests. BUG=598021 ==========
(note: I updated the issue description to obey the weird rule that the first line of the description should be equal to the subject) I mostly just have nits at this point. Steven, do you think it would be good to get a net owner to take a look soon? https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... File net/cert/expect_staple_report.cc (right): https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... net/cert/expect_staple_report.cc:6: missing a bunch of #includes here https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... net/cert/expect_staple_report.cc:110: return out; unnecessary https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... File net/cert/expect_staple_report.h (right): https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... net/cert/expect_staple_report.h:5: #ifndef NET_CERT_OCSP_STAPLE_H NET_CERT_EXPECT_STAPLE_REPORT_H(here and below) https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... net/cert/expect_staple_report.h:33: // Represents where during the staple verification an error occured. nit: occurred (sorry) https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... net/cert/expect_staple_report.h:35: OK = 0, nit: no explicit assignments necessary https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... net/cert/expect_staple_report.h:40: NO_MATCHING_RESPONSE = 5 nit: trailing comma https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... File net/cert/expect_staple_report_unittest.cc (right): https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... net/cert/expect_staple_report_unittest.cc:16: const base::TimeDelta kAgeTenYears = base::TimeDelta::FromDays(3650); My preference would be to name this what it is used as (kOCSPResponseMaxAge perhaps?) rather than for the actual value it holds. https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... net/cert/expect_staple_report_unittest.cc:48: base::Time verify_time_; This should be below the methods. https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... net/cert/expect_staple_report_unittest.cc:57: } DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2040513003/diff/110001/net/http/transport_sec... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/110001/net/http/transport_sec... net/http/transport_security_state.cc:1145: &serialized_report)) nit: curly braces for a multi-line condition https://codereview.chromium.org/2040513003/diff/110001/net/http/transport_sec... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2040513003/diff/110001/net/http/transport_sec... net/http/transport_security_state.h:22: #include "net/cert/expect_staple_report.h" Should be able to forward-declare this and do the #include from the .cc https://codereview.chromium.org/2040513003/diff/110001/net/http/transport_sec... File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/2040513003/diff/110001/net/http/transport_sec... net/http/transport_security_state_unittest.cc:85: const base::TimeDelta& kAgeTenYears = base::TimeDelta::FromDays(3650); ditto about naming for what is it rather than the value https://codereview.chromium.org/2040513003/diff/110001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2040513003/diff/110001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:89: const base::TimeDelta kAgeOneWeek = base::TimeDelta::FromDays(7); ditto https://codereview.chromium.org/2040513003/diff/110001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:89: const base::TimeDelta kAgeOneWeek = base::TimeDelta::FromDays(7); It appears that mozilla::pkix requires responses without a nextUpdate to be fresh within 1 day: https://dxr.mozilla.org/mozilla-central/source/security/pkix/lib/pkixocsp.cpp... Would we want to match that? https://codereview.chromium.org/2040513003/diff/110001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:89: const base::TimeDelta kAgeOneWeek = base::TimeDelta::FromDays(7); Unfortunately this constant would have to be duplicated in the QUIC code, is that right? https://codereview.chromium.org/2040513003/diff/110001/net/ssl/ssl_info.h File net/ssl/ssl_info.h (right): https://codereview.chromium.org/2040513003/diff/110001/net/ssl/ssl_info.h#new... net/ssl/ssl_info.h:14: #include "net/cert/internal/parse_ocsp.h" unnecessary?
https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... File net/cert/expect_staple_report.cc (right): https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... net/cert/expect_staple_report.cc:6: On 2016/06/15 23:51:46, estark wrote: > missing a bunch of #includes here Done. https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... net/cert/expect_staple_report.cc:110: return out; On 2016/06/15 23:51:46, estark wrote: > unnecessary Done. https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... File net/cert/expect_staple_report.h (right): https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... net/cert/expect_staple_report.h:5: #ifndef NET_CERT_OCSP_STAPLE_H On 2016/06/15 23:51:46, estark wrote: > NET_CERT_EXPECT_STAPLE_REPORT_H(here and below) Done. Missed that when I renamed the file. :( https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... net/cert/expect_staple_report.h:33: // Represents where during the staple verification an error occured. On 2016/06/15 23:51:46, estark wrote: > nit: occurred (sorry) Done. I swear, I have two degrees and know how to spell things. https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... net/cert/expect_staple_report.h:35: OK = 0, On 2016/06/15 23:51:46, estark wrote: > nit: no explicit assignments necessary Done. https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... net/cert/expect_staple_report.h:40: NO_MATCHING_RESPONSE = 5 On 2016/06/15 23:51:46, estark wrote: > nit: trailing comma Done. https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... File net/cert/expect_staple_report_unittest.cc (right): https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... net/cert/expect_staple_report_unittest.cc:48: base::Time verify_time_; On 2016/06/15 23:51:46, estark wrote: > This should be below the methods. Done. https://codereview.chromium.org/2040513003/diff/110001/net/cert/expect_staple... net/cert/expect_staple_report_unittest.cc:57: } On 2016/06/15 23:51:46, estark wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2040513003/diff/110001/net/http/transport_sec... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/110001/net/http/transport_sec... net/http/transport_security_state.cc:1145: &serialized_report)) On 2016/06/15 23:51:46, estark wrote: > nit: curly braces for a multi-line condition Done. https://codereview.chromium.org/2040513003/diff/110001/net/http/transport_sec... net/http/transport_security_state.cc:1145: &serialized_report)) On 2016/06/15 23:51:46, estark wrote: > nit: curly braces for a multi-line condition Actually, I'm a little confused about this. I think I've had if-statements with single-line bodies go both ways when the conditional is multi-line. What is the "correct" rule with regard to braces and multi-line conditionals? https://codereview.chromium.org/2040513003/diff/110001/net/http/transport_sec... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2040513003/diff/110001/net/http/transport_sec... net/http/transport_security_state.h:22: #include "net/cert/expect_staple_report.h" On 2016/06/15 23:51:46, estark wrote: > Should be able to forward-declare this and do the #include from the .cc Done. https://codereview.chromium.org/2040513003/diff/110001/net/http/transport_sec... File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/2040513003/diff/110001/net/http/transport_sec... net/http/transport_security_state_unittest.cc:85: const base::TimeDelta& kAgeTenYears = base::TimeDelta::FromDays(3650); On 2016/06/15 23:51:47, estark wrote: > ditto about naming for what is it rather than the value Done. https://codereview.chromium.org/2040513003/diff/110001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2040513003/diff/110001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:89: const base::TimeDelta kAgeOneWeek = base::TimeDelta::FromDays(7); On 2016/06/15 23:51:47, estark wrote: > Unfortunately this constant would have to be duplicated in the QUIC code, is > that right? Does QUIC support OCSP stapling? I'm not very familiar with the protocol. https://codereview.chromium.org/2040513003/diff/110001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:89: const base::TimeDelta kAgeOneWeek = base::TimeDelta::FromDays(7); On 2016/06/15 23:51:47, estark wrote: > It appears that mozilla::pkix requires responses without a nextUpdate to be > fresh within 1 day: > https://dxr.mozilla.org/mozilla-central/source/security/pkix/lib/pkixocsp.cpp... > > Would we want to match that? 1 day seems short, but I have no actual basis for making that decision. https://codereview.chromium.org/2040513003/diff/110001/net/ssl/ssl_info.h File net/ssl/ssl_info.h (right): https://codereview.chromium.org/2040513003/diff/110001/net/ssl/ssl_info.h#new... net/ssl/ssl_info.h:14: #include "net/cert/internal/parse_ocsp.h" On 2016/06/15 23:51:47, estark wrote: > unnecessary? Done. Artifact of an earlier implementation.
Missed that there was a new patchset (why can't we have Gerrits notifications.) https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... File net/cert/expect_staple_report.cc (right): https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:34: !(response.this_update < response.next_update)) this_update >= next_update? https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:35: return false; Multiline conditionals should have braces. (I think the general rule is that if the condition+action takes more than two lines combined, it should have braces) https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:39: if (!(response.this_update < verify_time_der)) verify_time_der < response.this_update https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:41: if (response.has_next_update && !(verify_time_der < response.next_update)) next_update < verify_time_der or verify_time_der > next_update https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:45: der::GeneralizedTime lower_bound = ConvertBaseTime(verify_time - max_age); return this_update >= lower_bound? https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:64: std::unique_ptr<ExpectStapleReport> ExpectStapleReport::FromRawOCSPResponse( // static https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:91: Add: TODO(svaldez): Unify with GetOCSPCertStatus. https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:92: bool contains_correct_response = false; This should fail out immediately if any of the responses are REVOKED. https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:116: return out; Explicitly set StapleError::OK. https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... File net/cert/expect_staple_report.h (right): https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.h:60: nit: You probably want the accessors in the same order as the variables. https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... File net/cert/expect_staple_report_unittest.cc (right): https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report_unittest.cc:52: void SetUp() override { verify_time_ = base::Time::Now(); } Maybe hardcode this so that the tests don't break? https://codereview.chromium.org/2040513003/diff/130001/net/http/transport_sec... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/130001/net/http/transport_sec... net/http/transport_security_state.cc:1117: const X509Certificate& unverified_certificate, Do you actually need to ship both the certificates into here? https://codereview.chromium.org/2040513003/diff/130001/net/http/transport_sec... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2040513003/diff/130001/net/http/transport_sec... net/http/transport_security_state.h:389: void CheckExpectStaple(const HostPortPair& host_port_pair, Add comment about what this does. https://codereview.chromium.org/2040513003/diff/130001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.h (right): https://codereview.chromium.org/2040513003/diff/130001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.h:137: void CheckOCSP(const X509Certificate& verified_certificate, ReportOCSP? Especially if we're not actually changing connection state/security based on OCSP. You might as well also just have this function pull the certs/known_root off of the object instead of passing them in (Matching VerifyCT/ChannelID).
https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... File net/cert/expect_staple_report.cc (right): https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:34: !(response.this_update < response.next_update)) On 2016/06/16 11:14:38, svaldez wrote: > this_update >= next_update? I was using operator< because it's the only one implemented on der::GeneralizedTime, although we could add more operators to it. https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:35: return false; On 2016/06/16 11:14:38, svaldez wrote: > Multiline conditionals should have braces. > > (I think the general rule is that if the condition+action takes more than two > lines combined, it should have braces) Done. Good to know, I think I've seen it both ways. https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:39: if (!(response.this_update < verify_time_der)) On 2016/06/16 11:14:38, svaldez wrote: > verify_time_der < response.this_update Done. https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:41: if (response.has_next_update && !(verify_time_der < response.next_update)) On 2016/06/16 11:14:38, svaldez wrote: > next_update < verify_time_der > > or > > verify_time_der > next_update Done. https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:45: der::GeneralizedTime lower_bound = ConvertBaseTime(verify_time - max_age); On 2016/06/16 11:14:38, svaldez wrote: > return this_update >= lower_bound? I wrote it this way to make all the error checks explicit fails, in case more get added later. https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:64: std::unique_ptr<ExpectStapleReport> ExpectStapleReport::FromRawOCSPResponse( On 2016/06/16 11:14:38, svaldez wrote: > // static Done. https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:91: On 2016/06/16 11:14:38, svaldez wrote: > Add: > > TODO(svaldez): Unify with GetOCSPCertStatus. Done. https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:92: bool contains_correct_response = false; On 2016/06/16 11:14:38, svaldez wrote: > This should fail out immediately if any of the responses are REVOKED. If this was just verification, sure. But since we're reporting, we probably want to report every response regardless. https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:116: return out; On 2016/06/16 11:14:38, svaldez wrote: > Explicitly set StapleError::OK. Done. https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... File net/cert/expect_staple_report.h (right): https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.h:60: On 2016/06/16 11:14:38, svaldez wrote: > nit: You probably want the accessors in the same order as the variables. Done. https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... File net/cert/expect_staple_report_unittest.cc (right): https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report_unittest.cc:52: void SetUp() override { verify_time_ = base::Time::Now(); } On 2016/06/16 11:14:38, svaldez wrote: > Maybe hardcode this so that the tests don't break? Done. https://codereview.chromium.org/2040513003/diff/130001/net/http/transport_sec... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/130001/net/http/transport_sec... net/http/transport_security_state.cc:1117: const X509Certificate& unverified_certificate, On 2016/06/16 11:14:38, svaldez wrote: > Do you actually need to ship both the certificates into here? The reporting needs the |unverified_certificate| https://codereview.chromium.org/2040513003/diff/130001/net/http/transport_sec... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2040513003/diff/130001/net/http/transport_sec... net/http/transport_security_state.h:389: void CheckExpectStaple(const HostPortPair& host_port_pair, On 2016/06/16 11:14:38, svaldez wrote: > Add comment about what this does. Done. https://codereview.chromium.org/2040513003/diff/130001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.h (right): https://codereview.chromium.org/2040513003/diff/130001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.h:137: void CheckOCSP(const X509Certificate& verified_certificate, On 2016/06/16 11:14:38, svaldez wrote: > ReportOCSP? Especially if we're not actually changing connection state/security > based on OCSP. You might as well also just have this function pull the > certs/known_root off of the object instead of passing them in (Matching > VerifyCT/ChannelID). Done.
Probably reasonable to ask for a net OWNER to look over/stamp. I'm still a bit wary of using the raw (thisUpdate, nextUpdate) as the validity period, since there might be misbehaving servers who have a short period using those bounds, but that's probably something that will show up when actually testing against top sites. https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... File net/cert/expect_staple_report.cc (right): https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:34: !(response.this_update < response.next_update)) On 2016/06/16 19:20:18, dadrian wrote: > On 2016/06/16 11:14:38, svaldez wrote: > > this_update >= next_update? > > I was using operator< because it's the only one implemented on > der::GeneralizedTime, although we could add more operators to it. https://codereview.chromium.org/2075783002/ adds the other >=, <=, > operators for GeneralizedTime, so once that goes through you should be able to update.
dadrian@google.com changed reviewers: + rsleevi@chromium.org
Now with new der::GeneralizedTime operators! Adding rsleevi@ as a reviewer, per conversation in person while petting Karma. https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... File net/cert/expect_staple_report.cc (right): https://codereview.chromium.org/2040513003/diff/130001/net/cert/expect_staple... net/cert/expect_staple_report.cc:34: !(response.this_update < response.next_update)) On 2016/06/16 19:56:23, svaldez wrote: > On 2016/06/16 19:20:18, dadrian wrote: > > On 2016/06/16 11:14:38, svaldez wrote: > > > this_update >= next_update? > > > > I was using operator< because it's the only one implemented on > > der::GeneralizedTime, although we could add more operators to it. > > https://codereview.chromium.org/2075783002/ adds the other >=, <=, > operators > for GeneralizedTime, so once that goes through you should be able to update. Done.
Kicking over to Emily because I think there's a lot of design concerns here that make me a little uncomfortable. Happy to hash out further where the right layering fits. https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... File net/cert/expect_staple_report.h (right): https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report.h:46: // at least as recent as |verify_time - max_age|. DESIGN: This seems to violate the Single-Responsibility-Principle of SOLID design. The presumed single responsibility is being a container/abstraction for an OCSP report. However, this method makes it do OCSP parsing/validation, signature validation, and constructing a report. That seems well-outside the scope of here. From an API design, is it expected this will always be synchronous? In order to validate OCSP responses, we need an asynchronous API presumably (to consider trust status) https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... File net/cert/expect_staple_report_unittest.cc (right): https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report_unittest.cc:54: verify_time_ = base::Time::FromDoubleT(1466101795.0); Suggestion: base::Time::UnixEpoch() + base::TimeDelta::FromSeconds(...) (Makes it easier for people to update the value by just using a time-t converter) https://codereview.chromium.org/2040513003/diff/170001/net/http/transport_sec... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2040513003/diff/170001/net/http/transport_sec... net/http/transport_security_state.h:395: // |is_issued_by_known_root| is true. This also seems a big "SRP" concern. This doesn't have anything to do with persisting or storing the data in the transport security state - all you really care about is the domain policy for |host_port_pair|. Building a report, verifying, sending, etc - that all seems like something that should be done outside the TSS. Line 385-387 I'm totally down with, because that speaks to TSS's core thing - manage domain policies. This method seems to do... a lot more work. https://codereview.chromium.org/2040513003/diff/170001/net/http/transport_sec... net/http/transport_security_state.h:406: friend class ExpectStapleTest; I block the addition of friends because I'm a terrible person. The only friend should be this files tester https://codereview.chromium.org/2040513003/diff/170001/net/http/transport_sec... File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/2040513003/diff/170001/net/http/transport_sec... net/http/transport_security_state_unittest.cc:1880: bool ReportSent() { return latest_report() != ""; } return !latest_report().empty() https://codereview.chromium.org/2040513003/diff/170001/net/http/transport_sec... net/http/transport_security_state_unittest.cc:1885: void SetUp() override { https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... Prefer a constructor here https://codereview.chromium.org/2040513003/diff/170001/net/http/transport_sec... net/http/transport_security_state_unittest.cc:1897: static bool LoadOCSPFromFile(std::string file_name, OCSPTest* ocsp) { Why static? https://codereview.chromium.org/2040513003/diff/170001/net/http/transport_sec... net/http/transport_security_state_unittest.cc:1918: static void CheckExpectStapleReport( Why static? https://codereview.chromium.org/2040513003/diff/170001/net/http/transport_sec... net/http/transport_security_state_unittest.cc:1978: static bool SerializeExpectStapleReport( Why static? https://codereview.chromium.org/2040513003/diff/170001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2040513003/diff/170001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:89: const base::TimeDelta kOCSPResponseMaxAge = base::TimeDelta::FromDays(7); Why is this an aspect of the socket? This is about cryptographic policy, not related to SSL. https://codereview.chromium.org/2040513003/diff/170001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:1445: transport_security_state_->CheckExpectStaple( Putting it at this layer creates issues when multiplexing sockets. If I have Expect-Staple for mail.google.com but not google.com, this won't trigger if those connections ar emultiplexed. https://codereview.chromium.org/2040513003/diff/170001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:1448: kOCSPResponseMaxAge, ocsp_response_); BUG: You never set |ocsp_response_| DESIGN: Why even have it as a member?
https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... File net/cert/expect_staple_report.h (right): https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report.h:46: // at least as recent as |verify_time - max_age|. On 2016/06/16 21:49:29, Ryan Sleevi wrote: > DESIGN: This seems to violate the Single-Responsibility-Principle of SOLID > design. > > The presumed single responsibility is being a container/abstraction for an OCSP > report. However, this method makes it do OCSP parsing/validation, signature > validation, and constructing a report. That seems well-outside the scope of > here. > > From an API design, is it expected this will always be synchronous? In order to > validate OCSP responses, we need an asynchronous API presumably (to consider > trust status) Its unclear what the division should be otherwise, things like extracting the individual OCSP SingleResponses and checking age should be included somewhere in the ExpectStaple layer. It might make sense to have FromRawOCSPResponse take in an OCSPResponse instead of a string, but I don't see a clear split for the rest. At some point there should be a place where ExpectStaple receives an OCSP response, and then after that point deals with parsing, validation, single response extraction. Otherwise we'll need to pass an equivalent of an ocsp_verify_result all over the place to figure out what the StapleError should be. This API should never be async, since trust status checking only is based on the issuer and a single chain down from it, which we already have from the original certificate verification. https://codereview.chromium.org/2040513003/diff/170001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2040513003/diff/170001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:1448: kOCSPResponseMaxAge, ocsp_response_); On 2016/06/16 21:49:30, Ryan Sleevi wrote: > BUG: You never set |ocsp_response_| > DESIGN: Why even have it as a member? CT and ExpectStaple should probably use a shared ocsp_response_, instead of calling out to the SSL layer multiple times for it.
https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... File net/cert/expect_staple_report.h (right): https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report.h:26: // Stores the validity of a single stapled response. From simply reading the header, it's non-obvious why there's a vector of SingleResults here. If SingleResult applies to the entire stapled response, there should be only one, presumably - we have no plans to support multi-stapling. https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report.h:27: struct SingleResult { What is the API if a SingleResult couldn't be parsed? https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report.h:46: // at least as recent as |verify_time - max_age|. On 2016/06/17 13:33:51, svaldez wrote: > Its unclear what the division should be otherwise, things like extracting the > individual OCSP SingleResponses and checking age should be included somewhere in > the ExpectStaple layer. I don't think that intrinsically argues it belongs as a static method on an object though. Having a free function that handles this could be better, if we expect the behaviour to be consistent. > At some point there should be a place where ExpectStaple receives an OCSP > response, and then after that point deals with parsing, validation, single > response extraction. Otherwise we'll need to pass an equivalent of an > ocsp_verify_result all over the place to figure out what the StapleError should > be. Why isn't this part of cert verification? Since it's directly tied to the suitability of it? Why doesn't CertVerifyResult keep track? I do want to be clear my unease about having an ExpectStapleReport do OCSP validation. To me, that's a clear layering - one is simply reporting/diagnostic, the other is independent logic. I would suggest chatting with davidben@ in person to get his thoughts, if only because we were discussing it yesterday evening, but my gut is that OCSP parsing OCSP processing OCSP verification All belong in CertVerification. And they'd give out additional details on the CertVerifyResult, which could then be used to construct an ExpectStapleReport and send it. > This API should never be async, since trust status checking only is based on the > issuer and a single chain down from it, which we already have from the original > certificate verification. Unfortunately, on the platform APIs we use, the ability to trust a CA for certs but NOT trust it for OCSP is a thing. https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report.h:73: #endif /* NET_CERT_EXPECT_STAPLE_REPORT_H */ STYLE: Two spaces and use C++ comments #endif // NET_CERT_EXPECT_STAPLE_REPORT_H_ https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... File net/cert/expect_staple_report_unittest.cc (right): https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report_unittest.cc:17: const base::TimeDelta kOCSPResponseMaxAge = base::TimeDelta::FromDays(3650); This also makes me uncomfortable, FWIW - having callers supply the max age as an API parameter seems a bit like a large footgun. https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report_unittest.cc:20: std::string response; include what you use (IWYU): #include <string> https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report_unittest.cc:21: scoped_refptr<X509Certificate> certificate; IWYU: #include "base/memory/ref_counted.h" #include "net/cert/x509_certificate.h" https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report_unittest.cc:29: {"CA CERTIFICATE", &ca_data}, Why do you do this, when ca_data is unused? https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report_unittest.cc:33: if (!ReadTestDataFromPemFile(full_path, mappings)) API: ReadTestDataFromPemFile is designed around a ::testing::AssertionResult interface, specifically so that the failure can give sufficient diagnostics. That is, EXPECT_TRUE(ReadTestDataFromPemFile(...)); The way to propagate this is by having this function return a ::testing::AssertionResult, and return that. The only place this pattern isn't followed is the OCSP parsing code, and I'm not sure if that's a good pattern we want to mix in. https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report_unittest.cc:52: void SetUp() override { https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report_unittest.cc:74: auto report = MakeReport(ocsp); This is not an allowed usage of auto 1) Return type is not clear 2) Autos for pointer-semantics of creators is *especially* opaque. That is, you have to look on MakeReport to see are you getting a naked pointer, a unique pointer, a ref-counted, etc. There was discussion on chromium-dev regarding this pattern, with the conclusion being even for testing code, we should follow the style guide's remarks on auto, which is to eschew it when it hinders comprehensibility of the code. https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report_unittest.cc:78: const auto& stapled_responses = report->stapled_responses(); This is an OK auto fwiw :) https://codereview.chromium.org/2040513003/diff/170001/net/http/transport_sec... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2040513003/diff/170001/net/http/transport_sec... net/http/transport_security_state.h:431: static bool SerializeExpectStapleReport( DESIGN: Static privates are a design anti-pattern, in that no one can reach into them but the class, but they require definition in the .h (thus affecting consumers). The general argument "for" them is "So tests can call into them", but that means writing tests which aren't black-box tests, and those are generally bad tests. So the approach is to move static privates into the .cc file (since that's the only place they can be called) and in the unnamed namespace. The exception to this is when a single class spans multiple TUs (for example, foo.cc and foo_win.cc/foo_linux.cc, with _win/_linux needing to call some core functionality, and without using inheritance as the platform abstraction layer), but this is TransportSecurityStatic, so it doesn't rise to that level
https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... File net/cert/expect_staple_report.h (right): https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report.h:46: // at least as recent as |verify_time - max_age|. On 2016/06/17 13:33:51, svaldez wrote: > On 2016/06/16 21:49:29, Ryan Sleevi wrote: > > DESIGN: This seems to violate the Single-Responsibility-Principle of SOLID > > design. > > > > The presumed single responsibility is being a container/abstraction for an > OCSP > > report. However, this method makes it do OCSP parsing/validation, signature > > validation, and constructing a report. That seems well-outside the scope of > > here. > > > > From an API design, is it expected this will always be synchronous? In order > to > > validate OCSP responses, we need an asynchronous API presumably (to consider > > trust status) > > Its unclear what the division should be otherwise, things like extracting the > individual OCSP SingleResponses and checking age should be included somewhere in > the ExpectStaple layer. At some point ExpectStaple needs to have validation for every stapled SingleResponse, and be able to discern between various types of parsing or validation failures. There's the question of should the OCSP layer contain all that logic, and just hand ExpectStaple some object, which it can then serialize, or if ExpectStaple should handle the whole things. I'd say that these designs are effectively the same, with the only difference being the name of the code that's currently in expect_staple_report.h. The current ExpectStapleReport class doesn't actually do any reporting, it basically just does OCSP validation with detailed error information. The actual serialization and reporting takes place in TransportSecurityState, which is exactly how PKP reporting works. The ExpectStapleReport class could just as easily be named OCSPStaple, there's nothing particularly "report"-like about it, rather than basic validation. Then there's the question of how does the rest of the net stack interact with this class? Right now it's structured so that the OCSP parsing only happens if the server is in the preload list, and I think it makes sense to leave it that way. Ideally, OCSP verification would be added to CertVerifyResult and take place as part of the rest of certificate validation, but given that Expect Staple is only reporting and only for a tiny subset of hosts, I don't think it makes sense to add it to certificate verification, especially given that the prerequisite code for even validating signatures on OCSP responses still hasn't landed. In summary, I think the big questions are: 1. What should the ExpectStapleReport class be called, and where should it go? 2. How should the ExpectStapleReport class be constructed? > > It might make sense to have FromRawOCSPResponse take in an OCSPResponse instead > of a string, but I don't see a clear split for the rest. > > At some point there should be a place where ExpectStaple receives an OCSP > response, and then after that point deals with parsing, validation, single > response extraction. Otherwise we'll need to pass an equivalent of an > ocsp_verify_result all over the place to figure out what the StapleError should > be. > > This API should never be async, since trust status checking only is based on the > issuer and a single chain down from it, which we already have from the original > certificate verification. https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... File net/cert/expect_staple_report_unittest.cc (right): https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report_unittest.cc:17: const base::TimeDelta kOCSPResponseMaxAge = base::TimeDelta::FromDays(3650); On 2016/06/17 16:19:56, Ryan Sleevi wrote: > This also makes me uncomfortable, FWIW - having callers supply the max age as an > API parameter seems a bit like a large footgun. It's primarily like that for testing, although I suppose we could backdate all of the verification times in tests, and always set a hard limit of 1 day / 1 week / whatever. https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report_unittest.cc:29: {"CA CERTIFICATE", &ca_data}, On 2016/06/17 16:19:56, Ryan Sleevi wrote: > Why do you do this, when ca_data is unused? I'm not sure if the test files parse otherwise, since they have the CA certificate in them. It's currently unused because the code doesn't check signatures or issuers, as documented in https://crbug.com/620005. https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report_unittest.cc:74: auto report = MakeReport(ocsp); On 2016/06/17 16:19:56, Ryan Sleevi wrote: > This is not an allowed usage of auto > > 1) Return type is not clear > 2) Autos for pointer-semantics of creators is *especially* opaque. That is, you > have to look on MakeReport to see are you getting a naked pointer, a unique > pointer, a ref-counted, etc. > > There was discussion on chromium-dev regarding this pattern, with the conclusion > being even for testing code, we should follow the style guide's remarks on auto, > which is to eschew it when it hinders comprehensibility of the code. This was me being lazy when I was getting the tests to compile, and I forgot to go back and change it. https://codereview.chromium.org/2040513003/diff/170001/net/http/transport_sec... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2040513003/diff/170001/net/http/transport_sec... net/http/transport_security_state.h:406: friend class ExpectStapleTest; On 2016/06/16 21:49:29, Ryan Sleevi wrote: > I block the addition of friends because I'm a terrible person. The only friend > should be this files tester This is a derived class of this files tester. The other option would be to make SerializeExpectStapleReport public. https://codereview.chromium.org/2040513003/diff/170001/net/http/transport_sec... net/http/transport_security_state.h:431: static bool SerializeExpectStapleReport( On 2016/06/17 16:19:56, Ryan Sleevi wrote: > DESIGN: Static privates are a design anti-pattern, in that no one can reach into > them but the class, but they require definition in the .h (thus affecting > consumers). The general argument "for" them is "So tests can call into them", > but that means writing tests which aren't black-box tests, and those are > generally bad tests. > > So the approach is to move static privates into the .cc file (since that's the > only place they can be called) and in the unnamed namespace. > > The exception to this is when a single class spans multiple TUs (for example, > foo.cc and foo_win.cc/foo_linux.cc, with _win/_linux needing to call some core > functionality, and without using inheritance as the platform abstraction layer), > but this is TransportSecurityStatic, so it doesn't rise to that level It's currently structured like this to be able to inject ExpectStapleReports, in order to make the verification of the serialized form in tests feasible. I think this goes back to my earlier point of deciding on how TransportSecurityState should interact with ExpectStapleReport. Maybe the thing to do here is break out a reporter, similar to the ChromeExpectCTReporter, but Emily and I were trying to avoid doing that, especially since most of the useful code is already inside of an anonymous namespace in transport_security_state.cc anyway. https://codereview.chromium.org/2040513003/diff/170001/net/http/transport_sec... File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/2040513003/diff/170001/net/http/transport_sec... net/http/transport_security_state_unittest.cc:1897: static bool LoadOCSPFromFile(std::string file_name, OCSPTest* ocsp) { On 2016/06/16 21:49:29, Ryan Sleevi wrote: > Why static? Matching other tests, and this doesn't/shouldn't need to touch member variables? Although I'll admit I'm not particularly familiar with this part of the style guide. https://codereview.chromium.org/2040513003/diff/170001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2040513003/diff/170001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:1445: transport_security_state_->CheckExpectStaple( On 2016/06/16 21:49:30, Ryan Sleevi wrote: > Putting it at this layer creates issues when multiplexing sockets. > > If I have Expect-Staple for http://mail.google.com but not http://google.com, this won't > trigger if those connections ar emultiplexed. Why isn't this an issue for Expect CT?
https://codereview.chromium.org/2040513003/diff/170001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2040513003/diff/170001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:1445: transport_security_state_->CheckExpectStaple( On 2016/06/17 17:26:55, dadrian wrote: > On 2016/06/16 21:49:30, Ryan Sleevi wrote: > > Putting it at this layer creates issues when multiplexing sockets. > > > > If I have Expect-Staple for http://mail.google.com but not http://google.com, > this won't > > trigger if those connections ar emultiplexed. > > Why isn't this an issue for Expect CT? Expect-CT is processed per-request at the URLRequestHTTPJob layer. That's more-or-less out of necessity for Expect-CT, because we require an Expect-CT HTTP response header. Another interesting question is "Why isn't this an issue for PKP reporting?", and IIRC the answer to that is "It is." That is, for PKP reporting, we don't always check pins when pooling (only for QUIC and SPDY), so if a request for mail.google.com gets sent over a connection that was originally made for a request to google.com, we won't send a pinning report (or enforce pins, for that matter). Is my understanding correct, Ryan? (This is all for PKP enforcing-with-reporting, not PKP-Report-Only.) The downside to doing reporting at the URLRequestHTTPJob layer is that we will send a lot more reports (every request to google.com instead of every connection to google.com). (I'm pondering whether it is useful to a site operator to receive a report in Ryan's example of multiplexing mail.google.com and google.com requests, but have to run to an interview, will ponder more later.)
https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... File net/cert/expect_staple_report.h (right): https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report.h:46: // at least as recent as |verify_time - max_age|. On 2016/06/17 17:26:54, dadrian wrote: > At some point ExpectStaple needs to have validation for every stapled > SingleResponse, and be able to discern between various types of parsing or > validation failures. There's the question of should the OCSP layer contain all > that logic, and just hand ExpectStaple some object, which it can then serialize, > or if ExpectStaple should handle the whole things. That presupposes "ExpectStaple" is a thing. I'm challenging that notion of the class design, at the fundamental level. I'm suggesting, yes, that certificate verification is where we should capture all of the OCSP-related verification logic, and we should surface sufficient fidelity of information to explain the results. > I'd say that these designs are effectively the same, with the only difference > being the name of the code that's currently in expect_staple_report.h. I disagree on that, but apologies if my concerns are not clearer. Even if the code performs the same logical tasks, the structure and design of it significantly affect maintainability and testability. > The > current ExpectStapleReport class doesn't actually do any reporting, it basically > just does OCSP validation with detailed error information. Right, I consider that a bit of a smell - and I can't determine if it's solely naming or if it's design, but I lean on the latter. > The actual > serialization and reporting takes place in TransportSecurityState, which is > exactly how PKP reporting works. Whether or not that's a good thing is an open question on the other bug :) > The ExpectStapleReport class could just as > easily be named OCSPStaple, there's nothing particularly "report"-like about it, > rather than basic validation. Right, and I would push back on that design with similar concerns. > Then there's the question of how does the rest of the net stack interact with > this class? Right now it's structured so that the OCSP parsing only happens if > the server is in the preload list, and I think it makes sense to leave it that > way. I don't, because we're already doing OCSP work in CertVerifier by supplying the stapled response, which we're then passing to the crypto libraries (NSS and Windows only, because the OS X code is a do-nothing stub through 10.11 but is fixed in Sierra) > Ideally, OCSP verification would be added to CertVerifyResult and take > place as part of the rest of certificate validation, but given that Expect > Staple is only reporting and only for a tiny subset of hosts, I don't think it > makes sense to add it to certificate verification, especially given that the > prerequisite code for even validating signatures on OCSP responses still hasn't > landed. I'm not sure why you say that. Is there something I'm missing? This seems the correct end goal, and is consistent with the short-term need. (In general, I push back pretty hard whenever someone says "Temporary", because it has yet to be so in my experience) > In summary, I think the big questions are: > > 1. What should the ExpectStapleReport class be called, and where should it go? > 2. How should the ExpectStapleReport class be constructed? Without wanting to be too contrarian, given how much I've pushed back, I don't think these are the right questions. 1) What is our end-goal for how OCSP fits in the network stack? 2) What is the necessary minimum implementation to meet ExpectStaple? To me, #1 is clearly that OCSP stapling is integrated into certificate verification - the CertVerifier interface was intentionally designed to accommodate this. Our current implementations (NSS & Windows) don't allow us to fill out diagnostic information about it - but there's no reason we can't use our in-house OCSP parsing/validation code, across all implementations, to do that. The pivot point for adding such logic is the CertVerifyProc base-class, which performs the pre-/post-(platform) verification tasks, so that all implementations have consistent inputs/outputs to CVP::VerifyInternal. So adding an OCSP verification step - presumably, after the OS verification logic - to CVP::Verify is the right approach, signaling that data back up via CertVerifyResult. For reporting/ExpectStaple, the TSS maintains a database of what hosts expect stapling (which Emily already did). The reporting - for Expect - can go a number of places, depending on what the ExpectStaple semantics are meant to imply. If, like PKP-RO, it triggers based on header processing, then that goes into URLRequestHttpJob Something like UrlRequestHttpJob::OnWhatever[] if (tss->ShouldExpectOCSPStapling(...) && !ssl_info.ocsp.was_awesome) { tss->SendExpectOCSPReport(ssl_info.ocsp, ...); } If ExpectStaple is meant to be preload-based only (no headers), then we'd need to push it into the triumvirate, for now - SSLClientSocket, SpdySession::CanPool, and ProofVerifierChromium - to send the results after getting the CertVerifyResult from the CertVerifier.
https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... File net/cert/expect_staple_report.h (right): https://codereview.chromium.org/2040513003/diff/170001/net/cert/expect_staple... net/cert/expect_staple_report.h:27: struct SingleResult { On 2016/06/17 16:19:54, Ryan Sleevi wrote: > What is the API if a SingleResult couldn't be parsed? Not currently exposed cleanly, I added some comments to the the Expect-Staple design doc with thoughts on how to expose errors. https://docs.google.com/document/d/1aISglJIIwglcOAhqNfK-2vtQl-_dWAapc-VLDh-9-BE/
https://codereview.chromium.org/2040513003/diff/190001/net/cert/cert_verify_p... File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/2040513003/diff/190001/net/cert/cert_verify_p... net/cert/cert_verify_proc.cc:254: return; nit: omit. https://codereview.chromium.org/2040513003/diff/190001/net/cert/cert_verify_p... net/cert/cert_verify_proc.cc:334: CheckOCSP(ocsp_response, verify_result); Should this be done even for blacklisted keys? https://codereview.chromium.org/2040513003/diff/190001/net/cert/internal/pars... File net/cert/internal/parse_ocsp.h (right): https://codereview.chromium.org/2040513003/diff/190001/net/cert/internal/pars... net/cert/internal/parse_ocsp.h:283: // verify_time - max_age. Might want to reword to "verify_time <= thisUpdate + max_age". https://codereview.chromium.org/2040513003/diff/190001/net/cert/ocsp_verify_r... File net/cert/ocsp_verify_result.cc (right): https://codereview.chromium.org/2040513003/diff/190001/net/cert/ocsp_verify_r... net/cert/ocsp_verify_result.cc:16: void OCSPVerifyResult::Reset() { Clear stapled_response? https://codereview.chromium.org/2040513003/diff/190001/net/der/parse_values.cc File net/der/parse_values.cc (right): https://codereview.chromium.org/2040513003/diff/190001/net/der/parse_values.c... net/der/parse_values.cc:386: der::GeneralizedTime ConvertBaseTime(const base::Time::Exploded& exploded) { Is this called from anywhere other than here? Might just want to provide ConvertBaseUTCTime if not. https://codereview.chromium.org/2040513003/diff/190001/net/http/transport_sec... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/190001/net/http/transport_sec... net/http/transport_security_state.cc:160: std::string OCSPCertStatusToString(OCSPCertStatus::Status status) { Possibly attach this to ocsp_verify_result? https://codereview.chromium.org/2040513003/diff/190001/net/http/transport_sec... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2040513003/diff/190001/net/http/transport_sec... net/http/transport_security_state.h:400: // |is_issued_by_known_root| is true. Update comment. https://codereview.chromium.org/2040513003/diff/190001/net/tools/testserver/m... File net/tools/testserver/minica.py (right): https://codereview.chromium.org/2040513003/diff/190001/net/tools/testserver/m... net/tools/testserver/minica.py:287: elif ocsp_date == OCSP_DATE_YOUNG: EARLY? https://codereview.chromium.org/2040513003/diff/190001/net/url_request/url_re... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/2040513003/diff/190001/net/url_request/url_re... net/url_request/url_request_unittest.cc:9250: ::testing::AssertionResult DoConnection( Is this change necessary? https://codereview.chromium.org/2040513003/diff/190001/net/url_request/url_re... net/url_request/url_request_unittest.cc:9275: ::testing::AssertionResult DoConnection( Same.
Next suggested split: Split the "move OCSP to CertVerifyProc" into a separate CL. That's something that can be easily bitten in to. https://codereview.chromium.org/2040513003/diff/190001/net/cert/ocsp_verify_r... File net/cert/ocsp_verify_result.h (right): https://codereview.chromium.org/2040513003/diff/190001/net/cert/ocsp_verify_r... net/cert/ocsp_verify_result.h:57: base::Optional<OCSPCertStatus::Status> cert_status; Reviewer Feels time: I'd really love to avoid introducing base::Optional into net/. I think it's a mistake (in boost and C++17) and one of those things that I think is really gross that we did in //base anyways but that doesn't mean it's all that nice. https://codereview.chromium.org/2040513003/diff/190001/net/cert/ocsp_verify_r... net/cert/ocsp_verify_result.h:59: // Any stapled responses. From the doc and discussion with Emily, I think this bears clarifying a bit more what you're talking about (e.g. is this the BasicResponse data, etc). When in doubt, cite RFC :) https://codereview.chromium.org/2040513003/diff/190001/net/http/transport_sec... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2040513003/diff/190001/net/http/transport_sec... net/http/transport_security_state.cc:160: std::string OCSPCertStatusToString(OCSPCertStatus::Status status) { On 2016/06/23 14:03:15, svaldez wrote: > Possibly attach this to ocsp_verify_result? I'm actually supportive of leaving it here, since these strings are meaningful in the report-API sense, not just the C++ API sense https://codereview.chromium.org/2040513003/diff/190001/net/http/transport_sec... net/http/transport_security_state.cc:160: std::string OCSPCertStatusToString(OCSPCertStatus::Status status) { return "const char*" - don't force a string coercion unless the caller wants it :) https://codereview.chromium.org/2040513003/diff/190001/net/http/transport_sec... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2040513003/diff/190001/net/http/transport_sec... net/http/transport_security_state.h:22: #include "net/cert/x509_certificate.h" Don't introduce this headers https://codereview.chromium.org/2040513003/diff/190001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.h (right): https://codereview.chromium.org/2040513003/diff/190001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.h:137: void ReportOCSP(); I'd like to avoid adding a dedicated state here; it's actually on the agenda of "cleanups Sleevi wants to hack on" to clean up how the CT/cert verification stuff works, but let's not add an extra method for it https://codereview.chromium.org/2040513003/diff/190001/net/test/spawned_test_... File net/test/spawned_test_server/base_test_server.h (right): https://codereview.chromium.org/2040513003/diff/190001/net/test/spawned_test_... net/test/spawned_test_server/base_test_server.h:96: }; Document |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
