Copilot commented on code in PR #13723:
URL: https://github.com/apache/trafficserver/pull/13723#discussion_r4085274689
##########
tests/gold_tests/tls/tls_sni_groups.test.py:
##########
@@ -20,7 +20,9 @@
Test SNI configuration server_groups_list
'''
# The groups function was added in OpenSSL 1.1.1
-Test.SkipUnless(Condition.HasOpenSSLVersion("1.1.1"))
+Test.SkipUnless(
+ Condition.HasOpenSSLVersion("1.1.1"),
+ Condition.HasProgram("openssl", "openssl needs to be installed and in PATH
for this test"))
Review Comment:
The test now depends on the `openssl` *executable* (its supported flags and
its output format), but the gating condition is `HasOpenSSLVersion(\"1.1.1\")`,
which may reflect a different OpenSSL (e.g., the library Traffic Server is
linked with) than the `openssl` binary found in `PATH`. This can lead to
running a test that the local `openssl s_client` cannot execute/parse (notably
around TLS 1.3 features and 3.5+ output changes). Prefer gating based on the
`openssl` CLI version (e.g., by probing `openssl version`) or add a dedicated
condition that asserts the `openssl` program meets the minimum version needed
for the specific assertions you parse.
##########
tests/gold_tests/tls/tls_sni_groups.test.py:
##########
@@ -74,16 +76,17 @@
tr = Test.AddTestRun("Test 0: x25519")
tr.Processes.Default.StartBefore(server)
tr.Processes.Default.StartBefore(Test.Processes.ts)
-tr.MakeCurlCommand(
- "-v --ciphers ECDHE-RSA-AES256-GCM-SHA384 --resolve
'bbb.com:{0}:127.0.0.1' -k https://bbb.com:{0}".format(
- ts.Variables.ssl_port),
- ts=ts)
+tr.Processes.Default.Command = (
+ 'printf "GET / HTTP/1.1\\r\\nHost: bbb.com\\r\\nConnection:
close\\r\\n\\r\\n" | '
+ 'openssl s_client -connect 127.0.0.1:{0} -servername bbb.com -tls1_2 '
+ '-cipher ECDHE-RSA-AES256-GCM-SHA384
-ign_eof'.format(ts.Variables.ssl_port))
Review Comment:
This relies on a shell pipeline (`printf ... | openssl s_client ...`). If
the test runner executes commands without going through a shell (or uses a
non-POSIX shell), the pipe and quoting won’t work. If the harness supports it,
prefer passing an argv-style command and feeding the HTTP request via the
process stdin (or using a small helper script) to avoid shell-dependent
behavior.
##########
tests/gold_tests/tls/tls_sni_groups.test.py:
##########
@@ -74,16 +76,17 @@
tr = Test.AddTestRun("Test 0: x25519")
tr.Processes.Default.StartBefore(server)
tr.Processes.Default.StartBefore(Test.Processes.ts)
-tr.MakeCurlCommand(
- "-v --ciphers ECDHE-RSA-AES256-GCM-SHA384 --resolve
'bbb.com:{0}:127.0.0.1' -k https://bbb.com:{0}".format(
- ts.Variables.ssl_port),
- ts=ts)
+tr.Processes.Default.Command = (
+ 'printf "GET / HTTP/1.1\\r\\nHost: bbb.com\\r\\nConnection:
close\\r\\n\\r\\n" | '
+ 'openssl s_client -connect 127.0.0.1:{0} -servername bbb.com -tls1_2 '
+ '-cipher ECDHE-RSA-AES256-GCM-SHA384
-ign_eof'.format(ts.Variables.ssl_port))
tr.ReturnCode = 0
tr.StillRunningAfter = ts
ts.Disk.traffic_out.Content += Testers.ContainsExpression(
"Setting groups list from server_groups_list to x25519", "Should log
setting the server groups")
+# OpenSSL renamed this line from "Server Temp Key" to "Peer Temp Key" in 3.5.
tr.Processes.Default.Streams.All = Testers.IncludesExpression(
- f"SSL connection using TLSv1.2 / ECDHE-RSA-AES256-GCM-SHA384 / x25519",
"Curl should log using x25519 in the SSL connection")
+ r"(Server|Peer) Temp Key: X25519", "the key exchange should use the x25519
group this SNI pins")
Review Comment:
This match is potentially too strict for some OpenSSL `s_client` variants
that include a legacy key type prefix in the temp key line (e.g., `Server Temp
Key: ECDH, X25519, ...`). Consider broadening the expression to allow an
optional `ECDH, ` (and/or other prefixes OpenSSL emits) between `Temp Key:` and
the group name so the test doesn’t false-fail across OpenSSL patch-level
differences.
##########
tests/gold_tests/tls/tls_sni_groups.test.py:
##########
@@ -100,14 +103,16 @@
# Hybrid ECDH PQ key exchange TLS groups were added in OpenSSL 3.5
if Condition.HasOpenSSLVersion("3.5.0"):
tr = Test.AddTestRun("Test 2: X25519MLKEM768")
- tr.MakeCurlCommand(
- "-v --tls13-ciphers TLS_AES_256_GCM_SHA384 --resolve
'aaa.com:{0}:127.0.0.1' -k https://aaa.com:{0}".format(
- ts.Variables.ssl_port),
- ts=ts)
+ tr.Processes.Default.Command = (
+ 'printf "GET / HTTP/1.1\\r\\nHost: aaa.com\\r\\nConnection:
close\\r\\n\\r\\n" | '
+ 'openssl s_client -connect 127.0.0.1:{0} -servername aaa.com -tls1_3 '
+ '-ciphersuites TLS_AES_256_GCM_SHA384
-ign_eof'.format(ts.Variables.ssl_port))
Review Comment:
The HTTP request text and most of the `openssl s_client` invocation are
duplicated between Test 0 and Test 2. To keep future updates (headers, flags,
timeouts, parsing) consistent, consider factoring the request string and/or the
common `s_client` command construction into a small helper (local function or
shared string template) and only vary host, tls version, and cipher/ciphersuite
per run.
--
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]
