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.

Sign up for GitHub

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

fix: remove unnecessary RC4 restriction #5170

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+27 −29
Open

fix: remove unnecessary RC4 restriction #5170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1a7d4a1
fix: remove unnecessary RC4 restriction
lrstewart Mar 7, 2025
ba58933
cross_compatibility should test more sanity policies
lrstewart Mar 7, 2025
9d460b4
fix ruff...?
lrstewart Mar 7, 2025
e4b3f55
address pr feedback
lrstewart Mar 11, 2025
9dcce87
fix last test?
lrstewart Mar 11, 2025
53a932b
missing include
lrstewart Mar 11, 2025
60e0142
ruff D:
lrstewart Mar 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions crypto/s2n_stream_cipher_rc4.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,6 @@ static const EVP_CIPHER *s2n_evp_rc4()

static bool s2n_stream_cipher_rc4_available(void)
{
if (s2n_is_in_fips_mode()) {
return false;
}
/* RC4 MIGHT be available in Openssl-3.0, depending on whether or not the
* "legacy" provider is loaded. However, for simplicity, assume that RC4
* is unavailable.
Expand Down
9 changes: 8 additions & 1 deletion tests/integrationv2/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ def __le__(self, other):
return self.value <= other.value

def __eq__(self, other):
return self.value == other.value
return other and self.value == other.value

def __str__(self):
return self.name
Expand Down Expand Up @@ -511,6 +511,13 @@ class Ciphers(object):
"PQ-TLS-1-3-2023-06-01", Protocols.TLS12, False, False, s2n=True, pq=True
)

SECURITY_POLICY_DEFAULT = Cipher(
"default", Protocols.TLS12, False, False, s2n=True, pq=False
)
SECURITY_POLICY_DEFAULT_TLS13 = Cipher(
"default_tls13", Protocols.TLS12, False, False, s2n=True, pq=False
)

SECURITY_POLICY_20210816 = Cipher(
"20210816", Protocols.TLS12, False, False, s2n=True, pq=False
)
Expand Down
23 changes: 8 additions & 15 deletions tests/integrationv2/test_cross_compatibility.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,16 @@
ALL_TEST_CURVES,
ALL_TEST_CERTS,
)
from common import ProviderOptions, Protocols, data_bytes
from common import Ciphers, ProviderOptions, Protocols, data_bytes
from fixtures import managed_process # lgtm [py/unused-import]
from providers import Provider, S2N, OpenSSL
from utils import invalid_test_parameters, get_parameter_name, to_bytes

S2N_TEST_POLICIES = [
Ciphers.SECURITY_POLICY_DEFAULT,
Ciphers.SECURITY_POLICY_DEFAULT_TLS13,
]

S2N_RESUMPTION_MARKER = to_bytes("Resumed session")
CLOSE_MARKER_BYTES = data_bytes(10)

Expand Down Expand Up @@ -177,19 +182,15 @@ def test_s2n_new_server_old_ticket(


@pytest.mark.uncollect_if(func=invalid_test_parameters)
@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)
Comment on lines -180 to -183
Copy link
Contributor Author

@lrstewart lrstewart Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

@pytest.mark.parametrize("provider", [S2N], ids=get_parameter_name)
@pytest.mark.parametrize("other_provider", [S2N], ids=get_parameter_name)
def test_s2n_old_client_new_ticket(
managed_process,
tmp_path,
cipher,
curve,
certificate,
protocol,
provider,
other_provider,
):
Expand All @@ -199,8 +200,6 @@ def test_s2n_old_client_new_ticket(
options = ProviderOptions(
port=next(available_ports),
cipher=cipher,
curve=curve,
protocol=protocol,
insecure=True,
use_session_ticket=True,
)
Expand Down Expand Up @@ -246,19 +245,15 @@ def test_s2n_old_client_new_ticket(


@pytest.mark.uncollect_if(func=invalid_test_parameters)
@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)
@pytest.mark.parametrize("provider", [S2N], ids=get_parameter_name)
@pytest.mark.parametrize("other_provider", [S2N], ids=get_parameter_name)
def test_s2n_new_client_old_ticket(
managed_process,
tmp_path,
cipher,
curve,
certificate,
protocol,
provider,
other_provider,
):
Expand All @@ -268,8 +263,6 @@ def test_s2n_new_client_old_ticket(
options = ProviderOptions(
port=next(available_ports),
cipher=cipher,
curve=curve,
protocol=protocol,
insecure=True,
use_session_ticket=True,
)
Expand Down
9 changes: 7 additions & 2 deletions tests/integrationv2/test_serialization.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from enum import Enum, auto

from configuration import available_ports
from common import ProviderOptions, Protocols, random_str
from common import Ciphers, ProviderOptions, Protocols, random_str
from fixtures import managed_process # lgtm [py/unused-import]
from providers import Provider, S2N
from utils import invalid_test_parameters, get_parameter_name, to_bytes
Expand All @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm yeah maybe using the security policy 20210816 is better here? That one doesn't have TLS1.3 cipher suites.

Protocols.TLS13.value: Ciphers.SECURITY_POLICY_DEFAULT_TLS13,
}


class MainlineRole(Enum):
Serialize = auto()
Expand Down Expand Up @@ -62,7 +67,7 @@ def test_server_serialization_backwards_compat(

options = ProviderOptions(
port=next(available_ports),
protocol=protocol,
cipher=S2N_TEST_POLICIES[protocol.value],
insecure=True,
)

Expand Down
12 changes: 4 additions & 8 deletions tests/unit/s2n_rc4_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "crypto/s2n_cipher.h"
#include "crypto/s2n_fips.h"
#include "crypto/s2n_hmac.h"
#include "crypto/s2n_libcrypto.h"
#include "crypto/s2n_openssl.h"
#include "s2n_test.h"
#include "stuffer/s2n_stuffer.h"
Expand All @@ -38,9 +39,9 @@ int main(int argc, char **argv)
EXPECT_FALSE(s2n_rc4.is_available());
}

/* Test FIPS does not support RC4 */
if (s2n_is_in_fips_mode()) {
EXPECT_FALSE(s2n_rc4.is_available());
/* awslc does support RC4 */
if (s2n_libcrypto_is_awslc()) {
EXPECT_TRUE(s2n_rc4.is_available());
}

struct s2n_connection *conn = NULL;
Expand All @@ -54,11 +55,6 @@ int main(int argc, char **argv)

EXPECT_SUCCESS(s2n_disable_tls13_in_test());

if (s2n_is_in_fips_mode()) {
/* Skip when FIPS mode is set as FIPS mode does not support RC4 */
END_TEST();
}

EXPECT_NOT_NULL(conn = s2n_connection_new(S2N_SERVER));
EXPECT_OK(s2n_get_public_random_data(&r));

Expand Down
Loading