Comment on attachment 8565123 [details] [diff] [review] Add telemetry to measure the impact of matching KeyUsage to key exchange algorithm Review of attachment 8565123 [details] [diff] [review]: ----------------------------------------------------------------- Cool - r=me with nits addressed. ::: security/certverifier/CertVerifier.cpp @@ +516,5 @@ > /*optional*/ const SECItem* stapledOCSPResponse, > Time time, > /*optional*/ void* pinarg, > const char* hostname, > + /*XXX: optional*/ const KeyUsage* keaKeyUsage, Why the "XXX" here? ::: security/certverifier/CertVerifier.h @@ +58,2 @@ > bool saveIntermediatesInPermanentDatabase = false, > Flags flags = 0, nit: let's update the /*optional*/ comments for saveIntermediates... and flags here too ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +1058,5 @@ > + unsigned int value; > + > + // We need to map the key usage values to different values for telemetry > + // because telemetry bucket 0 is the "invalid/other" bucket and because > + // mozilla::pkix doesn't have a stable ABI so they mozilla::pkix values typo: s/they/the/

Comment on attachment 8565146 [details] [diff] [review] Add telemetry to measure the impact of matching cipher suite key type to certificate key type Review of attachment 8565146 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but I think we should be a little more defensive. r=me with that addressed. ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +1001,5 @@ > + return; // do nothing for impossible case > + } > + > + CERTCertificate* cert = CERT_LIST_HEAD(certList)->cert; > + ScopedSECKEYPublicKey key(CERT_ExtractPublicKey(cert)); I think we should do a little more error-checking: assert(!(CERT_LIST_END(CERT_LIST_HEAD(certList), certList))); and if (key) { Telemetry::Accumulate(...); }

Nits from review of v1 addressed, re-implemented in terms of SSL_GetPreliminaryChannelInfo.

Comments on v1 addressed, rebased on top of Part 1, v2.

Comment on attachment 8585896 [details] [diff] [review] Part 1: Add telemetry to measure the impact of matching KeyUsage to key exchange algorithm [v2] Review of attachment 8585896 [details] [diff] [review]: ----------------------------------------------------------------- LGTM - I just have a question about setting actualKeyUsage in VerifyCert after calling BuildCertChainForOneKeyUsage. r=me with that addressed. ::: security/certverifier/CertVerifier.cpp @@ +331,4 @@ > ocspStaplingStatus); > if (rv == Success) { > + if (actualKeyUsage) { > + *actualKeyUsage = KeyUsage::digitalSignature; Doesn't actualKeyUsage get set by BuildCertChainForOneKeyUsage / BuildCertChainWrapper? (and, more to the point, mightn't it be different from KeyUsage::digitalSignature?) @@ +361,4 @@ > ocspStaplingStatus); > if (rv == Success) { > + if (actualKeyUsage) { > + *actualKeyUsage = KeyUsage::digitalSignature; Same question. @@ +385,4 @@ > ocspStaplingStatus); > + if (rv == Success) { > + if (actualKeyUsage) { > + *actualKeyUsage = KeyUsage::digitalSignature; Here too. ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +1495,5 @@ > stapledOCSPResponse = &csa->items[0]; > } > > + KeyUsage keaKeyUsage; > + { Why the additional scope?

Comment on attachment 8585898 [details] [diff] [review] Part 2: Add telemetry to measure the impact of matching cipher suite key type to certificate key type [v2] Review of attachment 8585898 [details] [diff] [review]: ----------------------------------------------------------------- Great - r=me.

Comment on attachment 8585896 [details] [diff] [review] Part 1: Add telemetry to measure the impact of matching KeyUsage to key exchange algorithm [v2] Review of attachment 8585896 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/certverifier/CertVerifier.cpp @@ +331,4 @@ > ocspStaplingStatus); > if (rv == Success) { > + if (actualKeyUsage) { > + *actualKeyUsage = KeyUsage::digitalSignature; Yes, you are right. I think this was search & replace gone wrong during editing. @@ +361,4 @@ > ocspStaplingStatus); > if (rv == Success) { > + if (actualKeyUsage) { > + *actualKeyUsage = KeyUsage::digitalSignature; Yep. @@ +385,4 @@ > ocspStaplingStatus); > + if (rv == Success) { > + if (actualKeyUsage) { > + *actualKeyUsage = KeyUsage::digitalSignature; Right. ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +1495,5 @@ > stapledOCSPResponse = &csa->items[0]; > } > > + KeyUsage keaKeyUsage; > + { Just trying to isolate this code without making it a separate function. Would you prefer it without the additional scope?

Comment on attachment 8585896 [details] [diff] [review] Part 1: Add telemetry to measure the impact of matching KeyUsage to key exchange algorithm [v2] Review of attachment 8585896 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +1495,5 @@ > stapledOCSPResponse = &csa->items[0]; > } > > + KeyUsage keaKeyUsage; > + { My sense is it would be fine without the additional scope, but with a blank line before and after, as it is now. Not a big deal, though - I just found it puzzling.

I removed the unnecessary chunks that David pointed out.

This is a bug worth fixing but I don't have the time to finish this, so unassigning myself.