Closed Bug 1040446 Opened 5 years ago Closed 5 years ago

Improve error code names/descriptions for invalid basic constraints

Categories

(Core :: Security: PSM, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox33 + fixed
firefox34 --- fixed

People

(Reporter: briansmith, Assigned: keeler, Mentored)

References

Details

Attachments

(2 files)

Now that we have an easy-to-customize error reporting mechanism in pkix/Result.h and pkix/pkixnss.h, we should improve the error codes returned for the case of invalid basic constraints. The badness is described by this comment in the source code:

      // We use Result::ERROR_CA_CERT_INVALID here so we can distinguish
      // this error from other errors, given that NSS does not have a "CA cert
      // used as end-entity" error code since it doesn't have such a
      // prohibition. We should add such an error code and stop abusing
      // Result::ERROR_CA_CERT_INVALID this way.
Attached patch patchSplinter Review
Brian, let me know if you have time to review patches like this. If not, I can find another reviewer.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8471025 - Flags: review?(brian)
Whiteboard: [good next bug]
Comment on attachment 8471025 [details] [diff] [review]
patch

Review of attachment 8471025 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have an opinion on the wording of the error message text, other than it seems better than the error message text that is currently being presented for this problem.
Attachment #8471025 - Flags: review?(brian) → review+
https://hg.mozilla.org/mozilla-central/rev/ce8b92c9ad63
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
Attached patch patch for betaSplinter Review
Approval Request Comment
[Feature/regressing bug #]: mozilla::pkix (this is needed for bug 1034124, which fixes a significant compatibility issue)
[User impact if declined]: see bug 1034124
[Describe test coverage new/current, TBPL]: there is adequate test coverage
[Risks and why]: low
[String/UUID change made/needed]: none (the lack of localizable strings is handled in bug 1052529)
Attachment #8486075 - Flags: review+
Attachment #8486075 - Flags: approval-mozilla-beta?
[Tracking Requested - why for this release]: see comment 5
Comment on attachment 8486075 [details] [diff] [review]
patch for beta

Approving this prereq for bug 1034124 and bug 1039064 for beta.
Attachment #8486075 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8486075 [details] [diff] [review]
patch for beta

This doesn't apply to beta at all.
Attachment #8486075 - Attachment is obsolete: true
Flags: needinfo?(dkeeler)
Comment on attachment 8486075 [details] [diff] [review]
patch for beta

Ah, this applies on top of bug 1039064. Still holding off on pushing this until bug 1034124 comment 25 is sorted out, though.
Attachment #8486075 - Attachment is obsolete: false
Yes, sorry - looks like you've got it figured out, but in any case the order should be bug 1039064, then this bug, then bug 1034124.
Flags: needinfo?(dkeeler)
You need to log in before you can comment on or make changes to this bug.