Closed Bug 962693 Opened 6 years ago Closed 6 years ago

Add call to add arbitrary errors to verifylog in certverify

Categories

(Core :: Security: PSM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: cviecco, Assigned: cviecco)

References

Details

Attachments

(1 file, 1 obsolete file)

Several bugs would like to insert errors not computed by NSS into the verifylog. We need a common mechanism to do this
Assignee: nobody → cviecco
Blocks: 744204
Attachment #8363824 - Attachment is obsolete: true
Attachment #8363854 - Flags: review?(dkeeler)
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.
Attachment #8363854 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/mozilla-central/rev/2cb9c4d1af29
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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.)
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.