Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16158005:
Adds CreateSelfSignedCertEC to x509_util.h in preparation of persistent DTLS identity store for Web… (Closed)
|
Created:
7 years, 6 months ago by jiayl Modified:
7 years, 6 months ago Reviewers:
Ryan Sleevi CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdds CreateSelfSignedCertEC to x509_util.h in preparation of persistent DTLS identity store for WebRTC.
BUG=
Patch Set 1 #
Total comments: 10
Patch Set 2 : #Patch Set 3 : #
Total comments: 2
Messages
Total messages: 11 (0 generated)
This is forked out of https://codereview.chromium.org/15969025/
On 2013/06/04 23:57:58, jiayl wrote: > This is forked out of https://codereview.chromium.org/15969025/ Ping. Could you take a look?
https://codereview.chromium.org/16158005/diff/1/net/cert/x509_util.h File net/cert/x509_util.h (right): https://codereview.chromium.org/16158005/diff/1/net/cert/x509_util.h#newcode51 net/cert/x509_util.h:51: // |subject| is a distinguished name defined in RFC4514. API DESIGN: Introducing a dependency on RFC4514 is not good. Further, it's not even what NSS supports - NSS is based on RFC 1485/1779/2253 If you're just passing common name, let's just call it common name and treat it as that. https://codereview.chromium.org/16158005/diff/1/net/cert/x509_util_nss.cc File net/cert/x509_util_nss.cc (right): https://codereview.chromium.org/16158005/diff/1/net/cert/x509_util_nss.cc#new... net/cert/x509_util_nss.cc:375: DCHECK(key); DESIGN: You probably want to be looking at IsSupportedValidityRange() to make sure your callers are supplying valid validity periods. https://codereview.chromium.org/16158005/diff/1/net/cert/x509_util_nss.cc#new... net/cert/x509_util_nss.cc:387: cert = NULL; There's no need for this "= NULL" https://codereview.chromium.org/16158005/diff/1/net/cert/x509_util_nss_unitte... File net/cert/x509_util_nss_unittest.cc (right): https://codereview.chromium.org/16158005/diff/1/net/cert/x509_util_nss_unitte... net/cert/x509_util_nss_unittest.cc:193: // Create a sample ASCII weborigin. I find this comment very confusing. There's nothing relevant to "weborigin" in this code. Seems like the comment itself is entirely unnecessary. https://codereview.chromium.org/16158005/diff/1/net/cert/x509_util_nss_unitte... net/cert/x509_util_nss_unittest.cc:197: scoped_ptr<crypto::ECPrivateKey> key(crypto::ECPrivateKey::Create()); ASSERT_TRUE(key); Your test will explode otherwise.
PTAL. Thanks! https://codereview.chromium.org/16158005/diff/1/net/cert/x509_util.h File net/cert/x509_util.h (right): https://codereview.chromium.org/16158005/diff/1/net/cert/x509_util.h#newcode51 net/cert/x509_util.h:51: // |subject| is a distinguished name defined in RFC4514. On 2013/06/06 23:26:15, Ryan Sleevi wrote: > API DESIGN: Introducing a dependency on RFC4514 is not good. > > Further, it's not even what NSS supports - NSS is based on RFC 1485/1779/2253 > > If you're just passing common name, let's just call it common name and treat it > as that. Done. https://codereview.chromium.org/16158005/diff/1/net/cert/x509_util_nss.cc File net/cert/x509_util_nss.cc (right): https://codereview.chromium.org/16158005/diff/1/net/cert/x509_util_nss.cc#new... net/cert/x509_util_nss.cc:375: DCHECK(key); On 2013/06/06 23:26:15, Ryan Sleevi wrote: > DESIGN: You probably want to be looking at IsSupportedValidityRange() to make > sure your callers are supplying valid validity periods. CreateCertificate already checks range validity and returns NULL cert if not valid. https://codereview.chromium.org/16158005/diff/1/net/cert/x509_util_nss.cc#new... net/cert/x509_util_nss.cc:387: cert = NULL; On 2013/06/06 23:26:15, Ryan Sleevi wrote: > There's no need for this "= NULL" Done. https://codereview.chromium.org/16158005/diff/1/net/cert/x509_util_nss_unitte... File net/cert/x509_util_nss_unittest.cc (right): https://codereview.chromium.org/16158005/diff/1/net/cert/x509_util_nss_unitte... net/cert/x509_util_nss_unittest.cc:193: // Create a sample ASCII weborigin. On 2013/06/06 23:26:15, Ryan Sleevi wrote: > I find this comment very confusing. There's nothing relevant to "weborigin" in > this code. > > Seems like the comment itself is entirely unnecessary. Done. https://codereview.chromium.org/16158005/diff/1/net/cert/x509_util_nss_unitte... net/cert/x509_util_nss_unittest.cc:197: scoped_ptr<crypto::ECPrivateKey> key(crypto::ECPrivateKey::Create()); On 2013/06/06 23:26:15, Ryan Sleevi wrote: > ASSERT_TRUE(key); > > Your test will explode otherwise. Done.
Could you take a look?
Please update the BUG= to reflect the bug. In examining this change, I'm trying to understand why you've opted for ECC keys for WebRTC, given that RSA will be much more interoperable. Is WebRTC's DTLS-SRTP layer prepared to generate both RSA and ECC certs on the fly? Have you thought about how to wire that up when requesting the identity? That is, the choice of cert alg & signature is somewhat contingent upon the DTLS negotiation. https://codereview.chromium.org/16158005/diff/14001/net/cert/x509_util_nss.cc File net/cert/x509_util_nss.cc (right): https://codereview.chromium.org/16158005/diff/14001/net/cert/x509_util_nss.cc... net/cert/x509_util_nss.cc:378: "CN=" + common_name, From an API perspective, it seems better to allow the caller to specify the subject, consistent with "CreateSelfSignedCert" - that is, don't force the CN= This mirrors X509Certificate::CreateSelfSigned
It seems x509Certificate::CreateSelfSigned can be used in DTLSIdentityStore to generate the cert. If that's true, this change will be unnecessary. Ryan, do you see any problem in using the existing x509Certificate::CreateSelfSigned? https://codereview.chromium.org/16158005/diff/14001/net/cert/x509_util_nss.cc File net/cert/x509_util_nss.cc (right): https://codereview.chromium.org/16158005/diff/14001/net/cert/x509_util_nss.cc... net/cert/x509_util_nss.cc:378: "CN=" + common_name, On 2013/06/11 20:01:12, Ryan Sleevi wrote: > From an API perspective, it seems better to allow the caller to specify the > subject, consistent with "CreateSelfSignedCert" - that is, don't force the CN= > > This mirrors X509Certificate::CreateSelfSigned Done.
On 2013/06/11 20:55:21, jiayl wrote: > It seems x509Certificate::CreateSelfSigned can be used in DTLSIdentityStore to > generate the cert. If that's true, this change will be unnecessary. > Ryan, do you see any problem in using the existing > x509Certificate::CreateSelfSigned? Depends on what you're using it for! Remoting has been fine with it. WebRTC is still going to have to solve these problems for DTLS client auth - https://code.google.com/p/chromium/codesearch#chromium/src/remoting/base/rsa_...
It seems these two implementations are essentially the same: libjingle: NSSKeyPair<https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjingle/source/talk/base/nssidentity.h&cl=GROK&ct=xref_jump_to_def&l=45&gsn=NSSKeyPair> ::Generate<https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjingle/source/talk/base/nssidentity.cc&ct=xref_usages&gs=cpp:talk_base::class-NSSKeyPair::Generate()@chromium/third_party/libjingle/source/talk/base/nssidentity.cc%257Cdef&l=61&gsn=Generate> chrome X509Certificate: CERTCertificate<https://code.google.com/p/chromium/codesearch#chromium/src/usr/include/nss/certt.h&cl=GROK&l=40&ct=xref_jump_to_def&gsn=CERTCertificate> *<https://code.google.com/p/chromium/codesearch#google3/GENERATED/figments/cpp/PointerTo/CERTCertificate.cc&cl=GROK&l=3&ct=xref_jump_to_def&gsn=*> CreateCertificate<https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/x509_util_nss.cc&l=85&ct=xref_usages&gs=cpp:net::%253Canonymous-namespace%253E::CreateCertificate(SECKEYPublicKeyStr%2520*,%2520const%2520std::basic_string%253Cchar%253E%2520&,%2520unsigned%2520int,%2520base::Time,%2520base::Time)@chromium/net/cert/x509_util_nss.cc%257Cdef&gsn=CreateCertificate> The only difference will be that Libjingle creates the RSA private key as sensitive and non-permanent, while the RSAPrivateKey class in Chrome does not provide such an option, but only insensitive&non-permanent¬-portable, or sensitive&permanent. Justin & Eric, will it be OK to remove the sensitive attribute from the key? On Tue, Jun 11, 2013 at 2:07 PM, <rsleevi@chromium.org> wrote: > On 2013/06/11 20:55:21, jiayl wrote: > >> It seems x509Certificate::**CreateSelfSigned can be used in >> DTLSIdentityStore to >> generate the cert. If that's true, this change will be unnecessary. >> Ryan, do you see any problem in using the existing >> x509Certificate::**CreateSelfSigned? >> > > Depends on what you're using it for! > > Remoting has been fine with it. WebRTC is still going to have to solve > these > problems for DTLS client auth - > https://code.google.com/p/**chromium/codesearch#chromium/** > src/remoting/base/rsa_key_**pair.cc&sq=package:chromium&q=** > CreateSelfSigned&type=cs&l=94<https://code.google.com/p/chromium/codesearch#chromium/src/remoting/base/rsa_key_pair.cc&sq=package:chromium&q=CreateSelfSigned&type=cs&l=94> > > > > https://codereview.chromium.**org/16158005/<https://codereview.chromium.org/1... >
There is no security being provided to libjingle in its current approach, because it always runs in the renderer, which we consider to be 'attacker controlled' for security boundaries. On Tue, Jun 11, 2013 at 2:28 PM, Jiayang Liu <jiayl@chromium.org> wrote: > It seems these two implementations are essentially the same: > libjingle: NSSKeyPair::Generate > chrome X509Certificate: CERTCertificate* CreateCertificate > > The only difference will be that Libjingle creates the RSA private key as > sensitive and non-permanent, while the RSAPrivateKey class in Chrome does > not provide such an option, but only insensitive&non-permanent¬-portable, > or sensitive&permanent. > > Justin & Eric, > > will it be OK to remove the sensitive attribute from the key? > > > > > > > On Tue, Jun 11, 2013 at 2:07 PM, <rsleevi@chromium.org> wrote: >> >> On 2013/06/11 20:55:21, jiayl wrote: >>> >>> It seems x509Certificate::CreateSelfSigned can be used in >>> DTLSIdentityStore to >>> generate the cert. If that's true, this change will be unnecessary. >>> Ryan, do you see any problem in using the existing >>> x509Certificate::CreateSelfSigned? >> >> >> Depends on what you're using it for! >> >> Remoting has been fine with it. WebRTC is still going to have to solve >> these >> problems for DTLS client auth - >> >> https://code.google.com/p/chromium/codesearch#chromium/src/remoting/base/rsa_... >> >> >> >> https://codereview.chromium.org/16158005/ > >
I see. Then I think we are fine to use X509Certificate::CreateSelfSigned. On Tue, Jun 11, 2013 at 2:31 PM, Ryan Sleevi <rsleevi@chromium.org> wrote: > There is no security being provided to libjingle in its current > approach, because it always runs in the renderer, which we consider to > be 'attacker controlled' for security boundaries. > > On Tue, Jun 11, 2013 at 2:28 PM, Jiayang Liu <jiayl@chromium.org> wrote: > > It seems these two implementations are essentially the same: > > libjingle: NSSKeyPair::Generate > > chrome X509Certificate: CERTCertificate* CreateCertificate > > > > The only difference will be that Libjingle creates the RSA private key as > > sensitive and non-permanent, while the RSAPrivateKey class in Chrome does > > not provide such an option, but only > insensitive&non-permanent¬-portable, > > or sensitive&permanent. > > > > Justin & Eric, > > > > will it be OK to remove the sensitive attribute from the key? > > > > > > > > > > > > > > On Tue, Jun 11, 2013 at 2:07 PM, <rsleevi@chromium.org> wrote: > >> > >> On 2013/06/11 20:55:21, jiayl wrote: > >>> > >>> It seems x509Certificate::CreateSelfSigned can be used in > >>> DTLSIdentityStore to > >>> generate the cert. If that's true, this change will be unnecessary. > >>> Ryan, do you see any problem in using the existing > >>> x509Certificate::CreateSelfSigned? > >> > >> > >> Depends on what you're using it for! > >> > >> Remoting has been fine with it. WebRTC is still going to have to solve > >> these > >> problems for DTLS client auth - > >> > >> > https://code.google.com/p/chromium/codesearch#chromium/src/remoting/base/rsa_... > >> > >> > >> > >> https://codereview.chromium.org/16158005/ > > > > > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
