Closed Bug 731485 Opened 12 years ago Closed 10 years ago

Add the SSL_PeerCertificateChain function

Categories

(NSS :: Libraries, enhancement, P1)

enhancement

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)

Attached patch Proposed patch by Adam Langley (obsolete) — 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 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)
Target Milestone: 3.13.4 → 3.13.5
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+
Target Milestone: 3.13.5 → 3.14
Target Milestone: 3.14 → 3.14.1
Whiteboard: [Implement Bob's suggestions in comment 2]
(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.
Target Milestone: 3.14.1 → 3.14.2
Target Milestone: 3.14.2 → 3.14.3
Target Milestone: 3.14.3 → 3.15.1
Target Milestone: 3.15.1 → 3.15.2
Priority: P2 → P1
Target Milestone: 3.15.2 → 3.15.3
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+
Wan-Teh, is there remaining work to do be, or can this bug get closed?
changing target milestone to 3.15.4
Target Milestone: 3.15.3 → 3.15.4
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.

Attachment

General

Creator:
Created:
Updated:
Size: