fix: remove unnecessary RC4 restriction #5170
Conversation
| @pytest.mark.parametrize("cipher", ALL_TEST_CIPHERS, ids=get_parameter_name) | ||
| @pytest.mark.parametrize("curve", ALL_TEST_CURVES, ids=get_parameter_name) | ||
| @pytest.mark.parametrize("cipher", S2N_TEST_POLICIES, ids=get_parameter_name) | ||
| @pytest.mark.parametrize("certificate", ALL_TEST_CERTS, ids=get_parameter_name) | ||
| @pytest.mark.parametrize("protocol", RESUMPTION_PROTOCOLS, ids=get_parameter_name) |
There was a problem hiding this comment.
The S2N provider doesn't actually respect curve, most protocols, or most ciphers. It just always sets "test_all" or "test_all_tls12": https://github.com/lrstewart/s2n/blob/25b08ba9acd1745d85a10f56b8e1bd0a686a1683/tests/integrationv2/providers.py#L257-L263
By removing these parameters and setting actual policies, I'm just making the behavior more explicit.
| @@ -17,6 +17,11 @@ | |||
| SERVER_DATA = f"Some random data from the server:" + random_str(10) | |||
| CLIENT_DATA = f"Some random data from the client:" + random_str(10) | |||
|
|
|||
| S2N_TEST_POLICIES = { | |||
| Protocols.TLS12.value: Ciphers.SECURITY_POLICY_DEFAULT, | |||
There was a problem hiding this comment.
If we update the default policy to support TLS 1.3, I think we'd silently lose TLS 1.2 coverage here. Should the TLS 1.2 policy maybe be pinned? Or maybe assert that TLS 1.2 is negotiated in these tests?
There was a problem hiding this comment.
Hmmm yeah maybe using the security policy 20210816 is better here? That one doesn't have TLS1.3 cipher suites.
Release Summary:
Resolved issues:
Customers switching between awslc and awslc-fips noted the lack of RC4 as a difference that could cause issues, although they're unclear whether or not they actually need RC4 on awslc-fips.
Description of changes:
RC4 is very deprecated, but technically still supported by s2n-tls with the right policy. However, we marked the algorithm as unavailable in fips mode in lrstewart@3c88612, apparently because we ran into issues with the integration tests.
Our policy is that we try to support any algorithm that the libcrypto supports. Since awslc-fips supports RC4, let's turn RC4 back on when using awslc-fips.
Call-outs:
RC4 wouldn't be available from openssl-3.0-fips without additional work (its definitely not supported by the fips provider), but it's also not available from openssl-3.0 without additional work. We already have another check in is_available for cover the openssl-3.0 case, both fips and non-fips.
Testing:
The RC4 unit tests pass with awslc-fips. I've also run the happy path integration test locally with RC4 added to the ciphers list.
We already support RC4 with other libcryptos (minimally awslc, and I think openssl-1.0.2) but don't test it in the integ tests, so this change doesn't really create a new integ testing gap.
Failing cross-compatibility tests
Some of the integration tests were failing. Specifically, "test_cross_compatibility::test_s2n_old_client_new_ticket". The problem is that s2n_head (the old version of s2n) doesn't support RC4, but the new s2n does. The new s2n client and server can handshake with RC4 (and always do, since it's at the top of test_all_tls12 and test_all). But s2n_head can't use the ticket produced, so throws an error while attempting to set it on the connection. It is NOT a handshake failure.
For now, I switched "test_cross_compatibility" to use more reasonable policies, and to be more clear / explicit about what parameters it was actually testing. Instead of using test_all_tls12 and test_all regardless of the cipher/curve/protocol set, I removed those parameters and explicitly set default and default_tls13. We could add more policies if we want.
Is this a bigger problem we should address? Maybe? I would hope that clients are handling the possibility of errors when setting a ticket on a connection, and won't consider those errors fatal. Setting a ticket can also fail for an unknown ticket format if we updated the ticket format. I'm also not sure how we could avoid this possibility of an error, beyond simply silently ignoring the invalid ticket.
RC4 in the integration tests
We're still running into issues with the integration tests :) Although I think they're different issues this time.
In order to use RC4 with a provider other than S2N, openssl-1.1.1 has to be compiled with "enable-weak-ssl-ciphers". The version of openssl we use in our integration tests (both nix and codebuild image) is not compiled with "enable-weak-ssl-ciphers". To be clear, the problem is openssl as an integv2 libssl provider, not openssl as a libcrypto used by s2n-tls.
So if we want to test RC4 in the integration tests, we'll need to compile openssl-1.1.1 with "enable-weak-ssl-ciphers". Unfortunately, that will also apply to using openssl-1.1.1 as a libcrypto. I'm worried that most of our customers won't be using "enable-weak-ssl-ciphers", and therefore we could miss customer-impacting bugs in our testing if we configure ourselves differently. Alternatively we could have a separate openssl-1.1.1 build just for the integ tests, but I really don't think that's worthwhile.
I have moved the integration test changes to a separate branch: c3d9e65 I can open a separate issue to add that testing, if we think the tradeoffs are worthwhile.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.