Repository navigation
feat: add opt-in AIA certificate chain fetching (tlsAIAFetch) - #6137
Conversation
Go's crypto/tls does not fetch missing intermediate certificates via the Authority Information Access (AIA) extension; browsers do. Servers that send only a leaf certificate and rely on AIA to fill in the chain cause k6 TLS handshakes to fail with "x509: certificate signed by unknown authority" even when the chain is technically valid. This commit adds a new `tlsAIAFetch` option (env: K6_TLS_AIA_FETCH) that enables AIA chain fetching on a per-test basis. When enabled, k6 intercepts failed chain verification, follows IssuingCertificateURL fields (BFS, up to 5 hops), retries verification with any fetched intermediates, and then restores hostname verification — exactly what browsers do. Implementation details: - lib/netext/aia.go: core WrapTLSConfigForAIAFetching function; uses a separate http.Client (not measured as test traffic) with a 10s timeout; process-level sync.Map cache so repeated handshakes pay the HTTP cost at most once per AIA URL; circular-reference protection via seen map - lib/options.go: new TLSAIAFetch null.Bool option (default off) - internal/js/runner.go: opt-in wiring — wraps the per-VU tls.Config when tlsAIAFetch: true; no-op when insecureSkipTLSVerify is also true - internal/js/modules/k6/grpc/client.go: fix buildTLSConfig to copy VerifyPeerCertificate and VerifyConnection callbacks from the parent config, so AIA verification is not silently bypassed for gRPC connections that supply explicit TLS options (cacerts/cert/key) Tests: - lib/netext/aia_test.go: 10 parallel unit tests covering happy path, complete chain passthrough, disabled/plain-Go regression, InsecureSkip- Verify passthrough, unreachable URL, fetch timeout, HTTP error, malformed cert, circular AIA references, and hostname-mismatch enforcement - internal/js/runner_aia_test.go: full VU integration test (Source and Archive runners) verifying the enabled/disabled toggle end-to-end Fixes #5941 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- aia.go: split var block so gochecknoglobals nolint directives are per-variable (golangci-lint v2 requires inline directives); extract buildVerifyPeerFn, verifyWithAIA, and buildVerifyConnFn to bring WrapTLSConfigForAIAFetching within the gocognit limit; fix makezero by using append instead of make+copy for the BFS queue; wrap the fetchAIAIntermediates signature to stay within the lll limit; remove unused //nolint:gosec on direct assignment (G402 only fires on struct literals); add //nolint:gosec to httpClient.Do for G704 (SSRF); add //nolint:contextcheck to runner.go call site (AIA fetches intentionally use context.Background inside the TLS callback) - aia_test.go: use closeBody helper for error-path tests (bodyclose); _, _ = fmt.Fprintln for errcheck; change nullLogger return type from *logrus.Logger to logrus.FieldLogger (forbidigo); add //nolint:noctx to net.Listen call; remove nolint directives that are unused because gosec/forcetypeassert are excluded for test files - runner_aia_test.go: remove unused //nolint:forcetypeassert - kill_other.go: remove obsolete // +build constraint (modernize) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Without this, tlsAIAFetch set in a script's options block would be silently dropped when k6 merges option sources (script + CLI flags + env vars), causing AIA fetching to never activate despite being configured. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
I ran again the end-to-end test chain in https://github.com/grafana/sm-k6-tls-test to ensure nothing changed against the previous PR, results are clean. |
|
@vtorosyan for me it seems ok, but to be sure I asked claude for review. Here is the output: Blocker — gRPC ignores user-supplied CA when
|
When the VU tls.Config is AIA-wrapped and a grpc.connect() call supplies its own cacerts, buildTLSConfig previously copied the parent's VerifyPeerCertificate/VerifyConnection callbacks. Those callbacks captured the VU config's RootCAs, so the gRPC-supplied cp was silently ignored and the handshake failed with "x509: certificate signed by unknown authority" even when the correct CA was provided. Instead, re-wrap the gRPC-local tls.Config with WrapTLSConfigForAIAFetching when AIA is enabled at the VU level. The new closure captures the gRPC config, so its RootCAs (cp) is used for chain verification. Adds a targeted test that reproduces the failure and verifies the fix.
Some CAs (notably Sectigo and others) serve intermediate certificates as PKCS#7/CMS bundles rather than raw DER or PEM. k6 does not currently parse that format, so the fetched intermediate is silently dropped and the chain remains incomplete — the user sees an "unknown authority" handshake failure with no clue that PKCS#7 was the cause. Detect PKCS#7 responses via the Content-Type header and by scanning for the ASN.1 signedData OID, and emit a Warn log pointing at the URL so users can diagnose the situation. Full PKCS#7 parser support is tracked as a follow-up.
|
@szkiba thank you for the feedback! I have pushed some changes:
Regression check: I also ran the existing TLS scenarios in my sm-k6-tls-test suite against the fixed binary - all still pass, confirming the HTTP AIA path is not regressed. |
| } | ||
| // Re-wrap so the verification closure reads this config's RootCAs, not the VU's. | ||
| if aiaEnabled { | ||
| tlsCfg = netext.WrapTLSConfigForAIAFetching(tlsCfg, logger, nil) |
There was a problem hiding this comment.
Here if the AIA is enabled we would've already wrtapped it and set the wrapper and InsecureSkipVeriryf.
Above we do copy the latter but not the former but this wrapper funciton checks the latter and doesn't do the wrapping. So this makes gRPC completely insecure.
Please do add a test that checks taht if gRPC certificates do not match and the AIA is enabled it will fail
There was a problem hiding this comment.
I concur, it's what I tried to explain here: https://github.com/grafana/k6/pull/6137/changes#r3621521110
There was a problem hiding this comment.
Thanks both, really good catch!
buildTLSConfig now takes insecureSkipVerify from state.Options.InsecureSkipTLSVerify.Bool (the user's actual intent) instead of parentConfig.InsecureSkipVerify (which the AIA wrapper had flipped to true internally, causing the re-wrap to short-circuit and leave verification off). See 9c0391b
Also added TestBuildTLSConfig_AIARejectsMismatchedCACert covering the scenario you described. I made sure to run the test before and it failed against the pre-fix code and passes now.
There was a problem hiding this comment.
I think that, when aiaEnabled is on, the VU's tls.Config reaches here with InsecureSkipVerify already set to true by the wrapper, so buildTLSConfig copies that into the fresh config, and the re-wrap sees it and early-returns.
Can you walk through what VerifyPeerCertificate ends up being on the returned config for a gRPC connection that passes cacerts, please?
| // Rebuild the AIA fetcher against the new options — Blacklist / BlockedHostnames / | ||
| // Hosts / Resolver may have changed and the fetcher's dialer captures them. | ||
| runnerDialer := &netext.Dialer{ | ||
| Dialer: r.BaseDialer, | ||
| Resolver: r.Resolver, | ||
| Blacklist: opts.BlacklistIPs, | ||
| BlockedHostnames: opts.BlockedHostnames.Trie, | ||
| Hosts: opts.Hosts.Trie, | ||
| } | ||
| r.aiaFetcher = netext.NewAIAFetcher(runnerDialer.DialContext) |
There was a problem hiding this comment.
I guess it's not critical, but this allocation could be gated behind:
if tlsAIAFetch := opts.TLSAIAFetch; tlsAIAFetch.Valid && tlsAIAFetch.Bool {
...
}
joanlopez
left a comment
There was a problem hiding this comment.
LGTM overall, one thing I want to double check though.
In buildVerifyConnFn, we skip verification when cs.ServerName == "", but looks like Go strips that to "" for IP-literal server names, as SNI can't carry IPs.
So for any bare-IP target, this just skips verification entirely.
Go doesn't seem to do that though, it verifies against the raw config.ServerName, and VerifyHostname does check IP literals against the cert's IP SANs, so it's a real check we'd be bypassing here.
Is skipping verification for IP targets on purpose, or should we be checking against the raw ServerName instead of cs.ServerName?
cc/ @vtorosyan @mstoykov
|
triage: waiting on @vtorosyan ideas on @joanlopez 's comment above before we move on with merging 🙇🏻 |
|
@joanlopez nice catch - I spent some time trying to assess the impact and the best way to move forward. So indeed, returning simply I have tried to properly fix it but it is getting messy... so I am proposing instead just to fail-close for empty ServerName + AIA. In practice the impact is that a user who wants tlsAIAFetch + a bare-IP target will get an error. So they have to either use a hostname, or disable tlsAIAFetch, or set insecureSkipTLSVerify. I think the impact of this is going to be small, Public CAs generally don't issue certs for IP addresses afaik. So this should be more of a corner case that affects a narrow slice of users. If you give me your +1 / -1, I will:
|
|
@vtorosyan I'm happy with your trade-off, I think it's a good balance 👍🏻 As you said, this will most likely only affect a narrow slice of users, and if that's not the case, we will hear about that, and we can always rework it. Happy to go with this, let me know once you're done with the actions you mentioned. Thanks! 🙇🏻 🙌🏻 |
For IP-literal targets Go strips the ServerName from ConnectionState (SNI can't carry IPs), so the AIA verify callback previously returned nil, accepting any valid CA-signed chain. Return an error instead so the handshake fails; users hitting bare-IP targets must disable tlsAIAFetch or set insecureSkipTLSVerify. Test suite updates dial via "localhost" so existing paths keep exercising real hostname verification, plus a new test covers the IP-literal rejection path.
|
@joanlopez a6dfed7 - thank you! |
The fail-close change in the AIA verify callback now rejects IP-literal targets. TestVUIntegrationAIAFetching dialed 127.0.0.1 with a matching IP SAN, which used to work but now hits the closed path. Switch the leaf's SAN and the dial address to "localhost" so SNI carries a ServerName the wrapper can verify.
Clarify when certificate fetching helps and its hostname requirement. Related: grafana/k6#6137
Original PR and review comments are here: #5942
What?
Go's crypto/tls does not fetch missing intermediate certificates via the Authority Information Access (AIA) extension; browsers do. Servers that send only a leaf certificate and rely on AIA to fill in the chain cause k6 TLS handshakes to fail with "x509: certificate signed by unknown authority" even when the chain is technically valid.
This commit adds a new
tlsAIAFetchoption (env: K6_TLS_AIA_FETCH) that enables AIA chain fetching on a per-test basis. When enabled, k6 intercepts failed chain verification, follows IssuingCertificateURL fields (BFS, up to 5 hops), retries verification with any fetched intermediates, and then restores hostname verification — exactly what browsers do.Implementation details:
Tests:
Checklist
make check) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)
Fixes #5941