Adding cipher tag to the serverFromString

[KaviHarjani]

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

[KaviHarjani]

please review

[adiroiban]

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

[KaviHarjani]

please review

[glyph]

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.

[KaviHarjani]

please review

[glyph]

(Thanks a ton for getting those tests passing!)

[KaviHarjani]

please review

[KaviHarjani]

@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

[adiroiban]

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.

[KaviHarjani]

@adiroiban what do you suggest?
Should we go ahead with making changes to the API ?

[adiroiban]

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.

[KaviHarjani]

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.

Hi @adiroiban I am having problems writing the test, I see there is a custom handshake done for clientFromString but I don't see any of those for serverFromString, so no reference point
I tried the implementation with WrappingFactoryTests but just hitting roadblocks there, could you give hints

    @skipIf(skipSSL, skipSSLReason)
    def test_sslWithCustomCipher(self):
        """
        A cipher list is supported, using the OpenSSL format.
        The colon (:) from the OpenSSL format is replaced with a comma (,).
        Limitations to the test -> Not checked the ciphers returned.
        """
        server = endpoints.serverFromString(
            reactor,
            "ssl:1234:privateKey=%s:"
            "certKey=%s:cipher=ALL,!ADH,@STRENGTH,+RSA,-DSA,SHA1+DES"
            % (escapedPEMPathName, escapedPEMPathName),
        )

        factory = object()
        d = server.listen(factory)

        receivedHosts = []

        def checkPortAndServer(port):
            receivedHosts.append(port.getHost())

        d.addCallback(checkPortAndServer)

        self.assertIsInstance(server, endpoints.SSL4ServerEndpoint)
        ctx = server._sslContextFactory.getContext()
        self.assertIsInstance(ctx, ContextType)

class WrappingFactoryTests(unittest.TestCase):
    """
    Test the behaviour of our ugly implementation detail C{_WrappingFactory}.
    """

    def test_doStart(self):
        """
        L{_WrappingFactory.doStart} passes through to the wrapped factory's
        C{doStart} method, allowing application-specific setup and logging.
        """
        factory = ClientFactory()
        wf = endpoints._WrappingFactory(factory)
        wf.doStart()
        self.assertEqual(1, factory.numPorts)

I tried using ServerFactory instead of ClientFactory but that did not work either
Could you please help me out?

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

[adiroiban]

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 IHandshakeListener

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.