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)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: cviecco, Assigned: cviecco)
References
Details
Attachments
(1 file, 1 obsolete file)
1.58 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
Several bugs would like to insert errors not computed by NSS into the verifylog. We need a common mechanism to do this
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #8363824 -
Attachment is obsolete: true
Attachment #8363854 -
Flags: review?(dkeeler)
Comment 3•11 years ago
|
||
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?
Assignee | ||
Comment 4•11 years ago
|
||
I matters as the code that decides to put overrides or not only looks at the log.
Comment 5•11 years ago
|
||
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?
Assignee | ||
Comment 6•11 years ago
|
||
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 7•11 years ago
|
||
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+
Assignee | ||
Comment 8•11 years ago
|
||
Comment 9•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 10•11 years ago
|
||
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. :)
Comment 11•11 years ago
|
||
(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
Comment 12•11 years ago
|
||
(for the record, insertErrorIntoVerifyLog has been #ifdeffed out for the time being, as part of bug 968491.)
Updated•11 years ago
|
Version: unspecified → Trunk
You need to log in
before you can comment on or make changes to this bug.
Description
•