Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21561003:
Add a utility method to convert SPKI from DER to JWK, so far implemented only for EC P256v1 (which … (Closed)
|
Created:
7 years, 4 months ago by juanlang Modified:
7 years, 4 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, wtc Base URL:
https://chromium.googlesource.com/chromium/src.git@domain-bound-public-key Visibility:
Public. |
DescriptionAdd a utility method to convert SPKI from DER to JWK, so far implemented only for EC P256v1 (which is used for TLS channel IDs.)
BUG=259097
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216163
Patch Set 1 #
Total comments: 8
Patch Set 2 : Style nits addressed #Patch Set 3 : Move to jwk_serializer #
Total comments: 12
Patch Set 4 : More nits addressed #
Total comments: 6
Patch Set 5 : Bug fix and nits addressed #Patch Set 6 : s/uint8_t/unsigned char/g, for windows compile #
Total comments: 4
Messages
Total messages: 15 (0 generated)
Hi Ryan, is this something along what you had in mind? I only added support for EC P256, because the cumulative amount of essentially dead code needed to support a longer list arbitrary types seemed like a bad thing, but if there are specific types you think I should support, I'm happy to. More probable is that you meant something different: please let me what that is if so :)
On 2013/08/01 18:22:51, juanlang wrote: > Hi Ryan, is this something along what you had in mind? Ping. If it helps, I can send a WIP CL that makes use of this to export these from the ServerBoundCertService too.
So yeah, this is actually pretty much what I had in mind, and I think this CL is a great start. Most of what I have is style nits, but a few technical remarks below. The only thing from seeing this code is makes me think whether we should push this down further into src/crypto, rather than net/cert... I'm guessing the reason is because subjectPublicKeyInfo is an X.509 field, even though it's widely used in other specs, whereas crypto/'s use of RSA & EC private keys use primitive forms (eg: PKCS#1 / PKCS#8). The other thing I'd wonder about is whether we should create a dedicated file for JWK (rather than x509_util). I'm not sure on this though - how much JOSE functionality do you seeing being implemented? Just the JWK formatting? net/cert/jwk_serializer.h ? Just trying to make sure we're grouping files appropriately if we see this functionality growing. Cheers https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util.h File net/cert/x509_util.h (right): https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util.h#newcode57 net/cert/x509_util.h:57: base::StringPiece spki, "const base::StringPiece&" [Yes, I realize there is frequent debate internally on this point - particularly for 64-bit - but this is at least what the Chromium team uses as the vastly dominant pattern] https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_nss.cc File net/cert/x509_util_nss.cc (right): https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_nss.cc#new... net/cert/x509_util_nss.cc:359: static bool ConvertEcPrime256v1PublicKeyInfoToJwk( Chromium prefers the unnamed namespace, especially when that's the dominant form in the file. Shift the static functions to line 240, and drop the statics. https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_nss.cc#new... net/cert/x509_util_nss.cc:366: // key, but as long as there are at least enough bytes and the first byte has It's only bogus until you realize that for BIT_STRING types, NSS stores the length not as byte length but *bit* length. See lines 1255, 1285, and 1313 of http://mxr.mozilla.org/nss/source/lib/cryptohi/seckey.c (Yes, subtle...) >> 3 will give you the byte-len. You can see this with the macro DER_ConvertBitString in http://mxr.mozilla.org/nss/source/lib/util/secdert.h#124 (although this mutates the item in place) eg: see also http://mxr.mozilla.org/nss/source/lib/cryptohi/seckey.c#268 https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_nss.cc#new... net/cert/x509_util_nss.cc:374: public_key_jwk->SetString(std::string("crv"), std::string("P-256")); Remove the explicit std::string constructors - let implicit conversion handle this. public_key_jwk->SetString("alg", "EC"); public_key_jwk->SetString("crv", "P-256"); https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_nss_unitte... File net/cert/x509_util_nss_unittest.cc (right): https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_nss_unitte... net/cert/x509_util_nss_unittest.cc:181: static const unsigned char spki_ec[] = { kSpkiEc, as per http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Consta... same for other constants https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_nss_unitte... net/cert/x509_util_nss_unittest.cc:230: EXPECT_EQ(std::string("P-256"), string_value); eliminate all the extra std::string() stuff here. If you're having trouble with these strings, use EXPECT_STREQ https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_nss_unitte... net/cert/x509_util_nss_unittest.cc:273: kEcPointSize)); Please consider running clang-format on this test. For example, this EQ should be on a new line indented four spaces EXPECT_EQ( 0, memcmp(..., ..., ...)); https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_openssl.cc File net/cert/x509_util_openssl.cc (right): https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_openssl.cc... net/cert/x509_util_openssl.cc:66: return false; Should be fairly similar, right?
Thanks very much for your help, Ryan. Especially, for reading between the lines and catching the style issues I was wondering about as I wrote this. On 2013/08/02 23:08:43, Ryan Sleevi wrote: > The only thing from seeing this code is makes me think whether we should push > this down further into src/crypto, rather than net/cert... > > I'm guessing the reason is because subjectPublicKeyInfo is an X.509 field, even > though it's widely used in other specs, whereas crypto/'s use of RSA & EC > private keys use primitive forms (eg: PKCS#1 / PKCS#8). Yeah, this is a little unclear to me too, as part of this code is dealing with der decoding, and part is using primitive forms. I'm leaning toward staying with net, just because the bulk of the code deals with der decoding (mainly oid comparison.) I could imagine that, if the variety of primitive key types grows, the jwk formatting code would rely on help from the crypto code to unpack a primitive key, but since there isn't much demand for primitive key decoding yet, I hope we don't get there. > The other thing I'd wonder about is whether we should create a dedicated file > for JWK (rather than x509_util). I'm not sure on this though - how much JOSE > functionality do you seeing being implemented? Just the JWK formatting? Yes, that's as much as I know of for now. The only future work we're hoping for is some sort of proof-key federation protocol, see http://www.browserauth.net/proof-key-federation-protocols. For now the proposal there is to use x.509 certs, but we'll see whether there's some web api-friendlier alternative available. > net/cert/jwk_serializer.h ? > > Just trying to make sure we're grouping files appropriately if we see this > functionality growing. Thanks, this is exactly the kind of help I like from a reviewer more familiar with the code base than I: I don't like sticking things in "util" any more than I have to, as, in the limit, the entire codebase is a utility of something else. I like this, but please confirm you're ok with sticking with net/cert/. > https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util.h > File net/cert/x509_util.h (right): > > https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util.h#newcode57 > net/cert/x509_util.h:57: base::StringPiece spki, > "const base::StringPiece&" [Yes, I realize there is frequent debate internally > on this point - particularly for 64-bit - but this is at least what the Chromium > team uses as the vastly dominant pattern] Yes, this was a question I had, thanks for spotting it. Apologies for not resetting my "what's the nearest code do" metric when I moved this out of content/. Done. > https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_nss.cc > File net/cert/x509_util_nss.cc (right): > > https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_nss.cc#new... > net/cert/x509_util_nss.cc:359: static bool > ConvertEcPrime256v1PublicKeyInfoToJwk( > Chromium prefers the unnamed namespace, especially when that's the dominant form > in the file. > > Shift the static functions to line 240, and drop the statics. Thanks, that helps. Done. > https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_nss.cc#new... > net/cert/x509_util_nss.cc:366: // key, but as long as there are at least enough > bytes and the first byte has > It's only bogus until you realize that for BIT_STRING types, NSS stores the > length not as byte length but *bit* length. Ah! Duh. Thanks for the clue, I should have been more suspicious that the value I saw was not random, but consistent, and guessed what it could have been. Comment updated and code made more strict. > https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_nss.cc#new... > net/cert/x509_util_nss.cc:374: public_key_jwk->SetString(std::string("crv"), > std::string("P-256")); > Remove the explicit std::string constructors - let implicit conversion handle > this. Done. > https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_nss_unitte... > File net/cert/x509_util_nss_unittest.cc (right): > > https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_nss_unitte... > net/cert/x509_util_nss_unittest.cc:181: static const unsigned char spki_ec[] = { > kSpkiEc, D'oh, sorry about that. Done. > https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_nss_unitte... > net/cert/x509_util_nss_unittest.cc:230: EXPECT_EQ(std::string("P-256"), > string_value); > eliminate all the extra std::string() stuff here. > > If you're having trouble with these strings, use EXPECT_STREQ Done. > https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_nss_unitte... > net/cert/x509_util_nss_unittest.cc:273: kEcPointSize)); > Please consider running clang-format on this test. > > For example, this EQ should be on a new line indented four spaces > EXPECT_EQ( > 0, > memcmp(..., > ..., > ...)); clang-format slightly disagrees with you. I followed its suggestion of: EXPECT_EQ(0, memcmp(..., ..., ...)); In other words, done? > https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_openssl.cc > File net/cert/x509_util_openssl.cc (right): > > https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_openssl.cc... > net/cert/x509_util_openssl.cc:66: return false; > Should be fairly similar, right? Ah, should I also implement this for openssl? For which other platforms? The only use of this I'm proposing is for TLS channel IDs, which are only implemented for nss, but as a general purpose function I can see that it should be more broadly implemented. I don't know which platforms I should care about and which I should ignore. For whatever it's worth, I'm comfortable enough also implementing this using MS's crypto api, but not at all for ios.
Note you can reply using the WebUI, so at least it can track the discussion cleaner. (eg: double click a review comment) Neither here nor there though :) On 2013/08/05 17:50:16, juanlang wrote: > Thanks very much for your help, Ryan. Especially, for reading between the lines > and catching the style issues I was wondering about as I wrote this. > > On 2013/08/02 23:08:43, Ryan Sleevi wrote: > > The only thing from seeing this code is makes me think whether we should push > > this down further into src/crypto, rather than net/cert... > > > > I'm guessing the reason is because subjectPublicKeyInfo is an X.509 field, > even > > though it's widely used in other specs, whereas crypto/'s use of RSA & EC > > private keys use primitive forms (eg: PKCS#1 / PKCS#8). > > Yeah, this is a little unclear to me too, as part of this code is dealing with > der decoding, and part is using primitive forms. I'm leaning toward staying with > net, just because the bulk of the code deals with der decoding (mainly oid > comparison.) I could imagine that, if the variety of primitive key types grows, > the jwk formatting code would rely on help from the crypto code to unpack a > primitive key, but since there isn't much demand for primitive key decoding yet, > I hope we don't get there. > > > The other thing I'd wonder about is whether we should create a dedicated file > > for JWK (rather than x509_util). I'm not sure on this though - how much JOSE > > functionality do you seeing being implemented? Just the JWK formatting? > > Yes, that's as much as I know of for now. The only future work we're hoping for > is some sort of proof-key federation protocol, see > http://www.browserauth.net/proof-key-federation-protocols. For now the proposal > there is to use x.509 certs, but we'll see whether there's some web > api-friendlier alternative available. > > > net/cert/jwk_serializer.h ? > > > > Just trying to make sure we're grouping files appropriately if we see this > > functionality growing. > > Thanks, this is exactly the kind of help I like from a reviewer more familiar > with the code base than I: I don't like sticking things in "util" any more than > I have to, as, in the limit, the entire codebase is a utility of something else. > I like this, but please confirm you're ok with sticking with net/cert/. Yes, net/cert/jwk_serializer.h SGTM (especially given x5c/x5u types) https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_nss_unitte... > > net/cert/x509_util_nss_unittest.cc:273: kEcPointSize)); > > Please consider running clang-format on this test. > > > > For example, this EQ should be on a new line indented four spaces > > EXPECT_EQ( > > 0, > > memcmp(..., > > ..., > > ...)); > > clang-format slightly disagrees with you. I followed its suggestion of: > EXPECT_EQ(0, > memcmp(..., > ..., > ...)); > > In other words, done? If clang-format is happy, WFM. > > > https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_openssl.cc > > File net/cert/x509_util_openssl.cc (right): > > > > > https://codereview.chromium.org/21561003/diff/1/net/cert/x509_util_openssl.cc... > > net/cert/x509_util_openssl.cc:66: return false; > > Should be fairly similar, right? > > Ah, should I also implement this for openssl? For which other platforms? The > only use of this I'm proposing is for TLS channel IDs, which are only > implemented for nss, but as a general purpose function I can see that it should > be more broadly implemented. I don't know which platforms I should care about > and which I should ignore. For whatever it's worth, I'm comfortable enough also > implementing this using MS's crypto api, but not at all for ios. We've slowly eliminated most of the platform-specific crypto in favour of (NSS or OpenSSL), where OpenSSL is only used on Chrome for Android (and derived projects). There's been talk of supporting Channel ID on OpenSSL, and a desire from the Android team to do so (they've included the OpenSSL patches), but that's not urgently pressing at the moment, as I understand it. If you can implement this now (for OpenSSL), that would cover all platforms, and also make sure all the test coverage is glued up, but I can live with TODO()ing this for the channel ID support if needed.
On 2013/08/05 18:12:10, Ryan Sleevi wrote: > Note you can reply using the WebUI, so at least it can track the discussion > cleaner. (eg: double click a review comment) Yeah, me and my vim hands don't like mousing very much. codereview should learn to parse replies :) (And, noted for next time.) > Yes, net/cert/jwk_serializer.h SGTM (especially given x5c/x5u types) Done. > > Ah, should I also implement this for openssl? For which other platforms? The > > only use of this I'm proposing is for TLS channel IDs, which are only > > implemented for nss, but as a general purpose function I can see that it > should > > be more broadly implemented. I don't know which platforms I should care about > > and which I should ignore. For whatever it's worth, I'm comfortable enough > also > > implementing this using MS's crypto api, but not at all for ios. > > We've slowly eliminated most of the platform-specific crypto in favour of (NSS > or OpenSSL), where OpenSSL is only used on Chrome for Android (and derived > projects). > > There's been talk of supporting Channel ID on OpenSSL, and a desire from the > Android team to do so (they've included the OpenSSL patches), but that's not > urgently pressing at the moment, as I understand it. > > If you can implement this now (for OpenSSL), that would cover all platforms, and > also make sure all the test coverage is glued up, but I can live with TODO()ing > this for the channel ID support if needed. In this case, I'm not sure adding it to Android is worth it. The only intended use for this, so far, is to export the TLS channel ID to extensions. There's no support for extensions in Android, nor any plan to add them, so I think it's worth not adding dead code for the time being. I don't feel super strongly about it, but in general I don't like dead code.
https://codereview.chromium.org/21561003/diff/12001/net/cert/jwk_serializer_n... File net/cert/jwk_serializer_nss.cc (right): https://codereview.chromium.org/21561003/diff/12001/net/cert/jwk_serializer_n... net/cert/jwk_serializer_nss.cc:27: // nit: extraneous // https://codereview.chromium.org/21561003/diff/12001/net/cert/jwk_serializer_n... net/cert/jwk_serializer_nss.cc:101: SECKEY_DestroySubjectPublicKeyInfo(spki); If you want added RAII safety, https://code.google.com/p/chromium/codesearch#chromium/src/google_apis/cup/cl... https://codereview.chromium.org/21561003/diff/12001/net/cert/jwk_serializer_n... File net/cert/jwk_serializer_nss_unittest.cc (right): https://codereview.chromium.org/21561003/diff/12001/net/cert/jwk_serializer_n... net/cert/jwk_serializer_nss_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. Rather than define an NSS-only unittest, just call this jwk_serializer_unittest.cc Then either use an #ifdef or use .gyp to exclude this file for use_openssl==1 https://codereview.chromium.org/21561003/diff/12001/net/cert/jwk_serializer_n... net/cert/jwk_serializer_nss_unittest.cc:13: static const unsigned char kP256SpkiPrefix[] = { nit: Comment explaining what this prefix is (I can see it's a SEQUENCE of a SEQUENCE and something - I'm guessing OID + NULL params, but that's just because I read ASN.1 :p) https://codereview.chromium.org/21561003/diff/12001/net/cert/jwk_serializer_n... net/cert/jwk_serializer_nss_unittest.cc:21: static const unsigned char kSpkiEc[] = { nit: comments https://codereview.chromium.org/21561003/diff/12001/net/cert/jwk_serializer_n... net/cert/jwk_serializer_nss_unittest.cc:36: static const unsigned char kSpkiEcWithZeroXY[] = { nit: comments
Thanks again, Ryan. Here you go. And, qq: what about function capitalization? The Chromium/Google style guides don't provide any guidance here, where I'm mixing acronyms with English words in the same function name. https://codereview.chromium.org/21561003/diff/12001/net/cert/jwk_serializer_n... File net/cert/jwk_serializer_nss.cc (right): https://codereview.chromium.org/21561003/diff/12001/net/cert/jwk_serializer_n... net/cert/jwk_serializer_nss.cc:27: // On 2013/08/06 19:50:38, Ryan Sleevi wrote: > nit: extraneous // Done. https://codereview.chromium.org/21561003/diff/12001/net/cert/jwk_serializer_n... net/cert/jwk_serializer_nss.cc:101: SECKEY_DestroySubjectPublicKeyInfo(spki); On 2013/08/06 19:50:38, Ryan Sleevi wrote: > If you want added RAII safety, > https://code.google.com/p/chromium/codesearch#chromium/src/google_apis/cup/cl... Nifty. Done. https://codereview.chromium.org/21561003/diff/12001/net/cert/jwk_serializer_n... File net/cert/jwk_serializer_nss_unittest.cc (right): https://codereview.chromium.org/21561003/diff/12001/net/cert/jwk_serializer_n... net/cert/jwk_serializer_nss_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/08/06 19:50:38, Ryan Sleevi wrote: > Rather than define an NSS-only unittest, just call this > jwk_serializer_unittest.cc > > Then either use an #ifdef or use .gyp to exclude this file for use_openssl==1 Done. https://codereview.chromium.org/21561003/diff/12001/net/cert/jwk_serializer_n... net/cert/jwk_serializer_nss_unittest.cc:13: static const unsigned char kP256SpkiPrefix[] = { On 2013/08/06 19:50:38, Ryan Sleevi wrote: > nit: Comment explaining what this prefix is (I can see it's a SEQUENCE of a > SEQUENCE and something - I'm guessing OID + NULL params, but that's just because > I read ASN.1 :p) Done. https://codereview.chromium.org/21561003/diff/12001/net/cert/jwk_serializer_n... net/cert/jwk_serializer_nss_unittest.cc:21: static const unsigned char kSpkiEc[] = { On 2013/08/06 19:50:38, Ryan Sleevi wrote: > nit: comments Done. https://codereview.chromium.org/21561003/diff/12001/net/cert/jwk_serializer_n... net/cert/jwk_serializer_nss_unittest.cc:36: static const unsigned char kSpkiEcWithZeroXY[] = { On 2013/08/06 19:50:38, Ryan Sleevi wrote: > nit: comments Done.
LGTM, mod nit/bug https://codereview.chromium.org/21561003/diff/19001/net/cert/jwk_serializer.h File net/cert/jwk_serializer.h (right): https://codereview.chromium.org/21561003/diff/19001/net/cert/jwk_serializer.h... net/cert/jwk_serializer.h:22: NET_EXPORT_PRIVATE bool ConvertSPKIFromDERToJWK( Non-normatively, http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Type_N... suggests that acronyms are only capitalized on the first latter ConvertSpkiFromDerToJwk Although within net/ (and Chromium in general), that's not really widely followed (eg: everything "SSL") https://codereview.chromium.org/21561003/diff/19001/net/cert/jwk_serializer_n... File net/cert/jwk_serializer_nss.cc (right): https://codereview.chromium.org/21561003/diff/19001/net/cert/jwk_serializer_n... net/cert/jwk_serializer_nss.cc:75: base::DictionaryValue* public_key_jwk) { BUG? Missing crypto::EnsureNSSInit() ? https://codereview.chromium.org/21561003/diff/19001/net/cert/jwk_serializer_n... net/cert/jwk_serializer_nss.cc:82: ScopedCERTSubjectPublicKeyInfo; nit: I'd suggest moving this into the unnamed namespace, rather than a function-level typedef, but either is valid.
Thanks, Ryan. Committing after trybots give the go ahead. https://codereview.chromium.org/21561003/diff/19001/net/cert/jwk_serializer.h File net/cert/jwk_serializer.h (right): https://codereview.chromium.org/21561003/diff/19001/net/cert/jwk_serializer.h... net/cert/jwk_serializer.h:22: NET_EXPORT_PRIVATE bool ConvertSPKIFromDERToJWK( On 2013/08/06 21:59:42, Ryan Sleevi wrote: > Non-normatively, > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Type_N... > suggests that acronyms are only capitalized on the first latter > > ConvertSpkiFromDerToJwk Thanks. Done. > Although within net/ (and Chromium in general), that's not really widely > followed (eg: everything "SSL") Yeah, I was thrown by that, e.g. ExtractSPKIFromDERCert. https://codereview.chromium.org/21561003/diff/19001/net/cert/jwk_serializer_n... File net/cert/jwk_serializer_nss.cc (right): https://codereview.chromium.org/21561003/diff/19001/net/cert/jwk_serializer_n... net/cert/jwk_serializer_nss.cc:75: base::DictionaryValue* public_key_jwk) { On 2013/08/06 21:59:42, Ryan Sleevi wrote: > BUG? Missing crypto::EnsureNSSInit() ? Yikes, thanks. Done. https://codereview.chromium.org/21561003/diff/19001/net/cert/jwk_serializer_n... net/cert/jwk_serializer_nss.cc:82: ScopedCERTSubjectPublicKeyInfo; On 2013/08/06 21:59:42, Ryan Sleevi wrote: > nit: I'd suggest moving this into the unnamed namespace, rather than a > function-level typedef, but either is valid. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/juanlang@google.com/21561003/17007
Retried try job too often on mac_rel for step(s) remoting_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/juanlang@google.com/21561003/17007
Message was sent while issue was closed.
Change committed as 216163
Message was sent while issue was closed.
Patch set 6 LGTM. I only did a cursory review of the CL. https://chromiumcodereview.appspot.com/21561003/diff/17007/net/cert/jwk_seria... File net/cert/jwk_serializer_nss.cc (right): https://chromiumcodereview.appspot.com/21561003/diff/17007/net/cert/jwk_seria... net/cert/jwk_serializer_nss.cc:25: static const int kPrime256v1EncodingType = 4; Nit: this 4 means the encoding format is uncompressed, so it may be better to name this constant kUncompressedEncodingType. https://chromiumcodereview.appspot.com/21561003/diff/17007/net/cert/jwk_seria... net/cert/jwk_serializer_nss.cc:28: // NSS gives the length as the bit length. Yes, I confirm this. spki->subjectPublicKey is a BIT STRING, so the length is the number of bits. https://chromiumcodereview.appspot.com/21561003/diff/17007/net/cert/jwk_seria... File net/cert/jwk_serializer_openssl.cc (right): https://chromiumcodereview.appspot.com/21561003/diff/17007/net/cert/jwk_seria... net/cert/jwk_serializer_openssl.cc:6: #include "net/cert/jwk_serializer.h" Nit: the Style Guide recommends net/cert/jwk_serializer.h should be included first, like this: #include "net/cert/jwk_serializer.h" #include "base/logging.h" https://chromiumcodereview.appspot.com/21561003/diff/17007/net/cert/jwk_seria... File net/cert/jwk_serializer_unittest.cc (right): https://chromiumcodereview.appspot.com/21561003/diff/17007/net/cert/jwk_seria... net/cert/jwk_serializer_unittest.cc:29: static const unsigned int kEcPointSize = 32U; The constant name kEcPointSize is confusing because an EC point consists of the x and y coordinates, but 32U is the size of just one coordinate. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
