Support tls-server-end-point channel bindings for HTTP authentication.

[net/http auth] Support channel bindings for HTTP authentication.

Start using tls-server-end-point channel bindings for HTTP
authentication if a certificate is available. The current implementation
should work on Windows and Posix. Currently only SHA-256, SHA-384, and
SHA-512 are supported for generating channel bindings.

BUG=270219
R=rsleevi@chromium.org,davidben@chromium.org

Committed: https://crrev.com/5ffd5d79244e6c9928ed465fcfed8a136d04140a
Cr-Commit-Position: refs/heads/master@{#382858}

[asanka, 2015-10-15 02:29:04]

rsleevi: Not ready for commit. This is an opinion solicitation round for the
non-SHA-256 digest cases in RFC 5929. It looks like tls-server-end-point is the
only binding type we can use against Windows hosts. So I added a method to
x509_util.

* Should it go somewhere else?
* What should I do to get at the certificate's signatureAlgorithm?

The meat of the CL is in the x509_util, http_auth_handler_negotiate,
http_auth_sspi_win and http_auth_gssapi_posix. I still need to plumb it to
http_auth_handler_ntlm even though only Windows will be able to use it. See
go/chrome-auth-lab for the test lab which I used for this.

Also there's some refactors coming down the pipe
(https://codereview.chromium.org/1391053002,
https://codereview.chromium.org/1383613002,
https://codereview.chromium.org/1383613002,
https://codereview.chromium.org/1157333005,
https://codereview.chromium.org/1381493004) which should improve the http_auth
code. But I wanted to get this one out first.

[Ryan Sleevi, 2015-10-17 00:34:26]

On 2015/10/15 02:29:04, asanka wrote:
> rsleevi: Not ready for commit. This is an opinion solicitation round for the
> non-SHA-256 digest cases in RFC 5929. It looks like tls-server-end-point is
the
> only binding type we can use against Windows hosts. So I added a method to
> x509_util.

Yeah, the only MSFT I'm aware of that uses tls-server-unique CBs is the Kerberos
binding for RDP when handling CredSSP-forwarded credentials. It negotiates a TLS
session, then does tls-server-unique CB for an inner Kerberos exchange which
includes a delegated auth token, and then wraps the smartcard PIN using the
Kerb-negotiated keys (Yeah, oy vey)

> * Should it go somewhere else?

x509_util_nss / x509_util_openssl seem more favourable to your use case; still
passing an X509Certificate along. I'd be curious to know why this hangs off
X509Certificate, rather than where it seems to more logically fit, on the
SSLClientSocket. I could understand concerns, but given that we also have key
material extraction being used for Token Binding (ExportKeyingMaterial), it is
not without precedent that we have portions of the HTTP/ layer needing to know
how to extract certain elements from the underlying transport - EKM or Token
Binding.

If you made it on the SSLClientSocket, that'd be much easier to handle the
platform-specific bits (I presume you still care about iOS, at least until they
move off to WKWebView)

> * What should I do to get at the certificate's signatureAlgorithm?

I'd suggest you chat with davidben@ to see if he has a preferred way. The
'gross' answer (that I don't think he'd like) is that:
- For BoringSSL X509* x, look at EVP_get_digestbyobj(alg->sig_alg->algorithm),
but that's reaching in to X509* and I suspect David would much rather prefer
just to add a BoringSSL function to handle this (and the above example doesn't
handle rsa-pss correctly, so we could hide that detail in that function)
- For NSS, look at my 6-year-old (!!!) patch for NSS at
https://bugzilla.mozilla.org/show_bug.cgi?id=563276 (line 397 in the new patch
at https://bugzilla.mozilla.org/attachment.cgi?id=443031&action=diff is the
start of ssl3_get_end_point_channel_binding ), which shows how to extract it
from an NSS-backed certificate

Both of those could land in the SSLClientSocket, and that would get you platform
ubiquity.

[asanka, 2016-03-18 16:31:13]

Patchset #3 (id:40001) has been deleted

[asanka, 2016-03-18 16:31:23]

Patchset #3 (id:60001) has been deleted

[asanka, 2016-03-18 16:46:57]

asanka@chromium.org changed reviewers:
+ davidben@chromium.org

[asanka, 2016-03-18 16:46:58]

I believe this agrees with our discussion over email. I only implemented it for
OpenSSL/BoringSSL since at this point Chrome on iOS no longer cares about
channel bindings.

The relevant bits are in x509_util*. The HttpAuth* changes are largely plumbing.
We are pushing through a HttpResponseInfo through the auth stack since it needs
the headers as well as the SSLInfo now. The Negotiate handler extracts the
channel bindings and passes it along if it's relevant.

Currently GSSAPI and SSPI for Negotiate uses the channel bindings. I'll add NTLM
as well momentarily.

[asanka, 2016-03-18 16:47:52]

Description was changed from

==========
[net/http auth] Support channel bindings for HTTP authentication.

Start using tls-server-end-point channel bindings for HTTP
authentication if a certificate is available. The current implementation
should work on Windows and Posix. Currently only SHA-256 is supported
for generating channel bindings.

BUG=270219
R=rsleevi@chromium.org
==========

to

==========
[net/http auth] Support channel bindings for HTTP authentication.

Start using tls-server-end-point channel bindings for HTTP
authentication if a certificate is available. The current implementation
should work on Windows and Posix. Currently only SHA-256, SHA-384, and
SHA-512 are supported for generating channel bindings.

BUG=270219
R=rsleevi@chromium.org
==========

[asanka, 2016-03-18 16:48:11]

Description was changed from

==========
[net/http auth] Support channel bindings for HTTP authentication.

Start using tls-server-end-point channel bindings for HTTP
authentication if a certificate is available. The current implementation
should work on Windows and Posix. Currently only SHA-256, SHA-384, and
SHA-512 are supported for generating channel bindings.

BUG=270219
R=rsleevi@chromium.org
==========

to

==========
[net/http auth] Support channel bindings for HTTP authentication.

Start using tls-server-end-point channel bindings for HTTP
authentication if a certificate is available. The current implementation
should work on Windows and Posix. Currently only SHA-256, SHA-384, and
SHA-512 are supported for generating channel bindings.

BUG=270219
R=rsleevi@chromium.org,davidben@chromium.org
==========

[Ryan Sleevi, 2016-03-18 21:27:40]

The thing that makes me a little uncomfortable is the dependency on
HttpResponseInfo, rather than passing down the SSLInfo as a new parameter. The
concern is largely one of dependency scoping leak; on the whole, it's not
unreasonable, but I just favor narrow and explicit dependencies.

Other than that, LGTM % nits.

https://codereview.chromium.org/1408433006/diff/80001/net/http/http_auth_hand...
File net/http/http_auth_handler_factory.h (right):

https://codereview.chromium.org/1408433006/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_factory.h:83: // TODO(asanka): Update comment.
*cough*? ;)

https://codereview.chromium.org/1408433006/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_factory.h:99: // TODO(asanka): Update comment.
*cough* ;)

https://codereview.chromium.org/1408433006/diff/80001/net/http/http_auth_hand...
File net/http/http_auth_handler_negotiate.cc (right):

https://codereview.chromium.org/1408433006/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_negotiate.cc:36: auto colon =
channel_binding_token.find(':');
Is it necessary to split this? I'm not sure why you treat "type" as a magic
value - why not just let it be logged w/o further interpretation?

https://codereview.chromium.org/1408433006/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_negotiate.cc:37: if (colon ==
channel_binding_token.npos || colon < 1 ||
Shouldn't this be std::string::npos? It's a static data member, not instance.

https://codereview.chromium.org/1408433006/diff/80001/net/http/http_auth_sspi...
File net/http/http_auth_sspi_win.cc (right):

https://codereview.chromium.org/1408433006/diff/80001/net/http/http_auth_sspi...
net/http/http_auth_sspi_win.cc:391: DCHECK_EQ(0u,
bindings_desc->dwAcceptorOffset);
Why these DCHECKs? This is guaranteed by 378/380

[asanka, 2016-03-23 04:51:30]

https://codereview.chromium.org/1408433006/diff/80001/net/http/http_auth_hand...
File net/http/http_auth_handler_factory.h (right):

https://codereview.chromium.org/1408433006/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_factory.h:83: // TODO(asanka): Update comment.
On 2016/03/18 at 21:27:40, Ryan Sleevi wrote:
> *cough*? ;)

:-P

Comments updated here and elsewhere.

https://codereview.chromium.org/1408433006/diff/80001/net/http/http_auth_hand...
File net/http/http_auth_handler_negotiate.cc (right):

https://codereview.chromium.org/1408433006/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_negotiate.cc:36: auto colon =
channel_binding_token.find(':');
On 2016/03/18 at 21:27:40, Ryan Sleevi wrote:
> Is it necessary to split this? I'm not sure why you treat "type" as a magic
value - why not just let it be logged w/o further interpretation?

Done. Yeah, went with logging the token in hex without trying to interpret the
type.

https://codereview.chromium.org/1408433006/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_negotiate.cc:37: if (colon ==
channel_binding_token.npos || colon < 1 ||
On 2016/03/18 at 21:27:40, Ryan Sleevi wrote:
> Shouldn't this be std::string::npos? It's a static data member, not instance.

Either is valid for a static member of a class, but the fully qualified form is
more readable. I'd have changed it to std::string::npos, but this block got
removed since we are now logging the token without trying to determine its type.

https://codereview.chromium.org/1408433006/diff/80001/net/http/http_auth_sspi...
File net/http/http_auth_sspi_win.cc (right):

https://codereview.chromium.org/1408433006/diff/80001/net/http/http_auth_sspi...
net/http/http_auth_sspi_win.cc:391: DCHECK_EQ(0u,
bindings_desc->dwAcceptorOffset);
On 2016/03/18 at 21:27:40, Ryan Sleevi wrote:
> Why these DCHECKs? This is guaranteed by 378/380

Removed.

[asanka, 2016-03-23 04:53:09]

On 2016/03/18 at 21:27:40, rsleevi wrote:
> The thing that makes me a little uncomfortable is the dependency on
HttpResponseInfo, rather than passing down the SSLInfo as a new parameter. The
concern is largely one of dependency scoping leak; on the whole, it's not
unreasonable, but I just favor narrow and explicit dependencies.
> 

Yeah, that's a valid concern. I switched to passing a SSLInfo where appropriate
rather than passing the entire HttpResponseInfo.

[asanka, 2016-03-23 16:14:33]

The CQ bit was checked by asanka@chromium.org

[asanka, 2016-03-23 16:14:33]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1408433006/#ps100001
(title: "Narrower dependencies, update comments, address review comments.")

[commit-bot: I haz the power, 2016-03-23 16:14:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408433006/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408433006/100001

[commit-bot: I haz the power, 2016-03-23 16:20:58]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-03-23 16:22:12]

Description was changed from

==========
[net/http auth] Support channel bindings for HTTP authentication.

Start using tls-server-end-point channel bindings for HTTP
authentication if a certificate is available. The current implementation
should work on Windows and Posix. Currently only SHA-256, SHA-384, and
SHA-512 are supported for generating channel bindings.

BUG=270219
R=rsleevi@chromium.org,davidben@chromium.org
==========

to

==========
[net/http auth] Support channel bindings for HTTP authentication.

Start using tls-server-end-point channel bindings for HTTP
authentication if a certificate is available. The current implementation
should work on Windows and Posix. Currently only SHA-256, SHA-384, and
SHA-512 are supported for generating channel bindings.

BUG=270219
R=rsleevi@chromium.org,davidben@chromium.org

Committed: https://crrev.com/5ffd5d79244e6c9928ed465fcfed8a136d04140a
Cr-Commit-Position: refs/heads/master@{#382858}
==========

[commit-bot: I haz the power, 2016-03-23 16:22:13]

Patchset 4 (id:??) landed as
https://crrev.com/5ffd5d79244e6c9928ed465fcfed8a136d04140a
Cr-Commit-Position: refs/heads/master@{#382858}

[eroman, 2017-03-02 21:13:01]

eroman@chromium.org changed reviewers:
+ eroman@chromium.org

[eroman, 2017-03-02 21:13:04]

https://codereview.chromium.org/1408433006/diff/100001/net/cert/x509_util_ope...
File net/cert/x509_util_openssl.cc (right):

https://codereview.chromium.org/1408433006/diff/100001/net/cert/x509_util_ope...
net/cert/x509_util_openssl.cc:332: case net::DigestAlgorithm::Sha1:
Is this fall-through intentional? Seems like either:

* It should return false if Sha1 is not supported
* It should set EVP_sha1()
* There needs to be a comment explaining why a fallthrough is intentional.

[asanka, 2017-03-02 21:28:57]

https://codereview.chromium.org/1408433006/diff/100001/net/cert/x509_util_ope...
File net/cert/x509_util_openssl.cc (right):

https://codereview.chromium.org/1408433006/diff/100001/net/cert/x509_util_ope...
net/cert/x509_util_openssl.cc:332: case net::DigestAlgorithm::Sha1:
On 2017/03/02 21:13:02, eroman wrote:
> Is this fall-through intentional? Seems like either:
> 
> * It should return false if Sha1 is not supported
> * It should set EVP_sha1()
> * There needs to be a comment explaining why a fallthrough is intentional.

This is per https://tools.ietf.org/html/rfc5929#section-4.1 which states that if
the digest algorithm is MD5, SHA-1 or SHA-256, then the channel bindings should
be based on a SHA-256 digest of the certificate.

We don't support the MD5 case.

[eroman, 2017-03-02 22:12:05]

Thanks Asanka