Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(18)

Issue 68303009: [webcrypto] Add RSASSA-PKCS1-v1_5 sign and verify for NSS. (Closed)

Created:
5 years, 1 month ago by padolph
Modified:
4 years, 11 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.

Description

[webcrypto] Add RSASSA-PKCS1-v1_5 sign and verify for NSS. BUG=245025 TEST=content_unittests --gtest_filter="WebCryptoImpl*" Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245430

Patch Set 1 #

Patch Set 2 : rebase + minor test fix #

Total comments: 8

Patch Set 3 : rebase #

Patch Set 4 : rebase #

Patch Set 5 : fixes for eroman #

Total comments: 2

Patch Set 6 : rebase #

Patch Set 7 : rewrote to use higher-level NSS functions, and added known-answer tests #

Patch Set 8 : rebase #

Patch Set 9 : rebase #

Total comments: 7

Patch Set 10 : rebase #

Patch Set 11 : fixes for eroman #

Total comments: 9

Patch Set 12 : fixes for eroman and rebase #

Total comments: 15

Patch Set 13 : fixes for eroman, and MAYBE'ed new tests (missed in last rebase) #

Total comments: 26

Patch Set 14 : rebase #

Patch Set 15 : fixes for rsleevi #

Patch Set 16 : rebase plus minor fix #

Total comments: 8

Patch Set 17 : rebase #

Patch Set 18 : fixes for eroman #

Patch Set 19 : minor code formatting fix #

Unified diffs Side-by-side diffs Delta from patch set Stats (+631 lines, -116 lines) Patch
M content/renderer/webcrypto/webcrypto_impl_nss.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 6 chunks +108 lines, -1 line 0 comments Download
M content/renderer/webcrypto/webcrypto_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 17 chunks +494 lines, -99 lines 0 comments Download
M content/renderer/webcrypto/webcrypto_util.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +2 lines, -0 lines 0 comments Download
M content/renderer/webcrypto/webcrypto_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +27 lines, -16 lines 0 comments Download

Messages

Total messages: 40 (0 generated)
padolph
5 years, 1 month ago (2013-11-13 03:24:28 UTC) #1
eroman
Looks fine to me, passing off to @rsleevi. https://codereview.chromium.org/68303009/diff/40001/content/renderer/webcrypto/webcrypto_impl_nss.cc File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/40001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode246 content/renderer/webcrypto/webcrypto_impl_nss.cc:246: blink::WebCryptoAlgorithm::createNull(); ...
5 years ago (2013-11-20 23:57:44 UTC) #2
padolph
https://codereview.chromium.org/68303009/diff/40001/content/renderer/webcrypto/webcrypto_impl_nss.cc File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/40001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode246 content/renderer/webcrypto/webcrypto_impl_nss.cc:246: blink::WebCryptoAlgorithm::createNull(); On 2013/11/20 23:57:44, eroman wrote: > [optional] Rather ...
5 years ago (2013-11-21 01:20:13 UTC) #3
Ryan Sleevi
Please add known answer tests, which would spot the BUG highlighted below. https://codereview.chromium.org/68303009/diff/150001/content/renderer/webcrypto/webcrypto_impl_nss.cc File content/renderer/webcrypto/webcrypto_impl_nss.cc ...
5 years ago (2013-11-21 01:45:57 UTC) #4
padolph
On 2013/11/21 01:45:57, Ryan Sleevi wrote: > Please add known answer tests, which would spot ...
5 years ago (2013-11-21 02:42:08 UTC) #5
Ryan Sleevi
On 2013/11/21 02:42:08, padolph wrote: > On 2013/11/21 01:45:57, Ryan Sleevi wrote: > > Please ...
5 years ago (2013-11-21 02:58:47 UTC) #6
padolph
https://codereview.chromium.org/68303009/diff/150001/content/renderer/webcrypto/webcrypto_impl_nss.cc File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/150001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode651 content/renderer/webcrypto/webcrypto_impl_nss.cc:651: if (!DigestInternal(hash_algorithm, data, data_size, &digest)) On 2013/11/21 01:45:57, Ryan ...
5 years ago (2013-11-28 02:29:09 UTC) #7
padolph
On 2013/11/21 01:45:57, Ryan Sleevi wrote: > Please add known answer tests, which would spot ...
5 years ago (2013-11-28 02:32:04 UTC) #8
eroman
https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcrypto/webcrypto_impl_nss.cc File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode260 content/renderer/webcrypto/webcrypto_impl_nss.cc:260: // TODO(padolph): Move to webcrypto_util This can be done ...
5 years ago (2013-12-06 02:20:41 UTC) #9
padolph
https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcrypto/webcrypto_impl_nss.cc File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode260 content/renderer/webcrypto/webcrypto_impl_nss.cc:260: // TODO(padolph): Move to webcrypto_util On 2013/12/06 02:20:42, eroman ...
5 years ago (2013-12-06 18:52:52 UTC) #10
padolph
(manual email) https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:260: // TODO(padolph): Move to webcrypto_util On 2013/12/06 ...
5 years ago (2013-12-06 18:53:51 UTC) #11
Ryan Sleevi
OOO today, and from cellphone, but... Don't rely on NSS's auto detection. Supply explicitly the ...
5 years ago (2013-12-06 19:30:57 UTC) #12
eroman
Thanks for the feedback Ryan, enjoy your vacation :) @Paul: Since we decided to restrict ...
5 years ago (2013-12-06 20:22:37 UTC) #13
eroman
(With the caveat that the RSA should have also been imported with SHA-256 as the ...
5 years ago (2013-12-06 21:33:44 UTC) #14
padolph
On 2013/12/06 20:22:37, eroman wrote: > Thanks for the feedback Ryan, enjoy your vacation :) ...
5 years ago (2013-12-07 00:38:22 UTC) #15
padolph
On 2013/12/06 21:33:44, eroman wrote: > (With the caveat that the RSA should have also ...
5 years ago (2013-12-07 00:40:55 UTC) #16
padolph
https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcrypto/webcrypto_impl_nss.cc File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/250003/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode1051 content/renderer/webcrypto/webcrypto_impl_nss.cc:1051: SEC_OID_UNKNOWN, On 2013/12/06 18:52:52, padolph wrote: > On 2013/12/06 ...
5 years ago (2013-12-07 00:41:03 UTC) #17
padolph
(manual email) On 2013/12/06 20:22:37, eroman wrote: > Thanks for the feedback Ryan, enjoy your ...
5 years ago (2013-12-07 00:46:14 UTC) #18
eroman
On Fri, Dec 6, 2013 at 4:46 PM, Paul Adolph <padolph@netflix.com> wrote: > (manual email) ...
5 years ago (2013-12-10 02:06:15 UTC) #19
padolph
> > On 2013/12/06 21:33:44, eroman wrote: > > > (With the caveat that the ...
4 years, 11 months ago (2013-12-19 19:55:58 UTC) #20
eroman
L G T M after a rebase. I was expecting Ryan to give the final ...
4 years, 11 months ago (2013-12-19 22:56:37 UTC) #21
padolph
https://codereview.chromium.org/68303009/diff/310001/content/renderer/webcrypto/webcrypto_impl_nss.cc File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/310001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode1035 content/renderer/webcrypto/webcrypto_impl_nss.cc:1035: const SECOidTag hash_alg_tag = WebCryptoAlgorithmToNssSecOidShaTag( On 2013/12/19 22:56:38, eroman ...
4 years, 11 months ago (2013-12-20 00:03:39 UTC) #22
padolph
Does anybody mind the huge test vector set for this CL? IMO it gets in ...
4 years, 11 months ago (2013-12-20 00:15:07 UTC) #23
eroman
LGTM. I don't mind the big interleaving the test data, I think it is consistent ...
4 years, 11 months ago (2013-12-20 01:12:49 UTC) #24
padolph
https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcrypto/webcrypto_impl_nss.cc File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode86 content/renderer/webcrypto/webcrypto_impl_nss.cc:86: return SEC_OID_UNKNOWN; On 2013/12/20 01:12:50, eroman wrote: > This ...
4 years, 11 months ago (2013-12-20 01:44:26 UTC) #25
padolph
(manual email) Are we still waiting for an LGTM from Ryan? https://codereview.chromium.org/68303009/diff/330001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): ...
4 years, 11 months ago (2013-12-20 01:46:42 UTC) #26
Ryan Sleevi
https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcrypto/webcrypto_impl_nss.cc File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode972 content/renderer/webcrypto/webcrypto_impl_nss.cc:972: hash_alg_tag); This is unnecessary. The design of this API ...
4 years, 11 months ago (2013-12-20 02:08:50 UTC) #27
padolph
https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcrypto/webcrypto_impl_nss.cc File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcrypto/webcrypto_impl_nss.cc#newcode972 content/renderer/webcrypto/webcrypto_impl_nss.cc:972: hash_alg_tag); On 2013/12/20 02:08:50, Ryan Sleevi wrote: > This ...
4 years, 11 months ago (2013-12-21 02:44:17 UTC) #28
padolph
(manual email) https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/68303009/diff/350001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:972: hash_alg_tag); On 2013/12/20 02:08:50, Ryan Sleevi wrote: ...
4 years, 11 months ago (2013-12-21 02:48:30 UTC) #29
Bryan Eyler
lgtm
4 years, 11 months ago (2014-01-15 22:19:21 UTC) #30
eroman
lgtm https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcrypto/webcrypto_impl_unittest.cc File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcrypto/webcrypto_impl_unittest.cc#newcode146 content/renderer/webcrypto/webcrypto_impl_unittest.cc:146: const std::string kPublicKeySpkiDerHex = Chromium style doesn't allow ...
4 years, 11 months ago (2014-01-15 23:10:01 UTC) #31
padolph
https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcrypto/webcrypto_impl_unittest.cc File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcrypto/webcrypto_impl_unittest.cc#newcode146 content/renderer/webcrypto/webcrypto_impl_unittest.cc:146: const std::string kPublicKeySpkiDerHex = On 2014/01/15 23:10:02, eroman wrote: ...
4 years, 11 months ago (2014-01-16 02:02:03 UTC) #32
padolph
(manual email) 2 minutes ago #32 https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/68303009/diff/480001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:146: const std::string kPublicKeySpkiDerHex ...
4 years, 11 months ago (2014-01-16 02:05:19 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/68303009/640001
4 years, 11 months ago (2014-01-16 02:30:03 UTC) #34
commit-bot: I haz the power
Retried try job too often on win_x64_rel for step(s) base_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_rel&number=67845
4 years, 11 months ago (2014-01-16 04:36:32 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/68303009/910001
4 years, 11 months ago (2014-01-16 20:11:29 UTC) #36
eroman
(This isn't going to work until I roll NSS forward too, cancelling commit).
4 years, 11 months ago (2014-01-16 20:25:39 UTC) #37
padolph
Oh. Sorry to jump the gun. On Thu, Jan 16, 2014 at 12:25 PM, <eroman@chromium.org> ...
4 years, 11 months ago (2014-01-16 20:38:11 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/68303009/910001
4 years, 11 months ago (2014-01-17 00:57:06 UTC) #39
commit-bot: I haz the power
4 years, 11 months ago (2014-01-17 06:15:34 UTC) #40
Message was sent while issue was closed.
Change committed as 245430

Powered by Google App Engine
This is Rietveld 408576698