Change SSL_PeerCertificateChain to return a CERTCertList.

Change SSL_PeerCertificateChain to return a CERTCertList.

R=agl@chromium.org,rsleevi@chromium.org
BUG=none
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226126

[Ryan Sleevi, 2013-09-27 23:47:36]

https://codereview.chromium.org/25107004/diff/1/net/third_party/nss/ssl/sslau...
File net/third_party/nss/ssl/sslauth.c (right):

https://codereview.chromium.org/25107004/diff/1/net/third_party/nss/ssl/sslau...
net/third_party/nss/ssl/sslauth.c:46: return chain;
you return chain here, but return NULL on line 51 and 42 to indicate error.

You also fail to set an error code on line 42/41. Intentional?

https://codereview.chromium.org/25107004/diff/1/net/third_party/nss/ssl/sslau...
net/third_party/nss/ssl/sslauth.c:65: PORT_SetError(SEC_ERROR_NO_MEMORY);
This will clobber any errors set by CERT_AddCertToListTail (if it sets any).
Intentional?

[wtc, 2013-09-27 23:49:40]

agl,rsleevi: please review.

bemasc: just FYI. I am changing the function prototype of
SSL_PeerCertificateChain.

https://codereview.chromium.org/25107004/diff/6001/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

https://codereview.chromium.org/25107004/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl.h:444: SSL_IMPORT CERTCertList
*SSL_PeerCertificateChain(PRFileDesc *fd);

This function can also be called on the server side. If client authentication is
not requested, there is no peer certificate. Returning NULL in this case seems
simpler for the caller than returning an empty CERTCertList.

https://codereview.chromium.org/25107004/diff/6001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/sslauth.c (right):

https://codereview.chromium.org/25107004/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslauth.c:45: PORT_SetError(SSL_ERROR_NO_CERTIFICATE);

SSL_ERROR_NO_CERTIFICATE seems to be intended for the SSLv3 no_certificate alert
error. I am overloading this error code.

[wtc, 2013-09-28 00:55:01]

rsleevi: thanks for the review. Please review the diffs between patch sets 1 and
3.

https://codereview.chromium.org/25107004/diff/1/net/third_party/nss/ssl/sslau...
File net/third_party/nss/ssl/sslauth.c (right):

https://codereview.chromium.org/25107004/diff/1/net/third_party/nss/ssl/sslau...
net/third_party/nss/ssl/sslauth.c:46: return chain;
On 2013/09/27 23:47:36, Ryan Sleevi wrote:
> you return chain here, but return NULL on line 51 and 42 to indicate error.

Fixed.

> You also fail to set an error code on line 42/41. Intentional?

Yes. ssl_FindSocket sets error code.

https://codereview.chromium.org/25107004/diff/1/net/third_party/nss/ssl/sslau...
net/third_party/nss/ssl/sslauth.c:65: PORT_SetError(SEC_ERROR_NO_MEMORY);
On 2013/09/27 23:47:36, Ryan Sleevi wrote:
> This will clobber any errors set by CERT_AddCertToListTail (if it sets any).
> Intentional?

Fixed. I saw some NSS code that sets SEC_ERROR_NO_MEMORY on CERT_NewCertList and
CERT_AddCertToListTail failures. I found that it is not necessary. (Perhaps it
was necessary before.)

[Ryan Sleevi, 2013-09-28 01:02:50]

lgtm

[agl, 2013-09-30 21:16:14]

lgtm

[commit-bot: I haz the power, 2013-09-30 22:28:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/25107004/57001

[commit-bot: I haz the power, 2013-10-01 03:30:47]

Change committed as 226126