Several bugs would like to insert errors not computed by NSS into the verifylog. We need a common mechanism to do this

The only errors that need to be in the VerifyLog are overridable errors. Otherwise, it doesn't matter if they're in the log or not, AFAICT. I don't think we're adding new kinds of overridable errors, so I'm not sure how having more errors in the log helps. For testing?

I matters as the code that decides to put overrides or not only looks at the log.

Right - so maybe we should change that code, instead (it's around https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/SSLServerCertVerification.cpp#552 ). Could we have VerifyCert set whatever errors we care about (e.g. "pinning mismatch") and have CreateCertErrorRunnable check for them specifically?

I dislike this option as it assumes that there is only one error we care about (I believe we should be able to tell the user a much as possible about errors). Adding this function will allow us to report multiple errors to the users.

Comment on attachment 8363854 [details] [diff] [review] certverify-add-insertErrorIntoVerifyLog (v1.1) Review of attachment 8363854 [details] [diff] [review]: ----------------------------------------------------------------- Ok - I think this is fine. Let's add a comment explaining that this is temporary and have it point to a bug filed where we plan to remove it/refactor the error-handling. ::: security/manager/ssl/src/CertVerifier.cpp @@ +42,5 @@ > +static SECStatus > +insertErrorIntoVerifyLog(CERTCertificate* cert, const PRErrorCode err, > + CERTVerifyLog* verifyLog){ > + PR_LOG(gPIPNSSLog, PR_LOG_ERROR, > + ("VerifyCert: insering error into LOG!\n")); I'm not sure this logging message is necessary.

This bug's added static-function is currently triggering this build warning (in clang, & likely also in gcc): > security/certverifier/CertVerifier.cpp:66:1: warning: unused function 'insertErrorIntoVerifyLog' [-Wunused-function] Presumably there are usages of this method coming up? I'm hoping so, in the interests of cleaner build output. :)

(In reply to Daniel Holbert [:dholbert] from comment #10) > Presumably there are usages of this method coming up? I'm hoping so, in the > interests of cleaner build output. :) Camilo is working on adding a usage. Also, I'm working on making it unnecessary; in fact, I've already removed it in my local tree: https://hg.mozilla.org/try/rev/8eb880ba6295

(for the record, insertErrorIntoVerifyLog has been #ifdeffed out for the time being, as part of bug 968491.)