Closed Bug 962693 Opened 11 years ago Closed 11 years ago

Add call to add arbitrary errors to verifylog in certverify

Categories

(Core :: Security: PSM, enhancement)

enhancement
Not set
normal

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+
Status: NEW → RESOLVED
Closed: 11 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
Blocks: 968491
(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.

Attachment

General

Created:
Updated:
Size: