Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a sig algs bug and add some tests #2160

Closed
wants to merge 3 commits into from

Conversation

mattcaswell
Copy link
Member

Checklist
  • tests are added or updated
  • CLA is signed
Description of change

This PR fixes a bug in the SSL_set1_sigalgs() macro. Prior to this fix, use of the macro results in a compilation failure. The bug was actually already fixed once in commit 75fdee0, but the fix was only applied to the 1.0.2 branch for some reason.

I've also added some tests for this.

The first 2 commits are for 1.1.0 and master. The third commit is only for master as it is TLSv1.3 specific.

This macro has a typo in it which makes it unusable. This issue was already
fixed in 1.0.2 in commit 75fdee0, but the same fix was not applied to
other branches.
@mattcaswell mattcaswell added 1.1.0 branch: master Merge to master branch labels Dec 30, 2016
@t-j-h
Copy link
Member

t-j-h commented Dec 30, 2016

+1 with minor comment fix applied

@mattcaswell
Copy link
Member Author

I pushed a new version to fix a travis failure (variables in the new test were not declared static). I also fixed the comment while I was at it.

@t-j-h please reconfirm

@t-j-h
Copy link
Member

t-j-h commented Dec 30, 2016

I dont see any new commits on this pull request since my +1 - am I missing something?

@mattcaswell
Copy link
Member Author

I modified the existing commits and force pushed.

The only difference is the comment change and all these variables are now declared static where they weren't before:

static const int validlist1[] = {NID_sha256, EVP_PKEY_RSA};
static const int validlist2[] = {NID_sha256, EVP_PKEY_RSA, NID_sha512, EVP_PKEY_EC};
static const int validlist3[] = {NID_sha512, EVP_PKEY_EC};
static const int invalidlist1[] = {NID_undef, EVP_PKEY_RSA};
static const int invalidlist2[] = {NID_sha256, NID_undef};
static const int invalidlist3[] = {NID_sha256, EVP_PKEY_RSA, NID_sha256};
static const int invalidlist4[] = {NID_sha256};
static const sigalgs_list testsigalgs[] = {

@t-j-h
Copy link
Member

t-j-h commented Dec 30, 2016

+1

levitte pushed a commit that referenced this pull request Dec 30, 2016
This macro has a typo in it which makes it unusable. This issue was already
fixed in 1.0.2 in commit 75fdee0, but the same fix was not applied to
other branches.

Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from #2160)
(cherry picked from commit fb3ae0e)
levitte pushed a commit that referenced this pull request Dec 30, 2016
Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from #2160)
(cherry picked from commit f1b25aa)
levitte pushed a commit that referenced this pull request Dec 30, 2016
This macro has a typo in it which makes it unusable. This issue was already
fixed in 1.0.2 in commit 75fdee0, but the same fix was not applied to
other branches.

Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from #2160)
levitte pushed a commit that referenced this pull request Dec 30, 2016
Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from #2160)
levitte pushed a commit that referenced this pull request Dec 30, 2016
We need a new API for TLSv1.3 sig algs

Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from #2160)
@mattcaswell
Copy link
Member Author

Pushed. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants