Add client-side support for the origin bound certificate TLS extension.

Add client-side support for the origin bound certificate TLS extension.

BUG=None
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=92576

[wtc, 2011-07-13 01:17:15]

When you have addressed the comments below and uploaded a
new patch set, please also ask agl to review.

http://codereview.chromium.org/7327029/diff/5001/net/third_party/nss/ssl/ssl3...
File net/third_party/nss/ssl/ssl3ext.c (right):

http://codereview.chromium.org/7327029/diff/5001/net/third_party/nss/ssl/ssl3...
net/third_party/nss/ssl/ssl3ext.c:251: { ssl_ob_cert_xtn,           
&ssl3_ClientHandleOBCertXtn },
Nit: list this after ssl_cert_status_xtn for consistency
with the clientHelloSendersTLS array.

http://codereview.chromium.org/7327029/diff/5001/net/third_party/nss/ssl/ssl3...
net/third_party/nss/ssl/ssl3ext.c:258: { ssl_ob_cert_xtn,           
&ssl3_ClientHandleOBCertXtn },
BUG: remove this.  In SSL 3.0 the only extension we support
is the renegotiation info extension.

http://codereview.chromium.org/7327029/diff/5001/net/third_party/nss/ssl/ssl3...
net/third_party/nss/ssl/ssl3ext.c:1875: PRUint32 maxBytes)
Align the arguments properly.

Make the same change to other functions below.

http://codereview.chromium.org/7327029/diff/5001/net/third_party/nss/ssl/ssl3...
net/third_party/nss/ssl/ssl3ext.c:1880: if (!ss)
Indentation seems wrong.

http://codereview.chromium.org/7327029/diff/5001/net/third_party/nss/ssl/ssl3...
net/third_party/nss/ssl/ssl3ext.c:1910: /* TODO(rkn): Is enableOBCerts true only
when we actually want an OBCert? */
Good question.  I believe that the condition "we actually
want an OBCert" requires both the SSL_ENABLE_OB_CERTS and
SSL_REQUEST_CERTIFICATE options to be set.  Please find
out how that translates to code.

FYI, here's how the NSS selfserv test program requests SSL
client auth:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/cmd/selfserv...

http://codereview.chromium.org/7327029/diff/5001/net/third_party/nss/ssl/ssl3...
net/third_party/nss/ssl/ssl3ext.c:1963: ss->ssl3.hs.may_get_cert_status =
PR_TRUE;
I think this line should be deleted.  (It must have been copied
from the cert_status extension handler.)

http://codereview.chromium.org/7327029/diff/5001/net/third_party/nss/ssl/ssli...
File net/third_party/nss/ssl/sslimpl.h (right):

http://codereview.chromium.org/7327029/diff/5001/net/third_party/nss/ssl/ssli...
net/third_party/nss/ssl/sslimpl.h:354: 
Delete this blank line.

http://codereview.chromium.org/7327029/diff/5001/net/third_party/nss/ssl/ssli...
net/third_party/nss/ssl/sslimpl.h:1597: PRUint32 maxBytes);
This reminds me that you should merge your upstream NSS
cached info extension patch to Chromium, which has the
server cached info extension sender.

http://codereview.chromium.org/7327029/diff/5001/net/third_party/nss/ssl/sslt.h
File net/third_party/nss/ssl/sslt.h (right):

http://codereview.chromium.org/7327029/diff/5001/net/third_party/nss/ssl/sslt...
net/third_party/nss/ssl/sslt.h:210: ssl_ob_cert_xtn                  = 0xff0f  
/* experimental number */
Nit: probably should use the TAB character.  See the small
red character?

[rkn, 2011-07-13 21:37:37]

agl: Please review.

[agl, 2011-07-13 21:48:29]

LGTM

http://codereview.chromium.org/7327029/diff/9001/net/third_party/nss/ssl/ssl3...
File net/third_party/nss/ssl/ssl3ext.c (right):

http://codereview.chromium.org/7327029/diff/9001/net/third_party/nss/ssl/ssl3...
net/third_party/nss/ssl/ssl3ext.c:1875: PRUint32 maxBytes)
I would line this up with the "sslSocket" of the previous line. But I'll leave
NSS style comments to wtc from this point.

http://codereview.chromium.org/7327029/diff/9001/net/third_party/nss/ssl/sslt.h
File net/third_party/nss/ssl/sslt.h (right):

http://codereview.chromium.org/7327029/diff/9001/net/third_party/nss/ssl/sslt...
net/third_party/nss/ssl/sslt.h:210: ssl_ob_cert_xtn                  = 0xff0f	/*
experimental number */
There are no specifically reserved extension numbers so I'd be tempted to carry
on allocating around 13172, as we have done before. 13175 is the next one.

[rkn, 2011-07-13 21:57:24]

http://codereview.chromium.org/7327029/diff/9001/net/third_party/nss/ssl/sslt.h
File net/third_party/nss/ssl/sslt.h (right):

http://codereview.chromium.org/7327029/diff/9001/net/third_party/nss/ssl/sslt...
net/third_party/nss/ssl/sslt.h:210: ssl_ob_cert_xtn                  = 0xff0f	/*
experimental number */
On 2011/07/13 21:48:29, agl wrote:
> There are no specifically reserved extension numbers so I'd be tempted to
carry
> on allocating around 13172, as we have done before. 13175 is the next one.

The number 0xff0f was specified here
<http://balfanz.github.com/tls-obc-spec/draft-balfanz-tls-obc-00.html>, though
I'm not sure what the rationale was.

[agl, 2011-07-13 22:01:38]

http://codereview.chromium.org/7327029/diff/9001/net/third_party/nss/ssl/sslt.h
File net/third_party/nss/ssl/sslt.h (right):

http://codereview.chromium.org/7327029/diff/9001/net/third_party/nss/ssl/sslt...
net/third_party/nss/ssl/sslt.h:210: ssl_ob_cert_xtn                  = 0xff0f	/*
experimental number */
On 2011/07/13 21:57:24, rkn wrote:
> The number 0xff0f was specified here
> <http://balfanz.github.com/tls-obc-spec/draft-balfanz-tls-obc-00.html>, though
> I'm not sure what the rationale was.

Probably picked out of the air, but then so was 13172 :)

Fine to go with 0xff0f since it's in the draft spec.

[wtc, 2011-07-13 22:13:47]

LGTM.  Please fix the nits below before you commit the CL.
Thanks!

http://codereview.chromium.org/7327029/diff/9001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/7327029/diff/9001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:961: rv = SSL_OptionSet(nss_fd_,
SSL_ENABLE_OB_CERTS, PR_TRUE);
Sorry I didn't get it when you asked me about the PR_TRUE
argument here.  We should use a ssl_config_.ob_certs_enabled
option until the new code is ready to be turned on by default.

You can pass PR_FALSE for now to disable this.

http://codereview.chromium.org/7327029/diff/9001/net/third_party/nss/ssl/ssl3...
File net/third_party/nss/ssl/ssl3ext.c (right):

http://codereview.chromium.org/7327029/diff/9001/net/third_party/nss/ssl/ssl3...
net/third_party/nss/ssl/ssl3ext.c:1875: PRUint32 maxBytes)
In these three functions, please align the third argument like
this:

PRInt32
ssl3_SendOBCertXtn(sslSocket * ss, PRBool append,
                   PRUint32 maxBytes)

http://codereview.chromium.org/7327029/diff/9001/net/third_party/nss/ssl/ssl3...
net/third_party/nss/ssl/ssl3ext.c:1901: = ssl_ob_cert_xtn;
Please add curly braces around the if statement's body.

Put the '=' sign on the previous line.

Nit: the existing senders seem to all use a xtnData local variable.
I suggest you do the same for consistency.

if (!ss->sec.isServer) {
    TLSExtensionData *xtnData = &ss->xtnData;
    xtnData->advertised[xtnData->numAdvertised++] =
        ssl_ob_cert_xtn;
}

http://codereview.chromium.org/7327029/diff/9001/net/third_party/nss/ssl/ssls...
File net/third_party/nss/ssl/sslsock.c (right):

http://codereview.chromium.org/7327029/diff/9001/net/third_party/nss/ssl/ssls...
net/third_party/nss/ssl/sslsock.c:882: case SSL_ENABLE_OB_CERTS:  on =
ssl_defaults.enableOBCerts;   break;
Nit: align the "on = " and "break" with the cases above.

[commit-bot: I haz the power, 2011-07-13 23:35:49]

Try job failure for 7327029-13002 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, steps "update_scripts, compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2011-07-14 17:11:27]

Try job failure for 7327029-15001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2011-07-14 18:45:32]

Change committed as 92576