fix: rectify ciphers compliance check by damdo · Pull Request #41 · openshift/tls-scanner

damdo · 2026-04-02T10:19:15Z

fix: complete IANA-to-OpenSSL cipher map and fix incorrect mappings
The IanaCipherToOpenSSLCipherMap had two problems preventing cipher compliance from passing:
- Several mappings pointed to wrong OpenSSL names (e.g. TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 mapped to TLS_AES_128_GCM_SHA256 instead of ECDHE-RSA-AES128-GCM-SHA256), and identity entries (OpenSSL -> OpenSSL) not aligned with the IANA-to-OpenSSL purpose.
- Ciphers from OpenShift TLS profile (Old, Intermediate, Modern) were missing IANA entries, so any endpoint offering those ciphers would fail compliance when reported in IANA format.
  Clean the map to contain only genuine IANA -> OpenSSL translations covering all ciphers across all three profiles.
  In checkCipherCompliance, fall back to using the cipher name as-is when no IANA mapping exists (testssl.sh already reports in OpenSSL format via FS_ECDHE),
  and log a warning when a cipher is not found in the expected set.
fix: populate TlsCiphers from testssl.sh key exchange data
Cipher compliance (ciphers field in *_tls_config_compliance) always returned false
because TlsCiphers was never populated after the nmap-to-testssl.sh migration.
The cipher data was successfully extracted into TlsKeyExchange.ForwardSecrecy.ECDHE
but never wired back to TlsCiphers, so checkCipherCompliance always received an empty slice.
Wire TlsCiphers from ForwardSecrecy.ECDHE in batchScan and remove
the dead TlsCipherStrength field (nmap leftover, never populated by testssl.sh).
fix: extract ciphers from cipher-tls1_* findings instead of FS_ECDHE
TlsCiphers was populated from FS_ECDHE which only contains TLS 1.2
forward-secrecy ciphers. TLS 1.3 ciphers and non-FS suites (e.g.
static RSA key exchange) were silently excluded from compliance checks.
Add ExtractCiphersFromTestSSL which parses cipher-tls1_2_* and
cipher-tls1_3_* findings directly, capturing every cipher the server
offers regardless of key exchange type. Replace the FS_ECDHE workaround
in batchScan with this new extraction.

Fixes #39

damdo · 2026-04-02T10:19:45Z

/assign @richardsonnick @smith-xyz @rhmdnd

smith-xyz · 2026-04-02T15:09:32Z

@damdo see my comments - nothing really blocking we could merge it in as is, seems to fix the issue

damdo · 2026-04-03T12:38:15Z

/retest

ingvagabund · 2026-04-07T16:12:32Z

Checking the tls-scanner artifacts from the latest CI job in https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_tls-scanner/41/pull-ci-openshift-tls-scanner-main-tls13-conformance/2040046236580450304/artifacts/tls13-conformance/tls-scanner-run/artifacts/tls-scanner/results.json I still don't see tls_ciphers field set in the port_results map. Is the job testing the latest changes? I'd expect it to be set after 1e40b80#diff-95e0a286796962f784af743a32471dada7258c4789b441aaca0a77653e3c1155R362 change. Given the produced json has e.g. this (for openshift-kube-scheduler-operator-868d647944-cn4vf pod):

      "port_results": [
        {
          ...
          "tls_versions": [
            "TLSv1.2",
            "TLSv1.3"
          ],
          "tls_key_exchange": {
            "forward_secrecy": {
              "supported": true,
              "ecdhe": [
                "TLS_AES_256_GCM_SHA384",
                "TLS_CHACHA20_POLY1305_SHA256",
                "ECDHE-RSA-AES256-GCM-SHA384",
                "ECDHE-RSA-CHACHA20-POLY1305",
                "TLS_AES_128_GCM_SHA256",
                "ECDHE-RSA-AES128-GCM-SHA256"
              ],
              ...
            }
          },
          ...
        }
      ],
      ...
      "pod": {
        "Name": "openshift-kube-scheduler-operator-868d647944-cn4vf",

tls_key_exchange.forward_secrecy.ecdhe has non-empty list. So 1e40b80#diff-95e0a286796962f784af743a32471dada7258c4789b441aaca0a77653e3c1155R362 change is expected to produce non-empty list for tls_ciphers.

ingvagabund · 2026-04-08T12:28:54Z

Running a patched version on top of this PR with --namespace-filter=openshift-kube-scheduler-operator added into the job template. The following findings is produced:

[{"finding":"10.129.0.37:8443 appears to support TLS 1.3 ONLY, switching from timeout --preserve-status 5 /opt/testssl/bin/openssl.Linux.x86_64 to /usr/bin/openssl automagically","id":"optimal_proto","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"INFO"},{"finding":"No engine or GOST support via engine with your /usr/bin/openssl","id":"engine_problem","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"WARN"},{"finding":"HTTP","id":"service","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"INFO"},{"finding":"No 128 cipher limit bug","id":"pre_128cipher","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"INFO"},{"finding":"not offered","id":"SSLv2","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"OK"},{"finding":"not offered","id":"SSLv3","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"OK"},{"finding":"not offered","id":"TLS1","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"INFO"},{"finding":"not offered","id":"TLS1_1","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"INFO"},{"finding":"not offered","id":"TLS1_2","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"INFO"},{"finding":"offered with final","id":"TLS1_3","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"OK"},{"finding":"not possible for TLS 1.3-only hosts","id":"NPN","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"WARN"},{"finding":"http/1.1","id":"ALPN","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"INFO"},{"cwe":"CWE-327","finding":"not offered","id":"cipherlist_NULL","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"OK"},{"cwe":"CWE-327","finding":"not offered","id":"cipherlist_aNULL","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"OK"},{"cwe":"CWE-327","finding":"not offered","id":"cipherlist_EXPORT","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"OK"},{"cwe":"CWE-327","finding":"not offered","id":"cipherlist_LOW","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"OK"},{"cwe":"CWE-310","finding":"not offered","id":"cipherlist_3DES_IDEA","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"INFO"},{"cwe":"CWE-310","finding":"not offered","id":"cipherlist_OBSOLETED","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"INFO"},{"finding":"not offered","id":"cipherlist_STRONG_NOFS","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"INFO"},{"finding":"offered","id":"cipherlist_STRONG_FS","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"OK"},{"finding":"offered","id":"FS","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"OK"},{"finding":"TLS_AES_256_GCM_SHA384 TLS_CHACHA20_POLY1305_SHA256 TLS_AES_128_GCM_SHA256","id":"FS_ciphers","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"INFO"},{"finding":"X25519MLKEM768","id":"FS_KEMs","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"OK"},{"finding":"prime256v1 secp384r1 secp521r1 X25519","id":"FS_ECDHE_curves","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"OK"},{"finding":"RSA-PSS-RSAE+SHA512 RSA-PSS-RSAE+SHA384 RSA-PSS-RSAE+SHA256","id":"FS_TLS13_sig_algs","ip":"10.129.0.37/10.129.0.37","port":"8443","severity":"INFO"}]

Following the changes in ExtractCiphersFromTestSSL there's unfortunately no id prefixed with cipher-tls1_ key. Thus, the list of ciphers is always empty.

EDIT: #41 (comment) has more details about how to make the mentioned findings appear.

damdo · 2026-04-14T15:31:33Z

Hey @ingvagabund I've added your commit from #41 here.

damdo · 2026-04-16T16:09:46Z

/assign @richardsonnick @smith-xyz @ingvagabund

smith-xyz · 2026-04-16T17:24:18Z

/lgtm
/approve
/hold for @richardsonnick

openshift-ci · 2026-04-16T17:25:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damdo, smith-xyz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [smith-xyz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ingvagabund · 2026-04-17T09:02:22Z

LGTM

richardsonnick · 2026-04-17T14:56:19Z

/unhold
/lgtm

richardsonnick · 2026-04-17T14:57:25Z

/override ci/prow/tls13-conformance

openshift-ci · 2026-04-17T14:59:54Z

@richardsonnick: Overrode contexts on behalf of richardsonnick: ci/prow/tls13-conformance

Details

In response to this:

/override ci/prow/tls13-conformance

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

damdo · 2026-04-17T15:31:01Z

/test unit

Cipher compliance (ciphers field in *_tls_config_compliance) always returned false because TlsCiphers was never populated after the nmap-to-testssl.sh migration. The cipher data was successfully extracted into TlsKeyExchange.ForwardSecrecy.ECDHE but never wired back to TlsCiphers, so checkCipherCompliance always received an empty slice. Wire TlsCiphers from ForwardSecrecy.ECDHE in batchScan and remove the dead TlsCipherStrength field (nmap leftover, never populated by testssl.sh). Fixes openshift#39

The IanaCipherToOpenSSLCipherMap had two problems preventing cipher compliance from passing: 1. Several mappings pointed to wrong OpenSSL names (e.g. TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 mapped to TLS_AES_128_GCM_SHA256 instead of ECDHE-RSA-AES128-GCM-SHA256), and identity entries (OpenSSL -> OpenSSL) not aligned with the IANA-to-OpenSSL purpose. 2. Ciphers from every OpenShift TLS profile (Old, Intermediate, Modern) were missing IANA entries, so any endpoint offering those ciphers would fail compliance when reported in IANA format. Clean the map to contain only genuine IANA -> OpenSSL translations covering all ciphers across all three profiles. In checkCipherCompliance, fall back to using the cipher name as-is when no IANA mapping exists (testssl.sh already reports in OpenSSL format via FS_ECDHE), and log a warning when a cipher is not found in the expected set. Fixes openshift#39

TlsCiphers was populated from FS_ECDHE which only contains TLS 1.2 forward-secrecy ciphers. TLS 1.3 ciphers and non-FS suites (e.g. static RSA key exchange) were silently excluded from compliance checks. Add ExtractCiphersFromTestSSL which parses cipher-tls1_2_* and cipher-tls1_3_* findings directly, capturing every cipher the server offers regardless of key exchange type. Replace the FS_ECDHE workaround in batchScan with this new extraction.

openshift-merge-bot · 2026-04-17T16:07:28Z

/retest-required

Remaining retests: 0 against base HEAD 827d142 and 2 for PR HEAD a7e7552 in total

…SL + cipher map

smith-xyz · 2026-04-17T19:52:42Z

/lgtm

openshift-ci Bot assigned rhmdnd, richardsonnick and smith-xyz Apr 2, 2026

openshift-ci Bot requested review from rhmdnd and richardsonnick April 2, 2026 10:19

smith-xyz reviewed Apr 2, 2026

View reviewed changes

Comment thread internal/scanner/scanner.go Outdated

smith-xyz reviewed Apr 2, 2026

View reviewed changes

Comment thread internal/scanner/scanner.go Outdated

smith-xyz reviewed Apr 2, 2026

View reviewed changes

Comment thread internal/scanner/testssl.go Outdated

damdo force-pushed the fix-39 branch from 45d931d to 0f803ad Compare April 2, 2026 18:59

damdo requested a review from smith-xyz April 2, 2026 19:01

ingvagabund mentioned this pull request Apr 8, 2026

wip: 41 debug #42

Closed

smith-xyz mentioned this pull request Apr 16, 2026

Add cipher per proto flag #43

Closed

openshift-ci Bot assigned ingvagabund Apr 16, 2026

openshift-ci Bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Apr 16, 2026

smith-xyz approved these changes Apr 16, 2026

View reviewed changes

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 16, 2026

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2026

damdo and others added 4 commits April 17, 2026 18:00

fix: run testssl.sh with -E and extract the ciphers from the findings

5502f66

damdo force-pushed the fix-39 branch from a7e7552 to 22ee3e3 Compare April 17, 2026 19:18

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2026

test: adjust existing tests, add coverage for ExtractCiphersFromTestS…

819bbd2

…SL + cipher map

damdo force-pushed the fix-39 branch from 22ee3e3 to 819bbd2 Compare April 17, 2026 19:18

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2026

openshift-merge-bot Bot merged commit 199e60b into openshift:main Apr 17, 2026
3 of 5 checks passed

qodo-for-rh-openshift Bot mentioned this pull request May 20, 2026

fix(k8s): use net.SplitHostPort for lsof IPv6 parsing and preserve spec-declared ports #69

Merged

qodo-for-rh-openshift Bot mentioned this pull request Jul 23, 2026

DPTP-5090: Add unsigned TLS posture attestation emitter with component rollup #84

Open

Uh oh!

Conversation

damdo commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damdo commented Apr 2, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smith-xyz commented Apr 2, 2026

Uh oh!

damdo commented Apr 3, 2026

Uh oh!

ingvagabund commented Apr 7, 2026

Uh oh!

ingvagabund commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damdo commented Apr 14, 2026

Uh oh!

damdo commented Apr 16, 2026

Uh oh!

smith-xyz commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci Bot commented Apr 16, 2026

Uh oh!

ingvagabund commented Apr 17, 2026

Uh oh!

richardsonnick commented Apr 17, 2026

Uh oh!

richardsonnick commented Apr 17, 2026

Uh oh!

openshift-ci Bot commented Apr 17, 2026

Uh oh!

damdo commented Apr 17, 2026

Uh oh!

openshift-merge-bot Bot commented Apr 17, 2026

Uh oh!

smith-xyz commented Apr 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

damdo commented Apr 2, 2026 •

edited

Loading

ingvagabund commented Apr 8, 2026 •

edited

Loading

smith-xyz commented Apr 16, 2026 •

edited

Loading