Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24276011:
WebCrypto: Implement importKey(), sign(), and verify() for HMAC in OpenSSL (Closed)
|
Created:
7 years, 3 months ago by padolph Modified:
7 years, 2 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionWebCrypto: Implement importKey(), sign(), and verify() for HMAC in OpenSSL
BUG=267888
R=bryaneyler@google.com,eroman@chromium.org,ellyjones@chromium.org,rsleevi@chromium.org,jochen@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226410
Patch Set 1 #Patch Set 2 : Minor corrections #Patch Set 3 : clang-format -style=Chromium reformatting #
Total comments: 14
Patch Set 4 : rebase #Patch Set 5 : changes for eroman review #Patch Set 6 : rebase only #Patch Set 7 : fixed rebase problems #Patch Set 8 : Added verify() to make rebased unit tests pass #Patch Set 9 : rebase #
Messages
Total messages: 18 (0 generated)
Had to disable digest unit test, pending my impl in https://codereview.chromium.org/23621050/
hey, thanks for your patch. However, I couldn't find you on the list of authorized contributors. Could you please make sure to get yourself added to the list of authorized contributors from Netflix first? thanks
On 2013/09/23 11:16:02, jochen wrote: > hey, > > thanks for your patch. However, I couldn't find you on the list of authorized > contributors. Could you please make sure to get yourself added to the list of > authorized contributors from Netflix first? > > thanks Found out how to get on the Netflix list; working on this now. Thank you.
Deferring to Elly/Sleevi for the OpenSSL code, but otherwise lgtm. https://codereview.chromium.org/24276011/diff/8001/content/renderer/webcrypto... File content/renderer/webcrypto_impl_openssl.cc (right): https://codereview.chromium.org/24276011/diff/8001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:7: #include <vector> style nit: probably need to sort this after <openssl/*> https://codereview.chromium.org/24276011/diff/8001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:23: explicit SymKeyHandle(const unsigned char* key_data, unsigned key_data_size) style nit: can remove "explicit" https://codereview.chromium.org/24276011/diff/8001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:90: *handle = sym_key.Pass(); style nit: You could get rid of the sym_key temporary and write this as: handle->reset(new SymKeyHandle(...)); https://codereview.chromium.org/24276011/diff/8001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:106: DCHECK(key.algorithm().id() == WebKit::WebCryptoAlgorithmIdHmac); style nit: You can use DCHECK_EQ() in this case (gives nicer output on failures) https://codereview.chromium.org/24276011/diff/8001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:149: // which will result if the raw_key vector is empty; an entirely valid Is a 0-byte key a supported use-case? On the NSS side we are failing ImportKey when given empty key bytes. I believe we should match that behavior here. https://codereview.chromium.org/24276011/diff/8001/content/renderer/webcrypto... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/24276011/diff/8001/content/renderer/webcrypto... content/renderer/webcrypto_impl_unittest.cc:32: TEST_F(WebCryptoImplTest, DISABLED_DigestSampleSets) { Looks like this is a dependency with your other changelist. Depending on the order you land these, please match the recommendation in that other review and use: #if !defined(USE_OPENSSL) instead of DISABLED_
https://codereview.chromium.org/24276011/diff/8001/content/renderer/webcrypto... File content/renderer/webcrypto_impl_openssl.cc (right): https://codereview.chromium.org/24276011/diff/8001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:7: #include <vector> On 2013/09/25 00:00:59, eroman wrote: > style nit: probably need to sort this after <openssl/*> I followed the include header ordering recommendation here: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... "C library, C++ library, other libraries' .h, your project's .h" I think I applied this correctly, but is there some other guideline I should be using? https://codereview.chromium.org/24276011/diff/8001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:23: explicit SymKeyHandle(const unsigned char* key_data, unsigned key_data_size) On 2013/09/25 00:00:59, eroman wrote: > style nit: can remove "explicit" Done. https://codereview.chromium.org/24276011/diff/8001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:90: *handle = sym_key.Pass(); On 2013/09/25 00:00:59, eroman wrote: > style nit: You could get rid of the sym_key temporary and write this as: > > handle->reset(new SymKeyHandle(...)); Done. https://codereview.chromium.org/24276011/diff/8001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:106: DCHECK(key.algorithm().id() == WebKit::WebCryptoAlgorithmIdHmac); On 2013/09/25 00:00:59, eroman wrote: > style nit: You can use DCHECK_EQ() in this case (gives nicer output on failures) Done. https://codereview.chromium.org/24276011/diff/8001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:149: // which will result if the raw_key vector is empty; an entirely valid On 2013/09/25 00:00:59, eroman wrote: > Is a 0-byte key a supported use-case? On the NSS side we are failing ImportKey > when given empty key bytes. I believe we should match that behavior here. Done. https://codereview.chromium.org/24276011/diff/8001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:149: // which will result if the raw_key vector is empty; an entirely valid On 2013/09/25 00:00:59, eroman wrote: > Is a 0-byte key a supported use-case? On the NSS side we are failing ImportKey > when given empty key bytes. I believe we should match that behavior here. This code is required to pass Bryan's "Empty Sets" test in WebCryptoImplTest::HMACSampleSets, which passes for NSS. https://codereview.chromium.org/24276011/diff/8001/content/renderer/webcrypto... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/24276011/diff/8001/content/renderer/webcrypto... content/renderer/webcrypto_impl_unittest.cc:32: TEST_F(WebCryptoImplTest, DISABLED_DigestSampleSets) { On 2013/09/25 00:00:59, eroman wrote: > Looks like this is a dependency with your other changelist. Depending on the > order you land these, please match the recommendation in that other review and > use: > > #if !defined(USE_OPENSSL) > > instead of DISABLED_ Done.
https://codereview.chromium.org/24276011/diff/8001/content/renderer/webcrypto... File content/renderer/webcrypto_impl_openssl.cc (right): https://codereview.chromium.org/24276011/diff/8001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:7: #include <vector> On 2013/09/25 02:00:38, padolph wrote: > On 2013/09/25 00:00:59, eroman wrote: > > style nit: probably need to sort this after <openssl/*> > I followed the include header ordering recommendation here: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... > > "C library, C++ library, other libraries' .h, your project's .h" > > I think I applied this correctly, but is there some other guideline I should be > using? > My mistake then!
lgtm
can you add yourself to AUTHORS plz? otherwise, lgtm
(Paul is already part of AUTHORS, so good to go)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/24276011/22002
Retried try job too often on linux_chromeos for step(s) net_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
The commit-queue has been flaky today. I will resubmit the job. (No action necessary Paul)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/24276011/22002
Failed to apply patch for content/renderer/webcrypto/webcrypto_impl_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file content/renderer/webcrypto/webcrypto_impl_unittest.cc
Hunk #1 succeeded at 180 (offset 38 lines).
Hunk #2 succeeded at 264 (offset 38 lines).
Hunk #3 FAILED at 371.
Hunk #4 succeeded at 598 (offset 62 lines).
1 out of 4 hunks FAILED -- saving rejects to file
content/renderer/webcrypto/webcrypto_impl_unittest.cc.rej
Patch: content/renderer/webcrypto/webcrypto_impl_unittest.cc
Index: content/renderer/webcrypto/webcrypto_impl_unittest.cc
diff --git a/content/renderer/webcrypto/webcrypto_impl_unittest.cc
b/content/renderer/webcrypto/webcrypto_impl_unittest.cc
index
a659c7f24eaab61688de52601afb67a947008824..87ec7c5db2743b91a0c5da2bd51f738b3c52132e
100644
--- a/content/renderer/webcrypto/webcrypto_impl_unittest.cc
+++ b/content/renderer/webcrypto/webcrypto_impl_unittest.cc
@@ -142,6 +142,9 @@ class WebCryptoImplTest : public testing::Test {
WebCryptoImpl crypto_;
};
+// TODO(padolph) Enable these tests for OpenSSL once matching impl is available
+#if !defined(USE_OPENSSL)
+
TEST_F(WebCryptoImplTest, DigestSampleSets) {
// The results are stored here in hex format for readability.
//
@@ -223,6 +226,8 @@ TEST_F(WebCryptoImplTest, DigestSampleSets) {
}
}
+#endif // #if !defined(USE_OPENSSL)
+
TEST_F(WebCryptoImplTest, HMACSampleSets) {
struct TestCase {
WebKit::WebCryptoAlgorithmId algorithm;
@@ -366,6 +371,9 @@ TEST_F(WebCryptoImplTest, HMACSampleSets) {
}
}
+// TODO(padolph) Enable these tests for OpenSSL once matching impl is available
+#if !defined(USE_OPENSSL)
+
TEST_F(WebCryptoImplTest, AesCbcEncryptionFailures) {
WebKit::WebCryptoKey key = ImportSecretKeyFromRawHexString(
"2b7e151628aed2a6abf7158809cf4f3c",
@@ -531,4 +539,6 @@ TEST_F(WebCryptoImplTest, AesCbcSampleSets) {
}
}
+#endif // !defined(USE_OPENSSL)
+
} // namespace content
Paul, I think you may have to rebase this again?
On 2013/10/01 01:05:37, I haz the power (commit-bot) wrote:
> Failed to apply patch for
content/renderer/webcrypto/webcrypto_impl_unittest.cc:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
> patching file content/renderer/webcrypto/webcrypto_impl_unittest.cc
> Hunk #1 succeeded at 180 (offset 38 lines).
> Hunk #2 succeeded at 264 (offset 38 lines).
> Hunk #3 FAILED at 371.
> Hunk #4 succeeded at 598 (offset 62 lines).
> 1 out of 4 hunks FAILED -- saving rejects to file
> content/renderer/webcrypto/webcrypto_impl_unittest.cc.rej
>
> Patch: content/renderer/webcrypto/webcrypto_impl_unittest.cc
> Index: content/renderer/webcrypto/webcrypto_impl_unittest.cc
> diff --git a/content/renderer/webcrypto/webcrypto_impl_unittest.cc
> b/content/renderer/webcrypto/webcrypto_impl_unittest.cc
> index
>
a659c7f24eaab61688de52601afb67a947008824..87ec7c5db2743b91a0c5da2bd51f738b3c52132e
> 100644
> --- a/content/renderer/webcrypto/webcrypto_impl_unittest.cc
> +++ b/content/renderer/webcrypto/webcrypto_impl_unittest.cc
> @@ -142,6 +142,9 @@ class WebCryptoImplTest : public testing::Test {
> WebCryptoImpl crypto_;
> };
>
> +// TODO(padolph) Enable these tests for OpenSSL once matching impl is
available
> +#if !defined(USE_OPENSSL)
> +
> TEST_F(WebCryptoImplTest, DigestSampleSets) {
> // The results are stored here in hex format for readability.
> //
> @@ -223,6 +226,8 @@ TEST_F(WebCryptoImplTest, DigestSampleSets) {
> }
> }
>
> +#endif // #if !defined(USE_OPENSSL)
> +
> TEST_F(WebCryptoImplTest, HMACSampleSets) {
> struct TestCase {
> WebKit::WebCryptoAlgorithmId algorithm;
> @@ -366,6 +371,9 @@ TEST_F(WebCryptoImplTest, HMACSampleSets) {
> }
> }
>
> +// TODO(padolph) Enable these tests for OpenSSL once matching impl is
available
> +#if !defined(USE_OPENSSL)
> +
> TEST_F(WebCryptoImplTest, AesCbcEncryptionFailures) {
> WebKit::WebCryptoKey key = ImportSecretKeyFromRawHexString(
> "2b7e151628aed2a6abf7158809cf4f3c",
> @@ -531,4 +539,6 @@ TEST_F(WebCryptoImplTest, AesCbcSampleSets) {
> }
> }
>
> +#endif // !defined(USE_OPENSSL)
> +
> } // namespace content
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/24276011/55001
On 2013/10/01 18:14:43, Bryan Eyler wrote:
> Paul, I think you may have to rebase this again?
>
> On 2013/10/01 01:05:37, I haz the power (commit-bot) wrote:
> > Failed to apply patch for
> content/renderer/webcrypto/webcrypto_impl_unittest.cc:
> > While running patch -p1 --forward --force --no-backup-if-mismatch;
> > patching file content/renderer/webcrypto/webcrypto_impl_unittest.cc
> > Hunk #1 succeeded at 180 (offset 38 lines).
> > Hunk #2 succeeded at 264 (offset 38 lines).
> > Hunk #3 FAILED at 371.
> > Hunk #4 succeeded at 598 (offset 62 lines).
> > 1 out of 4 hunks FAILED -- saving rejects to file
> > content/renderer/webcrypto/webcrypto_impl_unittest.cc.rej
> >
> > Patch: content/renderer/webcrypto/webcrypto_impl_unittest.cc
> > Index: content/renderer/webcrypto/webcrypto_impl_unittest.cc
> > diff --git a/content/renderer/webcrypto/webcrypto_impl_unittest.cc
> > b/content/renderer/webcrypto/webcrypto_impl_unittest.cc
> > index
> >
>
a659c7f24eaab61688de52601afb67a947008824..87ec7c5db2743b91a0c5da2bd51f738b3c52132e
> > 100644
> > --- a/content/renderer/webcrypto/webcrypto_impl_unittest.cc
> > +++ b/content/renderer/webcrypto/webcrypto_impl_unittest.cc
> > @@ -142,6 +142,9 @@ class WebCryptoImplTest : public testing::Test {
> > WebCryptoImpl crypto_;
> > };
> >
> > +// TODO(padolph) Enable these tests for OpenSSL once matching impl is
> available
> > +#if !defined(USE_OPENSSL)
> > +
> > TEST_F(WebCryptoImplTest, DigestSampleSets) {
> > // The results are stored here in hex format for readability.
> > //
> > @@ -223,6 +226,8 @@ TEST_F(WebCryptoImplTest, DigestSampleSets) {
> > }
> > }
> >
> > +#endif // #if !defined(USE_OPENSSL)
> > +
> > TEST_F(WebCryptoImplTest, HMACSampleSets) {
> > struct TestCase {
> > WebKit::WebCryptoAlgorithmId algorithm;
> > @@ -366,6 +371,9 @@ TEST_F(WebCryptoImplTest, HMACSampleSets) {
> > }
> > }
> >
> > +// TODO(padolph) Enable these tests for OpenSSL once matching impl is
> available
> > +#if !defined(USE_OPENSSL)
> > +
> > TEST_F(WebCryptoImplTest, AesCbcEncryptionFailures) {
> > WebKit::WebCryptoKey key = ImportSecretKeyFromRawHexString(
> > "2b7e151628aed2a6abf7158809cf4f3c",
> > @@ -531,4 +539,6 @@ TEST_F(WebCryptoImplTest, AesCbcSampleSets) {
> > }
> > }
> >
> > +#endif // !defined(USE_OPENSSL)
> > +
> > } // namespace content
done
Message was sent while issue was closed.
Change committed as 226410 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
