-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 DH Key exchange incompatibility #10864
Conversation
A change in key types for DH keys was introduced in 42.0.0 such that DH key exchange with older versions (e.g. 41.0.7) no longer works. This affects key exchange with other OpenSSL servers as well. This commit reverts the code that attepmts to differentiate between DH and DHX key exchanges.
src/rust/src/backend/dh.rs
Outdated
} else { | ||
Ok(openssl::pkey::PKey::from_dh(dh)?) | ||
} | ||
Ok(openssl::pkey::PKey::from_dh(dh)?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So with this change it will never create a dhx
variant, which I think is more aggressive than we want.
I'm honestly unsure how it is that no tests fail because of that.
I think you only want to apply this change to the generate
codepaths. My suggest would be to add an allow_dhx
param (maybe a better name), pass false in generate, but true everywhere else.
tests/hazmat/primitives/test_dh.py
Outdated
@@ -473,6 +474,39 @@ def test_public_key_copy(self): | |||
|
|||
assert key1 == key2 | |||
|
|||
def test_old_public_key_exchange(self, backend): | |||
# This is a public key generated on cryptograpy version 41.0.7 | |||
key_data = textwrap.dedent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally we use keys from the vectors/
directory in our tests -- there should be an existing one with a matching format.
@alex sorry for the extreme lag, its taken me a while to get back to this. I don't love what I did here, but see if its along the lines of what you were looking for, both in the testing side and keygen side. None of the existing test keys in All comments and suggestions welcome... And one of the CI tests failed....so I am rapidly getting out of my depth here |
The twisted test failures here appear to be genuine DH failures, suggesting that there's an issue in this PR. It also likely indicates missing test cases in our own test suite. |
fixes pyca#10790 closes pyca#10864 closes pyca#11218
A change in key types for DH keys was introduced in 42.0.0 such that DH key exchange with older versions (e.g. 41.0.7) no longer works. This affects key exchange with other OpenSSL servers as well.
This commit reverts the code that attempts to differentiate between DH and DHX key exchanges.
Fixes issue #10790