Skip to content

Add TLS RSA-PSS certificate support#4368

Closed
snhenson wants to merge 5 commits intoopenssl:masterfrom
snhenson:tls_pss
Closed

Add TLS RSA-PSS certificate support#4368
snhenson wants to merge 5 commits intoopenssl:masterfrom
snhenson:tls_pss

Conversation

@snhenson
Copy link
Copy Markdown
Contributor

Checklist
  • tests are added or updated

This PR adds support for RSA-PSS certificate to TLS 1.3 and 1.2 as required by the current TLS 1.3 draft.

Recognise RSA-PSS certificate algorithm and add a new certificate
type.
Copy link
Copy Markdown
Contributor

@richsalz richsalz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit. which only proves I read the diff :) you might want to have matt look at this PR. Or not.

ssl/t1_lib.c Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blank line; flip this with the blank line that follows

Allo RSA certificate to be used for RSA-PSS signatures: this needs
to be explicit because RSA and RSA-PSS certificates are now distinct
types.
ssl/ssl_lib.c Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting the (preexisting) mismatch in this function as to whether braces are used for one-line conditional bodies.

ssl/ssl_lib.c Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be more clear to make this a standalone 'if' clause instead of 'else if' (and outdent the comment to match, moving it outside the braces) -- there shouldn't be any functional change from doing so.

@kroeckx
Copy link
Copy Markdown
Member

kroeckx commented Sep 15, 2017

Can you also add tests that fail?

ssl/ssl_locl.h Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of changing all these, but it's an internal data structure, so it shouldn't matter. (No change requested.)

@snhenson
Copy link
Copy Markdown
Contributor Author

OK added a couple of failing tests: PSS with no PSS signature algorithms and attempt to use PSS with TLS 1.1.

@snhenson
Copy link
Copy Markdown
Contributor Author

Ping, any more comments? The two original reviews are for a previous version of this PR.

@kaduk
Copy link
Copy Markdown
Contributor

kaduk commented Sep 19, 2017

No further comment from me; reconfirm +1

levitte pushed a commit that referenced this pull request Sep 20, 2017
Recognise RSA-PSS certificate algorithm and add a new certificate
type.

Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from #4368)
levitte pushed a commit that referenced this pull request Sep 20, 2017
Allo RSA certificate to be used for RSA-PSS signatures: this needs
to be explicit because RSA and RSA-PSS certificates are now distinct
types.

Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from #4368)
levitte pushed a commit that referenced this pull request Sep 20, 2017
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from #4368)
levitte pushed a commit that referenced this pull request Sep 20, 2017
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from #4368)
levitte pushed a commit that referenced this pull request Sep 20, 2017
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from #4368)
@snhenson
Copy link
Copy Markdown
Contributor Author

Thanks everyone, pushed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants