Closed
Bug 1215295
Opened 8 years ago
Closed 8 years ago
Support RSA-PSS for digital signing
Categories
(NSS :: Libraries, defect)
NSS
Libraries
Tracking
(firefox44 affected)
RESOLVED
FIXED
3.22
Tracking | Status | |
---|---|---|
firefox44 | --- | affected |
People
(Reporter: ttaubert, Assigned: ttaubert)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
1.46 KB,
patch
|
ttaubert
:
review+
mt
:
checked-in+
|
Details | Diff | Splinter Review |
In TLS 1.3, struct DigitallySigned switches from RSASSA-PKCS1-v1_5 to RSASSA-PSS signatures.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
Patch at: https://codereview.appspot.com/265700044 Eric, Martin, can you please take a look and give some feedback? I split it into three patches but didn't know how to do that with rietveld, so there it's just one big patch. This needs a few new tests of course but I first wanted to see whether that's the right direction. All existing tests pass. Thanks!
Flags: needinfo?(martin.thomson)
Flags: needinfo?(ekr)
Assignee | ||
Updated•8 years ago
|
Attachment #8674498 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8674499 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8674500 -
Attachment is obsolete: true
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Martin Thomson [:mt:] from comment #5) > I shall await the tests. Working on it, thanks!
Assignee | ||
Comment 7•8 years ago
|
||
Updated the patch at https://codereview.appspot.com/265700044/ I fixed a few mistakes and ensured that we not only support PSS signatures but also use ssl_sign_rsapss for TLS 1.3. I added quite a few tests that ensure we properly enforce PSS signatures in TLS 1.3. The tests run only with NSS_ENABLE_TLS_1_3=1.
Flags: needinfo?(martin.thomson)
Comment 8•8 years ago
|
||
Sorry to be a downer, but it would really be better if you restricted this patch to PKCS#11. We have quite a large TLS 1.3 patch already in flight and messing with 1.3 here is more likey to get in the way.
Flags: needinfo?(ekr)
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Eric Rescorla (:ekr) from comment #8) > Sorry to be a downer, but it would really be better if you restricted this > patch to PKCS#11. Ok, I can do only the PKCS#11 stuff now and keep the rest for after your patch landed. > We have quite a large TLS 1.3 patch already in flight and messing with 1.3 > here is more likey > to get in the way. Is there a bug for that?
Comment 10•8 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #9) > (In reply to Eric Rescorla (:ekr) from comment #8) > > Sorry to be a downer, but it would really be better if you restricted this > > patch to PKCS#11. > > Ok, I can do only the PKCS#11 stuff now and keep the rest for after your > patch landed. Well, I'm going to wire up PSS when I do it. A lot of the mechanisms have already change here. > > We have quite a large TLS 1.3 patch already in flight and messing with 1.3 > > here is more likey > > to get in the way. > > Is there a bug for that? There's a meta-bug somewhere.
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Eric Rescorla (:ekr) from comment #10) > (In reply to Tim Taubert [:ttaubert] from comment #9) > > (In reply to Eric Rescorla (:ekr) from comment #8) > > > Sorry to be a downer, but it would really be better if you restricted this > > > patch to PKCS#11. > > > > Ok, I can do only the PKCS#11 stuff now and keep the rest for after your > > patch landed. > > Well, I'm going to wire up PSS when I do it. A lot of the mechanisms > have already change here. Hmm, ok. Too bad. I'll write a few lower-level tests then.
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(martin.thomson)
Comment 12•8 years ago
|
||
Those lower-level tests are needed in any case.
Assignee | ||
Comment 13•8 years ago
|
||
(In reply to Eric Rescorla (:ekr) from comment #12) > Those lower-level tests are needed in any case. Yeah, agreed. Forgot about those.
Assignee | ||
Comment 14•8 years ago
|
||
New patch at: https://codereview.appspot.com/271240043
Flags: needinfo?(martin.thomson)
Flags: needinfo?(ekr)
Updated•8 years ago
|
Flags: needinfo?(ekr)
Assignee | ||
Comment 15•8 years ago
|
||
Patch updated and review feedback addressed: https://codereview.appspot.com/271240043 Rob, can you please take a look at the nss.def changes? Thanks!
Flags: needinfo?(rrelyea)
Flags: needinfo?(ekr)
Updated•8 years ago
|
Flags: needinfo?(ekr)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(martin.thomson)
Comment 16•8 years ago
|
||
https://hg.mozilla.org/projects/nss/rev/372db375adaa
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.22
Comment 17•8 years ago
|
||
Looks like some of our builders aren't happy about template<class T> using x<T> = y<t>; Macros to the rescue.
Attachment #8687464 -
Flags: review?(ttaubert)
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8687464 [details] [diff] [review] bug1215295-scoped.patch Review of attachment 8687464 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, thanks for fixing this so fast!
Attachment #8687464 -
Flags: review?(ttaubert) → review+
Comment 19•8 years ago
|
||
Comment on attachment 8687464 [details] [diff] [review] bug1215295-scoped.patch Review of attachment 8687464 [details] [diff] [review]: ----------------------------------------------------------------- https://hg.mozilla.org/projects/nss/rev/f741565a4e34
Attachment #8687464 -
Flags: checked-in+
Updated•8 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(rrelyea)
Updated•8 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•