Implement JwkSerializer for OpenSSL.

Implement JwkSerializer for OpenSSL.

Update the tests to use a real P-256 public key for the leading zero test.
OpenSSL will refuse to import a public key that's not actually on the curve.
(Actually importing it is somewhat tidier than sniffing at the SPKI and can
handle other point formats.)

This fixes ExternallyConnectableMessagingWithTlsChannelIdTest for the OpenSSL
port.

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

[agl, 2014-07-30 00:05:54]

lgtm

https://codereview.chromium.org/431453003/diff/40001/net/cert/jwk_serializer_...
File net/cert/jwk_serializer_openssl.cc (right):

https://codereview.chromium.org/431453003/diff/40001/net/cert/jwk_serializer_...
net/cert/jwk_serializer_openssl.cc:41: // TODO(juanlang): other curves
It seems dumb to leave this.

NID_secp224r1 -> "P-224"
NID_secp384r1 -> "P-384"
NID_secp521r1 -> "P-521"

https://codereview.chromium.org/431453003/diff/40001/net/cert/jwk_serializer_...
net/cert/jwk_serializer_openssl.cc:53: if (!x || !y ||
allocation can't fail in Chromium.

[davidben, 2014-07-30 00:13:33]

https://codereview.chromium.org/431453003/diff/40001/net/cert/jwk_serializer_...
File net/cert/jwk_serializer_openssl.cc (right):

https://codereview.chromium.org/431453003/diff/40001/net/cert/jwk_serializer_...
net/cert/jwk_serializer_openssl.cc:41: // TODO(juanlang): other curves
On 2014/07/30 00:05:53, agl wrote:
> It seems dumb to leave this.
> 
> NID_secp224r1 -> "P-224"
> NID_secp384r1 -> "P-384"
> NID_secp521r1 -> "P-521"

draft-ietf-jose-json-web-algorithms-28 doesn't specify P-224, but I've added the
others.

https://codereview.chromium.org/431453003/diff/40001/net/cert/jwk_serializer_...
net/cert/jwk_serializer_openssl.cc:53: if (!x || !y ||
On 2014/07/30 00:05:54, agl wrote:
> allocation can't fail in Chromium.

Done.

[Ryan Sleevi, 2014-07-30 00:18:11]

https://codereview.chromium.org/431453003/diff/60001/net/cert/jwk_serializer_...
File net/cert/jwk_serializer_openssl.cc (right):

https://codereview.chromium.org/431453003/diff/60001/net/cert/jwk_serializer_...
net/cert/jwk_serializer_openssl.cc:61: // The coordinates are encoded with
leading zeros included.
This is a violation of JWK, FWIW.

Are you matching NSS behaviour?

[davidben, 2014-07-30 00:23:23]

https://codereview.chromium.org/431453003/diff/60001/net/cert/jwk_serializer_...
File net/cert/jwk_serializer_openssl.cc (right):

https://codereview.chromium.org/431453003/diff/60001/net/cert/jwk_serializer_...
net/cert/jwk_serializer_openssl.cc:61: // The coordinates are encoded with
leading zeros included.
On 2014/07/30 00:18:10, Ryan Sleevi wrote:
> This is a violation of JWK, FWIW.

Hrm. Are you sure? This guy says:
http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-17#section-5.2...

"""
  The length of this octet string MUST
   be the full size of a coordinate for the curve specified in the "crv"
   parameter.  For example, if the value of "crv" is "P-521", the octet
   string must be 66 octets long.
"""

> Are you matching NSS behaviour?

Yeah, there's a test that goes out of it's way to assert this code does this.
(With an invalid key, but I've fixed it to use a valid key. Had to run openssl
ecparam in a loop until it gave me one that worked. :-) )

[Ryan Sleevi, 2014-07-30 00:37:00]

LGTM. I fail at encodings.

https://codereview.chromium.org/431453003/diff/60001/net/cert/jwk_serializer_...
File net/cert/jwk_serializer_openssl.cc (right):

https://codereview.chromium.org/431453003/diff/60001/net/cert/jwk_serializer_...
net/cert/jwk_serializer_openssl.cc:61: // The coordinates are encoded with
leading zeros included.
On 2014/07/30 00:23:22, David Benjamin wrote:
> On 2014/07/30 00:18:10, Ryan Sleevi wrote:
> > This is a violation of JWK, FWIW.
> 
> Hrm. Are you sure? This guy says:
>
http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-17#section-5.2...
> 
> """
>   The length of this octet string MUST
>    be the full size of a coordinate for the curve specified in the "crv"
>    parameter.  For example, if the value of "crv" is "P-521", the octet
>    string must be 66 octets long.
> """
> 
> > Are you matching NSS behaviour?
> 
> Yeah, there's a test that goes out of it's way to assert this code does this.
> (With an invalid key, but I've fixed it to use a valid key. Had to run openssl
> ecparam in a loop until it gave me one that worked. :-) )

Right, ok. JWK's gotten sloppy again. The issue with JWK is that all encodings
must use the "minimum number" of octets to encode, but it's splattered about
(see, e.g. 6.1.3.2 of JWA-31, which is the current draft)

That said,
http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-31#section-6.2...
is explicit about the x/y having the leading zero, so all good.

[davidben, 2014-07-30 00:39:46]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-07-30 00:41:12]

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

[commit-bot: I haz the power, 2014-07-30 09:18:28]

Change committed as 286454

[eroman, 2014-11-07 01:04:48]

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

[eroman, 2014-11-07 01:04:49]

https://codereview.chromium.org/431453003/diff/60001/net/cert/jwk_serializer_...
File net/cert/jwk_serializer_openssl.cc (right):

https://codereview.chromium.org/431453003/diff/60001/net/cert/jwk_serializer_...
net/cert/jwk_serializer_openssl.cc:75: base::Base64Encode(x_bytes, &x_b64);
Note that JWK uses base64url not vanilla base64 encoding (the difference is how
'+' and '/' are output)

[juanlang (chromium.org), 2014-11-07 01:08:18]

https://codereview.chromium.org/431453003/diff/60001/net/cert/jwk_serializer_...
File net/cert/jwk_serializer_openssl.cc (right):

https://codereview.chromium.org/431453003/diff/60001/net/cert/jwk_serializer_...
net/cert/jwk_serializer_openssl.cc:75: base::Base64Encode(x_bytes, &x_b64);
On 2014/11/07 01:04:49, eroman wrote:
> Note that JWK uses base64url not vanilla base64 encoding (the difference is
how
> '+' and '/' are output)

Yes, this was logged as bug 364749. I'm sorry I haven't gotten to it.