Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1329493005:
Provide RSA2048 as per RFC (Closed)
|
Created:
5 years, 3 months ago by torbjorng (webrtc) Modified:
5 years, 2 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
Descriptionprovide RSA2048 as per RFC
BUG=webrtc:4972
Committed: https://crrev.com/0df3eb03c9a6a8299d7e18c8c314ca58c2f0681e
Cr-Commit-Position: refs/heads/master@{#10209}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Allow full parameterization of RSA, curve id for ECDSA. #
Total comments: 43
Patch Set 3 : Address juberti's and hbos' feedback. #
Total comments: 8
Patch Set 4 : Rebase. #Patch Set 5 : Address hbos' comments. #
Total comments: 2
Patch Set 6 : Put KT_INVALID last. #
Total comments: 10
Patch Set 7 : Address hbos' concerns #
Total comments: 19
Patch Set 8 : Address feedback #Patch Set 9 : Rebase. #
Messages
Total messages: 49 (16 generated)
torbjorng@webrtc.org changed reviewers: + hbos@webrtc.org
Please take a look, Henrik. This adds RSA-2048 key generation, and tests generation and recognition of certificates based on RSA-2048. Not all unittests exercise RSA-2048 currently.
https://codereview.webrtc.org/1329493005/diff/1/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/1/webrtc/base/sslidentity.h#new... webrtc/base/sslidentity.h:117: // The WebRTC RFC draft mandates KT_ECDSA and KT_RSA2048. nit: How about having one comment per key type down in the list below instead of one comment for all key types over the enum? enum KeyType { // RSA with 1024-bit modulus (512-bit primes). KT_RSA1024, ... https://codereview.webrtc.org/1329493005/diff/1/webrtc/base/sslidentity.h#new... webrtc/base/sslidentity.h:124: KT_LAST KT_LAST should be before any alias/default key types. (Now KT_LAST will be KT_DEFAULT+1) https://codereview.webrtc.org/1329493005/diff/1/webrtc/base/sslidentity.h#new... webrtc/base/sslidentity.h:125: }; Should KT_RSA be a permanent alias for KT_RSA1024 or will this be removed? If temporary, add a // TODO(torborng).
juberti@google.com changed reviewers: + juberti@google.com
https://codereview.webrtc.org/1329493005/diff/1/webrtc/base/nssstreamadapter.cc File webrtc/base/nssstreamadapter.cc (right): https://codereview.webrtc.org/1329493005/diff/1/webrtc/base/nssstreamadapter.... webrtc/base/nssstreamadapter.cc:1102: if (key_type == KT_RSA1024 || key_type == KT_RSA2048) { An example of the problem caused by overloading KeyType. https://codereview.webrtc.org/1329493005/diff/1/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/1/webrtc/base/sslidentity.h#new... webrtc/base/sslidentity.h:119: KT_RSA1024, Squashing both key type and key length into a single enum feels wrong to me; it makes it hard to do an if test for "if key is RSA". I would prefer this to be a separate arg in SSLIdentityParams.
PTAL hbos. * A plain Generate call is now more clunky, KT_BLAH has become rtc::KeyTypeFull::BLAH(). * KT_DEFAULT is now expressed as rtc::KeyTypeFull(). Should we have rtc::KeyTypeFull.Default() instead for symmetry?
https://codereview.webrtc.org/1329493005/diff/20001/talk/app/webrtc/webrtcses... File talk/app/webrtc/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/1329493005/diff/20001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession_unittest.cc:600: rtc::SSLIdentity::Generate(identity_name, rtc::KeyTypeFull())) Here and other places: I prefer KeyTypeFull::Default() https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:46: static EVP_PKEY* MakeKey(KeyTypeFull key_type) { DCHECK that the parameters are in valid ranges etc. Or perhaps return NULL and LOG(LS_ERROR) is more appropriate given that this is what is done if the key type is not understood? See my other comment about validating parameters @ KeyTypeFull. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/ssladapter_un... File webrtc/base/ssladapter_unittest.cc (right): https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/ssladapter_un... webrtc/base/ssladapter_unittest.cc:137: ssl_identity_.reset(rtc::SSLIdentity::Generate(GetHostname(), key_type)); DCHECK(ssl_identity_.get()) after this line? https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:117: enum KeyType { KT_RSA, KT_ECDSA, KT_LAST, KT_DEFAULT = KT_RSA }; Is KT_DEFAULT still used/something we want to have given KeyTypeFull::Default()? https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:134: class KeyTypeFull { Document supported parameter ranges here or in Generate implementations. I suppose the supported ranges could be implementation-specific. Now there is only one implementation? Would be best if all implementations are required to support the same parameter sets so that you can be completely agnostic about what implementation is used when you call Generate. Assuming all implementations should support the same parameter sets consider adding a method for checking if a KeyTypeFull is a valid parameter set (boolean isValid() inside KeyTypeFull). Blink/chromium code will have to know if its generateCertificate input is valid or not, it would be nice if we didn't have validate parameters code in multiple places. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:136: explicit KeyTypeFull() { Remove explicit keyword here. I would make this constructor private in favor of using Default() publicly unless there is a reason for having a parameterless constructor. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:142: explicit KeyTypeFull(KeyType key_type) { I suggest replacing this with a factory function "static KeyTypeFull Default(KeyType key_type)", that way you don't have to have a mixture of factory functions and constructors, and the name hints what type of construction it does. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:154: int pub_exp = PUB_EXP_DEFAULT) { I'm happy with calling them RSA, ECDSA and Default. Factory functions are usually called CreateBlah but I couldn't find anything about this in the style guide. "Create" sounds a bit heavy for such a light-weight class, and its a more cumbersome name. As you and/or other reviewers please. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:174: ECCurve ec_params() const { Does it make sense to have an ECParams struct containing the curve in case we ever want to add a second parameter to the EC case? And so that KeyParams is a list of "<Algorithm>Params" members https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:189: KeyType IntKeyTypeFamilyToKeyType(int key_type_family); This file is already pretty long with multiple classes. Now that the key type stuff is more than a one line enum, consider moving all the key type-related stuff to a new file. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslstreamadap... File webrtc/base/sslstreamadapter.h (right): https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslstreamadap... webrtc/base/sslstreamadapter.h:172: KeyTypeFull key_type); Can we do default param key_type = KeyTypeFull::Default() instead of two functions?
juberti@google.com changed reviewers: + jbauch@webrtc.org
I think this might be trying to be too clever. Generally, I think we should support the following usages: Generate(KT_DEFAULT / KT_RSA / KT_ECDSA) // shorthand; prevents existing code from breaking Generate(KeyParams(KT_DEFAULT / KT_RSA / KT_ECDSA)) // get default RSA/ECDSA params Generate(KeyParams::RSA(2048, kRsaDefaultExponent)) Generate(KeyParams::RSA(2048, 65537)) Generate(KeyParams::ECDSA(EC_NIST_P256)) I don't think we need KeyParams::Default or KeyParams() https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:117: enum KeyType { KT_RSA, KT_ECDSA, KT_LAST, KT_DEFAULT = KT_RSA }; We don't usually use _LAST enums. Is there a specific need for this? https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:119: enum { RSA_MOD_SIZE_DEFAULT = 1024 }; These should be const ints, not enums. They should both have RSA_ prefix (or kRsa for const int) https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:134: class KeyTypeFull { I think KeyParams would be a better name here. You could use a nameless union instead of using union for KeyParams, and that will also make the code less verbose. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:136: explicit KeyTypeFull() { On 2015/09/29 13:53:18, hbos wrote: > Remove explicit keyword here. I would make this constructor private in favor of > using Default() publicly unless there is a reason for having a parameterless > constructor. I would just remove this constructor altogether. I don't see a need for a default ctor on this object given that we have KT_DEFAULT already. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:153: static KeyTypeFull RSA(int mod_size = RSA_MOD_SIZE_DEFAULT, Default parameters are discouraged by the style guide. I think if you use this form (as opposed to just passing KeyType), you should be expected to fill in all the parameters. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:161: static KeyTypeFull ECDSA(ECCurve curve = EC_NIST_P256) { Same here. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:167: static KeyTypeFull Default() { return RSA(); } Not sure we need this https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:212: KeyTypeFull key_type); as this is taking a structure, it should be const KeyParams&, rather than passing by value https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslstreamadap... File webrtc/base/sslstreamadapter.cc (right): https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslstreamadap... webrtc/base/sslstreamadapter.cc:83: std::string SSLStreamAdapter::GetDefaultSslCipher(SSLProtocolVersion version) { Why do we need to add this? https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslstreamadap... File webrtc/base/sslstreamadapter.h (right): https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslstreamadap... webrtc/base/sslstreamadapter.h:170: static std::string GetDefaultSslCipher(SSLProtocolVersion version); Why do we need to pass KeyTypeFull here? This function should not care about RSA key size.
Thanks for the feedback. PTAL juberti and hbos! https://codereview.webrtc.org/1329493005/diff/20001/talk/app/webrtc/webrtcses... File talk/app/webrtc/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/1329493005/diff/20001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession_unittest.cc:600: rtc::SSLIdentity::Generate(identity_name, rtc::KeyTypeFull())) On 2015/09/29 13:53:18, hbos wrote: > Here and other places: I prefer KeyTypeFull::Default() Done. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:117: enum KeyType { KT_RSA, KT_ECDSA, KT_LAST, KT_DEFAULT = KT_RSA }; On 2015/09/29 13:53:18, hbos wrote: > Is KT_DEFAULT still used/something we want to have given KeyTypeFull::Default()? It is still used in one place, associated to the identitystore. Will add a TODO for now. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:117: enum KeyType { KT_RSA, KT_ECDSA, KT_LAST, KT_DEFAULT = KT_RSA }; On 2015/09/29 16:44:05, juberti wrote: > We don't usually use _LAST enums. Is there a specific need for this? It is intended for array decls and loops, and is resilient to adding new enum tags. It is currently used for a vector declaration. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:119: enum { RSA_MOD_SIZE_DEFAULT = 1024 }; On 2015/09/29 16:44:05, juberti wrote: > These should be const ints, not enums. They should both have RSA_ prefix (or > kRsa for const int) Done. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:134: class KeyTypeFull { On 2015/09/29 13:53:18, hbos wrote: > Document supported parameter ranges here or in Generate implementations. I > suppose the supported ranges could be implementation-specific. Now there is only > one implementation? Would be best if all implementations are required to support > the same parameter sets so that you can be completely agnostic about what > implementation is used when you call Generate. > > Assuming all implementations should support the same parameter sets consider > adding a method for checking if a KeyTypeFull is a valid parameter set (boolean > isValid() inside KeyTypeFull). > Blink/chromium code will have to know if its generateCertificate input is valid > or not, it would be nice if we didn't have validate parameters code in multiple > places. Added an isValid() method. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:134: class KeyTypeFull { On 2015/09/29 16:44:05, juberti wrote: > I think KeyParams would be a better name here. You could use a nameless union > instead of using union for KeyParams, and that will also make the code less > verbose. Agreed. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:136: explicit KeyTypeFull() { On 2015/09/29 16:44:05, juberti wrote: > On 2015/09/29 13:53:18, hbos wrote: > > Remove explicit keyword here. I would make this constructor private in favor > of > > using Default() publicly unless there is a reason for having a parameterless > > constructor. > > I would just remove this constructor altogether. I don't see a need for a > default ctor on this object given that we have KT_DEFAULT already. Sure, but then we need a constructor for SSLIdentityParams (which includes a KeyParams). I'm adding that. However, a default constructor is required by gtest, so I put it back. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:153: static KeyTypeFull RSA(int mod_size = RSA_MOD_SIZE_DEFAULT, On 2015/09/29 16:44:05, juberti wrote: > Default parameters are discouraged by the style guide. I think if you use this > form (as opposed to just passing KeyType), you should be expected to fill in all > the parameters. The style guide allows it for ctors, I assumed that factory functions are like ctors in this respect. Apparently not, so I will change this. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:161: static KeyTypeFull ECDSA(ECCurve curve = EC_NIST_P256) { On 2015/09/29 16:44:05, juberti wrote: > Same here. Done. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:167: static KeyTypeFull Default() { return RSA(); } On 2015/09/29 16:44:05, juberti wrote: > Not sure we need this Removed. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:174: ECCurve ec_params() const { On 2015/09/29 13:53:18, hbos wrote: > Does it make sense to have an ECParams struct containing the curve in case we > ever want to add a second parameter to the EC case? And so that KeyParams is a > list of "<Algorithm>Params" members I think "named curve" is the only reasonable parameterization of ECDSA. Else, we'll need the polynomial coefficients and the coefficient field explicitly defined, which would be a bunch of bignum params. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:212: KeyTypeFull key_type); On 2015/09/29 16:44:05, juberti wrote: > as this is taking a structure, it should be const KeyParams&, rather than > passing by value This surely is always a good idea for large structs/classes. For a like this class which is <= 64 bits, things can live in registers... Anyway, this is not time-critical code, so I changed it. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslstreamadap... File webrtc/base/sslstreamadapter.cc (right): https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslstreamadap... webrtc/base/sslstreamadapter.cc:83: std::string SSLStreamAdapter::GetDefaultSslCipher(SSLProtocolVersion version) { On 2015/09/29 16:44:05, juberti wrote: > Why do we need to add this? Convenience (I've now instead adapted callers (in test code)). https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslstreamadap... File webrtc/base/sslstreamadapter.h (right): https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslstreamadap... webrtc/base/sslstreamadapter.h:170: static std::string GetDefaultSslCipher(SSLProtocolVersion version); On 2015/09/29 16:44:05, juberti wrote: > Why do we need to pass KeyTypeFull here? This function should not care about RSA > key size. It was convenient, but I now instead changed callers (and also cleaned up the default parameter usage from an old CL). https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslstreamadap... webrtc/base/sslstreamadapter.h:172: KeyTypeFull key_type); On 2015/09/29 13:53:18, hbos wrote: > Can we do default param key_type = KeyTypeFull::Default() instead of two > functions? Default arguments are frowned upon: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:46: static EVP_PKEY* MakeKey(KeyTypeFull key_type) { On 2015/09/29 13:53:18, hbos wrote: > DCHECK that the parameters are in valid ranges etc. > Or perhaps return NULL and LOG(LS_ERROR) is more appropriate given that this is > what is done if the key type is not understood? > > See my other comment about validating parameters @ KeyTypeFull. Did you forget to address this or are you letting OpenSSL decide the validity separately from our own IsValid()? https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:174: ECCurve ec_params() const { On 2015/10/01 11:43:19, torbjorng (webrtc) wrote: > On 2015/09/29 13:53:18, hbos wrote: > > Does it make sense to have an ECParams struct containing the curve in case we > > ever want to add a second parameter to the EC case? And so that KeyParams is a > > list of "<Algorithm>Params" members > > I think "named curve" is the only reasonable parameterization of ECDSA. > Else, we'll need the polynomial coefficients and the coefficient field > explicitly defined, which would be a bunch of bignum params. Acknowledged. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslstreamadap... File webrtc/base/sslstreamadapter.h (right): https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslstreamadap... webrtc/base/sslstreamadapter.h:172: KeyTypeFull key_type); On 2015/10/01 11:43:19, torbjorng (webrtc) wrote: > On 2015/09/29 13:53:18, hbos wrote: > > Can we do default param key_type = KeyTypeFull::Default() instead of two > > functions? > > Default arguments are frowned upon: > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html Acknowledged. https://codereview.webrtc.org/1329493005/diff/80001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1329493005/diff/80001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:409: } The implementation of our new Generate buddy just needs to create a default KeyParams and does nothing that is specific to OpenSSL (except call OpenSSLIdentity::Generate). You can achieve the same behavior with an SSLIdentity::Generate(string, KeyType) implementation calling SSLIdentity::Generate(string, const KeyParams&). Then other library implementations (if there still were any) would only need to implement the one Generate function that does the actual work. https://codereview.webrtc.org/1329493005/diff/80001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/80001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:132: KeyParams() { Hmm. If we have to have a default constructor for gtest reasons I think that's fine, but what about constructing something default and valid instead? Even though it's not a "necessary" constructor otherwise. KeyParams() : KeyParams(KT_DEFAULT) {} I really don't see the harm in having a parameterless constructor that creates something default and usable. But if you really want to discourage its usage then constructing something invalid like this is fine with me. https://codereview.webrtc.org/1329493005/diff/80001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:134: memset(¶ms_, 0xff, sizeof params_); // Bad values. Change "sizeof params_" to "sizeof(params_)" https://codereview.webrtc.org/1329493005/diff/80001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:179: // Check validity of a KeyParams object. Since the factory functions have nit: remove double space after first sentence.
Address hbos' comments. https://codereview.webrtc.org/1329493005/diff/80001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1329493005/diff/80001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:409: } On 2015/10/01 14:42:43, hbos wrote: > The implementation of our new Generate buddy just needs to create a default > KeyParams and does nothing that is specific to OpenSSL (except call > OpenSSLIdentity::Generate). > > You can achieve the same behavior with an SSLIdentity::Generate(string, KeyType) > implementation calling SSLIdentity::Generate(string, const KeyParams&). Then > other library implementations (if there still were any) would only need to > implement the one Generate function that does the actual work. Nice observation, done. https://codereview.webrtc.org/1329493005/diff/80001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/80001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:132: KeyParams() { On 2015/10/01 14:42:43, hbos wrote: > Hmm. If we have to have a default constructor for gtest reasons I think that's > fine, but what about constructing something default and valid instead? Even > though it's not a "necessary" constructor otherwise. > > KeyParams() : KeyParams(KT_DEFAULT) {} > > I really don't see the harm in having a parameterless constructor that creates > something default and usable. > But if you really want to discourage its usage then constructing something > invalid like this is fine with me. I think an API can be confusing if there are many redundant ways of doing the same thing. Also, that means one needs to maintain a more complex API. I prefer to have this make an undefined state. Let's see what the other reviewers think... https://codereview.webrtc.org/1329493005/diff/80001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:134: memset(¶ms_, 0xff, sizeof params_); // Bad values. On 2015/10/01 14:42:43, hbos wrote: > Change "sizeof params_" to "sizeof(params_)" OK.
https://codereview.webrtc.org/1329493005/diff/120001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/120001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:116: enum KeyType { KT_INVALID, KT_RSA, KT_ECDSA, KT_LAST, KT_DEFAULT = KT_RSA }; If we should have an invalid type I would place it after KT_LAST, code that iterate key types or have an array of size KT_LAST probably don't want this to include the invalid case. (Example: DtlsIdentityStoreImpl::request_info_)
PTAL juberti. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:46: static EVP_PKEY* MakeKey(KeyTypeFull key_type) { On 2015/10/01 14:42:43, hbos wrote: > On 2015/09/29 13:53:18, hbos wrote: > > DCHECK that the parameters are in valid ranges etc. > > Or perhaps return NULL and LOG(LS_ERROR) is more appropriate given that this > is > > what is done if the key type is not understood? > > > > See my other comment about validating parameters @ KeyTypeFull. > > Did you forget to address this or are you letting OpenSSL decide the validity > separately from our own IsValid()? I let boringssl decide at this abstraction level. The isValid method of KeyParams) is an indication of what our code implements at a given time. It is a good indication of what will work for Generate(), but it is possible that boringssl at some point decides that e.g., 1024-bit keys in RSA are too weak to be supported (IIRC they recently stopped allowing <= 1023 bit keys). Calling isValid should be done at a higher level, if at all. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:142: explicit KeyTypeFull(KeyType key_type) { On 2015/09/29 13:53:18, hbos wrote: > I suggest replacing this with a factory function "static KeyTypeFull > Default(KeyType key_type)", that way you don't have to have a mixture of factory > functions and constructors, and the name hints what type of construction it > does. Acknowledged. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:154: int pub_exp = PUB_EXP_DEFAULT) { On 2015/09/29 13:53:18, hbos wrote: > I'm happy with calling them RSA, ECDSA and Default. Factory functions are > usually called CreateBlah but I couldn't find anything about this in the style > guide. "Create" sounds a bit heavy for such a light-weight class, and its a more > cumbersome name. As you and/or other reviewers please. Acknowledged. https://codereview.webrtc.org/1329493005/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:189: KeyType IntKeyTypeFamilyToKeyType(int key_type_family); On 2015/09/29 13:53:18, hbos wrote: > This file is already pretty long with multiple classes. > Now that the key type stuff is more than a one line enum, consider moving all > the key type-related stuff to a new file. Let's consider that for a separate CL. (Incidentally, I'm reducing this file's size by purging SCHANNEL.) https://codereview.webrtc.org/1329493005/diff/80001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/80001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:179: // Check validity of a KeyParams object. Since the factory functions have On 2015/10/01 14:42:43, hbos wrote: > nit: remove double space after first sentence. Done. https://codereview.webrtc.org/1329493005/diff/120001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/120001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:116: enum KeyType { KT_INVALID, KT_RSA, KT_ECDSA, KT_LAST, KT_DEFAULT = KT_RSA }; On 2015/10/05 11:31:49, hbos wrote: > If we should have an invalid type I would place it after KT_LAST, code that > iterate key types or have an array of size KT_LAST probably don't want this to > include the invalid case. (Example: DtlsIdentityStoreImpl::request_info_) Good point! Done.
The CQ bit was checked by torbjorng@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1329493005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1329493005/140001
Found some more stuff (nits basically) when looking again with fresh eyes. lgtm if comments are addressed. https://codereview.webrtc.org/1329493005/diff/140001/webrtc/base/opensslident... File webrtc/base/opensslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/140001/webrtc/base/opensslident... webrtc/base/opensslidentity.h:106: KeyType key_type); You forgot to remove this one now that SSLIdentity::Generate does the polymorph thingy. https://codereview.webrtc.org/1329493005/diff/140001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/140001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:146: } I think "else" should be "else if (key_type == KT_RSA)" and another else that does what the default constructor is doing otherwise (set to KT_INVALID). Currently KeyParams(KT_LAST or KT_INVALID) would construct KeyParams(KT_RSA). Kind of a "nit" but if more key types are added and we for some reason forget to update this constructor (unlikely) we wouldn't get a usable RSA when we use our new key type and perhaps think that our new key type is being used. https://codereview.webrtc.org/1329493005/diff/140001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:193: } No need for "this_->". https://codereview.webrtc.org/1329493005/diff/140001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:203: } Here and rsa_params: Remove "//" and use RTC_DCHECK https://codereview.webrtc.org/1329493005/diff/140001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:220: // Parameters for generating a certificate. If common_name is non-empty, it nit: common_name -> |common_name|
Thanks, concerns addressed in new patch set. https://codereview.webrtc.org/1329493005/diff/140001/webrtc/base/opensslident... File webrtc/base/opensslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/140001/webrtc/base/opensslident... webrtc/base/opensslidentity.h:106: KeyType key_type); On 2015/10/05 15:17:05, hbos wrote: > You forgot to remove this one now that SSLIdentity::Generate does the polymorph > thingy. Done. https://codereview.webrtc.org/1329493005/diff/140001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/140001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:146: } On 2015/10/05 15:17:06, hbos wrote: > I think "else" should be "else if (key_type == KT_RSA)" and another else that > does what the default constructor is doing otherwise (set to KT_INVALID). > Currently KeyParams(KT_LAST or KT_INVALID) would construct KeyParams(KT_RSA). > Kind of a "nit" but if more key types are added and we for some reason forget to > update this constructor (unlikely) we wouldn't get a usable RSA when we use our > new key type and perhaps think that our new key type is being used. OK, I added checks and appropriate crashes to the code. https://codereview.webrtc.org/1329493005/diff/140001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:193: } On 2015/10/05 15:17:06, hbos wrote: > No need for "this_->". Done. https://codereview.webrtc.org/1329493005/diff/140001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:203: } On 2015/10/05 15:17:06, hbos wrote: > Here and rsa_params: Remove "//" and use RTC_DCHECK Done. https://codereview.webrtc.org/1329493005/diff/140001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:220: // Parameters for generating a certificate. If common_name is non-empty, it On 2015/10/05 15:17:05, hbos wrote: > nit: common_name -> |common_name| Done.
The CQ bit was checked by torbjorng@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1329493005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1329493005/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by torbjorng@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1329493005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1329493005/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping Justin.
Just minor stuff at this point. Overall this is looking good. https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/opensslident... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/opensslident... webrtc/base/opensslidentity.cc:66: key_params.ec_curve() == EC_NIST_P256) { I think you want to check the curve inside this if block, so you get an appropriate error (instead of "Key type not understood") https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/opensslident... File webrtc/base/opensslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/opensslident... webrtc/base/opensslidentity.h:36: static OpenSSLKeyPair* Generate(KeyType key_type); Is this old form still needed? https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:117: enum KeyType { KT_RSA, KT_ECDSA, KT_LAST, KT_INVALID, KT_DEFAULT = KT_RSA }; I didn't see any usage of KT_LAST in this CL. I could be persuaded if we actually used it, but is it serving a purpose? https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:133: KeyParams() { This seems wrong to me. Why not init with KT_DEFAULT here? Then you can avoid KT_INVALID altogether. (See comment below - you can probably just add a default argument of KT_DEFAULT to the ctor below). https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:153: static KeyParams RSA(int mod_size, int pub_exp) { I looked and you are correct; the style guide does allow for default arguments in ctors (and I presume, factory functions). I think this class will be simpler if you use default arguments, so go for it. https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:187: return (params_.rsa.mod_size >= 1024 && params_.rsa.mod_size <= 8192 && These should probably be constants. https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:224: SSLIdentityParams(const KeyParams& key_params) : key_params(key_params) {} It seems odd to have a ctor that only initializes one parameter. I would either make it initialize everything, or nothing. https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslstreamada... File webrtc/base/sslstreamadapter_unittest.cc (right): https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslstreamada... webrtc/base/sslstreamadapter_unittest.cc:227: rtc::SSLIdentityParams client_params((rtc::KeyParams(rtc::KT_DEFAULT))); Remove extra parens, here and below
https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:117: enum KeyType { KT_RSA, KT_ECDSA, KT_LAST, KT_INVALID, KT_DEFAULT = KT_RSA }; On 2015/10/07 06:35:22, juberti wrote: > I didn't see any usage of KT_LAST in this CL. I could be persuaded if we > actually used it, but is it serving a purpose? It is used today by dtlsidentitystore.h. It is probably the only usage and after the store is updated (different CL) I'm guessing it will no longer be needed. https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:133: KeyParams() { On 2015/10/07 06:35:22, juberti wrote: > This seems wrong to me. Why not init with KT_DEFAULT here? Then you can avoid > KT_INVALID altogether. (See comment below - you can probably just add a default > argument of KT_DEFAULT to the ctor below). (I would also prefer default constructor to create something default and usable and remove KT_INVALID) https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:153: static KeyParams RSA(int mod_size, int pub_exp) { On 2015/10/07 06:35:22, juberti wrote: > I looked and you are correct; the style guide does allow for default arguments > in ctors (and I presume, factory functions). I think this class will be simpler > if you use default arguments, so go for it. Yay!
Thanks for the feedback! PTAL juberti, hbos. https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/opensslident... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/opensslident... webrtc/base/opensslidentity.cc:66: key_params.ec_curve() == EC_NIST_P256) { On 2015/10/07 06:35:22, juberti wrote: > I think you want to check the curve inside this if block, so you get an > appropriate error (instead of "Key type not understood") Done. https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/opensslident... File webrtc/base/opensslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/opensslident... webrtc/base/opensslidentity.h:36: static OpenSSLKeyPair* Generate(KeyType key_type); On 2015/10/07 06:35:22, juberti wrote: > Is this old form still needed? Indeed no longer needed. https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:117: enum KeyType { KT_RSA, KT_ECDSA, KT_LAST, KT_INVALID, KT_DEFAULT = KT_RSA }; On 2015/10/07 06:35:22, juberti wrote: > I didn't see any usage of KT_LAST in this CL. I could be persuaded if we > actually used it, but is it serving a purpose? I have the habit of putting in a _LAST in order for people not to use any temporal last active enum tag as the enum's dimension. And since it is used for that purpose at the moment, I'll leave it in. https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:133: KeyParams() { On 2015/10/07 06:35:22, juberti wrote: > This seems wrong to me. Why not init with KT_DEFAULT here? Then you can avoid > KT_INVALID altogether. (See comment below - you can probably just add a default > argument of KT_DEFAULT to the ctor below). OK, I'll revert to that form. https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:153: static KeyParams RSA(int mod_size, int pub_exp) { On 2015/10/07 06:35:22, juberti wrote: > I looked and you are correct; the style guide does allow for default arguments > in ctors (and I presume, factory functions). I think this class will be simpler > if you use default arguments, so go for it. Great! The style guide is not 100% clear here. https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:187: return (params_.rsa.mod_size >= 1024 && params_.rsa.mod_size <= 8192 && On 2015/10/07 06:35:22, juberti wrote: > These should probably be constants. Done. https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:224: SSLIdentityParams(const KeyParams& key_params) : key_params(key_params) {} On 2015/10/07 06:35:22, juberti wrote: > It seems odd to have a ctor that only initializes one parameter. I would either > make it initialize everything, or nothing. Ack. It ended up like this since the KeyParams default ctor was voted against. :-) I'm removing this ugly SSLIdentityParams ctor and let its init rely on the default KeyParams ctor. https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslstreamada... File webrtc/base/sslstreamadapter_unittest.cc (right): https://codereview.webrtc.org/1329493005/diff/160001/webrtc/base/sslstreamada... webrtc/base/sslstreamadapter_unittest.cc:227: rtc::SSLIdentityParams client_params((rtc::KeyParams(rtc::KT_DEFAULT))); On 2015/10/07 06:35:23, juberti wrote: > Remove extra parens, here and below The extra parens are required; these variable declarations look too much as function declarations to the compiler.
lgtm
The CQ bit was checked by torbjorng@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1329493005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1329493005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by torbjorng@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1329493005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1329493005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1142)
henrikg@webrtc.org changed reviewers: + henrikg@webrtc.org
Rubberstamp lgtm
The CQ bit was checked by torbjorng@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1329493005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1329493005/200001
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/0df3eb03c9a6a8299d7e18c8c314ca58c2f0681e Cr-Commit-Position: refs/heads/master@{#10209}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:200001) has been created in https://codereview.webrtc.org/1397703002/ by torbjorng@webrtc.org. The reason for reverting is: Breaks chrome.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
