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

Adding custom cipher list to serverFromString #12134

Open
KaviHarjani opened this issue Apr 24, 2024 · 5 comments · May be fixed by #12137
Open

Adding custom cipher list to serverFromString #12134

KaviHarjani opened this issue Apr 24, 2024 · 5 comments · May be fixed by #12137

Comments

@KaviHarjani
Copy link

Is your feature request related to a problem? Please describe.
I have to limit the cipher from which requests could be accepted even in Tls1.2 and I was able to do it by getting the context from 'ssl.DefaultOpenSslContextFactory'
And setting the option using set_cipher_list

Describe the solution you'd like
I would like to set that while using serverFromString or after or before that before running the reactor
Since Daphne is using serverFromString this would be a good addition

Describe alternatives you've considered
I have considered making my own server to serve it and limit the cipher list but will have to migrate from Daphne to twisted
And not sure if any functionality would break doing that

@adiroiban
Copy link
Member

adiroiban commented Apr 24, 2024

Thanks for the report.

I am not sure that serverFromString is designed to provide such a level of customization.

In all my use cases, I am using serverFromString only for quick testing code. Never in production, where it needs a high degree of configuration.


Can you suggest what would be the expected string that will include custom ciphers?

OpensSSL cipher configuration is something like this HIGH:!PSK:!RSP:!eNULL:!aNULL:!RC4:!MD5:!DES:!3DES:!aDH:!kDH:!DSS ... but we already use the colon (:) as a separator ... maybe use a comma

ssl:443:privateKey=key.pem:certKey=crt.pem:cipher=HIGH,!PSK,!DSS


Feel free to summit a PR with a possible solution for this.

Regards

@glyph
Copy link
Member

glyph commented Apr 24, 2024

I am not sure that serverFromString is designed to provide such a level of customization.

Design-wise, any static configuration like this should be doable from the Twisted-provided plugins for serverFromString. The one place where extra customization for serverFromString is out of scope would be things like per-connection state or changing parameters based on the peer; basically if you need to run Python code to figure out your parameters you need to use a different API, we shouldn't have Python functions to call stuffed into the string. But a static ciphersuite selection string is a perfectly reasonable parameter to have in there.

(I will say that for most users we should be providing good defaults and strongly encouraging usage of those defaults so we can evolve them to be up-to-date in terms of consensus security practices, but it sounds like @KaviHarjani has a very clear and specific idea of what they want here)

@KaviHarjani KaviHarjani linked a pull request Apr 24, 2024 that will close this issue
@KaviHarjani
Copy link
Author

Hey guys this is my first open-source contribution
Thank you for giving me this opportunity @glyph @adiroiban
I didn't make many changes but the tests seem to have been failing for Windows and Macos

Could you guys help me here

@glyph
Copy link
Member

glyph commented Apr 24, 2024

thank you @KaviHarjani !

@glyph
Copy link
Member

glyph commented Apr 24, 2024

Could you guys help me here

The failures that you are pointing at are not on Windows & macOS, they are failures on every platform that runs with SSL. The message, twisted.trial.unittest.FailTest: (<twisted.internet.endpoints.SSL4ServerEndpoint object at 0x7f8a4e0add00>,) is not an instance of <class 'twisted.internet.endpoints.SSL4ServerEndpoint'> , is showing you that the object you are asserting about is a 1-element tuple containing the endpoint rather than the endpoint itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants