Switch //net to the new SPKI and PKCS#8 APIs.

Switch //net to the new SPKI and PKCS#8 APIs.

This fixes the new verifier's SPKI parser and gets us closer to losing the
legacy ASN.1 stack. This also rewrites the Android KeygenHandler
implementation. This was tested manually, but to make sure it's good, make
the tests actually parse out the SPKAC structure and verify the signature.
(Everything that implements keygen may freely assume BoringSSL, so we have
CBS and EVP available.)

In doing so, this revealed a bug in our Mac SPKAC code. It was omitting the
NULLs in both SPKI and signatureAlgorithm, and EVP_parse_public_key is
strict about including them, per the MUST in the spec. (I've confirmed that
Safari includes them, so it's just us that messed this up.)

This resolves the last TODO about laxness in VerifySignedDataTest, so we
should now be parsing SPKIs correctly. (Finally...)

BUG=499653, 522228
Committed: https://crrev.com/f43769e980b0948775860095c672d17969c70a19
Cr-Commit-Position: refs/heads/master@{#380774}

[davidben, 2016-03-03 16:17:23]

Description was changed from

==========
Switch //net to the new SPKI and PKCS#8 APIs.

This fixes the new verifier's SPKI parser and gets us closer to losing the
legacy ASN.1 stack.

BUG=499653,522228
==========

to

==========
Switch //net to the new SPKI and PKCS#8 APIs.

This fixes the new verifier's SPKI parser and gets us closer to losing the
legacy ASN.1 stack. This also rewrites the Android KeygenHandler
implementation. This was tested manually, but to make sure it's good, make
the tests actually parse out the SPKAC structure and verify the signature.
(Everything that implements keygen may freely assume BoringSSL, so we have
CBS and EVP available.)

In doing so, this revealed a bug in our Mac SPKAC code. It was omitting the
NULLs in both SPKI and signatureAlgorithm, and EVP_parse_public_key is
strict about including them, per the MUST in the spec. (I've confirmed that
Safari includes them, so it's just us that messed this up.)

BUG=499653,522228
==========

[davidben, 2016-03-03 16:20:14]

Description was changed from

==========
Switch //net to the new SPKI and PKCS#8 APIs.

This fixes the new verifier's SPKI parser and gets us closer to losing the
legacy ASN.1 stack. This also rewrites the Android KeygenHandler
implementation. This was tested manually, but to make sure it's good, make
the tests actually parse out the SPKAC structure and verify the signature.
(Everything that implements keygen may freely assume BoringSSL, so we have
CBS and EVP available.)

In doing so, this revealed a bug in our Mac SPKAC code. It was omitting the
NULLs in both SPKI and signatureAlgorithm, and EVP_parse_public_key is
strict about including them, per the MUST in the spec. (I've confirmed that
Safari includes them, so it's just us that messed this up.)

BUG=499653,522228
==========

to

==========
Switch //net to the new SPKI and PKCS#8 APIs.

This fixes the new verifier's SPKI parser and gets us closer to losing the
legacy ASN.1 stack. This also rewrites the Android KeygenHandler
implementation. This was tested manually, but to make sure it's good, make
the tests actually parse out the SPKAC structure and verify the signature.
(Everything that implements keygen may freely assume BoringSSL, so we have
CBS and EVP available.)

In doing so, this revealed a bug in our Mac SPKAC code. It was omitting the
NULLs in both SPKI and signatureAlgorithm, and EVP_parse_public_key is
strict about including them, per the MUST in the spec. (I've confirmed that
Safari includes them, so it's just us that messed this up.)

This resolves the last TODO about laxness in VerifySignedDataTest, so we
should now be parsing SPKIs correctly. (Finally...)

BUG=499653,522228
==========

[davidben, 2016-03-03 16:22:30]

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

[davidben, 2016-03-03 16:22:31]

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
File net/base/keygen_handler_mac.cc (right):

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_mac.cc:159: spkac.pkac.spki.algorithm.parameters.Length
= sizeof(kNullDer);
(We could also reimplement this whole thing in CBB in code shared with
keygen_handler_openssl.cc, but the fix was easy so it didn't seem worth
bothering.)

[Ryan Sleevi, 2016-03-03 18:32:06]

Most everything but the keygen handler LG.

To me, a lot of that reads like the anti-pattern of ASN.1 encoding. Maybe it's
just that I don't like the 'new hotness', or ideally, hopefully, there's
legitimate concern that just may not be obvious to you when writing the code.
Hopefully it doesn't come off too harsh - but it just seems like a very
error-prone way of writing things.

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
File net/base/keygen_handler_openssl.cc (right):

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:63: challenge_.size()) ||
Is this fine if challenge_ is empty? I suspect any issue is pre-existing, but
wasn't sure if the ASN1_STRING_set handled things differently than CBB_add_bytes

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:79: };
Surely there's a less-gross way than hardcoding - can't we rely on BoringSSL to
keep this AlgorithmIdentifier? Or is this part of what you're trying to excise?

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:88: !CBB_add_u8(&sig_bitstring, 0 /* padding
*/) ||
? Why is this necessary? Because the assumption is there's no unused bits in
sig_bitstring?

This isn't really padding then, is it?

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:94: !CBB_did_write(&sig_bitstring, sig_len))
{
This feels like such a gross way to do it. Is this really how you envision the
new BoringSSL signing world? This seems like so much boilerplate and so much
opportunity to get things wrong.

[davidben, 2016-03-03 19:58:15]

Long comments below. We might want to grab a VC and hash out exactly what the
layering here should be. I'd at one point assumed one layering and you
(correctly, I believe) pointed out that layering didn't make sense and it should
be something else. This reflects my current belief, but it's possible you had
something else in mind.

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
File net/base/keygen_handler_openssl.cc (right):

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:63: challenge_.size()) ||
On 2016/03/03 18:32:06, Ryan Sleevi wrote:
> Is this fine if challenge_ is empty? I suspect any issue is pre-existing, but
> wasn't sure if the ASN1_STRING_set handled things differently than
CBB_add_bytes

If challenge is empty, we'll emit an empty IA5STRING which I believe should be
correct. Empty strings are valid strings and the field isn't OPTIONAL or
anything. I have no clue what the old crazy code did. :-)

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:79: };
On 2016/03/03 18:32:06, Ryan Sleevi wrote:
> Surely there's a less-gross way than hardcoding - can't we rely on BoringSSL
to
> keep this AlgorithmIdentifier? Or is this part of what you're trying to
excise?
> 

There's OBJ_nid2cbb that would work. I didn't use it because I'm hoping to
excise that later too. OBJ_nid2cbb involves a HUGE table of every OID OpenSSL
knows about, which is ridiculous. That table ends up showing up in all of
Cronet's largest symbols. It's difficult to know what we can prune that table
because it's shared by the X.509 code and anything else that happens to ask
OpenSSL to parse or even pretty-print an OID.

I haven't decoupled BoringSSL core's internals from that table yet (have a
branch not yet reviewed), but I'm envisioning that, like the net/cert code, each
module which needs to know about particular OIDs would just embed them and
memcmp as appropriate. For the most part, OIDs of different "types" are
disjoint. No key type is also a hash type, no X.509 extension is also a
signature algorithm, etc.

The idea is that you'll only ship OID definitions you care about.

What do you think about instead only burning in the encoded form of the OID, but
keeping the rest structured? (So CBB calls for the NULLs, etc.) I did this just
because it's always constant and it seemed easy.

I'm also happy to write this assuming OBJ_nid2cbb is legal, since that hasn't
actually landed yet and maybe we'll, I dunno, have constants in BoringSSL for
the OID definitions or something. (Though, see note in comment below about who
should actually be handling signature algorithms.)

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:88: !CBB_add_u8(&sig_bitstring, 0 /* padding
*/) ||
On 2016/03/03 18:32:06, Ryan Sleevi wrote:
> ? Why is this necessary? Because the assumption is there's no unused bits in
> sig_bitstring?

I think we've had this conversation before. :-) It's not an assumption.
Signatures are byte strings full-stop. I do not know of a single crypto
specification that legitimately uses bit strings to encode signatures.
Everything that specifies an encoding gives back octet strings.

I say "legitimately" because X.509 and related specs nominally do, but don't
actually. They just encode them as BIT STRING but never actually use them that
way. Even RSA in X.509, whose signatures are a number mod N, encodes to byte
strings first and than passes through the common byte string to BIT STRING
conversion.

A 2051-bit RSA signature in X.509 will still be encoded in a BIT STRING with a
multiple of 8 bytes.

(The common byte string to BIT STRING conversion just happens to be what you'd
get if you order the bits within a byte in the same order as the DER encoding of
a BIT STRING. Then you have to prepend a zero because of how BIT STRINGs are
encoding in DER.)


> This isn't really padding then, is it?

This is how BIT STRINGs are encoded in DER. You zero-pad at the *end* and then
*prepend* a single byte telling you how many bits to remove from the *end*.
(Indeed because of that, it actually doesn't make sense to encode a 2051-bit RSA
signature with a partial number of bits. The signature is encoded big-endian and
you pad at the end, so you'll end up actually having to shift everything by 3
bits.)

In X.509's byte string to BIT STRING conversion, this works out to always
sticking a zero byte in front of everything. I figured "padding" was one way to
condense that nonsense, I'm not very happy with it. I sometimes call it a "phase
byte" for BIT STRING in general, but I think I'm the only one who says that and
started calling it "padding" for that reason.

What do you think it should be called?



Appending of citations:
---

https://tools.ietf.org/html/rfc3279#section-2.2.1
at the bottom:

   The RSA signature generation process and the encoding of the result
   is described in detail in PKCS #1 [RFC 2313].

Latest version of RFC 2313 is RFC 3448.

https://tools.ietf.org/html/rfc3447#section-8.2.1
   Output:
   S        signature, an octet string of length k, where k is the
            length in octets of the RSA modulus n

Alas, RFC 3279 and a few others didn't quite realize they made type error.
(ASN.1 specifies no order of bits within a byte for a BIT STRING. One typically
uses the order in the DER encoding, but, abstractly, one is a
std::vector<uint8_t> and the other a std::vector<bool>.)

By RFC 4055, they seem to have figured this out and actually spelled this out
more clearly:
https://tools.ietf.org/html/rfc4055#section-3.2

   To convert a signature value to a bit string, the most significant
   bit of the first octet of the signature value SHALL become the first
   bit of the bit string, and so on through the least significant bit of
   the last octet of the signature value, which SHALL become the last
   bit of the bit string.

This is, coincidentally, the same as the DER encoding, except for the phase
byte.

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:94: !CBB_did_write(&sig_bitstring, sig_len))
{
On 2016/03/03 18:32:06, Ryan Sleevi wrote:
> This feels like such a gross way to do it. Is this really how you envision the
> new BoringSSL signing world? This seems like so much boilerplate and so much
> opportunity to get things wrong.

Which piece of boilerplate in particular?

If you mean that we have to transcribe the ASN.1 type, someone has to do it. We
have to transcribe it in the Mac code too. If you prefer, I can make BoringSSL
provide a new version of the NETSCAPE_SPKI type. I figured this was such a
one-off, it may as well live in Chromium.


If you mean the reserve + did_write dance, it just avoids allocating a buffer
and stuff. *shrug* You can call it two step like that. You can also just use
EVP_PKEY_size to find the max size, though I find this confusing. (I think
historically they were lax about that vs. "key size in bytes" because RSA is the
same, and then they retconned EVP_PKEY_size to mean maximum signature size due
to DSA.) You can also allocate a separate buffer and stuff.

I can also add some EVP_DigestSignFinal variant that appends to a CBB. That
wouldn't be crazy.


If you mean the X.509-level assembly with signature algorithm and stuff, I am
hoping that most people will not have to deal with such nonsense. Hence the enum
and whatnot. :-)

At one point, I had something resembling AlgorithmIdentifier APIs in EVP. But
they still had ties to the old stack and you convinced me they were wrong
because net/cert will need to parse signature algorithms *and* enforce policies
on them. Accordingly, I've left BoringSSL core's APIs a layer below that. We
handle parsing SPKIs because that's used generally, but consumers that need
X.509 bits should use the X.509 stack built on top of it.

Except the new net/cert X.509 stack doesn't actually have signatureAlgorithm
mechanisms for creating X.509 signatures, only consuming them, hence this code
needs to do it itself until we have a clearer story there. (If we need one. I
actually think this code is pretty straight-forward.)

(Relatedly, the BoringSSL vs net/cert split is kind of awkward. We'll probably
need to figure out some of this at some point.)

[Ryan Sleevi, 2016-03-03 20:27:41]

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
File net/base/keygen_handler_openssl.cc (right):

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:63: challenge_.size()) ||
On 2016/03/03 19:58:15, davidben wrote:
> On 2016/03/03 18:32:06, Ryan Sleevi wrote:
> > Is this fine if challenge_ is empty? I suspect any issue is pre-existing,
but
> > wasn't sure if the ASN1_STRING_set handled things differently than
> CBB_add_bytes
> 
> If challenge is empty, we'll emit an empty IA5STRING which I believe should be
> correct. Empty strings are valid strings and the field isn't OPTIONAL or
> anything. I have no clue what the old crazy code did. :-)

Specifically, by using .data() rather than .c_str(), we can't guarantee that if
.size() == 0 that *data() will return anything meaningful (that is, it's
undefined behaviour). I wanted to ensure that this function, if .size() == 0,
did not try to access .data()

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:79: };
On 2016/03/03 19:58:15, davidben wrote:
> I haven't decoupled BoringSSL core's internals from that table yet (have a
> branch not yet reviewed), but I'm envisioning that, like the net/cert code,
each
> module which needs to know about particular OIDs would just embed them and
> memcmp as appropriate.

I do not think that would be a good state - it would duplicate that string
through multiple TUs. There's no guarantee that the linker will be able to
de-dup these strings.

That means everything dealing with MD5 will have to carry the full weight of MD5
with them everywhere.

I totally understand there are tradeoffs, but this feels like going too far in
the other way, in that it multiplies the size across TUs. Should we have a
common header with this value (extern'd)? If so, should that header live in
//net, //crypto, or BoringSSL?

That large table itself seems problematic because it prevents dead code
elimination, so I'm totally sensitive to that. Can we just have a common
constant accessible inter-TU?

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:88: !CBB_add_u8(&sig_bitstring, 0 /* padding
*/) ||
That was much more than needed, but still doesn't answer my question, because I
feel it misunderstood what I ask.

Both "padding" and "phase byte" don't make sense, *IF* this is the DER-encoding
signifier of the number of unused bits. Is this that byte?

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:94: !CBB_did_write(&sig_bitstring, sig_len))
{
On 2016/03/03 19:58:15, davidben wrote:
> If you mean that we have to transcribe the ASN.1 type, someone has to do it.
We
> have to transcribe it in the Mac code too. If you prefer, I can make BoringSSL
> provide a new version of the NETSCAPE_SPKI type. I figured this was such a
> one-off, it may as well live in Chromium.

Specifically, that the act of transcribing a SignatureAlgorithm + Signature pair
(which is consistent for all of the SignedData variants - X.509, CRL, OCSP,
SPKAC) into ASN.1; it seems like a common enough pattern that you already had to
duplicate it twice, but I anticipate we'd need to duplicate it more.

[davidben, 2016-03-03 21:59:37]

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
File net/base/keygen_handler_openssl.cc (right):

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:63: challenge_.size()) ||
On 2016/03/03 20:27:41, Ryan Sleevi wrote:
> On 2016/03/03 19:58:15, davidben wrote:
> > On 2016/03/03 18:32:06, Ryan Sleevi wrote:
> > > Is this fine if challenge_ is empty? I suspect any issue is pre-existing,
> but
> > > wasn't sure if the ASN1_STRING_set handled things differently than
> > CBB_add_bytes
> > 
> > If challenge is empty, we'll emit an empty IA5STRING which I believe should
be
> > correct. Empty strings are valid strings and the field isn't OPTIONAL or
> > anything. I have no clue what the old crazy code did. :-)
> 
> Specifically, by using .data() rather than .c_str(), we can't guarantee that
if
> .size() == 0 that *data() will return anything meaningful (that is, it's
> undefined behaviour). I wanted to ensure that this function, if .size() == 0,
> did not try to access .data()

If you pass CBB_add_bytes a length of zero, it's not going to do anything dumb
to it. If it does, that's a bug. We're not going to have ridiculous APIs like
that in BoringSSL, particularly not our own code. :-P

(But C++11 makes .c_str() and .data() identical, so this is moot.)

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:79: };
On 2016/03/03 20:27:41, Ryan Sleevi wrote:
> On 2016/03/03 19:58:15, davidben wrote:
> > I haven't decoupled BoringSSL core's internals from that table yet (have a
> > branch not yet reviewed), but I'm envisioning that, like the net/cert code,
> each
> > module which needs to know about particular OIDs would just embed them and
> > memcmp as appropriate.
> 
> I do not think that would be a good state - it would duplicate that string
> through multiple TUs. There's no guarantee that the linker will be able to
> de-dup these strings.

The string wouldn't be duplicated. That's largely the point. My branch (not yet
uploaded) in BoringSSL never needs to define a single constant more than once.
If it did, I would add a helper function for that type of OID (or higher-level
structure).

Nothing but an SPKI parser ever needs to process SPKI key type OIDs. So we put
them in the SPKI parser/encoder. Nothing but a parser for hash algorithm OIDs
ever needs to process hash algorithm OIDs. So we put them in the hash algorithm
parser/encoder, etc.

> That means everything dealing with MD5 will have to carry the full weight of
MD5
> with them everywhere.

I do not follow. We either make it so the function implicitly accepts everything
(which you convinced me was bad, and I agree), or code which accepts MD5 needs
to be able to dispatch around that.

> I totally understand there are tradeoffs, but this feels like going too far in
> the other way, in that it multiplies the size across TUs. Should we have a
> common header with this value (extern'd)? If so, should that header live in
> //net, //crypto, or BoringSSL?
> 
> That large table itself seems problematic because it prevents dead code
> elimination, so I'm totally sensitive to that. Can we just have a common
> constant accessible inter-TU?

It's certainly possible BoringSSL needs to have a master list of these
constants. Thus far, I haven't found this to be the case. I'd rather be able to
keep this stuff internal to the modules if possible. I think that's a much
better design as it avoids consumers having to know about OIDs.

I'd rather they simply know about things like "parse out a hash AlgID and give
me an EVP_MD" and "given an EVP_MD, write out the AlgID for it".

Likewise, you never actually need to write out id-rsaEncryption. You need to
write out an SPKI. Thus it makes much more sense for us to fold the
id-rsaEncryption definition into the SPKI and PKCS#8 APIs.

Unfortunately, net/cert/internal doesn't have a DER encoder or any of that
stack, so it has to be duplicated right now. See below.

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:88: !CBB_add_u8(&sig_bitstring, 0 /* padding
*/) ||
On 2016/03/03 20:27:41, Ryan Sleevi wrote:
> That was much more than needed, but still doesn't answer my question, because
I
> feel it misunderstood what I ask.
> 
> Both "padding" and "phase byte" don't make sense, *IF* this is the
DER-encoding
> signifier of the number of unused bits. Is this that byte?

Yes, this is that byte. I tend to call it the "phase byte", but I think I'm the
only one who does. It is not an assumption that it is zero. It is simply how you
encode signatures in X.509.

Would you be happy with

  !CBB_add_u8(&sig_bitstring, 0 /* no unused bits */)

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:94: !CBB_did_write(&sig_bitstring, sig_len))
{
On 2016/03/03 20:27:41, Ryan Sleevi wrote:
> On 2016/03/03 19:58:15, davidben wrote:
> > If you mean that we have to transcribe the ASN.1 type, someone has to do it.
> We
> > have to transcribe it in the Mac code too. If you prefer, I can make
BoringSSL
> > provide a new version of the NETSCAPE_SPKI type. I figured this was such a
> > one-off, it may as well live in Chromium.
> 
> Specifically, that the act of transcribing a SignatureAlgorithm + Signature
pair
> (which is consistent for all of the SignedData variants - X.509, CRL, OCSP,
> SPKAC) into ASN.1; it seems like a common enough pattern that you already had
to
> duplicate it twice, but I anticipate we'd need to duplicate it more.

I don't believe I've ever had to write this before. Verifying certificates, but
the net/cert stack seems to not bother with creating them. Certainly, this bit
really should be part of net/cert. I'm hoping someone can factor that out and
put it into net/cert where it belongs if we ever get a second copy.

In the meantime, net/cert seems to believe X.509 signature algorithms are its
prerogative, so I don't want to also have X.509 signature algorithm
representations in BoringSSL. Hence this code is Chromium.

[Ryan Sleevi, 2016-03-03 22:08:43]

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
File net/base/keygen_handler_openssl.cc (right):

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:79: };
On 2016/03/03 21:59:36, davidben wrote:
> Nothing but an SPKI parser ever needs to process SPKI key type OIDs. So we put
> them in the SPKI parser/encoder. Nothing but a parser for hash algorithm OIDs
> ever needs to process hash algorithm OIDs. So we put them in the hash
algorithm
> parser/encoder, etc.

I think we're in agreement here. Specifically, my concern was wanting to have
either something like

extern const uint8_t kMD5Encryption[stuff];

or something like

void GetMD5EncryptionOID(uint8_t*, size_t*);

or something like

enum SignatureAlgorithms {
  MD5,
  SHA-1
  SHA-256
};

void GetSignatureAlgorithm(SignatureAlgorithms, uint8_t*, size_t*);

or basically anything so that only 1 of the TUs has the full expansion. Is that
what you were thinking?

> I'd rather they simply know about things like "parse out a hash AlgID and give
> me an EVP_MD" and "given an EVP_MD, write out the AlgID for it".

+1, it's this comment that made me think we're on the same page.

To be clear, my goal was to avoid something like:
a.cc
static const uint8_t kMyMD5WithRSA[] = { ... };
b.cc
static const uint8_t kAnotherMD5WithRSA[] = { ... };
c.cc
static const uint8_t kThirdMD5WithRSA[] = { ... };


Because that's split across TUs, there's no guarantee that the linker will be
allowed to fold those strings into a single address.

> Likewise, you never actually need to write out id-rsaEncryption. You need to
> write out an SPKI. Thus it makes much more sense for us to fold the
> id-rsaEncryption definition into the SPKI and PKCS#8 APIs.

I think we're on the same page here.

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:88: !CBB_add_u8(&sig_bitstring, 0 /* padding
*/) ||
On 2016/03/03 21:59:36, davidben wrote:
> Would you be happy with
> 
>   !CBB_add_u8(&sig_bitstring, 0 /* no unused bits */)

Yup. That's all I was trying to understand - to make sure that this is the /* no
unused bits */ byte :)

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:94: !CBB_did_write(&sig_bitstring, sig_len))
{
On 2016/03/03 21:59:36, davidben wrote:
> I don't believe I've ever had to write this before. Verifying certificates,
but
> the net/cert stack seems to not bother with creating them. Certainly, this bit
> really should be part of net/cert. I'm hoping someone can factor that out and
> put it into net/cert where it belongs if we ever get a second copy.

Oh, I thought you were saying you had a second copy (for the Mac code) - that
is, in this CL.

If this is the only use, I agree; and I suspect we *should* factor this out into
a common function in //net/cert (ok, //net/base can't depend on //net/cert so
we'll figure that out later) to handle other SignedData constructs.

[davidben, 2016-03-03 22:25:40]

Updated CL slightly to reflect discussion.

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
File net/base/keygen_handler_openssl.cc (right):

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:79: };
On 2016/03/03 22:08:43, Ryan Sleevi wrote:
> On 2016/03/03 21:59:36, davidben wrote:
> > Nothing but an SPKI parser ever needs to process SPKI key type OIDs. So we
put
> > them in the SPKI parser/encoder. Nothing but a parser for hash algorithm
OIDs
> > ever needs to process hash algorithm OIDs. So we put them in the hash
> algorithm
> > parser/encoder, etc.
> 
> I think we're in agreement here. Specifically, my concern was wanting to have
> either something like
> 
> extern const uint8_t kMD5Encryption[stuff];
> 
> or something like
> 
> void GetMD5EncryptionOID(uint8_t*, size_t*);
> 
> or something like
> 
> enum SignatureAlgorithms {
>   MD5,
>   SHA-1
>   SHA-256
> };
> 
> void GetSignatureAlgorithm(SignatureAlgorithms, uint8_t*, size_t*);
> 
> or basically anything so that only 1 of the TUs has the full expansion. Is
that
> what you were thinking?

Yup. I haven't done that here since signature_algorithm.cc doesn't even know
about MD5 anyway and it doesn't have a DER encoder which makes it awkward.

> > I'd rather they simply know about things like "parse out a hash AlgID and
give
> > me an EVP_MD" and "given an EVP_MD, write out the AlgID for it".
> 
> +1, it's this comment that made me think we're on the same page.
> 
> To be clear, my goal was to avoid something like:
> a.cc
> static const uint8_t kMyMD5WithRSA[] = { ... };
> b.cc
> static const uint8_t kAnotherMD5WithRSA[] = { ... };
> c.cc
> static const uint8_t kThirdMD5WithRSA[] = { ... };
> 
> 
> Because that's split across TUs, there's no guarantee that the linker will be
> allowed to fold those strings into a single address.
> 
> > Likewise, you never actually need to write out id-rsaEncryption. You need to
> > write out an SPKI. Thus it makes much more sense for us to fold the
> > id-rsaEncryption definition into the SPKI and PKCS#8 APIs.
> 
> I think we're on the same page here.

Cool. Yeah, I'll be sure to send the start of the "detach from crypto/obj" CLs
your way.

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:88: !CBB_add_u8(&sig_bitstring, 0 /* padding
*/) ||
On 2016/03/03 22:08:43, Ryan Sleevi wrote:
> On 2016/03/03 21:59:36, davidben wrote:
> > Would you be happy with
> > 
> >   !CBB_add_u8(&sig_bitstring, 0 /* no unused bits */)
> 
> Yup. That's all I was trying to understand - to make sure that this is the /*
no
> unused bits */ byte :)

Done.

https://codereview.chromium.org/1742873002/diff/100001/net/base/keygen_handle...
net/base/keygen_handler_openssl.cc:94: !CBB_did_write(&sig_bitstring, sig_len))
{
On 2016/03/03 22:08:43, Ryan Sleevi wrote:
> On 2016/03/03 21:59:36, davidben wrote:
> > I don't believe I've ever had to write this before. Verifying certificates,
> but
> > the net/cert stack seems to not bother with creating them. Certainly, this
bit
> > really should be part of net/cert. I'm hoping someone can factor that out
and
> > put it into net/cert where it belongs if we ever get a second copy.
> 
> Oh, I thought you were saying you had a second copy (for the Mac code) - that
> is, in this CL.

Oh! No, I mean, the Mac code has an existing separate copy of serializing this
structure because historically we used the Mac APIs for this. I didn't want to
bother with deduplicating that here. Even then, it'd largely be about factoring
out the SPKAC construction and not the signature because Mac doesn't use
EVP_PKEY.

> If this is the only use, I agree; and I suspect we *should* factor this out
into
> a common function in //net/cert (ok, //net/base can't depend on //net/cert so
> we'll figure that out later) to handle other SignedData constructs.

Added a TODO about that.

[Ryan Sleevi, 2016-03-11 17:56:45]

Sorry, forgot to LGTM. Thanks for the changes, all good.

[davidben, 2016-03-11 19:11:29]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2016-03-11 19:12:40]

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

[commit-bot: I haz the power, 2016-03-11 19:49:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 19:49:58]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[davidben, 2016-03-11 21:24:56]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2016-03-11 21:25:24]

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

[commit-bot: I haz the power, 2016-03-11 23:01:11]

Description was changed from

==========
Switch //net to the new SPKI and PKCS#8 APIs.

This fixes the new verifier's SPKI parser and gets us closer to losing the
legacy ASN.1 stack. This also rewrites the Android KeygenHandler
implementation. This was tested manually, but to make sure it's good, make
the tests actually parse out the SPKAC structure and verify the signature.
(Everything that implements keygen may freely assume BoringSSL, so we have
CBS and EVP available.)

In doing so, this revealed a bug in our Mac SPKAC code. It was omitting the
NULLs in both SPKI and signatureAlgorithm, and EVP_parse_public_key is
strict about including them, per the MUST in the spec. (I've confirmed that
Safari includes them, so it's just us that messed this up.)

This resolves the last TODO about laxness in VerifySignedDataTest, so we
should now be parsing SPKIs correctly. (Finally...)

BUG=499653,522228
==========

to

==========
Switch //net to the new SPKI and PKCS#8 APIs.

This fixes the new verifier's SPKI parser and gets us closer to losing the
legacy ASN.1 stack. This also rewrites the Android KeygenHandler
implementation. This was tested manually, but to make sure it's good, make
the tests actually parse out the SPKAC structure and verify the signature.
(Everything that implements keygen may freely assume BoringSSL, so we have
CBS and EVP available.)

In doing so, this revealed a bug in our Mac SPKAC code. It was omitting the
NULLs in both SPKI and signatureAlgorithm, and EVP_parse_public_key is
strict about including them, per the MUST in the spec. (I've confirmed that
Safari includes them, so it's just us that messed this up.)

This resolves the last TODO about laxness in VerifySignedDataTest, so we
should now be parsing SPKIs correctly. (Finally...)

BUG=499653,522228
==========

[commit-bot: I haz the power, 2016-03-11 23:02:40]

Description was changed from

==========
Switch //net to the new SPKI and PKCS#8 APIs.

This fixes the new verifier's SPKI parser and gets us closer to losing the
legacy ASN.1 stack. This also rewrites the Android KeygenHandler
implementation. This was tested manually, but to make sure it's good, make
the tests actually parse out the SPKAC structure and verify the signature.
(Everything that implements keygen may freely assume BoringSSL, so we have
CBS and EVP available.)

In doing so, this revealed a bug in our Mac SPKAC code. It was omitting the
NULLs in both SPKI and signatureAlgorithm, and EVP_parse_public_key is
strict about including them, per the MUST in the spec. (I've confirmed that
Safari includes them, so it's just us that messed this up.)

This resolves the last TODO about laxness in VerifySignedDataTest, so we
should now be parsing SPKIs correctly. (Finally...)

BUG=499653,522228
==========

to

==========
Switch //net to the new SPKI and PKCS#8 APIs.

This fixes the new verifier's SPKI parser and gets us closer to losing the
legacy ASN.1 stack. This also rewrites the Android KeygenHandler
implementation. This was tested manually, but to make sure it's good, make
the tests actually parse out the SPKAC structure and verify the signature.
(Everything that implements keygen may freely assume BoringSSL, so we have
CBS and EVP available.)

In doing so, this revealed a bug in our Mac SPKAC code. It was omitting the
NULLs in both SPKI and signatureAlgorithm, and EVP_parse_public_key is
strict about including them, per the MUST in the spec. (I've confirmed that
Safari includes them, so it's just us that messed this up.)

This resolves the last TODO about laxness in VerifySignedDataTest, so we
should now be parsing SPKIs correctly. (Finally...)

BUG=499653,522228

Committed: https://crrev.com/f43769e980b0948775860095c672d17969c70a19
Cr-Commit-Position: refs/heads/master@{#380774}
==========

[commit-bot: I haz the power, 2016-03-11 23:02:42]

Patchset 7 (id:??) landed as
https://crrev.com/f43769e980b0948775860095c672d17969c70a19
Cr-Commit-Position: refs/heads/master@{#380774}