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

feat: optional rustls support #330

Merged
merged 15 commits into from
Feb 18, 2024
Merged

Conversation

sunmy2019
Copy link
Contributor

@sunmy2019 sunmy2019 commented Feb 15, 2024

This is the initial implementation of rustls support.

Use it by disabling native-tls-support flag and enabling rustls-support flag.

Notes:

  1. It passes all tests. Don't know if that's enough.
  2. There is no available package other than openssl that can handle all pkcs12 pbe methods. The only available create is p12 that handles some of the PBE methods. I choose one of them as you can see in examples/tls/create_self_signed_cert.sh
  3. features flags like: "rustls-support" -> "tls-support", "native-tls-support" -> "tls-support".

Feedback is welcome. I am new to this project.

@sunmy2019

This comment was marked as resolved.

@rapiz1
Copy link
Owner

rapiz1 commented Feb 15, 2024

https://github.com/taiki-e/cargo-hack has some flags to adjust the powerset behaviour

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@sunmy2019
Copy link
Contributor Author

I will add docs and more tests for rustls when the implementation is done and reviewed.

@rapiz1
Copy link
Owner

rapiz1 commented Feb 15, 2024

It passes all tests. Don't know if that's enough.

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.

@sunmy2019
Copy link
Contributor Author

sunmy2019 commented Feb 15, 2024

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 )

I run it locally by replacing native-tls with rustls.

I am figuring out how to test both in CI.

@sunmy2019
Copy link
Contributor Author

I added both tests to CI.

src/main.rs Outdated Show resolved Hide resolved
src/transport/websocket.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/transport/tls.rs Outdated Show resolved Hide resolved
Copy link
Owner

@rapiz1 rapiz1 left a 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(());
Copy link
Owner

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?

Copy link
Contributor Author

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.

@rapiz1 rapiz1 merged commit 4ac53a5 into rapiz1:main Feb 18, 2024
5 checks passed
@sunmy2019
Copy link
Contributor Author

I will add docs in the sequential PR.

@sunmy2019 sunmy2019 deleted the optional-rustls-support branch February 20, 2024 01:59
@skbeh
Copy link

skbeh commented Feb 21, 2024

@sunmy2019 The p12 crate is incompatible with pkcs12 cert generated by OpenSSL3: hjiayz/p12#13.

@sunmy2019
Copy link
Contributor Author

@sunmy2019 The p12 crate is incompatible with pkcs12 cert generated by OpenSSL3: hjiayz/p12#13.

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 p12) support. So we need to specify the PBE algorithm when creating the PKCS#12 archive.

In short, the command to create the PKCS#12 archive with rustls support is:

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

@skbeh
Copy link

skbeh commented Feb 21, 2024

@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.

@sunmy2019
Copy link
Contributor Author

@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?

@sunmy2019
Copy link
Contributor Author

sunmy2019 commented Feb 21, 2024

@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.

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 p12 and maintain our use case for it. There is a PR implementing what we need (hjiayz/p12#10), but no one is responding.

One strategy is to deprecate PKCS#12, and provide PCKS#8 options which is maintained by rustls itself.

@skbeh
Copy link

skbeh commented Mar 21, 2024

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.

@sunmy2019
Copy link
Contributor Author

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.

@skbeh
Copy link

skbeh commented Mar 24, 2024

@rapiz1 Also please update the document to use 3DES instead of RC2 (i.e. -keypbe PBE-SHA1-3DES -certpbe PBE-SHA1-3DES since 3DES provides not good but still acceptable encryption if someone ever wants it.

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.

3 participants