Copilot commented on code in PR #12857:
URL: https://github.com/apache/trafficserver/pull/12857#discussion_r2765481408
##########
tests/gold_tests/autest-site/conditions.test.ext:
##########
@@ -51,6 +52,51 @@ def IsOpenSSL(self):
"SSL library is not OpenSSL")
+def HasLegacyTLSSupport(self):
+ """Check if the system supports legacy TLS protocols (TLSv1.0 and TLSv1.1).
+
+ Modern OpenSSL 3.x installations often disable these protocols entirely,
+ even if the openssl binary still accepts the -tls1 flag and lists TLSv1
ciphers.
+
+ On Fedora/RHEL systems, the crypto-policies framework may disable legacy
+ TLS at runtime even when OpenSSL is compiled with support for it. This
+ causes 'openssl ciphers -v -tls1' to still list TLSv1 ciphers, but actual
+ TLS 1.0 connections will fail with "no protocols available".
+
+ This check attempts to create an actual TLSv1 connection to a known HTTPS
+ server to detect if the protocol is truly available at runtime.
+ """
+
+ def check_tls1_support():
+ try:
+ # Try to actually use TLSv1 against a real HTTPS server
+ # This catches crypto-policy restrictions that aren't visible in
cipher listings
+ # We use a well-known server that's likely to be reachable
+ result = subprocess.run(
+ ['openssl', 's_client', '-tls1', '-connect',
'www.google.com:443'],
+ capture_output=True,
+ text=True,
+ timeout=10,
+ input='' # Don't wait for input
+ )
+ # Combine stdout and stderr for checking
+ output = result.stdout + result.stderr
+ # If we get "no protocols available", TLSv1 is disabled by policy
+ if 'no protocols available' in output:
+ return False
+ # If we get a successful connection or a server-side TLS version
mismatch,
+ # it means TLSv1 is enabled on this client
+ return True
+ except subprocess.TimeoutExpired:
+ # If network is slow but TLSv1 was attempted, assume it's available
+ return True
+ except Exception:
+ # If we can't determine, assume TLSv1 is not available (safer)
+ return False
Review Comment:
`HasLegacyTLSSupport` shells out to `openssl s_client` against
`www.google.com:443`, which makes the test outcome depend on external
network/DNS availability and the remote server’s configuration. In
offline/locked-down CI this can lead to long timeouts or incorrect results
(especially since `TimeoutExpired` currently returns True). Prefer an
offline/local capability check (e.g., attempt to create a TLSv1/TLSv1.1 context
with the `ssl` module, or run `openssl s_client -tls1` against localhost and
treat connection-refused as "protocol available" but policy-disabled errors as
unsupported), and treat timeouts as "unsupported" to avoid false positives.
##########
tests/gold_tests/tls/tls_client_versions.test.py:
##########
@@ -24,6 +24,7 @@
# for special domain foo.com only offer TLSv1 and TLSv1_1
Test.SkipUnless(Condition.HasOpenSSLVersion("1.1.1"))
+Test.SkipUnless(Condition.HasLegacyTLSSupport(), "This test requires
TLSv1.0/TLSv1.1 support which is disabled on modern systems")
Review Comment:
This `SkipUnless` call passes a second positional string argument. In this
repository, `Test.SkipUnless(...)` is only used with `Condition` objects
(optionally multiple), not a separate reason string (see e.g.
tests/README.md:328-331). If `SkipUnless` doesn’t accept a reason parameter,
this will raise at runtime or be treated as an invalid condition. Consider
relying on `Condition.HasLegacyTLSSupport()`’s own failure message (or extend
the condition message) instead of passing a raw string here.
```suggestion
Test.SkipUnless(Condition.HasLegacyTLSSupport())
```
##########
tests/gold_tests/tls/tls_client_versions_minmax.test.py:
##########
@@ -24,6 +24,7 @@
# for special domain foo.com only offer TLSv1 and TLSv1_1
Test.SkipUnless(Condition.HasOpenSSLVersion("1.1.1"))
+Test.SkipUnless(Condition.HasLegacyTLSSupport(), "This test requires
TLSv1.0/TLSv1.1 support which is disabled on modern systems")
Review Comment:
This `SkipUnless` call passes a second positional string argument. In this
repository, `Test.SkipUnless(...)` is only used with `Condition` objects
(optionally multiple), not a separate reason string (see e.g.
tests/README.md:328-331). If `SkipUnless` doesn’t accept a reason parameter,
this will raise at runtime or be treated as an invalid condition. Consider
relying on `Condition.HasLegacyTLSSupport()`’s own failure message (or extend
the condition message) instead of passing a raw string here.
```suggestion
Test.SkipUnless(Condition.HasLegacyTLSSupport())
```
##########
tests/gold_tests/autest-site/conditions.test.ext:
##########
@@ -51,6 +52,51 @@ def IsOpenSSL(self):
"SSL library is not OpenSSL")
+def HasLegacyTLSSupport(self):
+ """Check if the system supports legacy TLS protocols (TLSv1.0 and TLSv1.1).
+
+ Modern OpenSSL 3.x installations often disable these protocols entirely,
+ even if the openssl binary still accepts the -tls1 flag and lists TLSv1
ciphers.
+
+ On Fedora/RHEL systems, the crypto-policies framework may disable legacy
+ TLS at runtime even when OpenSSL is compiled with support for it. This
+ causes 'openssl ciphers -v -tls1' to still list TLSv1 ciphers, but actual
+ TLS 1.0 connections will fail with "no protocols available".
+
+ This check attempts to create an actual TLSv1 connection to a known HTTPS
+ server to detect if the protocol is truly available at runtime.
+ """
+
+ def check_tls1_support():
+ try:
+ # Try to actually use TLSv1 against a real HTTPS server
+ # This catches crypto-policy restrictions that aren't visible in
cipher listings
+ # We use a well-known server that's likely to be reachable
+ result = subprocess.run(
+ ['openssl', 's_client', '-tls1', '-connect',
'www.google.com:443'],
+ capture_output=True,
+ text=True,
+ timeout=10,
+ input='' # Don't wait for input
+ )
+ # Combine stdout and stderr for checking
+ output = result.stdout + result.stderr
+ # If we get "no protocols available", TLSv1 is disabled by policy
+ if 'no protocols available' in output:
+ return False
+ # If we get a successful connection or a server-side TLS version
mismatch,
+ # it means TLSv1 is enabled on this client
+ return True
Review Comment:
This condition claims to check TLSv1.0 *and* TLSv1.1 support, but it only
probes `-tls1`. Either also probe `-tls1_1` (and require both, if that’s what
the tests need) or rename/update the messaging so it matches what is actually
being detected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
