Permit (EC)DHEPSK KEX on SECLEVEL3 #17759
Conversation
beldmit
added
approval: otc review pending
|
Should this go to 1.1.1 as well? |
mattcaswell
added
approval: review pending
| @@ -1020,6 +1020,8 @@ static int ssl_security_default_callback(const SSL *s, const SSL_CTX *ctx, | |||
| case SSL_SECOP_CIPHER_CHECK: | |||
| { | |||
| const SSL_CIPHER *c = other; | |||
| int pfs_mask = SSL_kEDH | SSL_kEECDH | SSL_kECDHEPSK | SSL_kDHEPSK; | |||
There was a problem hiding this comment.
Nit: This PR could be a good candidate to also consistently use SSL_kDHE instead of SSL_kEDH in our codebase, and the same goes for SSL_kECDHE over SSL_kEECDH.
We already have them defined as respective aliases, and even in the OTC we were not entirely sure about the meaning of the less usual aliases used here.
Lines 165 to 172 in a044af4
| @@ -1034,7 +1036,7 @@ static int ssl_security_default_callback(const SSL *s, const SSL_CTX *ctx, | |||
| return 0; | |||
| /* Level 3: forward secure ciphersuites only */ | |||
There was a problem hiding this comment.
Nit: I think it might be more readable to have L1023 initialize pfs_mask as zero, and set it to SSL_kDHE | SSL_kECDHE | SSL_kDHEPSK | SSL_kECDHEPSK here, so one can immediately verify what the check is doing.
|
|
||
| ok(run(test(['ssl_old_test', '-psk', '0102030405', '-cipher', '@SECLEVEL=3:DHE-PSK-AES128-CCM'])), | ||
| 'test auto DH meets security strength'); | ||
| } |
There was a problem hiding this comment.
- Without specifying
-tls1_2or earlier, this test is not really covering the new code: if I understand this recipe correctly, the default is equivalent to-tls1_3if nothing is specified. We should explicitly test for-tls1_2, not sure about previous versions. - Maybe we could add also negative tests here: testing that the RSA PSK ciphers are not accepted at SECLEVEL=3.
- Also maybe we should fill the gap and add tests to also cover the
SSL_kDHEandSSL_kECDHEin TLS < 1.3
There was a problem hiding this comment.
@mattcaswell you are more familiar with these tests than I am, can you check what I commented here?
There was a problem hiding this comment.
Without specifying -tls1_2 or earlier, this test is not really covering the new code: if I understand this recipe correctly, the default is equivalent to -tls1_3 if nothing is specified. We should explicitly test for -tls1_2, not sure about previous versions.
Yes - this is correct. Good catch.
There was a problem hiding this comment.
Too good catch - it doesn't work :)
I think during the call there was consensus about considering #17743 as a bug, which would make it eligible for backport to 1.1.1. |
|
@beldmit @mattcaswell I opened #17763 as an alternative to this: I started working on this yesterday but run out of time preparing the tests commit. Hopefully it should fix the CI errors. |
|
I see the same test failures as in my PR :( |
|
I don't think there is a need to discuss this on the OTC call. If it is a bug fix, it can go back to 1.1.1. If it isn't a bug fix it can't go into 3.0. Since it's going into 3.0, it's a bug fix. |
Resolves: #17743
Checklist