Skip to content

Comments

TLSv1.3: Fix test_sslcorrupt#1894

Closed
mattcaswell wants to merge 1 commit intoopenssl:masterfrom
mattcaswell:tlsv1_3-fix-test_sslcorrupt
Closed

TLSv1.3: Fix test_sslcorrupt#1894
mattcaswell wants to merge 1 commit intoopenssl:masterfrom
mattcaswell:tlsv1_3-fix-test_sslcorrupt

Conversation

@mattcaswell
Copy link
Member

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

The test loops through all the ciphers, attempting to test each one in turn.
However version negotiation happens before cipher selection, so with TLSv1.3
switched on if we use a non-TLSv1.3 compatible cipher suite we get "no
share cipher".

The test loops through all the ciphers, attempting to test each one in turn.
However version negotiation happens before cipher selection, so with TLSv1.3
switched on if we use a non-TLSv1.3 compatible cipher suite we get "no
share cipher".
Copy link
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.

stderr. or is this also part of your promise upgrade to the simpler test framework ? :)


ciphers = SSL_CTX_get_ciphers(cctx);
if (ciphers == NULL || sk_SSL_CIPHER_num(ciphers) != 1) {
printf("Unexpected ciphers set\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't these go to stderr?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just being consistent with the rest of the file

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, but errors should go to stderr. we waffle: just doing what's already done vs fix it all while you have the file opened. we should about a policy. but like i said, +1 to what you did.

@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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants