Test certificate_authorities field

Test certificate_authorities field

[ekr-rietveld, 2015-11-27 19:51:16]

MT, it would be nice if this checked DN, but this is an improvement.

[mt, 2015-11-28 01:30:43]

Add cert list to the raii header Tim wrote and use it. Also, I'm not seeing
anything in the way of checking that these calls actually *do* annoying.

On Nov 27, 2015 11:51 AM, <ekr-webrtc@rtfm.com> wrote:
>
> Reviewers: mt,
>
> Message:
> MT, it would be nice if this checked DN, but this is an improvement.
>
> Description:
> Test certificate_authorities field
>
> Please review this at https://codereview.appspot.com/276270043/
>
> Affected files (+11, -0 lines):
>   M external_tests/ssl_gtest/tls_agent.cc
>
>
> Index: external_tests/ssl_gtest/tls_agent.cc
> diff --git a/external_tests/ssl_gtest/tls_agent.cc
b/external_tests/ssl_gtest/tls_agent.cc
> index
2a41ecbc67ac473245f5712257c9359ea6077cd8..ce68fb0ee582dda63688092d8a80a7897282abff
100644
> --- a/external_tests/ssl_gtest/tls_agent.cc
> +++ b/external_tests/ssl_gtest/tls_agent.cc
> @@ -143,6 +143,7 @@ SECStatus TlsAgent::GetClientAuthDataHook(void* self,
PRFileDesc* fd,
>                                            CERTDistNames* caNames,
>                                            CERTCertificate** cert,
>                                            SECKEYPrivateKey** privKey) {
> +  EXPECT_EQ(1, caNames->nnames);
>    TlsAgent* agent = reinterpret_cast<TlsAgent*>(self);
>    if (agent->GetClientAuthCredentials(cert, privKey)) {
>      return SECSuccess;
> @@ -161,6 +162,16 @@ void TlsAgent::RequestClientAuth(bool requireAuth) {
>              SSL_OptionSet(ssl_fd_, SSL_REQUIRE_CERTIFICATE,
>                            requireAuth ? PR_TRUE : PR_FALSE));
>
> +  /* Put our own certificate as the trust anchor, just to populate it
> +   * with something. */
> +  CERTCertificate* cert = PK11_FindCertFromNickname(name_.c_str(),
nullptr);
> +  ASSERT_NE(nullptr, cert);
> +  CERTCertList* cert_list = CERT_NewCertList();
> +  EXPECT_EQ(SECSuccess, CERT_AddCertToListTail(cert_list, cert));
> +  EXPECT_EQ(SECSuccess,
> +            SSL_SetTrustAnchors(ssl_fd_, cert_list));
> +  CERT_DestroyCertList(cert_list);
> +
>    EXPECT_EQ(SECSuccess,
>              SSL_AuthCertificateHook(ssl_fd_,
&TlsAgent::ClientAuthenticated,
>                                      this));
>
>

[ekr-rietveld, 2015-11-28 01:37:42]

1. I didn't see that Taubert's patch had landed, I agree that would be better.

2. I do check on the client side that there is the right number of distinguished
names (previously it was 0). However, I don't check that the names are correct.

[ekr-rietveld, 2015-11-28 02:37:43]

MT, now with scoped ptrs.

https://codereview.appspot.com/276270043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/276270043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:146: EXPECT_EQ(1, caNames->nnames);
MT, see here. caNames->nnames was previously 0

[mt, 2015-12-05 10:10:27]

LGTM, act on the comment as you see fit.

https://codereview.appspot.com/276270043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/276270043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_agent.cc:173: cert.release();
The release isn't really necessary, is it?

[ekr-rietveld, 2015-12-05 18:51:42]

https://codereview.appspot.com/276270043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/276270043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_agent.cc:173: cert.release();
On 2015/12/05 10:10:26, mt wrote:
> The release isn't really necessary, is it?

Yes. The list takes ownership.