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(tls): Add support for rustls ignore_client_order #2042

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

emuellen
Copy link

@emuellen emuellen commented Nov 8, 2024

Motivation

When running the TLS Connector with the https://testssl.sh/ testing tool, it showed that the server cipher order was not respected. This can be exploited by attacks such as POODLE. It would be good if we can instruct the TlsAdapter to enforce the server cipher order.

Solution

The change in this pull request adds an additional builder function to enable the "ignore_client_order" parameter of the RustTLS Server config. By default, this parameter is disabled to ensure backward compatibility.

@shikhar
Copy link
Contributor

shikhar commented Nov 8, 2024

rustls only supports TLS1.2 and TLS1.3, so it is not vulnerable to POODLE, AFAIU

@emuellen
Copy link
Author

emuellen commented Nov 8, 2024

Yes, it's correct that it's not vulnerable for this specific attack, but maybe something similar might happen in the future with other cipher suites (?).
Controlling the cipher suite order is considered best practice by our corporate security department and therefore enforced in our security baselines. Thus it would be good if we could incorporate this non-breaking change, so that I can check some boxes without forking tonic :-)

@emuellen emuellen changed the title Add support for rustls ignore_client_order feat: Add support for rustls ignore_client_order Nov 10, 2024
@emuellen emuellen changed the title feat: Add support for rustls ignore_client_order feat(tls): Add support for rustls ignore_client_order Nov 10, 2024
@@ -56,11 +58,24 @@ impl ServerTlsConfig {
}
}

/// Sets whether the server's cipher preferences are followed instead of the client's.
/// It prevents attacks such as POODLE
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that tonic should maintain the document about some specific use cases same as rustls does not do.

Copy link
Author

Choose a reason for hiding this comment

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

@tottoto , ok sure. Shall I just remove line 62?

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