Closed
Bug 731485
Opened 12 years ago
Closed 10 years ago
Add the SSL_PeerCertificateChain function
Categories
(NSS :: Libraries, enhancement, P1)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.15.4
People
(Reporter: wtc, Assigned: wtc)
References
(Depends on 1 open bug)
Details
(Whiteboard: [Implement Bob's suggestions in comment 2])
Attachments
(1 file, 1 obsolete file)
3.10 KB,
patch
|
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
The attached patch was written by Adam Langley. Add the SSL_PeerCertificateChain function to return all the certificates presented by the SSL peer. Note that if the output array is too small, the function returns the actual certificate chain size, but still copies as many certificates as possible to the output array. Please let me know if you like this behavior. An alternative design is to add a SSL_PeerCertificateChainSize function and have the SSL_PeerCertificateChain function fail without doing anything if the output array is too small.
Attachment #601485 -
Flags: superreview?(rrelyea)
Attachment #601485 -
Flags: review?(bsmith)
Comment 1•12 years ago
|
||
Comment on attachment 601485 [details] [diff] [review] Proposed patch by Adam Langley The upcoming patch for bug 731478 will require changes to this patch, AFAICT. I will review the updated version. I do not understand the significance of the "if (!ss->opt.useSecurity)" check. Is it really needed? Like I mentioned above, I suspect that we will soon use the output of this function as the input to CERT_PKIXVerifyCert for "hint certs" or similar. I think a CERTCertList would be a more convenient structure for this and similar things that we would do in Gecko. But, if the extra copying/allocating that would be required is problematic for others, then we can continue to do things with the array of certificates.
Attachment #601485 -
Flags: review?(bsmith)
Assignee | ||
Updated•12 years ago
|
Target Milestone: 3.13.4 → 3.13.5
Comment 2•12 years ago
|
||
Comment on attachment 601485 [details] [diff] [review] Proposed patch by Adam Langley r+, but with some comments. I agree some form of certlist (we have several in NSS) would probably be the best interface. Clearly this patch requires the patch in bug 732478 as a prereq. I actually don't anticipate any of the changes to bug 732478 to actually affect this patch, but it's clear the patch is a prereq. We should probably set an error in the ss->opt.useSecurity branch of the test if it survives. (I can go either way, though it seems to be a useful test, and makes understanding this code fragment easier). bob
Attachment #601485 -
Flags: superreview?(rrelyea) → superreview+
Updated•12 years ago
|
Target Milestone: 3.13.5 → 3.14
Assignee | ||
Updated•12 years ago
|
Target Milestone: 3.14 → 3.14.1
Assignee | ||
Updated•12 years ago
|
Whiteboard: [Implement Bob's suggestions in comment 2]
Assignee | ||
Comment 3•12 years ago
|
||
(In reply to Robert Relyea from comment #2) > > Clearly this patch requires the patch in bug 732478 as a prereq. I actually > don't anticipate any of the changes to bug 732478 to actually affect this > patch, but it's clear the patch is a prereq. The bug number Bob mentioned should be bug 731478.
Assignee | ||
Updated•12 years ago
|
Target Milestone: 3.14.1 → 3.14.2
Updated•11 years ago
|
Target Milestone: 3.14.2 → 3.14.3
Assignee | ||
Updated•11 years ago
|
Target Milestone: 3.14.3 → 3.15.1
Assignee | ||
Updated•11 years ago
|
Target Milestone: 3.15.1 → 3.15.2
Assignee | ||
Updated•11 years ago
|
Priority: P2 → P1
Target Milestone: 3.15.2 → 3.15.3
Assignee | ||
Comment 5•11 years ago
|
||
I made the changes that Brian and Bob suggested in comment 1 and comment 2. I changed the function to return a CERTCertList. I preserved the !ss->opt.useSecurity test (I guess it is possible to use an SSL socket that doesn't do SSL and simply forwards data) but set the SSL_ERROR_NO_CERTIFICATE_ERROR error code. Ryan reviewed this patch at https://codereview.chromium.org/25107004/ Patch checked in: https://hg.mozilla.org/projects/nss/rev/9695a2d56c4d
Attachment #601485 -
Attachment is obsolete: true
Attachment #812186 -
Flags: checked-in+
Comment 6•11 years ago
|
||
Wan-Teh, is there remaining work to do be, or can this bug get closed?
Assignee | ||
Updated•10 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•