Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1898383003:
RTCCertificate serialization. (Closed)
DescriptionRTCCertificate serialization.
This CL adds the ability to convert RTCCertificate objects to and from
PEM string representations of it (its private key and certificate).
The RTCCertificate being a wrapper of SSLIdentity, this is where the
meat is.
Changes:
- SSLIdentity::PrivateKeyToPEMString() added. It together with the
already existing SSLCertificate::ToPEMString() yields both private
key and certificate PEM strings, both of which are required
parameters to SSLIdentity::FromPEMStrings().
- Its only implementation, OpenSSLIdentity::PrivateKeyToPemString().
- SSLIdentity::PublicKeyToPEMString() added, used by tests.
- sslidentity_unittest.cc updated:
* FromPEMStringsRSA and FromPEMStringsEC updated.
* CloneIdentityRSA and CloneIdentityECDSA added.
- RTCCertificate::To/FromPem added, using new class RTCCertificatePem.
- rtccertificate_unittest.cc: New test CloneWithPemSerialization.
- Renamed rtc_unittests.cc to rtccertificate_unittest.cc to match
convention.
BUG=webrtc:5794, chromium:581354
Committed: https://crrev.com/6b470a9413d2e65dd9078a63ed3f5f60e5ba5232
Cr-Commit-Position: refs/heads/master@{#12546}
Patch Set 1 #Patch Set 2 : Handle PEM_write_bio_PrivateKey failure #
Total comments: 2
Patch Set 3 : Verifying expiration time of clone #
Total comments: 15
Patch Set 4 : Equality tests using EVP_PKEY_cmp and X509_cmp #Patch Set 5 : Addressed nisse's comments #
Total comments: 8
Patch Set 6 : Function renames. OpenSSLKeyPair::FromPrivateKeyPEMString. Check no missing params. #Patch Set 7 : PublicKeyToPEMString #
Total comments: 16
Patch Set 8 : Addressed torbjorng's comments #
Total comments: 6
Patch Set 9 : Updated comments as per hta suggestions #Patch Set 10 : Rebase with master (std::unique_ptr) #
Messages
Total messages: 43 (15 generated)
Description was changed from ========== RTCCertificate serialization. This CL adds the ability to convert RTCCertificate objects to and from PEM string representations of it (its private key and certificate). The RTCCertificate being a wrapper of SSLIdentity, this is where the meat is. Changes: - SSLIdentity::PrivateKeyToPemString() added. It together with the already existing SSLCertificate::ToPEMString() yields both private key and certificate PEM strings, both of which are required parameters to SSLIdentity::FromPEMStrings(). - Its only implementation, OpenSSLIdentity::PrivateKeyToPemString(). - sslidentity_unittest.cc updated: * FromPEMStringsRSA and FromPEMStringsEC updated. * CloneIdentityRSA and CloneIdentityECDSA added. - RTCCertificate::To/FromPem added, using new class RTCCertificatePem. - rtccertificate_unittest.cc: New test CloneWithPemSerialization. BUG=webrtc:5794, chromium:581354 ========== to ========== RTCCertificate serialization. This CL adds the ability to convert RTCCertificate objects to and from PEM string representations of it (its private key and certificate). The RTCCertificate being a wrapper of SSLIdentity, this is where the meat is. Changes: - SSLIdentity::PrivateKeyToPemString() added. It together with the already existing SSLCertificate::ToPEMString() yields both private key and certificate PEM strings, both of which are required parameters to SSLIdentity::FromPEMStrings(). - Its only implementation, OpenSSLIdentity::PrivateKeyToPemString(). - sslidentity_unittest.cc updated: * FromPEMStringsRSA and FromPEMStringsEC updated. * CloneIdentityRSA and CloneIdentityECDSA added. - RTCCertificate::To/FromPem added, using new class RTCCertificatePem. - rtccertificate_unittest.cc: New test CloneWithPemSerialization. - Renamed rtc_unittests.cc to rtccertificate_unittest.cc to match convention. BUG=webrtc:5794, chromium:581354 ==========
Description was changed from ========== RTCCertificate serialization. This CL adds the ability to convert RTCCertificate objects to and from PEM string representations of it (its private key and certificate). The RTCCertificate being a wrapper of SSLIdentity, this is where the meat is. Changes: - SSLIdentity::PrivateKeyToPemString() added. It together with the already existing SSLCertificate::ToPEMString() yields both private key and certificate PEM strings, both of which are required parameters to SSLIdentity::FromPEMStrings(). - Its only implementation, OpenSSLIdentity::PrivateKeyToPemString(). - sslidentity_unittest.cc updated: * FromPEMStringsRSA and FromPEMStringsEC updated. * CloneIdentityRSA and CloneIdentityECDSA added. - RTCCertificate::To/FromPem added, using new class RTCCertificatePem. - rtccertificate_unittest.cc: New test CloneWithPemSerialization. - Renamed rtc_unittests.cc to rtccertificate_unittest.cc to match convention. BUG=webrtc:5794, chromium:581354 ========== to ========== RTCCertificate serialization. This CL adds the ability to convert RTCCertificate objects to and from PEM string representations of it (its private key and certificate). The RTCCertificate being a wrapper of SSLIdentity, this is where the meat is. Changes: - SSLIdentity::PrivateKeyToPemString() added. It together with the already existing SSLCertificate::ToPEMString() yields both private key and certificate PEM strings, both of which are required parameters to SSLIdentity::FromPEMStrings(). - Its only implementation, OpenSSLIdentity::PrivateKeyToPemString(). - sslidentity_unittest.cc updated: * FromPEMStringsRSA and FromPEMStringsEC updated. * CloneIdentityRSA and CloneIdentityECDSA added. - RTCCertificate::To/FromPem added, using new class RTCCertificatePem. - rtccertificate_unittest.cc: New test CloneWithPemSerialization. - Renamed rtc_unittests.cc to rtccertificate_unittest.cc to match convention. BUG=webrtc:5794, chromium:581354 ==========
hbos@webrtc.org changed reviewers: + hta@webrtc.org, torbjorng@webrtc.org
Please take a look, torbjorng and hta. Should I also add another crypto reviewer that knows openssl? https://codereview.webrtc.org/1898383003/diff/20001/webrtc/base/sslidentity_u... File webrtc/base/sslidentity_unittest.cc (right): https://codereview.webrtc.org/1898383003/diff/20001/webrtc/base/sslidentity_u... webrtc/base/sslidentity_unittest.cc:254: "-----END PRIVATE KEY-----\n"; This test failed before I updated the private key string! The previous private key would not match with the private key produced by identity->PrivateKeyToPemString(). The difference was not just "BEGIN RSA PRIVATE KEY" vs "BEGIN PRIVATE KEY", the contents was also different. To be up front about it: I don't know why there was a discrepency. I updated to a private key and certificate pair that I produced myself instead and then the test passed. I hope I'm not sweeping anything under the rug by updating these. https://codereview.webrtc.org/1898383003/diff/20001/webrtc/base/sslidentity_u... webrtc/base/sslidentity_unittest.cc:289: "vxegTWICIQCN34hvg8yXgcytAUrkW78KFcRS4RNL1TJ7NAg+JbJGnQ==\n" Same here.
torbjorng@webrtc.org changed reviewers: + nisse@webrtc.org
I think it would be good if nisse took a look at this CL too.
I did a minor update, verifying that the clone's expiration time is the same as the original. PTAL everyone (nisse, torbjorng, hta)
By using X509_cmp as suggested, you should be able to avoid changing the format of the example PEM strings to suit the your output. https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/fakesslidenti... File webrtc/base/fakesslidentity.h (right): https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/fakesslidenti... webrtc/base/fakesslidentity.h:101: virtual std::string PrivateKeyToPemString() const { Please use "override". https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/rtccertificat... File webrtc/base/rtccertificate_unittest.cc (right): https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/rtccertificat... webrtc/base/rtccertificate_unittest.cc:129: // Make sure the clone's PEM is identical to the original. I'm not sure this is a robust comparison, since presumably the underlying ASN1 data fields might appear in any order. Consider adding a comment about this potential problem to facilitate future debugging. Or better yet, use OpenSSL's X509_cmp.
I'm not very familiar with openssl. But I have a couple of comments and questions. https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:193: LOG_F(LS_ERROR) << "Failed to read private key"; s/read/write/ in error message. https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/rtccertificat... File webrtc/base/rtccertificate_unittest.cc (right): https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/rtccertificat... webrtc/base/rtccertificate_unittest.cc:39: scoped_ptr<SSLIdentity> identity( Do you really need the local variable? https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/rtccertificat... webrtc/base/rtccertificate_unittest.cc:129: // Make sure the clone's PEM is identical to the original. On 2016/04/21 15:16:00, torbjorng (webrtc) wrote: > I'm not sure this is a robust comparison, since presumably the underlying ASN1 > data fields might appear in any order. Consider adding a comment about this > potential problem to facilitate future debugging. > > Or better yet, use OpenSSL's X509_cmp. I have been trying to forget asn.1... So I take it PEM encoding uses BER rather than DER? Is that true of openssl in general? What fields can be permuted (being asn.1 sets rather than sequences)? https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/sslidentity_u... File webrtc/base/sslidentity_unittest.cc (left): https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/sslidentity_u... webrtc/base/sslidentity_unittest.cc:224: "-----BEGIN RSA PRIVATE KEY-----\n" It might be appropriate with a comment saying how this testdata was created. And it's not obvious to me why it must be updated for this cl?
PTAL torbjorng, nisse. Two more patch sets! https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/fakesslidenti... File webrtc/base/fakesslidentity.h (right): https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/fakesslidenti... webrtc/base/fakesslidentity.h:101: virtual std::string PrivateKeyToPemString() const { On 2016/04/21 15:16:00, torbjorng (webrtc) wrote: > Please use "override". This is a file where virtual is used everywhere. It makes sense to use override for new functions, but I think that will break chromium fyi bots where some compilation option says not to mix virtual and override in the same .cc, something the webrtc bots don't check for. I've had to revert or do follow up CLs before because of this. I think virtual->override replacements should be done in a separate CL. I could do that if you want. https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:193: LOG_F(LS_ERROR) << "Failed to read private key"; On 2016/04/22 11:38:25, nisse-webrtc wrote: > s/read/write/ in error message. Done. https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/rtccertificat... File webrtc/base/rtccertificate_unittest.cc (right): https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/rtccertificat... webrtc/base/rtccertificate_unittest.cc:39: scoped_ptr<SSLIdentity> identity( On 2016/04/22 11:38:25, nisse-webrtc wrote: > Do you really need the local variable? I do now that I RTC_CHECK. (I think I did before or it would complain about std::move but not sure.) https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/rtccertificat... webrtc/base/rtccertificate_unittest.cc:129: // Make sure the clone's PEM is identical to the original. On 2016/04/21 15:16:00, torbjorng (webrtc) wrote: > I'm not sure this is a robust comparison, since presumably the underlying ASN1 > data fields might appear in any order. Consider adding a comment about this > potential problem to facilitate future debugging. > > Or better yet, use OpenSSL's X509_cmp. X509_cmp only compares the certificate? Ah, there's an EVP_PKEY_cmp too. Added comparison functions in SSLIdentity and RTCCertificate. Only using this type of comparison in rtccertificate_unittest.cc. As for the comparison of PEM strings in sslidentity_unittest.cc, considering the same library call is used to produce both the original and clone's PEM I think it's a pretty good bet that the order will be the same, the string identical, and that the comparison will work and continue to work. But you're right, it is an assumption with no gurantees. I'm considering keeping the comparison in sslidentity_unittest.cc but have a comment explaining this in case the assumption stops holding. What do you think? (Of course I could remove the check now that I have a new comparison function, but keeping it still boosts my confidence that everything is working as intended.) https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/sslidentity_u... File webrtc/base/sslidentity_unittest.cc (left): https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/sslidentity_u... webrtc/base/sslidentity_unittest.cc:224: "-----BEGIN RSA PRIVATE KEY-----\n" On 2016/04/22 11:38:25, nisse-webrtc wrote: > It might be appropriate with a comment saying how this testdata was created. And > it's not obvious to me why it must be updated for this cl? It's not obvious to me either, but torbjorng explained how the data can be ordered differently, so with that in mind I think it makes sense that it wasn't identical if different "ToPem" functions were used. Added a comment about how they were produced.
https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/fakesslidenti... File webrtc/base/fakesslidentity.h (right): https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/fakesslidenti... webrtc/base/fakesslidentity.h:101: virtual std::string PrivateKeyToPemString() const { On 2016/04/22 13:19:29, hbos wrote: > On 2016/04/21 15:16:00, torbjorng (webrtc) wrote: > > Please use "override". > > This is a file where virtual is used everywhere. It makes sense to use override > for new functions, but I think that will break chromium fyi bots where some > compilation option says not to mix virtual and override in the same .cc, > something the webrtc bots don't check for. I've had to revert or do follow up > CLs before because of this. > > I think virtual->override replacements should be done in a separate CL. I could > do that if you want. I think the inconsistent-override warning applies per class, not per file (but I'm not 100% sure). I agree updating old code can be done in a separate cl. https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/rtccertificat... File webrtc/base/rtccertificate_unittest.cc (right): https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/rtccertificat... webrtc/base/rtccertificate_unittest.cc:39: scoped_ptr<SSLIdentity> identity( On 2016/04/22 13:19:29, hbos wrote: > On 2016/04/22 11:38:25, nisse-webrtc wrote: > > Do you really need the local variable? > > I do now that I RTC_CHECK. (I think I did before or it would complain about > std::move but not sure.) Ok. Can Generate ever fail? https://codereview.webrtc.org/1898383003/diff/80001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1898383003/diff/80001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:185: BIO* temp_memory_bio = BIO_new(BIO_s_mem()); I was a bit confused by the name if this class; I expected some explicit representation of both public and private key. If it's really representing a private key (as the new comparison operators indicate), name this just ToPEMString, similarly to the method on OpenSSLCertificate. https://codereview.webrtc.org/1898383003/diff/80001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1898383003/diff/80001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:235: static std::string DerToPem(const std::string& pem_type, This seems to indicate that DER encoding rules are used? If so, the "same" asn.1 object should always be serialized to a byte string in the same way, and the only possible differences in the PEM string is whitespace. So if we can get away with that, I think there's some benefit to doing a plain string comparison in the testcases, possibly after first removing all white space characters.
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:120001) has been deleted
PTAL nisse, torbjorng. Some confusion around key pair and only using private key. We're working with private key PEMs, but public key information can be part of the key pair structure. I think To/FromPEM with only private key is enough though. I also moved OpenSSLKeyPair fromPEM code from OpenSSLIdentity::FromPEMStrings into separate function OpenSSLKeyPair::FromPrivateKeyPEMString, and added a check in it that verifies that public key parameters are not missing from the resulting key pair. https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/fakesslidenti... File webrtc/base/fakesslidentity.h (right): https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/fakesslidenti... webrtc/base/fakesslidentity.h:101: virtual std::string PrivateKeyToPemString() const { On 2016/04/22 13:44:06, nisse-webrtc wrote: > On 2016/04/22 13:19:29, hbos wrote: > > On 2016/04/21 15:16:00, torbjorng (webrtc) wrote: > > > Please use "override". > > > > This is a file where virtual is used everywhere. It makes sense to use > override > > for new functions, but I think that will break chromium fyi bots where some > > compilation option says not to mix virtual and override in the same .cc, > > something the webrtc bots don't check for. I've had to revert or do follow up > > CLs before because of this. > > > > I think virtual->override replacements should be done in a separate CL. I > could > > do that if you want. > > I think the inconsistent-override warning applies per class, not per file (but > I'm not 100% sure). I agree updating old code can be done in a separate cl. Acknowledged. https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/rtccertificat... File webrtc/base/rtccertificate_unittest.cc (right): https://codereview.webrtc.org/1898383003/diff/40001/webrtc/base/rtccertificat... webrtc/base/rtccertificate_unittest.cc:39: scoped_ptr<SSLIdentity> identity( On 2016/04/22 13:44:06, nisse-webrtc wrote: > On 2016/04/22 13:19:29, hbos wrote: > > On 2016/04/22 11:38:25, nisse-webrtc wrote: > > > Do you really need the local variable? > > > > I do now that I RTC_CHECK. (I think I did before or it would complain about > > std::move but not sure.) > > Ok. Can Generate ever fail? While it shouldn't happen under normal circumstances, hence the RTC_CHECK, it *could* happen and the API says it returns null on failure. torbjorng might have a better answer as to when it might fail. https://codereview.webrtc.org/1898383003/diff/80001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1898383003/diff/80001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:185: BIO* temp_memory_bio = BIO_new(BIO_s_mem()); On 2016/04/22 13:44:07, nisse-webrtc wrote: > I was a bit confused by the name if this class; I expected some explicit > representation of both public and private key. > > If it's really representing a private key (as the new comparison operators > indicate), name this just ToPEMString, similarly to the method on > OpenSSLCertificate. Hmm, confusing indeed. The previously existing SSLIdentity/OpenSSLIdentity::FromPEMStrings constructs OpenSSLKeyPair just from private key using PEM_read_bio_PrivateKey. This new toPEM uses PEM_write_bio_PrivateKey. So at first I thought EVP_PKEY was just private key information and that OpenSSLKeyPair was a misleading name. But digging deeper, documentation confirms EVP_PKEY can contain both public and private key information. Even though we construct new EVP_PKEYs using "PrivateKey" PEMs, I just checked with EVP_PKEY_missing_parameters and it returns 0 indicating that the EVP_PKEY does have public key parameters present (or that the algorithm does not use parameters). There exists PEM_read/write_bio_PUBKEY functions too (taking EVP_PKEY), but they are not used by us. Question: It turns out EVP_PKEY_cmp is a *public* key comparison function - would this make the comparison less robust? Since OpenSSLIdentity can be cloned using private key and certificate PEMs this is all that is needed, but because of the key pair / private key confusion I will be clear in the function name that this toPEM is for private key. I can verify that EVP_PKEY_missing_parameters returns 0 though so that we don't end up with a broken EVP_PKEY (although I don't know if PUBKEY is used given that the OpenSSLCertificate must contain stuff like that too?). https://codereview.webrtc.org/1898383003/diff/80001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1898383003/diff/80001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:235: static std::string DerToPem(const std::string& pem_type, On 2016/04/22 13:44:07, nisse-webrtc wrote: > This seems to indicate that DER encoding rules are used? If so, the "same" asn.1 > object should always be serialized to a byte string in the same way, and the > only possible differences in the PEM string is whitespace. > > So if we can get away with that, I think there's some benefit to doing a plain > string comparison in the testcases, possibly after first removing all white > space characters. Acknowledged. Leaving comparisons as-is for now, not removing whitespace characters since it seems to be working.
I also verified that the public key exists by printing it. I'll add a getter and make it part of the unittest, then we know for sure both components of the key pair are present. Then we know for sure that PrivateKeyToPEMString() can act as a key pair ToPEMString(). Will update CL.
PTAL nisse, torbjorng. Now we still read and write the private key but we make sure the key pair is complete by inspecting public key as well in the unittests. I guess the public key comes for free.
Description was changed from ========== RTCCertificate serialization. This CL adds the ability to convert RTCCertificate objects to and from PEM string representations of it (its private key and certificate). The RTCCertificate being a wrapper of SSLIdentity, this is where the meat is. Changes: - SSLIdentity::PrivateKeyToPemString() added. It together with the already existing SSLCertificate::ToPEMString() yields both private key and certificate PEM strings, both of which are required parameters to SSLIdentity::FromPEMStrings(). - Its only implementation, OpenSSLIdentity::PrivateKeyToPemString(). - sslidentity_unittest.cc updated: * FromPEMStringsRSA and FromPEMStringsEC updated. * CloneIdentityRSA and CloneIdentityECDSA added. - RTCCertificate::To/FromPem added, using new class RTCCertificatePem. - rtccertificate_unittest.cc: New test CloneWithPemSerialization. - Renamed rtc_unittests.cc to rtccertificate_unittest.cc to match convention. BUG=webrtc:5794, chromium:581354 ========== to ========== RTCCertificate serialization. This CL adds the ability to convert RTCCertificate objects to and from PEM string representations of it (its private key and certificate). The RTCCertificate being a wrapper of SSLIdentity, this is where the meat is. Changes: - SSLIdentity::PrivateKeyToPEMString() added. It together with the already existing SSLCertificate::ToPEMString() yields both private key and certificate PEM strings, both of which are required parameters to SSLIdentity::FromPEMStrings(). - Its only implementation, OpenSSLIdentity::PrivateKeyToPemString(). - SSLIdentity::PublicKeyToPEMString() added, used by tests. - sslidentity_unittest.cc updated: * FromPEMStringsRSA and FromPEMStringsEC updated. * CloneIdentityRSA and CloneIdentityECDSA added. - RTCCertificate::To/FromPem added, using new class RTCCertificatePem. - rtccertificate_unittest.cc: New test CloneWithPemSerialization. - Renamed rtc_unittests.cc to rtccertificate_unittest.cc to match convention. BUG=webrtc:5794, chromium:581354 ==========
You might want to move to unique_ptr as per kwiberg's comments to the webrtc list. https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/opensslident... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/opensslident... webrtc/base/opensslidentity.cc:522: scoped_ptr<OpenSSLKeyPair> key_pair( Isn't scoped_ptr overkill here? https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/opensslident... File webrtc/base/opensslidentity.h (right): https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/opensslident... webrtc/base/opensslidentity.h:49: bool operator!=(const OpenSSLKeyPair& other) const; It is not clear that we should have a != operator. It will return true for invalid compares (which might make sense, or not). (Here and in for at least two more types.) https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/rtccertifica... File webrtc/base/rtccertificate.cc (right): https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/rtccertifica... webrtc/base/rtccertificate.cc:51: scoped_refptr<RTCCertificate> RTCCertificate::FromPem( Please use PEM xor Pem as part of method names. https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/sslidentity_... File webrtc/base/sslidentity_unittest.cc (right): https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:254: EXPECT_FALSE(*identity_rsa1_ == *identity_rsa2_); What's the probability for equality here? :-) https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:263: EXPECT_FALSE(*identity_ecdsa1_ == *identity_ecdsa2_); And here? :-) https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:272: "-----BEGIN PRIVATE KEY-----\n" Is the change from "BEGIN RSA PRIVATE" ... to "BEGIN PRIVATE" ... still needed? It's a bit worrying if only ASN1 derived keytype is allowed, and that "BEGIN BLAH" typing fails. (Ideally both should be allowed and tested for full compatibility.) https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:347: EXPECT_EQ(kCERT_PEM, identity->certificate().ToPEMString()); I find it doubtful if we should have any PEM string based EXPECTs. Please consider using just binary compare.
Drive-by metacomment. https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/sslidentity_... File webrtc/base/sslidentity_unittest.cc (right): https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:272: "-----BEGIN PRIVATE KEY-----\n" On 2016/04/26 13:35:51, torbjorng (webrtc) wrote: > Is the change from "BEGIN RSA PRIVATE" ... to "BEGIN PRIVATE" ... still needed? > It's a bit worrying if only ASN1 derived keytype is allowed, and that "BEGIN > BLAH" typing fails. (Ideally both should be allowed and tested for full > compatibility.) If I remember rightly, the --- BEGIN PRIVATE KEY --- is actually specified as part of the PKCS#11 specs. Indeed, it is not quite standard yet: https://tools.ietf.org/html/rfc5915 says --- BEGIN EC PRIVATE KEY --- - but we should go with what the libraries produce - if we have the need for reproducible binary data, Base 64 is the way to go. As far as I know, all formats in common use for carrying keys are DER encoded ASN.1 - not the format I'd have chosen looking back, but we are where we are.
What I wanted to say is that we should accept both format variants, unless that turns out to be too much work. And I also wanted to say that we should not change the test strings unless we have too change them; insisting on exact PEM string match is not ideal as two non-equal PEM strings which represent the same key are possible. We should either write or find some PEM/DER string comparer, or rely on the low-level _cmp functions.
PTAL torbjorng, nisse. The comparison operators use OpenSSL library calls, i.e. do binary compares, only unittests do PEM comparisons. The unittests' use of PEM comparisons might not be entirely robust, but I would argue are good enough for unittesting. We have two cases: 1) We create a SSLIdentity from a constant PEM string. There is no other SSLIdentity to do binary compare against, the only way we can verify that it is correct is to compare it to the PEM constant, so we do that with a string == and ToPEMString(). There could be variations in the header but there isn't since the constant was originally produced by our functions. If a patch changes the header it would be easy to fix the unittest from failing. I think a more complicated PEM comparator is overkill and would not protect against different ordering anyway. 2) We clone an SSLIdentity. In this case the == operators should be enough. To increase confidence I also compare their PEM strings. This holds, and I think it will continue to hold indefinitely since the same ToPEMString() function is used on both identities. But just in case it doesn't there is a comment explaining the problem with PEM comparison; we could remove it and only rely on binary == but I prefer it this way unless we discover a problem. Is this OK or do you want me to update the unittests? https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/opensslident... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/opensslident... webrtc/base/opensslidentity.cc:522: scoped_ptr<OpenSSLKeyPair> key_pair( On 2016/04/26 13:35:51, torbjorng (webrtc) wrote: > Isn't scoped_ptr overkill here? Done. https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/opensslident... File webrtc/base/opensslidentity.h (right): https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/opensslident... webrtc/base/opensslidentity.h:49: bool operator!=(const OpenSSLKeyPair& other) const; On 2016/04/26 13:35:51, torbjorng (webrtc) wrote: > It is not clear that we should have a != operator. It will return true for > invalid compares (which might make sense, or not). > > (Here and in for at least two more types.) I'm thinking if we have an "equals" then "not equals" should be well defined, that they go hand in hand if operators are used as expected. Do you want me to remove it? https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/rtccertifica... File webrtc/base/rtccertificate.cc (right): https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/rtccertifica... webrtc/base/rtccertificate.cc:51: scoped_refptr<RTCCertificate> RTCCertificate::FromPem( On 2016/04/26 13:35:51, torbjorng (webrtc) wrote: > Please use PEM xor Pem as part of method names. Done. https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/sslidentity_... File webrtc/base/sslidentity_unittest.cc (right): https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:254: EXPECT_FALSE(*identity_rsa1_ == *identity_rsa2_); On 2016/04/26 13:35:51, torbjorng (webrtc) wrote: > What's the probability for equality here? :-) I leave that as an exercise for the reader. :-) https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:272: "-----BEGIN PRIVATE KEY-----\n" On 2016/04/26 14:10:15, hta-webrtc wrote: > On 2016/04/26 13:35:51, torbjorng (webrtc) wrote: > > Is the change from "BEGIN RSA PRIVATE" ... to "BEGIN PRIVATE" ... still > needed? > > It's a bit worrying if only ASN1 derived keytype is allowed, and that "BEGIN > > BLAH" typing fails. (Ideally both should be allowed and tested for full > > compatibility.) > > If I remember rightly, the --- BEGIN PRIVATE KEY --- is actually specified as > part of the PKCS#11 specs. Indeed, it is not quite standard yet: > https://tools.ietf.org/html/rfc5915 says --- BEGIN EC PRIVATE KEY --- - but we > should go with what the libraries produce - if we have the need for reproducible > binary data, Base 64 is the way to go. > > As far as I know, all formats in common use for carrying keys are DER encoded > ASN.1 - not the format I'd have chosen looking back, but we are where we are. > The "BEGIN RSA" -> "BEGIN" change is only necessary because we do a string == compare as part of the unittest. A FromPEMStrings() should produce an SSLIdentity regardless of variation as long as it is valid according to OpenSSL. Because a PEM string comparison between a constant PEM string and a ToPEMString() call is only done in these two unittests, I think it is overkill to come up with a comparator that parses out the "---" bits, concatenates lines, and accepts both answers and variations. This will detect if it breaks, or if it breaks because the header changes after some change it would be easy to update the unittest. https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:347: EXPECT_EQ(kCERT_PEM, identity->certificate().ToPEMString()); On 2016/04/26 13:35:51, torbjorng (webrtc) wrote: > I find it doubtful if we should have any PEM string based EXPECTs. Please > consider using just binary compare. I can only do binary compares between two SSLIdentities. This tests PEM -> SSLIdentity (and is a previously existing test btw), I have no other binary thing to compare against. If the alternative is to remove the unittest, I think keeping it as-is is preferred?
https://codereview.webrtc.org/1898383003/diff/80001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1898383003/diff/80001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:185: BIO* temp_memory_bio = BIO_new(BIO_s_mem()); On 2016/04/25 14:23:23, hbos wrote: > On 2016/04/22 13:44:07, nisse-webrtc wrote: > > I was a bit confused by the name if this class; I expected some explicit > > representation of both public and private key. > > > > If it's really representing a private key (as the new comparison operators > > indicate), name this just ToPEMString, similarly to the method on > > OpenSSLCertificate. > > Hmm, confusing indeed. > > The previously existing SSLIdentity/OpenSSLIdentity::FromPEMStrings constructs > OpenSSLKeyPair just from private key using PEM_read_bio_PrivateKey. This new > toPEM uses PEM_write_bio_PrivateKey. So at first I thought EVP_PKEY was just > private key information and that OpenSSLKeyPair was a misleading name. But > digging deeper, documentation confirms EVP_PKEY can contain both public and > private key information. > > Even though we construct new EVP_PKEYs using "PrivateKey" PEMs, I just checked > with EVP_PKEY_missing_parameters and it returns 0 indicating that the EVP_PKEY > does have public key parameters present (or that the algorithm does not use > parameters). There exists PEM_read/write_bio_PUBKEY functions too (taking > EVP_PKEY), but they are not used by us. > > Question: It turns out EVP_PKEY_cmp is a *public* key comparison function - > would this make the comparison less robust? > > Since OpenSSLIdentity can be cloned using private key and certificate PEMs this > is all that is needed, but because of the key pair / private key confusion I > will be clear in the function name that this toPEM is for private key. I can > verify that EVP_PKEY_missing_parameters returns 0 though so that we don't end up > with a broken EVP_PKEY (although I don't know if PUBKEY is used given that the > OpenSSLCertificate must contain stuff like that too?). The private key always (are there any exceptions?) contains enough information to make it trivial to derive the corresponding public key. So to me, the difference between a "private key" and a "keypair" is more a question of use. The private key is used for creating signatures (or decrypting messages, but I guess we're only talking about signatures here?). While a keypair, in addition, is intended to provide the corresponding public key to others, possibly including additional meta data, certificates, etc. https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/opensslident... File webrtc/base/opensslidentity.h (right): https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/opensslident... webrtc/base/opensslidentity.h:49: bool operator!=(const OpenSSLKeyPair& other) const; On 2016/04/27 07:52:09, hbos wrote: > On 2016/04/26 13:35:51, torbjorng (webrtc) wrote: > > It is not clear that we should have a != operator. It will return true for > > invalid compares (which might make sense, or not). > > > > (Here and in for at least two more types.) > > I'm thinking if we have an "equals" then "not equals" should be well defined, > that they go hand in hand if operators are used as expected. Do you want me to > remove it? I think I'd prefer to drop the operator overloading. These comparison functions are obscure, and I don't think they should ever be used except by the tests. And then they deserve longer names making their use more explicit.
On 2016/04/27 07:52:09, hbos wrote: > PTAL torbjorng, nisse. > > The comparison operators use OpenSSL library calls, i.e. do binary compares, > only unittests do PEM comparisons. > > The unittests' use of PEM comparisons might not be entirely robust, but I would > argue are good enough for unittesting. We have two cases: > 1) We create a SSLIdentity from a constant PEM string. There is no other > SSLIdentity to do binary compare against, the only way we can verify that it is > correct is to compare it to the PEM constant, so we do that with a string == and > ToPEMString(). There could be variations in the header but there isn't since the > constant was originally produced by our functions. If a patch changes the header > it would be easy to fix the unittest from failing. I think a more complicated > PEM comparator is overkill and would not protect against different ordering > anyway. > 2) We clone an SSLIdentity. In this case the == operators should be enough. To > increase confidence I also compare their PEM strings. This holds, and I think it > will continue to hold indefinitely since the same ToPEMString() function is used > on both identities. But just in case it doesn't there is a comment explaining > the problem with PEM comparison; we could remove it and only rely on binary == > but I prefer it this way unless we discover a problem. > > Is this OK or do you want me to update the unittests? I think string compare is OK in the tests, since we're testing PEM conversion, it's right to examine the PEM data. If it turns out later to be too brittle, then I think the right way is to ignore the specific differences we don't care about like white space and optional key type in the PEM header.
On 2016/04/27 08:16:25, nisse-webrtc wrote: > On 2016/04/27 07:52:09, hbos wrote: > > PTAL torbjorng, nisse. > > > > The comparison operators use OpenSSL library calls, i.e. do binary compares, > > only unittests do PEM comparisons. > > > > The unittests' use of PEM comparisons might not be entirely robust, but I > would > > argue are good enough for unittesting. We have two cases: > > 1) We create a SSLIdentity from a constant PEM string. There is no other > > SSLIdentity to do binary compare against, the only way we can verify that it > is > > correct is to compare it to the PEM constant, so we do that with a string == > and > > ToPEMString(). There could be variations in the header but there isn't since > the > > constant was originally produced by our functions. If a patch changes the > header > > it would be easy to fix the unittest from failing. I think a more complicated > > PEM comparator is overkill and would not protect against different ordering > > anyway. > > 2) We clone an SSLIdentity. In this case the == operators should be enough. To > > increase confidence I also compare their PEM strings. This holds, and I think > it > > will continue to hold indefinitely since the same ToPEMString() function is > used > > on both identities. But just in case it doesn't there is a comment explaining > > the problem with PEM comparison; we could remove it and only rely on binary == > > but I prefer it this way unless we discover a problem. > > > > Is this OK or do you want me to update the unittests? > > I think string compare is OK in the tests, since we're testing PEM conversion, > it's right to examine the PEM data. If it turns out later to be too brittle, > then I think the right way is to ignore the specific differences we don't care > about like white space and optional key type in the PEM header. Agreed!
https://codereview.webrtc.org/1898383003/diff/80001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1898383003/diff/80001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:185: BIO* temp_memory_bio = BIO_new(BIO_s_mem()); On 2016/04/27 08:05:46, nisse-webrtc wrote: > On 2016/04/25 14:23:23, hbos wrote: > > On 2016/04/22 13:44:07, nisse-webrtc wrote: > > > I was a bit confused by the name if this class; I expected some explicit > > > representation of both public and private key. > > > > > > If it's really representing a private key (as the new comparison operators > > > indicate), name this just ToPEMString, similarly to the method on > > > OpenSSLCertificate. > > > > Hmm, confusing indeed. > > > > The previously existing SSLIdentity/OpenSSLIdentity::FromPEMStrings constructs > > OpenSSLKeyPair just from private key using PEM_read_bio_PrivateKey. This new > > toPEM uses PEM_write_bio_PrivateKey. So at first I thought EVP_PKEY was just > > private key information and that OpenSSLKeyPair was a misleading name. But > > digging deeper, documentation confirms EVP_PKEY can contain both public and > > private key information. > > > > Even though we construct new EVP_PKEYs using "PrivateKey" PEMs, I just checked > > with EVP_PKEY_missing_parameters and it returns 0 indicating that the EVP_PKEY > > does have public key parameters present (or that the algorithm does not use > > parameters). There exists PEM_read/write_bio_PUBKEY functions too (taking > > EVP_PKEY), but they are not used by us. > > > > Question: It turns out EVP_PKEY_cmp is a *public* key comparison function - > > would this make the comparison less robust? > > > > Since OpenSSLIdentity can be cloned using private key and certificate PEMs > this > > is all that is needed, but because of the key pair / private key confusion I > > will be clear in the function name that this toPEM is for private key. I can > > verify that EVP_PKEY_missing_parameters returns 0 though so that we don't end > up > > with a broken EVP_PKEY (although I don't know if PUBKEY is used given that the > > OpenSSLCertificate must contain stuff like that too?). > > The private key always (are there any exceptions?) contains enough information > to make it trivial to derive the corresponding public key. > > So to me, the difference between a "private key" and a "keypair" is more a > question of use. The private key is used for creating signatures (or decrypting > messages, but I guess we're only talking about signatures here?). While a > keypair, in addition, is intended to provide the corresponding public key to > others, possibly including additional meta data, certificates, etc. Oh good, then the private key -> key pair makes sense. I'm *guessing* that this is an OpenSSLKeyPair as opposed to an OpenSSLPrivateKey simply because the private key is stored in a "key pair" in OpenSSL. The public interfaces being implemented, SSLIdentity/SSLCertificate would only expose the OpenSSLCertificate and its PEM (consisting of X509*). So the private key is only useful for persistence/cloning, and is otherwise not public. And the public key must be part of the certificate? So there would be some duplicate information. https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/opensslident... File webrtc/base/opensslidentity.h (right): https://codereview.webrtc.org/1898383003/diff/160001/webrtc/base/opensslident... webrtc/base/opensslidentity.h:49: bool operator!=(const OpenSSLKeyPair& other) const; On 2016/04/27 08:05:46, nisse-webrtc wrote: > On 2016/04/27 07:52:09, hbos wrote: > > On 2016/04/26 13:35:51, torbjorng (webrtc) wrote: > > > It is not clear that we should have a != operator. It will return true for > > > invalid compares (which might make sense, or not). > > > > > > (Here and in for at least two more types.) > > > > I'm thinking if we have an "equals" then "not equals" should be well defined, > > that they go hand in hand if operators are used as expected. Do you want me to > > remove it? > > I think I'd prefer to drop the operator overloading. These comparison functions > are obscure, and I don't think they should ever be used except by the tests. And > then they deserve longer names making their use more explicit. From offline discussion: style guide prefers == over Equals function, keeping as-is.
lgtm https://codereview.webrtc.org/1898383003/diff/80001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1898383003/diff/80001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:185: BIO* temp_memory_bio = BIO_new(BIO_s_mem()); On 2016/04/27 09:00:44, hbos wrote: > And the public key > must be part of the certificate? Yes. More or less by definition, a certificate consist of a public key, some meta data, and a digital signature on it all. (In theory, the certificate could omit the public key and only include a hash of it. But that variation is not possible in x.509, as far as I'm aware).
lgtm
Awesome. PTAL hta! https://codereview.webrtc.org/1898383003/diff/80001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1898383003/diff/80001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:185: BIO* temp_memory_bio = BIO_new(BIO_s_mem()); On 2016/04/27 09:08:57, nisse-webrtc wrote: > On 2016/04/27 09:00:44, hbos wrote: > > And the public key > > must be part of the certificate? > > Yes. More or less by definition, a certificate consist of a public key, some > meta data, and a digital signature on it all. > > (In theory, the certificate could omit the public key and only include a hash of > it. But that variation is not possible in x.509, as far as I'm aware). Acknowledged.
lgtm my only comments are on comments. https://codereview.webrtc.org/1898383003/diff/180001/webrtc/base/rtccertifica... File webrtc/base/rtccertificate.h (right): https://codereview.webrtc.org/1898383003/diff/180001/webrtc/base/rtccertifica... webrtc/base/rtccertificate.h:25: // cloning and storing of certificates to disk. Do you want to say where the string format is defined (either by RFC number or "The string representations are those used by OpenSSL")? https://codereview.webrtc.org/1898383003/diff/180001/webrtc/base/sslidentity_... File webrtc/base/sslidentity_unittest.cc (right): https://codereview.webrtc.org/1898383003/diff/180001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:177: // To PEM strings. Nit: This comment is meaningless in itself. Consider: // This test works by creating PEM strings from the identity // and then converting the PEM strings back into an identity. https://codereview.webrtc.org/1898383003/diff/180001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:270: // |identity->certificate().ToPEMString()|. You might want to add: // If the crypto library is updated, and the update changes the // string form of the keys, these will have to be updated too.
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from torbjorng@webrtc.org, nisse@webrtc.org, hta@webrtc.org Link to the patchset: https://codereview.webrtc.org/1898383003/#ps200001 (title: "Updated comments as per hta suggestions")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1898383003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1898383003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios32_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/7059) ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/7043) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/9377) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/9301) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/14585) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/13234) linux_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...) linux_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/14650) mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/14210) mac_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/10933) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/2728) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/8950) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/14663)
https://codereview.webrtc.org/1898383003/diff/180001/webrtc/base/rtccertifica... File webrtc/base/rtccertificate.h (right): https://codereview.webrtc.org/1898383003/diff/180001/webrtc/base/rtccertifica... webrtc/base/rtccertificate.h:25: // cloning and storing of certificates to disk. On 2016/04/28 10:18:12, hta-webrtc wrote: > Do you want to say where the string format is defined (either by RFC number or > "The string representations are those used by OpenSSL")? Done. https://codereview.webrtc.org/1898383003/diff/180001/webrtc/base/sslidentity_... File webrtc/base/sslidentity_unittest.cc (right): https://codereview.webrtc.org/1898383003/diff/180001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:177: // To PEM strings. On 2016/04/28 10:18:12, hta-webrtc wrote: > Nit: This comment is meaningless in itself. > Consider: > > // This test works by creating PEM strings from the identity > // and then converting the PEM strings back into an identity. Done. https://codereview.webrtc.org/1898383003/diff/180001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:270: // |identity->certificate().ToPEMString()|. On 2016/04/28 10:18:12, hta-webrtc wrote: > You might want to add: > > // If the crypto library is updated, and the update changes the > // string form of the keys, these will have to be updated too. Done.
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from torbjorng@webrtc.org, nisse@webrtc.org, hta@webrtc.org Link to the patchset: https://codereview.webrtc.org/1898383003/#ps220001 (title: "Rebase with master (std::unique_ptr)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1898383003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1898383003/220001
Message was sent while issue was closed.
Description was changed from ========== RTCCertificate serialization. This CL adds the ability to convert RTCCertificate objects to and from PEM string representations of it (its private key and certificate). The RTCCertificate being a wrapper of SSLIdentity, this is where the meat is. Changes: - SSLIdentity::PrivateKeyToPEMString() added. It together with the already existing SSLCertificate::ToPEMString() yields both private key and certificate PEM strings, both of which are required parameters to SSLIdentity::FromPEMStrings(). - Its only implementation, OpenSSLIdentity::PrivateKeyToPemString(). - SSLIdentity::PublicKeyToPEMString() added, used by tests. - sslidentity_unittest.cc updated: * FromPEMStringsRSA and FromPEMStringsEC updated. * CloneIdentityRSA and CloneIdentityECDSA added. - RTCCertificate::To/FromPem added, using new class RTCCertificatePem. - rtccertificate_unittest.cc: New test CloneWithPemSerialization. - Renamed rtc_unittests.cc to rtccertificate_unittest.cc to match convention. BUG=webrtc:5794, chromium:581354 ========== to ========== RTCCertificate serialization. This CL adds the ability to convert RTCCertificate objects to and from PEM string representations of it (its private key and certificate). The RTCCertificate being a wrapper of SSLIdentity, this is where the meat is. Changes: - SSLIdentity::PrivateKeyToPEMString() added. It together with the already existing SSLCertificate::ToPEMString() yields both private key and certificate PEM strings, both of which are required parameters to SSLIdentity::FromPEMStrings(). - Its only implementation, OpenSSLIdentity::PrivateKeyToPemString(). - SSLIdentity::PublicKeyToPEMString() added, used by tests. - sslidentity_unittest.cc updated: * FromPEMStringsRSA and FromPEMStringsEC updated. * CloneIdentityRSA and CloneIdentityECDSA added. - RTCCertificate::To/FromPem added, using new class RTCCertificatePem. - rtccertificate_unittest.cc: New test CloneWithPemSerialization. - Renamed rtc_unittests.cc to rtccertificate_unittest.cc to match convention. BUG=webrtc:5794, chromium:581354 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== RTCCertificate serialization. This CL adds the ability to convert RTCCertificate objects to and from PEM string representations of it (its private key and certificate). The RTCCertificate being a wrapper of SSLIdentity, this is where the meat is. Changes: - SSLIdentity::PrivateKeyToPEMString() added. It together with the already existing SSLCertificate::ToPEMString() yields both private key and certificate PEM strings, both of which are required parameters to SSLIdentity::FromPEMStrings(). - Its only implementation, OpenSSLIdentity::PrivateKeyToPemString(). - SSLIdentity::PublicKeyToPEMString() added, used by tests. - sslidentity_unittest.cc updated: * FromPEMStringsRSA and FromPEMStringsEC updated. * CloneIdentityRSA and CloneIdentityECDSA added. - RTCCertificate::To/FromPem added, using new class RTCCertificatePem. - rtccertificate_unittest.cc: New test CloneWithPemSerialization. - Renamed rtc_unittests.cc to rtccertificate_unittest.cc to match convention. BUG=webrtc:5794, chromium:581354 ========== to ========== RTCCertificate serialization. This CL adds the ability to convert RTCCertificate objects to and from PEM string representations of it (its private key and certificate). The RTCCertificate being a wrapper of SSLIdentity, this is where the meat is. Changes: - SSLIdentity::PrivateKeyToPEMString() added. It together with the already existing SSLCertificate::ToPEMString() yields both private key and certificate PEM strings, both of which are required parameters to SSLIdentity::FromPEMStrings(). - Its only implementation, OpenSSLIdentity::PrivateKeyToPemString(). - SSLIdentity::PublicKeyToPEMString() added, used by tests. - sslidentity_unittest.cc updated: * FromPEMStringsRSA and FromPEMStringsEC updated. * CloneIdentityRSA and CloneIdentityECDSA added. - RTCCertificate::To/FromPem added, using new class RTCCertificatePem. - rtccertificate_unittest.cc: New test CloneWithPemSerialization. - Renamed rtc_unittests.cc to rtccertificate_unittest.cc to match convention. - RTCCertificate/SSLIdentity comparison operators. BUG=webrtc:5794, chromium:581354 ==========
Message was sent while issue was closed.
Description was changed from ========== RTCCertificate serialization. This CL adds the ability to convert RTCCertificate objects to and from PEM string representations of it (its private key and certificate). The RTCCertificate being a wrapper of SSLIdentity, this is where the meat is. Changes: - SSLIdentity::PrivateKeyToPEMString() added. It together with the already existing SSLCertificate::ToPEMString() yields both private key and certificate PEM strings, both of which are required parameters to SSLIdentity::FromPEMStrings(). - Its only implementation, OpenSSLIdentity::PrivateKeyToPemString(). - SSLIdentity::PublicKeyToPEMString() added, used by tests. - sslidentity_unittest.cc updated: * FromPEMStringsRSA and FromPEMStringsEC updated. * CloneIdentityRSA and CloneIdentityECDSA added. - RTCCertificate::To/FromPem added, using new class RTCCertificatePem. - rtccertificate_unittest.cc: New test CloneWithPemSerialization. - Renamed rtc_unittests.cc to rtccertificate_unittest.cc to match convention. - RTCCertificate/SSLIdentity comparison operators. BUG=webrtc:5794, chromium:581354 ========== to ========== RTCCertificate serialization. This CL adds the ability to convert RTCCertificate objects to and from PEM string representations of it (its private key and certificate). The RTCCertificate being a wrapper of SSLIdentity, this is where the meat is. Changes: - SSLIdentity::PrivateKeyToPEMString() added. It together with the already existing SSLCertificate::ToPEMString() yields both private key and certificate PEM strings, both of which are required parameters to SSLIdentity::FromPEMStrings(). - Its only implementation, OpenSSLIdentity::PrivateKeyToPemString(). - SSLIdentity::PublicKeyToPEMString() added, used by tests. - sslidentity_unittest.cc updated: * FromPEMStringsRSA and FromPEMStringsEC updated. * CloneIdentityRSA and CloneIdentityECDSA added. - RTCCertificate::To/FromPem added, using new class RTCCertificatePem. - rtccertificate_unittest.cc: New test CloneWithPemSerialization. - Renamed rtc_unittests.cc to rtccertificate_unittest.cc to match convention. BUG=webrtc:5794, chromium:581354 Committed: https://crrev.com/6b470a9413d2e65dd9078a63ed3f5f60e5ba5232 Cr-Commit-Position: refs/heads/master@{#12546} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/6b470a9413d2e65dd9078a63ed3f5f60e5ba5232 Cr-Commit-Position: refs/heads/master@{#12546} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
