Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2066603004:
Return enum from TransportSecurityState::CheckPublicKeyPins (Closed)
DescriptionReturn enum from TransportSecurityState::CheckPublicKeyPins
BUG=618775
Committed: https://crrev.com/8f89466532aebd328b3bbff4db8a9dd3bfc3d631
Cr-Commit-Position: refs/heads/master@{#401140}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Add comments #
Total comments: 7
Patch Set 3 : Add DCHECK() #
Total comments: 4
Patch Set 4 : Flatten DCHECK #
Total comments: 1
Patch Set 5 : Kill words. #Patch Set 6 : Rebase #Patch Set 7 : Set CERT_STATUS_PINNED_KEY_MISSING #
Messages
Total messages: 36 (12 generated)
Description was changed from ========== Return enum from TransportSecurityState::CheckPublicKeyPins BUG=618775 ========== to ========== Return enum from TransportSecurityState::CheckPublicKeyPins BUG=618775 ==========
dadrian@google.com changed reviewers: + estark@chromium.org, rsleevi@chromium.org
Follow up from last week's change. This could be taken further to store the enum instead of the bool in each socket type, or it could be left as is.
Mostly questions for Emily here https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... net/http/transport_security_state.cc:687: return pin_validity; I'm not sure why you changed it, but we prefer to do short-circuits early. if (!is_issued_by_known_root) return pin_validity; if (pin_validity == ... ) { LOG(ERROR) << *pinning_failure_log; // rsleevi note: No idea why we're doing this and I don't think it's right but I'll let Emily chime in ReportUMAOnPinFailure(...); // No idea why we're doing this either since I doubt anyone's checking, but again defer to Emily but I suspect it's been broken since the new generator landed } UMA_HISTOGRAM_BOOLEAN(..., pin_validity == PKPStatus::OK); https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... net/http/transport_security_state.h:111: enum class PKPStatus { VIOLATED = -1, OK = 0, BYPASSED = 1 }; STYLE: No need to assign explicit values. STYLE: No benefit from making it an "enum class" here because it doesn't allow forward declaration STYLE: Add documentation explaining what these statuses mean. Example enum PKPStatus { // Pinning was enabled and the necessary pins were not present. VIOLATED, // Pinning was not enabled, or it was and the certificate satisfied // the pins. OK, // Pinning was enabled, but ignored due to local policy, such as // a local trust anchor. BYPASSED, }; NAMING: Is "violated" the right status here? RFC 7469 doesn't call it violated, nor does our code (I guess, not until report_uri was introduced). In the context of "CheckPublicKeyPins", what are natural ways of reading the possible results? https://codereview.chromium.org/2066603004/diff/1/net/quic/crypto/proof_verif... File net/quic/crypto/proof_verifier_chromium.cc (right): https://codereview.chromium.org/2066603004/diff/1/net/quic/crypto/proof_verif... net/quic/crypto/proof_verifier_chromium.cc:354: break; Should this be an explicit/intentional // Fall through rather than break; So that it's clear we're treating bypassed as equal to OK
https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... net/http/transport_security_state.cc:687: return pin_validity; On 2016/06/15 02:04:03, Ryan Sleevi wrote: > I'm not sure why you changed it, but we prefer to do short-circuits early. > > if (!is_issued_by_known_root) > return pin_validity; > if (pin_validity == ... ) { > LOG(ERROR) << *pinning_failure_log; // rsleevi note: No idea why we're doing > this and I don't think it's right but I'll let Emily chime in > ReportUMAOnPinFailure(...); // No idea why we're doing this either since I > doubt anyone's checking, but again defer to Emily but I suspect it's been broken > since the new generator landed Both |pinning_failure_log| and the UMA histograms long predate me. AFAIK nobody looks at them and we should probably remove them. What do you mean by "the new generator" though? > } > UMA_HISTOGRAM_BOOLEAN(..., pin_validity == PKPStatus::OK);
https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... net/http/transport_security_state.cc:687: return pin_validity; On 2016/06/15 02:35:00, estark wrote: > What do you mean by "the new generator" though? In order for the histograms to be useful, the UMA ID must never change, and gaps must never be added. I'm not sure with Lucas' script to generate the preload dataset if we're still preserving the immutability of that - I'd be surprised if we were, because that requires a state going back to the first time we were doing the preloads. Otherwise, the UMA is only valid for the TransportSecurityState version of the dataset, which isn't that useful.
https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... net/http/transport_security_state.cc:687: return pin_validity; On 2016/06/15 02:35:00, estark wrote: > On 2016/06/15 02:04:03, Ryan Sleevi wrote: > > I'm not sure why you changed it, but we prefer to do short-circuits early. > > > > if (!is_issued_by_known_root) > > return pin_validity; > > if (pin_validity == ... ) { > > LOG(ERROR) << *pinning_failure_log; // rsleevi note: No idea why we're > doing > > this and I don't think it's right but I'll let Emily chime in > > ReportUMAOnPinFailure(...); // No idea why we're doing this either since I > > doubt anyone's checking, but again defer to Emily but I suspect it's been > broken > > since the new generator landed > > Both |pinning_failure_log| and the UMA histograms long predate me. AFAIK nobody > looks at them and we should probably remove them. > > What do you mean by "the new generator" though? > > > } > > UMA_HISTOGRAM_BOOLEAN(..., pin_validity == PKPStatus::OK); > |pinning_failure_log| is added to the certificate error reports, but that's it. https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... net/http/transport_security_state.h:111: enum class PKPStatus { VIOLATED = -1, OK = 0, BYPASSED = 1 }; On 2016/06/15 02:04:03, Ryan Sleevi wrote: > STYLE: No need to assign explicit values. > STYLE: No benefit from making it an "enum class" here because it doesn't allow > forward declaration > STYLE: Add documentation explaining what these statuses mean. > > Example > > enum PKPStatus { > // Pinning was enabled and the necessary pins were not present. > VIOLATED, > > // Pinning was not enabled, or it was and the certificate satisfied > // the pins. > OK, > > // Pinning was enabled, but ignored due to local policy, such as > // a local trust anchor. > BYPASSED, > }; > > NAMING: Is "violated" the right status here? RFC 7469 doesn't call it violated, > nor does our code (I guess, not until report_uri was introduced). In the context > of "CheckPublicKeyPins", what are natural ways of reading the possible results? I went with violated because that's what I've heard in conversation, both in and out of Google, although I have no particular attachment to the name. https://codereview.chromium.org/2066603004/diff/1/net/quic/crypto/proof_verif... File net/quic/crypto/proof_verifier_chromium.cc (right): https://codereview.chromium.org/2066603004/diff/1/net/quic/crypto/proof_verif... net/quic/crypto/proof_verifier_chromium.cc:354: break; On 2016/06/15 02:04:03, Ryan Sleevi wrote: > Should this be an explicit/intentional > > // Fall through > > rather than break; > > So that it's clear we're treating bypassed as equal to OK I'm wary of using any non-empty fall through, just in a case a future enum value is added. Instead, we could add a comment indicating we're treating bypass as equal to OK.
https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... net/http/transport_security_state.cc:687: return pin_validity; On 2016/06/15 02:44:36, Ryan Sleevi wrote: > On 2016/06/15 02:35:00, estark wrote: > > What do you mean by "the new generator" though? > > In order for the histograms to be useful, the UMA ID must never change, and gaps > must never be added. I'm not sure with Lucas' script to generate the preload > dataset if we're still preserving the immutability of that - I'd be surprised if > we were, because that requires a state going back to the first time we were > doing the preloads. > > Otherwise, the UMA is only valid for the TransportSecurityState version of the > dataset, which isn't that useful. Ah, I see. https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... net/http/transport_security_state.cc:1142: return PKPStatus::VIOLATED; This whole little block should really be a DCHECK, I think. bool found_state = GetDynamicPKPState(...) || GetStaticDomainState(...); DCHECK(found_state); Maybe that change doesn't belong in this CL though.
Updated to address rsleevi@'s comments, although I haven't touched the UMA code because I don't think we have consensus on that. https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... net/http/transport_security_state.cc:687: return pin_validity; On 2016/06/15 02:48:04, estark wrote: > On 2016/06/15 02:44:36, Ryan Sleevi wrote: > > On 2016/06/15 02:35:00, estark wrote: > > > What do you mean by "the new generator" though? > > > > In order for the histograms to be useful, the UMA ID must never change, and > gaps > > must never be added. I'm not sure with Lucas' script to generate the preload > > dataset if we're still preserving the immutability of that - I'd be surprised > if > > we were, because that requires a state going back to the first time we were > > doing the preloads. > > > > Otherwise, the UMA is only valid for the TransportSecurityState version of the > > dataset, which isn't that useful. > > Ah, I see. Added the bail-out-early, but am currently leaving the UMA code for another CL. https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... net/http/transport_security_state.cc:1142: return PKPStatus::VIOLATED; On 2016/06/15 02:48:04, estark wrote: > This whole little block should really be a DCHECK, I think. > > bool found_state = GetDynamicPKPState(...) || GetStaticDomainState(...); > DCHECK(found_state); > > Maybe that change doesn't belong in this CL though. Acknowledged. https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... net/http/transport_security_state.h:111: enum class PKPStatus { VIOLATED = -1, OK = 0, BYPASSED = 1 }; On 2016/06/15 02:47:55, dadrian wrote: > On 2016/06/15 02:04:03, Ryan Sleevi wrote: > > STYLE: No need to assign explicit values. > > STYLE: No benefit from making it an "enum class" here because it doesn't allow > > forward declaration > > STYLE: Add documentation explaining what these statuses mean. > > > > Example > > > > enum PKPStatus { > > // Pinning was enabled and the necessary pins were not present. > > VIOLATED, > > > > // Pinning was not enabled, or it was and the certificate satisfied > > // the pins. > > OK, > > > > // Pinning was enabled, but ignored due to local policy, such as > > // a local trust anchor. > > BYPASSED, > > }; > > > > NAMING: Is "violated" the right status here? RFC 7469 doesn't call it > violated, > > nor does our code (I guess, not until report_uri was introduced). In the > context > > of "CheckPublicKeyPins", what are natural ways of reading the possible > results? > > I went with violated because that's what I've heard in conversation, both in and > out of Google, although I have no particular attachment to the name. I left it with enum class because I think we want the scoping benefits?
https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... net/http/transport_security_state.h:111: enum class PKPStatus { VIOLATED = -1, OK = 0, BYPASSED = 1 }; On 2016/06/15 18:58:28, dadrian wrote: > I left it with enum class because I think we want the scoping benefits? Scoping benefits? We don't gain anything in terms of scoping/naming resolution, since we're a nested class, just in type safety. Was that what you meant?
https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... net/http/transport_security_state.h:111: enum class PKPStatus { VIOLATED = -1, OK = 0, BYPASSED = 1 }; On 2016/06/15 19:02:23, Ryan Sleevi wrote: > On 2016/06/15 18:58:28, dadrian wrote: > > I left it with enum class because I think we want the scoping benefits? > > Scoping benefits? We don't gain anything in terms of scoping/naming resolution, > since we're a nested class, just in type safety. Was that what you meant? Both? We're trying to represent the result of PKP actions, not TransportSecurityState as a whole, so saying TransportSecurityState::OK (vs TransportSecurityState::PKPStatus::OK) shouldn't have any meaning unless we convert every TransportSecurityState operation to use the same enum.
On 2016/06/15 19:33:16, dadrian wrote: > https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... > File net/http/transport_security_state.h (right): > > https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... > net/http/transport_security_state.h:111: enum class PKPStatus { VIOLATED = -1, > OK = 0, BYPASSED = 1 }; > On 2016/06/15 19:02:23, Ryan Sleevi wrote: > > On 2016/06/15 18:58:28, dadrian wrote: > > > I left it with enum class because I think we want the scoping benefits? > > > > Scoping benefits? We don't gain anything in terms of scoping/naming > resolution, > > since we're a nested class, just in type safety. Was that what you meant? > > Both? We're trying to represent the result of PKP actions, not > TransportSecurityState as a whole, so saying TransportSecurityState::OK (vs > TransportSecurityState::PKPStatus::OK) shouldn't have any meaning unless we > convert every TransportSecurityState operation to use the same enum. So do we have consensus on what needs to happen for this CL to committed? Are we modifying the UMA or logger calls?
On 2016/06/20 17:33:40, dadrian wrote: > On 2016/06/15 19:33:16, dadrian wrote: > > > https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... > > File net/http/transport_security_state.h (right): > > > > > https://codereview.chromium.org/2066603004/diff/1/net/http/transport_security... > > net/http/transport_security_state.h:111: enum class PKPStatus { VIOLATED = -1, > > OK = 0, BYPASSED = 1 }; > > On 2016/06/15 19:02:23, Ryan Sleevi wrote: > > > On 2016/06/15 18:58:28, dadrian wrote: > > > > I left it with enum class because I think we want the scoping benefits? > > > > > > Scoping benefits? We don't gain anything in terms of scoping/naming > > resolution, > > > since we're a nested class, just in type safety. Was that what you meant? > > > > Both? We're trying to represent the result of PKP actions, not > > TransportSecurityState as a whole, so saying TransportSecurityState::OK (vs > > TransportSecurityState::PKPStatus::OK) shouldn't have any meaning unless we > > convert every TransportSecurityState operation to use the same enum. > > So do we have consensus on what needs to happen for this CL to committed? Are we > modifying the UMA or logger calls? I think it would be fine to leave the UMA/logging calls alone in this CL; we can clean those up in a subsequent CL, to avoid making unrelated changes here. (Maybe you could file a bug so we don't forget, though.) Ryan: sounds like David wanted the enum class to get names like TransportSecurityState::PKPStatus::OK instead of TransportSecurityState::OK... how do you feel about that? Would you prefer an ordinary enum with values like PKP_STATUS_OK?
If we punt the UMA, we should file a bug. I can buy this as an LGTM % nits As far as naming, the best I've got is "broken" or "mismatched" rather than "violated". https://codereview.chromium.org/2066603004/diff/20001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2066603004/diff/20001/net/http/transport_secu... net/http/transport_security_state.cc:1143: return PKPStatus::VIOLATED; Isn't this really a fourth state - an error one? estark: Should we [D]CHECKing this? Isn't this always a programmer error? https://codereview.chromium.org/2066603004/diff/20001/net/quic/crypto/proof_v... File net/quic/crypto/proof_verifier_chromium.cc (right): https://codereview.chromium.org/2066603004/diff/20001/net/quic/crypto/proof_v... net/quic/crypto/proof_verifier_chromium.cc:353: // BYPASSED is treated the same as OK STYLE: Should be a complete sentence, with punctuation. https://codereview.chromium.org/2066603004/diff/20001/net/quic/crypto/proof_v... net/quic/crypto/proof_verifier_chromium.cc:355: break; Still nervous about the claim "is" on line 353, since that's not what the code is doing structurally. Really it's a question of omitting the break or not, and https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements totally allows it to be omitted.
https://codereview.chromium.org/2066603004/diff/20001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2066603004/diff/20001/net/http/transport_secu... net/http/transport_security_state.cc:1143: return PKPStatus::VIOLATED; On 2016/06/21 00:35:22, Ryan Sleevi wrote: > Isn't this really a fourth state - an error one? > > estark: Should we [D]CHECKing this? Isn't this always a programmer error? Yeah, I think I also suggested DCHECKing this in an earlier comment. If you're okay doing that in this CL, and David's willing to do it, maybe we should go ahead and do that. (Certainly seems preferable to adding a fourth state only to remove it in a later cleanup CL.)
Filed https://crbug.com/621980 and added the DCHECK. Will commit once estark OK's the DCHECK. https://codereview.chromium.org/2066603004/diff/20001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2066603004/diff/20001/net/http/transport_secu... net/http/transport_security_state.cc:1143: return PKPStatus::VIOLATED; On 2016/06/21 00:47:34, estark wrote: > On 2016/06/21 00:35:22, Ryan Sleevi wrote: > > Isn't this really a fourth state - an error one? > > > > estark: Should we [D]CHECKing this? Isn't this always a programmer error? > > Yeah, I think I also suggested DCHECKing this in an earlier comment. If you're > okay doing that in this CL, and David's willing to do it, maybe we should go > ahead and do that. (Certainly seems preferable to adding a fourth state only to > remove it in a later cleanup CL.) Done. https://codereview.chromium.org/2066603004/diff/20001/net/quic/crypto/proof_v... File net/quic/crypto/proof_verifier_chromium.cc (right): https://codereview.chromium.org/2066603004/diff/20001/net/quic/crypto/proof_v... net/quic/crypto/proof_verifier_chromium.cc:353: // BYPASSED is treated the same as OK On 2016/06/21 00:35:22, Ryan Sleevi wrote: > STYLE: Should be a complete sentence, with punctuation. Done. https://codereview.chromium.org/2066603004/diff/20001/net/quic/crypto/proof_v... net/quic/crypto/proof_verifier_chromium.cc:355: break; On 2016/06/21 00:35:22, Ryan Sleevi wrote: > Still nervous about the claim "is" on line 353, since that's not what the code > is doing structurally. > > Really it's a question of omitting the break or not, and > https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements > totally allows it to be omitted. Done.
https://codereview.chromium.org/2066603004/diff/40001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2066603004/diff/40001/net/http/transport_secu... net/http/transport_security_state.cc:687: UMA_HISTOGRAM_BOOLEAN("Net.PublicKeyPinSuccess", pin_success); nit: you could just inline this https://codereview.chromium.org/2066603004/diff/40001/net/http/transport_secu... net/http/transport_security_state.cc:1146: if (!found_state) { No need to handle this case. You should just DCHECK(found_state) and then move on to `return CheckPinsAndMaybeSendReport(...)`. (See https://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...)
https://codereview.chromium.org/2066603004/diff/40001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2066603004/diff/40001/net/http/transport_secu... net/http/transport_security_state.cc:687: UMA_HISTOGRAM_BOOLEAN("Net.PublicKeyPinSuccess", pin_success); On 2016/06/21 18:13:42, estark wrote: > nit: you could just inline this Done. https://codereview.chromium.org/2066603004/diff/40001/net/http/transport_secu... net/http/transport_security_state.cc:1146: if (!found_state) { On 2016/06/21 18:13:42, estark wrote: > No need to handle this case. You should just DCHECK(found_state) and then move > on to `return CheckPinsAndMaybeSendReport(...)`. (See > https://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...) Done.
Thanks! lgtm with one more tiny nit https://codereview.chromium.org/2066603004/diff/60001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2066603004/diff/60001/net/http/transport_secu... net/http/transport_security_state.cc:1144: // been called, so if found_state is false, its an error. sorry, one more tiny nit: pipes around |found_state| and apostrophe on "its". Though you could maybe just cut it off at "to have been called" and leave it at that.
The CQ bit was checked by dadrian@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/2066603004/#ps80001 (title: "Kill words.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066603004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by dadrian@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/2066603004/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066603004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dadrian@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/2066603004/#ps120001 (title: "Set CERT_STATUS_PINNED_KEY_MISSING")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066603004/120001
Message was sent while issue was closed.
Description was changed from ========== Return enum from TransportSecurityState::CheckPublicKeyPins BUG=618775 ========== to ========== Return enum from TransportSecurityState::CheckPublicKeyPins BUG=618775 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Return enum from TransportSecurityState::CheckPublicKeyPins BUG=618775 ========== to ========== Return enum from TransportSecurityState::CheckPublicKeyPins BUG=618775 Committed: https://crrev.com/8f89466532aebd328b3bbff4db8a9dd3bfc3d631 Cr-Commit-Position: refs/heads/master@{#401140} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/8f89466532aebd328b3bbff4db8a9dd3bfc3d631 Cr-Commit-Position: refs/heads/master@{#401140} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
