Add SSL_get_client_certificate_types.

Add SSL_get_client_certificate_types.

Exposes the certificate_types parameter in a CertificateRequest.

BUG=165446

[davidben, 2014-04-25 00:46:22]

I can land this in google3 first if that's more convenient. (It's just client
bits.) Wasn't sure what was the preferred style for returning an array of
numbers... I can make this copy it instead or something.

Chromium-side change is here: https://codereview.chromium.org/257513008/

[agl, 2014-04-25 01:03:35]

(Yep, should be in google3.)

https://codereview.chromium.org/254723002/diff/20001/openssl/ssl/ssl_cert.c
File openssl/ssl/ssl_cert.c (right):

https://codereview.chromium.org/254723002/diff/20001/openssl/ssl/ssl_cert.c#n...
openssl/ssl/ssl_cert.c:666: if (s->type != SSL_ST_CONNECT ||
Maybe:

if (s->server ||

https://codereview.chromium.org/254723002/diff/20001/openssl/ssl/ssl_cert.c#n...
openssl/ssl/ssl_cert.c:667: ((s->version >> 8) != SSL3_VERSION_MAJOR) ||
s->version > SSL3_VERSION && s->version != DTLS1_BAD_VER, I think.

[davidben, 2014-04-25 17:03:33]

> (Yep, should be in google3.)

Alright. I'll transfer this after these reviews are done just to keep it in one
place unless you want me to move it earlier. It's easier for me to test this out
of a Chromium checkout.

https://codereview.chromium.org/254723002/diff/20001/openssl/ssl/ssl_cert.c
File openssl/ssl/ssl_cert.c (right):

https://codereview.chromium.org/254723002/diff/20001/openssl/ssl/ssl_cert.c#n...
openssl/ssl/ssl_cert.c:666: if (s->type != SSL_ST_CONNECT ||
On 2014/04/25 01:03:35, agl wrote:
> Maybe:
> 
> if (s->server ||
> 

Done.

https://codereview.chromium.org/254723002/diff/20001/openssl/ssl/ssl_cert.c#n...
openssl/ssl/ssl_cert.c:667: ((s->version >> 8) != SSL3_VERSION_MAJOR) ||
On 2014/04/25 01:03:35, agl wrote:
> s->version > SSL3_VERSION && s->version != DTLS1_BAD_VER, I think.

Do you mean < SSL3_VERSION? Did that version. (Owwww. Why'd I have to go look up
how DTLS version numbers work... what were they thinking)

Would it be sufficient to drop all of these and just leave the s->s3 check? That
seems somewhat simpler. I just pulled these checks from SSL_get_client_CA_list.

[wtc, 2014-04-25 17:42:03]

Review comments on patch set 3:

The patch is not in the right format (which should be a patch that creates your
patch). Have we given up on creating patches in the right format?

https://codereview.chromium.org/254723002/diff/40001/openssl/include/openssl/...
File openssl/include/openssl/ssl.h (right):

https://codereview.chromium.org/254723002/diff/40001/openssl/include/openssl/...
openssl/include/openssl/ssl.h:1985: void SSL_get_client_certificate_types(SSL
*s, char **ctype, size_t *ctype_num);

1. Should |s| be a const SSL * pointer? See the SSL_get_client_CA_list functio
prototype.

2. |ctype| should be const char ** to prevent the caller from modifying the
s->s3->tmp.ctype buffer with *ctype. 

3. In TLS 1.2, CertificateRequest has a new supported_signature_algorithms
member:

      struct {
          ClientCertificateType certificate_types<1..2^8-1>;
          SignatureAndHashAlgorithm
            supported_signature_algorithms<2^16-1>;
          DistinguishedName certificate_authorities<0..2^16-1>;
      } CertificateRequest;

So we may also want to add a SSL_get_server_supported_signature_algorithms
function.

[wtc, 2014-04-25 17:59:58]

https://codereview.chromium.org/254723002/diff/40001/openssl/ssl/ssl_cert.c
File openssl/ssl/ssl_cert.c (right):

https://codereview.chromium.org/254723002/diff/40001/openssl/ssl/ssl_cert.c#n...
openssl/ssl/ssl_cert.c:668: s->s3 == NULL)

I would match the code in SSL_get_client_CA_list:

        if (s->type == SSL_ST_CONNECT)
                { /* we are in the client */
                if (((s->version>>8) == SSL3_VERSION_MAJOR) &&
                        (s->s3 != NULL))
                        {
                        *ctype = s->s3->tmp.ctype;
                        *ctype_num = s->s3->tmp.ctype_num;
                        }
                else
                        {
                        *ctype = NULL;
                        *ctype_num = 0;
                        }
                }
        else
                {
                /* TODO: implement this on the server side. */
                *ctype = NULL;
                *ctype_num = 0;
                }

[davidben, 2014-05-02 22:42:02]

https://codereview.chromium.org/254723002/diff/40001/openssl/include/openssl/...
File openssl/include/openssl/ssl.h (right):

https://codereview.chromium.org/254723002/diff/40001/openssl/include/openssl/...
openssl/include/openssl/ssl.h:1985: void SSL_get_client_certificate_types(SSL
*s, char **ctype, size_t *ctype_num);
On 2014/04/25 17:42:03, wtc wrote:
> 
> 1. Should |s| be a const SSL * pointer? See the SSL_get_client_CA_list functio
> prototype.

Done.

> 2. |ctype| should be const char ** to prevent the caller from modifying the
> s->s3->tmp.ctype buffer with *ctype. 

Done.

> 3. In TLS 1.2, CertificateRequest has a new supported_signature_algorithms
> member:
> 
>       struct {
>           ClientCertificateType certificate_types<1..2^8-1>;
>           SignatureAndHashAlgorithm
>             supported_signature_algorithms<2^16-1>;
>           DistinguishedName certificate_authorities<0..2^16-1>;
>       } CertificateRequest;
> 
> So we may also want to add a SSL_get_server_supported_signature_algorithms
> function.

Probably. Although OpenSSL doesn't actually save that right now. It just
remembers the first supported hash algorithm per signature algorithm and
discards rest; that's sufficient if you're using that to negotiate the hash
function in your CertificateVerify, but not if you want to also negotiate the
signatures in the client's certificate chain. We can support that additionally
later I guess? Not sure if it (or this, for that matter) is especially
interesting. :-)

https://codereview.chromium.org/254723002/diff/40001/openssl/ssl/ssl_cert.c
File openssl/ssl/ssl_cert.c (right):

https://codereview.chromium.org/254723002/diff/40001/openssl/ssl/ssl_cert.c#n...
openssl/ssl/ssl_cert.c:668: s->s3 == NULL)
On 2014/04/25 17:59:59, wtc wrote:
> 
> I would match the code in SSL_get_client_CA_list:
> 
>         if (s->type == SSL_ST_CONNECT)
>                 { /* we are in the client */
>                 if (((s->version>>8) == SSL3_VERSION_MAJOR) &&
>                         (s->s3 != NULL))
>                         {
>                         *ctype = s->s3->tmp.ctype;
>                         *ctype_num = s->s3->tmp.ctype_num;
>                         }
>                 else
>                         {
>                         *ctype = NULL;
>                         *ctype_num = 0;
>                         }
>                 }
>         else
>                 {
>                 /* TODO: implement this on the server side. */
>                 *ctype = NULL;
>                 *ctype_num = 0;
>                 }

There's not really anything to implement on the server's end. The server
implementation just outputs the types it knows about and are compatible with the
cipher suite chosen. The library user doesn't get to set it right now.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/openss...

Otherwise, yeah, that's what the original version of the patch was. :-)

I've changed it to just check s->s3. I think that's much simpler and the
structure is initialized to 0 anyway, so ctype_num will be 0.

[wtc, 2014-05-06 17:10:07]

Patch set 6 LGTM. Just some nits.

https://codereview.chromium.org/254723002/diff/40001/openssl/ssl/ssl_cert.c
File openssl/ssl/ssl_cert.c (right):

https://codereview.chromium.org/254723002/diff/40001/openssl/ssl/ssl_cert.c#n...
openssl/ssl/ssl_cert.c:668: s->s3 == NULL)

On 2014/05/02 22:42:02, David Benjamin wrote:
>
> There's not really anything to implement on the server's end. The server
> implementation just outputs the types it knows about and are compatible with
the
> cipher suite chosen. The library user doesn't get to set it right now.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/openss...

I see. The function prototype of SSL_get_client_certificate_types makes it hard
to implement on the server side because we can't return a pointer that points to
an array inside the |s| structure. We will need to change
SSL_get_client_certificate_types to copy the results to an output buffer.

https://codereview.chromium.org/254723002/diff/100001/openssl/include/openssl...
File openssl/include/openssl/ssl.h (right):

https://codereview.chromium.org/254723002/diff/100001/openssl/include/openssl...
openssl/include/openssl/ssl.h:1985: void SSL_get_client_certificate_types(const
SSL *s, const char **ctype, size_t *ctype_num);

Nit: if this line is longer than 80 characters, we probably should fold it.

https://codereview.chromium.org/254723002/diff/100001/openssl/ssl/ssl.h
File openssl/ssl/ssl.h (right):

https://codereview.chromium.org/254723002/diff/100001/openssl/ssl/ssl.h#newco...
openssl/ssl/ssl.h:1985: void SSL_get_client_certificate_types(const SSL *s,
const char **ctype, size_t *ctype_num);

Nit: this line may need to be folded.

https://codereview.chromium.org/254723002/diff/100001/openssl/ssl/ssl_cert.c
File openssl/ssl/ssl_cert.c (right):

https://codereview.chromium.org/254723002/diff/100001/openssl/ssl/ssl_cert.c#...
openssl/ssl/ssl_cert.c:663: void SSL_get_client_certificate_types(const SSL *s,
const char **ctype, size_t *ctype_num)

1. Nit: this line may need to be folded.

2. Nit: We should define this function before the xname_cmp function. xname_cmp
is used by the SSL_load_client_CA_file function below.

[davidben, 2014-05-06 21:54:01]

https://codereview.chromium.org/254723002/diff/40001/openssl/ssl/ssl_cert.c
File openssl/ssl/ssl_cert.c (right):

https://codereview.chromium.org/254723002/diff/40001/openssl/ssl/ssl_cert.c#n...
openssl/ssl/ssl_cert.c:668: s->s3 == NULL)
On 2014/05/06 17:10:08, wtc wrote:
> 
> On 2014/05/02 22:42:02, David Benjamin wrote:
> >
> > There's not really anything to implement on the server's end. The server
> > implementation just outputs the types it knows about and are compatible with
> the
> > cipher suite chosen. The library user doesn't get to set it right now.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/openss...
> 
> I see. The function prototype of SSL_get_client_certificate_types makes it
hard
> to implement on the server side because we can't return a pointer that points
to
> an array inside the |s| structure. We will need to change
> SSL_get_client_certificate_types to copy the results to an output buffer.

Oh, yeah, we could do that and just call the function here. I'm not sure when
you'd need to do that as a server though. In the client CA case, the library
user configures it. In the certificate_types case, OpenSSL doesn't let the
library user change it anyway.

https://codereview.chromium.org/254723002/diff/100001/openssl/include/openssl...
File openssl/include/openssl/ssl.h (right):

https://codereview.chromium.org/254723002/diff/100001/openssl/include/openssl...
openssl/include/openssl/ssl.h:1985: void SSL_get_client_certificate_types(const
SSL *s, const char **ctype, size_t *ctype_num);
On 2014/05/06 17:10:08, wtc wrote:
> 
> Nit: if this line is longer than 80 characters, we probably should fold it.

Done.

https://codereview.chromium.org/254723002/diff/100001/openssl/ssl/ssl.h
File openssl/ssl/ssl.h (right):

https://codereview.chromium.org/254723002/diff/100001/openssl/ssl/ssl.h#newco...
openssl/ssl/ssl.h:1985: void SSL_get_client_certificate_types(const SSL *s,
const char **ctype, size_t *ctype_num);
On 2014/05/06 17:10:08, wtc wrote:
> 
> Nit: this line may need to be folded.

Done.

https://codereview.chromium.org/254723002/diff/100001/openssl/ssl/ssl_cert.c
File openssl/ssl/ssl_cert.c (right):

https://codereview.chromium.org/254723002/diff/100001/openssl/ssl/ssl_cert.c#...
openssl/ssl/ssl_cert.c:663: void SSL_get_client_certificate_types(const SSL *s,
const char **ctype, size_t *ctype_num)
On 2014/05/06 17:10:08, wtc wrote:
> 
> 1. Nit: this line may need to be folded.

Done.

> 2. Nit: We should define this function before the xname_cmp function.
xname_cmp
> is used by the SSL_load_client_CA_file function below.

Done.

[wtc, 2014-05-06 23:49:44]

Patch set 7 LGTM.

https://codereview.chromium.org/254723002/diff/40001/openssl/ssl/ssl_cert.c
File openssl/ssl/ssl_cert.c (right):

https://codereview.chromium.org/254723002/diff/40001/openssl/ssl/ssl_cert.c#n...
openssl/ssl/ssl_cert.c:668: s->s3 == NULL)

On 2014/05/06 21:54:01, David Benjamin wrote:
> I'm not sure when
> you'd need to do that as a server though. In the client CA case, the library
> user configures it. In the certificate_types case, OpenSSL doesn't let the
> library user change it anyway.

I agree SSL_get_client_certificate_types doesn't seem as useful as
SSL_get_client_CA_list on the server side because the library user can't
configure the certificate types.

[davidben, 2014-05-07 23:23:07]

And updated to match the google3 version. (Fixed ctype to be unsigned.)

[wtc, 2014-05-08 17:57:52]

Patch set 8 LGTM.

https://codereview.chromium.org/254723002/diff/140001/patches.chromium/0014-e...
File patches.chromium/0014-export_certificate_types.patch (right):

https://codereview.chromium.org/254723002/diff/140001/patches.chromium/0014-e...
patches.chromium/0014-export_certificate_types.patch:1: diff --git
android-openssl.orig/include/openssl/ssl.h android-openssl/include/openssl/ssl.h

This patch needs to be renamed 0015-xxx because
https://codereview.chromium.org/259963009/ just added
patches.chromium/0014-new_channelid.patch.

[davidben, 2014-05-08 21:40:05]

https://codereview.chromium.org/254723002/diff/140001/patches.chromium/0014-e...
File patches.chromium/0014-export_certificate_types.patch (right):

https://codereview.chromium.org/254723002/diff/140001/patches.chromium/0014-e...
patches.chromium/0014-export_certificate_types.patch:1: diff --git
android-openssl.orig/include/openssl/ssl.h android-openssl/include/openssl/ssl.h
On 2014/05/08 17:57:53, wtc wrote:
> 
> This patch needs to be renamed 0015-xxx because
> https://codereview.chromium.org/259963009/ just added
> patches.chromium/0014-new_channelid.patch.

Done.

[davidben, 2014-05-12 20:20:27]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-05-12 20:20:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/254723002/160001

[commit-bot: I haz the power, 2014-05-12 20:21:13]

Change committed as 269864