-
Notifications
You must be signed in to change notification settings - Fork 483
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
feat: optional rustls support #330
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
https://github.com/taiki-e/cargo-hack has some flags to adjust the powerset behaviour |
I will add docs and more tests for rustls when the implementation is done and reviewed. |
Did you check how will the integration test run with this new compile flag? I think we're only running with the default tls implementation ( native-tls ). If we run the test with both native-tls and rustls, we can be sure it's correct. |
I run it locally by replacing native-tls with rustls. I am figuring out how to test both in CI. |
I added both tests to CI. |
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.
Well done and thank you very much! Here's my last question
test("tests/for_udp/websocket_tls_transport.toml", Type::Udp).await?; | ||
|
||
Ok(()) | ||
} | ||
|
||
#[instrument] | ||
async fn test(config_path: &'static str, t: Type) -> Result<()> { | ||
if cfg!(not(all(feature = "client", feature = "server"))) { | ||
// Skip the test if the client or the server is not enabled | ||
return Ok(()); |
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.
Why we didn't need this before?
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.
This combinations are never run in CI before.
I ran cargo hack test
locally and met this.
I will add docs in the sequential PR. |
@sunmy2019 The |
See my updated docs here: #337 One difference is that, the crate we use for loading PKCS#12 archives can only handle limited types of PBE algorithms. We only support PKCS#12 archives that they (crate In short, the command to create the PKCS#12 archive with openssl pkcs12 -export -out identity.pfx -inkey server.key -in server.crt -certfile ca_chain_certs.crt \
-keypbe PBE-SHA1-3DES -certpbe PBE-SHA1-3DES |
@sunmy2019 Both SHA1 and 3DES are not secure according to today's security standards. IMHO, a more updated crate should be used to handle modern algorithms. |
Do you have suggestions? |
That makes sense. I think we should support the default PBEs for newer formats, but I am unsure how. Currently, the best-supported crate is still the openssl. One strategy is to fork One strategy is to deprecate PKCS#12, and provide PCKS#8 options which is maintained by |
According to https://stackoverflow.com/a/62863490/7878845, PKCS#12 file can be generated without encryption, which could be an alternative to not provide false sense of security. |
I've been wondering what's the point of this password in this program when you write down the password in a separate file. |
@rapiz1 Also please update the document to use 3DES instead of RC2 (i.e. |
This is the initial implementation of rustls support.
Use it by disabling
native-tls-support
flag and enablingrustls-support
flag.Notes:
p12
that handles some of the PBE methods. I choose one of them as you can see inexamples/tls/create_self_signed_cert.sh
Feedback is welcome. I am new to this project.