Fix AIA chasing: convert DER intermediate certs to PEM - #24683
Open
ian28223 wants to merge 7 commits into
Open
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 97934c6 | Docs | Datadog PR Page | Give us feedback! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bafc3a4599
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
CA Issuers URIs conventionally serve DER-encoded certificates (RFC 5280), but `TlsConfig.tls_intermediate_ca_certs` and `ssl.SSLContext.load_verify_locations` both expect PEM strings. Raw DER bytes were stored directly, causing a pydantic ValidationError whenever AIA chasing fetched an intermediate cert. The fetched cert is now parsed as DER first, falling back to PEM if that fails, then converted to PEM before storage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ian28223
force-pushed
the
ian.bucad/fix-aia-chasing-der-to-pem
branch
from
July 24, 2026 07:59
bafc3a4 to
8307db1
Compare
HadhemiDD
requested changes
Jul 31, 2026
Addresses review feedback with comments rather than behavior changes: load_x509_certificate only parses the first cert in a PEM bundle, and a failed intermediate is skipped rather than aborting the whole chase. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…is already known CA Issuers URIs conventionally serve a single DER cert, but some issuers return a PEM bundle with multiple certificates (e.g. a cross-signed cert plus its own issuer). load_x509_certificates now returns every certificate found instead of only the first, and AIA chasing for a given certificate is skipped once its issuer is already present among the certs collected so far, avoiding redundant fetches (e.g. for a self-signed root already included in the same bundle). This supersedes the single-cert docstring caveat and the partial-failure comment added in response to prior review feedback, since both certs from a bundle are now retained and chasing naturally stops once the chain is locally complete. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Every log line in the AIA chasing path is now prefixed "AIA chasing:" and includes the relevant hostname/URI/cert subject, so it's clear from logs alone whether chasing was attempted and where it failed. The retried request (after mounting recovered certs) previously had no logging at all if it still failed with SSLError -- that path is now caught, logged, and re-raised so a still-incomplete chain doesn't surface as a bare, unexplained SSLError. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Validation ReportAll 21 validations passed. Show details
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Converts intermediate CA certificates fetched during AIA (Authority Information Access) chasing to PEM before storing them in
tls_intermediate_ca_certs. The fetched response is parsed as DER first (the RFC 5280 convention for CA Issuers responses), falling back to a PEM parse if that fails, since some issuers serve PEM directly — including PEM bundles containing multiple certificates (e.g. a cross-signed cert alongside its own issuer). All certificates found are recorded, and AIA chasing for a given certificate is skipped once its issuer is already present among the certs collected so far, avoiding redundant network fetches (e.g. for a self-signed root already included in the same bundle).Motivation
CA Issuers URIs conventionally serve certificates DER-encoded, but
TlsConfig.tls_intermediate_ca_certs(atuple[str, ...]) andssl.SSLContext.load_verify_locations(cadata=...)both expect PEM strings. When AIA chasing fetched a DER-encoded intermediate cert, the raw bytes were stored unmodified, causing apydantic_core.ValidationError("unable to parse raw data as a unicode string") whenever an HTTPS target relied on AIA chasing to complete an incomplete certificate chain. This was surfaced via a customer flare from thenutanixintegration (AGENT-16632), but the bug is in the sharedRequestsWrapperused by all integrations.Detecting the format via a parse attempt (DER, then PEM on failure) is more reliable than sniffing content for a
-----BEGINprefix, since only an actual parse confirms the data is valid — and ordering DER first keeps the common case on the fast path.Fix verified locally against a repro environment: a TLS server presenting an incomplete chain (leaf only) plus a plain-HTTP endpoint serving the DER-encoded intermediate at the leaf's AIA URI, confirming the exact
ValidationErrorbefore the fix and a successful request after.Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged