-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 cipher tag to the serverFromString #12137
base: trunk
Are you sure you want to change the base?
Conversation
please review |
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.
Many thanks. The changes look good and are a good start.
Things blocking the merge:
- dedicated release notes fragment
- handling of non-string arguments
- lack of documentation
How are people expected to know about this feature ?
I was expected to see it documented in the docstring of serverFromString
similar to what we have for other rules https://docs.twistedmatrix.com/en/stable/api/twisted.internet.endpoints.html#serverFromString
there is also this documentation page https://docs.twisted.org/en/stable/core/howto/endpoints.html#endpoint-types-included-with-twisted
and I think that it would help to document this feature here.
It would be nice to have a followup for clientFromString ... but this needs to be done in a separate ticket/PR
We should focus on the serverFromString for this PR.
Thanks again
please review |
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.
Please get the tests passing and patch coverage to 100% before requesting another review so as to make efficient use of reviewer time; we can't integrate changes that don't pass our basic automated quality checks.
Co-authored-by: Glyph <code@glyph.im>
…to kaviharjani_12134
… and defaults to the latest
please review |
(Thanks a ton for getting those tests passing!) |
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
…to kaviharjani_12134
please review |
@adiroiban appreciate your reviews and apologise, I think I made most of the changes you requested except for the test one, not sure as you said custom handshake would be too much, and that is why I marked those as resolved, I added comments with reference to the change I have made changes and added an Exception, giving a better explanation of what is going on when cipher is wrong Please let me know if it is good to go Also, one more request @glyph @adiroiban if any one of you guys could help me with the patch, not sure why it says the code isn't covered and points to the uncovered test code |
if cipher: | ||
try: | ||
cipherBytes = cipher.replace(",", ":").encode("ascii") | ||
cf.getContext().set_cipher_list(cipherBytes) |
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.
I guess that this will work.
By understanding is that context factories might not cache the context.
This is why, we might need to change the API to something like this ... but this will break all existing context factory implementation.
So I guess that this is somehow ok for now.
We can refactor once someone nees to used this code with a custom context factory.
cf.setOpenSSLCipherList(cipherStr)
If we delegate the cipher list setting to the context factory, then we no longer need the TLS handshake tests for endpoints... we will just rely on the context factory tests.
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.
@adiroiban what do you suggest?
Should we go ahead with making changes to the API ?
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.
Thanks for the changes.
I don't think that the new test is correct... the assertion is never called.
Also, I am not sure that converting SSLError to a generic Exception is a good idea.
I think that at this point is best to just raise the low-level SSLError.... as we don't have any other better Twisted specific exception.
I am still trying to find a good reason for not writing a proper test for the happy path.
I think that we should use iosim here and do a TLS handshake, to check that the code works as expections.
We use a ContextFactory here...but the cipher is set on a specific context and not an the context factory itself.
Later in the code execution, the ContextFactory might return a different context instance, and then the cipher list is not actually used in the TLS handshake.
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
for more information, see https://pre-commit.ci
Hi @adiroiban I am having problems writing the test, I see there is a custom handshake done for
I tried using ServerFactory instead of ClientFactory but that did not work either Also with this change since it will not be cached to context, is there possibilities of the handshake to happen outside of the given cipher list |
You can start by writing an end to end test using the exact same steps and API as you would implement a TLS client->server connection outside of the testing code Just listening for a port will not trigger any TLS handshake. I think that you need to pass a real factory that uses a protocol that implements Then make a client connection to trigger the handshake and check the negotiated protocol... or check the protocol negotiation failure. The test can run over localhost with a randomly allocated port. |
Scope and purpose
Fixes #12134
Customising serverFromString further to handle ciphers as well,
Daphne extends the use case for twisted by the "-e" flag, this make
Daphe and Twisted more flexible, helping it add a static cipher to accept from
Futher detail here