tls: expose issuer cert hash and serial from peer certificate chain by stias · Pull Request #43723 · envoyproxy/envoy

stias · 2026-03-03T10:02:20Z

Add issuerPeerCertificateHash() and issuerPeerCertificateSerial() to the SSL connection info stack, exposing SHA256 fingerprint and serial number of the direct issuer (CA) certificate from the peer's TLS certificate chain.

Changes:

envoy/ssl/connection.h: add pure virtual declarations
source/common/tls/connection_info_impl_base: implement via SSL_get_peer_full_cert_chain + sk_X509_value(chain, 1)
source/common/quic/quic_ssl_connection_info.h: stub returning
source/common/tls/utility: add getSha256DigestFromCertificate() and getSha1DigestFromCertificate() helpers; refactor existing callers
source/common/tls/BUILD: add hex_lib dependency to utility_lib
source/extensions/filters/common/lua/wrappers: expose as issuerPeerCertificateHash() and issuerPeerCertificateSerial() in Lua
source/common/formatter/stream_info_formatter: add DOWNSTREAM_PEER_ISSUER_FINGERPRINT_256 and DOWNSTREAM_PEER_ISSUER_SERIAL access log formatter commands
test/mocks/ssl/mocks.h: add mock methods
test/common/tls/utility_test: add 4 unit tests
test/common/tls/ssl_socket_test: add GetIssuerPeerCertificateDigest integration test; extend TestUtilOptions
test/extensions/filters/http/lua/lua_filter_test: fix function names and add assertions
test/common/formatter/substitution_formatter_test: add 6 test cases
docs: update substitution_formatter.rst and lua_filter.rst

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2026-03-03T10:02:27Z

Hi @stias, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #43723 was opened by stias.

see: more, trace.

stias · 2026-03-03T10:03:50Z

related issue: #43722

Add issuerPeerCertificateHash() and issuerPeerCertificateSerial() to the SSL connection info stack, exposing SHA256 fingerprint and serial number of the direct issuer (CA) certificate from the peer's TLS certificate chain (chain index 1). Changes: - envoy/ssl/connection.h: add pure virtual declarations - source/common/tls/connection_info_impl_base: implement via SSL_get_peer_full_cert_chain + sk_X509_value(chain, 1) - source/common/quic/quic_ssl_connection_info.h: stub returning - source/common/tls/utility: add getSha256DigestFromCertificate() and getSha1DigestFromCertificate() helpers; refactor existing callers - source/common/tls/BUILD: add hex_lib dependency to utility_lib - source/extensions/filters/common/lua/wrappers: expose as issuerPeerCertificateHash() and issuerPeerCertificateSerial() in Lua - source/common/formatter/stream_info_formatter: add DOWNSTREAM_PEER_ISSUER_FINGERPRINT_256 and DOWNSTREAM_PEER_ISSUER_SERIAL access log formatter commands - test/mocks/ssl/mocks.h: add mock methods - test/common/tls/utility_test: add 4 unit tests - test/common/tls/ssl_socket_test: add GetIssuerPeerCertificateDigest integration test; extend TestUtilOptions - test/extensions/filters/http/lua/lua_filter_test: fix function names and add assertions - test/common/formatter/substitution_formatter_test: add 6 test cases - docs: update substitution_formatter.rst and lua_filter.rst Signed-off-by: stias <seokjun.yang@mycraft.kr>

ggreenway

/wait

envoy/ssl/connection.h

source/common/tls/connection_info_impl_base.cc

…in index Previously, issuerPeerCertificateHash() and issuerPeerCertificateSerial() returned data from the peer leaf certificate itself, not the issuer CA cert. Also, the implementation assumed the issuer is always at index 1 in the chain, which is not reliable. This commit introduces getIssuerFromValidatedChain(), a helper that identifies the true issuer CA certificate from the validated peer cert chain by: 1. Requiring a successfully validated chain (via SslExtendedSocketInfo) to prevent returning data from an unvalidated/attacker-controlled chain. 2. Using AKI/SKI byte comparison as a cheap pre-filter to skip non-issuers without a crypto operation. X509_check_issued() is deliberately avoided because its strict DER name comparison can fail on semantically equivalent names encoded with different ASN.1 string types (e.g. PrintableString vs UTF8String). 3. Falling back to X509_verify() (public key cryptographic check) as the authoritative issuer confirmation. The access log formatters %DOWNSTREAM_PEER_ISSUER_FINGERPRINT_256% and %DOWNSTREAM_PEER_ISSUER_SERIAL%, as well as the Lua API methods issuerPeerCertificateHash() and issuerPeerCertificateSerial(), now correctly return attributes of the CA certificate that signed the peer leaf cert. Updated documentation and tests to reflect the new behavior and requirements: requires a validated peer cert chain (e.g., mTLS with require_client_certificate: true and a validation_context). Signed-off-by: stias <seokjun.yang@mycraft.kr>

Rename issuerPeerCertificateHash() → sha256PeerCertificateIssuerDigest() and issuerPeerCertificateSerial() → serialNumberPeerCertificateIssuer() to eliminate naming ambiguity. The old names were misleading because issuerPeerCertificate() returns the Issuer DN field of the peer certificate (a string), so issuerPeerCertificateHash() appeared to be sha256(issuerPeerCertificate()) — i.e. a hash of that DN string — which is not what it computes. The new names follow the existing Envoy convention: sha256PeerCertificateDigest() → SHA-256 of the peer cert sha256PeerCertificateIssuerDigest() → SHA-256 of the issuer CA cert serialNumberPeerCertificate() → serial of the peer cert serialNumberPeerCertificateIssuer() → serial of the issuer CA cert No behavioral change; rename only. Signed-off-by: stias <seokjun.yang@mycraft.kr>

Two issues in the test: - require_client_certificate was not set, so ClientValidationStatus was never set to Validated, causing getIssuerFromValidatedChain() to always return nullptr and the issuer assertions to fail. - setExpectedSerialNumber() was missing; serialNumberPeerCertificate() is checked unconditionally in testUtil(), so omitting it caused != TEST_SAN_DNS3_CERT_SERIAL failures. Fix by adding require_client_certificate: true to the server context and setting setExpectedSerialNumber(TEST_SAN_DNS3_CERT_SERIAL). Signed-off-by: stias <seokjun.yang@mycraft.kr>

…suerFromValidatedChain Previously getIssuerFromValidatedChain() returned nullptr when the client sent only a leaf certificate (no intermediate chain), making sha256PeerCertificateIssuerDigest() and serialNumberPeerCertificateIssuer() return empty strings in that case. Add a second lookup path: after exhausting the peer-supplied chain, use X509_STORE_CTX_get1_issuer() to locate the issuer directly in the trusted CA store. The candidate is confirmed with X509_verify() for consistency with Path 1. This covers the common case where the issuer is the immediate trust anchor already loaded via validation_context.trusted_ca. Because X509_STORE_CTX_get1_issuer() returns a new reference (owned by the caller), change the return type from X509* to bssl::UniquePtr<X509> to unify ownership across both paths. Path 1 now calls X509_up_ref() on the chain candidate before wrapping it. Signed-off-by: stias <seokjun.yang@mycraft.kr>

stias · 2026-03-04T07:31:01Z

For reference, I built Envoy locally and verified that it works as expected.

mtls_issuer_test.yaml

static_resources:
  listeners:
    - name: listener_0
      address:
        socket_address:
          address: 0.0.0.0
          port_value: 10000
      filter_chains:
        - filters:
            - name: envoy.filters.network.http_connection_manager
              typed_config:
                "@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
                stat_prefix: ingress_http
                access_log:
                  - name: envoy.access_loggers.stdout
                    typed_config:
                      "@type": type.googleapis.com/envoy.extensions.access_loggers.stream.v3.StdoutAccessLog
                      log_format:
                        text_format_source:
                          inline_string: |
                            [ACCESS LOG]
                              peer_issuer_dn:          %DOWNSTREAM_PEER_ISSUER%
                              peer_issuer_fp256:       %DOWNSTREAM_PEER_ISSUER_FINGERPRINT_256%
                              peer_issuer_serial:      %DOWNSTREAM_PEER_ISSUER_SERIAL%
                              peer_fingerprint_256:    %DOWNSTREAM_PEER_FINGERPRINT_256%
                              peer_serial:             %DOWNSTREAM_PEER_SERIAL%
                              peer_chain_serials:      %DOWNSTREAM_PEER_CHAIN_SERIALS%
                http_filters:
                  - name: envoy.filters.http.lua
                    typed_config:
                      "@type": type.googleapis.com/envoy.extensions.filters.http.lua.v3.Lua
                      default_source_code:
                        inline_string: |
                          function envoy_on_request(request_handle)
                            local ssl = request_handle:streamInfo():downstreamSslConnection()
                            if ssl == nil then
                              request_handle:headers():add("x-mtls-error", "no-ssl")
                              return
                            end

                            local issuer_hash   = ssl:sha256PeerCertificateIssuerDigest()
                            local issuer_serial = ssl:serialNumberPeerCertificateIssuer()

                            local issuer_dn     = ssl:issuerPeerCertificate()
                            local peer_serial   = ssl:serialNumberPeerCertificate()
                            local peer_fp256    = ssl:sha256PeerCertificateDigest()

                            request_handle:logInfo("=== issuer cert info ===")
                            request_handle:logInfo("issuer_dn:      " .. issuer_dn)
                            request_handle:logInfo("issuer_fp256:   " .. issuer_hash)
                            request_handle:logInfo("issuer_serial:  " .. issuer_serial)
                            request_handle:logInfo("peer_serial:    " .. peer_serial)
                            request_handle:logInfo("peer_fp256:     " .. peer_fp256)

                            request_handle:headers():add("x-issuer-fp256",   issuer_hash)
                            request_handle:headers():add("x-issuer-serial",  issuer_serial)
                            request_handle:headers():add("x-issuer-dn",      issuer_dn)
                            request_handle:headers():add("x-peer-serial",    peer_serial)
                          end
                  - name: envoy.filters.http.router
                    typed_config:
                      "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router
                route_config:
                  name: local_route
                  virtual_hosts:
                    - name: local_service
                      domains: ["*"]
                      routes:
                        - match:
                            prefix: "/"
                          direct_response:
                            status: 200
                            body:
                              inline_string: "mTLS issuer test OK\n"
          transport_socket:
            name: envoy.transport_sockets.tls
            typed_config:
              "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.DownstreamTlsContext
              require_client_certificate: true
              common_tls_context:
                tls_certificates:
                  - certificate_chain:
                      filename: "test/common/tls/test_data/no_san_cert.pem"
                    private_key:
                      filename: "test/common/tls/test_data/no_san_key.pem"
                validation_context:
                  trusted_ca:
                    filename: "test/common/tls/test_data/intermediate_ca_cert_chain.pem"

admin:
  address:
    socket_address:
      address: 127.0.0.1
      port_value: 9901

curl scenario

curl -v https://localhost:10000/ \
  --cacert test/common/tls/test_data/ca_cert.pem \
  --cert test/common/tls/test_data/no_san_chain.pem \
  --key test/common/tls/test_data/no_san_key.pem -k

curl -v https://localhost:10000/ \
  --cacert test/common/tls/test_data/ca_cert.pem \
  --cert test/common/tls/test_data/san_dns3_chain.pem \
  --key test/common/tls/test_data/san_dns3_key.pem -k

Envoy execution:
$ bazel-bin/source/exe/envoy-static --config-path configs/mtls_issuer_test.yaml --log-level info

Actual Output

$ bazel-bin/source/exe/envoy-static --config-path configs/mtls_issuer_test.yaml --log-level info
# curl -v https://localhost:10000/   --cacert test/common/tls/test_data/ca_cert.pem   --cert test/common/tls/test_data/no_san_chain.pem   --key test/common/tls/test_data/no_san_key.pem -k
[2026-03-04 07:28:45.020][478618][info][lua] [source/extensions/filters/common/lua/lua.cc:26] script log: === issuer cert info ===
[2026-03-04 07:28:45.020][478618][info][lua] [source/extensions/filters/common/lua/lua.cc:26] script log: issuer_dn:      CN=Test CA,OU=Lyft Engineering,O=Lyft,L=San Francisco,ST=California,C=US
[2026-03-04 07:28:45.020][478618][info][lua] [source/extensions/filters/common/lua/lua.cc:26] script log: issuer_fp256:   2c744b40ce28e2eea7ad41e505d39e89eff0c5ce11a943ded4f87e231bed3632
[2026-03-04 07:28:45.020][478618][info][lua] [source/extensions/filters/common/lua/lua.cc:26] script log: issuer_serial:  34aac3658c934935a02ee3a011cdca4b7ca0a839
[2026-03-04 07:28:45.020][478618][info][lua] [source/extensions/filters/common/lua/lua.cc:26] script log: peer_serial:    3ab370888bba18071dd1beaf74e78c613e490563
[2026-03-04 07:28:45.020][478618][info][lua] [source/extensions/filters/common/lua/lua.cc:26] script log: peer_fp256:     8404a810f988c0cc25d2f4f345d16fdc00f0082a3bb5f330c588329c92178490
[ACCESS LOG]
  peer_issuer_dn:          CN=Test CA,OU=Lyft Engineering,O=Lyft,L=San Francisco,ST=California,C=US
  peer_issuer_fp256:       2c744b40ce28e2eea7ad41e505d39e89eff0c5ce11a943ded4f87e231bed3632
  peer_issuer_serial:      34aac3658c934935a02ee3a011cdca4b7ca0a839
  peer_fingerprint_256:    8404a810f988c0cc25d2f4f345d16fdc00f0082a3bb5f330c588329c92178490
  peer_serial:             3ab370888bba18071dd1beaf74e78c613e490563
  peer_chain_serials:      3ab370888bba18071dd1beaf74e78c613e490563,3ab370888bba18071dd1beaf74e78c613e490561

# curl -v https://localhost:10000/   --cacert test/common/tls/test_data/ca_cert.pem   --cert test/common/tls/test_data/san_dns3_chain.pem   --key test/common/tls/test_data/san_dns3_key.pem   -k
[2026-03-04 07:30:39.647][478601][info][lua] [source/extensions/filters/common/lua/lua.cc:26] script log: === issuer cert info ===
[2026-03-04 07:30:39.647][478601][info][lua] [source/extensions/filters/common/lua/lua.cc:26] script log: issuer_dn:      CN=Test Intermediate CA,OU=Lyft Engineering,O=Lyft,L=San Francisco,ST=California,C=US
[2026-03-04 07:30:39.647][478601][info][lua] [source/extensions/filters/common/lua/lua.cc:26] script log: issuer_fp256:   fa83825871e3671fe862ad77e888828734dd7f9e6bdee75edd51904479258b06
[2026-03-04 07:30:39.647][478601][info][lua] [source/extensions/filters/common/lua/lua.cc:26] script log: issuer_serial:  3ab370888bba18071dd1beaf74e78c613e490561
[2026-03-04 07:30:39.647][478601][info][lua] [source/extensions/filters/common/lua/lua.cc:26] script log: peer_serial:    089024d8b14338ef6afb7b70b52d70e0b1318fcb
[2026-03-04 07:30:39.648][478601][info][lua] [source/extensions/filters/common/lua/lua.cc:26] script log: peer_fp256:     ba0351d0085efede86ab77f09f18b862fb1678ef03c7b067e81bc8db724c3b1e
[ACCESS LOG]
  peer_issuer_dn:          CN=Test Intermediate CA,OU=Lyft Engineering,O=Lyft,L=San Francisco,ST=California,C=US
  peer_issuer_fp256:       fa83825871e3671fe862ad77e888828734dd7f9e6bdee75edd51904479258b06
  peer_issuer_serial:      3ab370888bba18071dd1beaf74e78c613e490561
  peer_fingerprint_256:    ba0351d0085efede86ab77f09f18b862fb1678ef03c7b067e81bc8db724c3b1e
  peer_serial:             089024d8b14338ef6afb7b70b52d70e0b1318fcb
  peer_chain_serials:      089024d8b14338ef6afb7b70b52d70e0b1318fcb,3ab370888bba18071dd1beaf74e78c613e490561

Add a test that verifies the AKI/SKI pre-filter in getIssuerFromValidatedChain() correctly skips a decoy CA cert that does not match the leaf's Authority Key Identifier, and selects the correct Intermediate CA that does match. Chain sent by client (san_dns3_with_decoy_chain.pem): [0] leaf [1] Root CA (decoy, AKI mismatch) [2] Intermediate CA (correct) Expected: sha256PeerCertificateIssuerDigest() and serialNumberPeerCertificateIssuer() return Intermediate CA values, not Root CA values. Together with GetIssuerPeerCertificateDigest (leaf + chain, Path 1) and GetIssuerPeerCertificateDigestLeafOnly (leaf only, Path 2), this completes coverage for all three significant code paths in getIssuerFromValidatedChain(). Signed-off-by: stias <seokjun.yang@mycraft.kr>

stias · 2026-03-04T11:46:29Z

@ggreenway Could you please review it once more?

ggreenway

/wait

source/common/tls/connection_info_impl_base.cc

Replace the manual AKI/SKI + X509_verify issuer search with X509_verify_cert to build a proper validated chain, then extract chain[1] as the direct issuer. The previous approach only checked whether a candidate certificate's public key could verify the leaf signature, without confirming that the candidate itself chains to a trust anchor. A client could craft a certificate containing the real CA's public key (publicly available) that would pass signature verification but is not part of the validated trust path. The new implementation: - Builds an X509_STORE_CTX with the peer chain as untrusted intermediates and the Envoy trust store - Calls X509_STORE_CTX_set_default and X509_VERIFY_PARAM_set1 to mirror the handshake verification (purpose checks, PARTIAL_CHAIN, CRL flags) - Runs X509_verify_cert to construct the validated chain - Returns chain[1] which is guaranteed to be on the verified path from leaf to trust anchor Signed-off-by: stias <seokjun.yang@mycraft.kr>

stias · 2026-03-05T14:30:13Z

@ggreenway

I have updated the code to retrieve the issuer only after narrowing the scope to what can be guaranteed as a validated chain by OpenSSL.

I understand you may be busy, but I would appreciate it if you could review it once again.

stias · 2026-03-06T11:42:00Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces new functionality to expose the issuer certificate's SHA-256 hash and serial number from the peer's TLS certificate chain. The changes are exposed through access log formatters (%DOWNSTREAM_PEER_ISSUER_FINGERPRINT_256%, %DOWNSTREAM_PEER_ISSUER_SERIAL%) and the Lua filter API.

The implementation is robust, using X509_verify_cert to reconstruct the validated chain and reliably identify the issuer certificate. The code is well-structured, with new utility functions to avoid duplication, and is accompanied by a comprehensive set of unit and integration tests that cover various scenarios, including complex certificate chains.

My only feedback is a minor suggestion to improve the precision of the documentation for the new features to more accurately reflect the implementation logic.

_{Note: Security Review did not run due to the size of the PR.}

ggreenway

I'm uncomfortable with the approach of re-verifying the chain, because the certificate validator is an extension, and there's no way to know how it validates and builds a chain, and thus there's no way to ensure that this code performs the same validation.

It would be safer to store the validated chain in the validator, but that will have tradeoffs of extra memory use, although that may be acceptable. If you pursue that route, it would be helpful to have some measurement of the memory overhead involved.

/wait

Previously, sha256PeerCertificateIssuerDigest and serialNumberPeerCertificateIssuer re-verified the peer certificate chain in getIssuerFromValidatedChain() to obtain the issuer cert. This is unsafe because CertValidator is an extension point and custom validators may build chains differently than the re-verification logic. Instead, capture the validated chain at verification time and store it per-connection. Each CertValidator now populates validated_chain in ValidationResults after successful X509_verify_cert(). The chain is passed through SslExtendedSocketInfo to SslHandshakerImpl, where ConnectionInfoImplBase::validatedPeerIssuer() retrieves the issuer. Changes: - Add validated_chain field to ValidationResults - Add setValidatedCertChain() to SslExtendedSocketInfo interface - Store validated chain in SslHandshakerImpl, expose via validatedPeerIssuer() override - Capture verified chain in DefaultCertValidator and SPIFFEValidator - Remove getIssuerFromValidatedChain() re-verification logic Signed-off-by: stias <seokjun.yang@mycraft.kr>

stias · 2026-03-09T14:19:09Z

Memory overhead measurement

X509_up_ref() increments the reference count without duplicating the X509 object. The per-connection overhead is only the std::vector container and its pointers:

std::vector overhead: 24 bytes
Per-cert pointer (bssl::UniquePtr<X509>): 8 bytes each
Typical chain (leaf + intermediate + root): 48 bytes per connection (theoretical)

The X509 objects themselves are already held in memory by BoringSSL's SSL session state; we only add a reference.

Benchmark validation

A micro-benchmark (test/common/tls/validated_chain_memory_benchmark.cc) was run to verify the above analysis. It allocates N chains of 3 ref-counted certs (leaf + intermediate + root) via X509_up_ref and measures heap growth using Memory::Stats::totalCurrentlyAllocated() (tcmalloc).

Benchmark	Time	CPU	Iterations	bytes_per_chain	total_bytes
BM_ValidatedChainMemoryOverhead/1000	0.526 ms	0.526 ms	1307	56.576	56.576k
BM_ValidatedChainMemoryOverhead/10000	5.71 ms	5.71 ms	129	59.0336	590.336k
BM_ValidatedChainMemoryOverhead/100000	67.3 ms	67.3 ms	10	56.0845	5.60845M

The measured overhead is ~56 bytes/connection — slightly above the theoretical 48 bytes due to tcmalloc's allocation size class rounding (the vector's internal buffer of 24 bytes is rounded up to 32 bytes by the allocator).

Concurrent mTLS connections	Theoretical (48B)	Measured (~56B)
1,000	~48 KB	~56 KB
10,000	~480 KB	~590 KB
100,000	~4.8 MB	~5.6 MB

Add a micro-benchmark to measure per-connection memory overhead of storing the validated certificate chain via X509_up_ref(). This provides concrete numbers to justify the memory tradeoff of caching the chain instead of re-verifying it. Measured ~56 bytes/connection for a typical 3-cert chain (leaf + intermediate + root), which amounts to ~590KB at 10K concurrent mTLS connections. Signed-off-by: stias <seokjun.yang@mycraft.kr>

stias · 2026-03-09T14:21:32Z

@ggreenway Could you please take another look at the PR at your convenience?

stias · 2026-03-09T14:21:49Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces new functionalities to expose the issuer certificate hash and serial number from the peer certificate chain in TLS connections. It includes changes across multiple files, adding new methods to retrieve the issuer's SHA256 fingerprint and serial number, exposing these in Lua scripts, and adding access log formatter commands. The changes also include new unit tests and updates to documentation.

_{Note: Security Review is unavailable for this PR.}

I am having trouble creating individual review comments. Click here to see my feedback.

source/common/tls/connection_info_impl_base.cc (125-130)

This logic to compute the SHA256 digest is duplicated in Utility::getSha256DigestFromCertificate. Consider calling the utility function directly to avoid code duplication.

        if (!cert) {
          return std::string{};
        }

        std::vector<uint8_t> computed_hash(SHA256_DIGEST_LENGTH);
        unsigned int n;
        X509_digest(cert.get(), EVP_sha256(), computed_hash.data(), &n);
        RELEASE_ASSERT(n == computed_hash.size(), "");
        return Hex::encode(computed_hash);

References

Avoid code duplication by reusing existing utility functions.

source/common/tls/connection_info_impl_base.cc (125)

Call Utility::getSha256DigestFromCertificate(*cert) instead.

        if (!cert) {
          return std::string{};
        }
        return Utility::getSha256DigestFromCertificate(*cert);

References

Avoid code duplication by reusing existing utility functions.

source/common/tls/connection_info_impl_base.cc (150)

Call Utility::getSha1DigestFromCertificate(*cert) instead.

        if (!cert) {
          return std::string{};
        }
        return Utility::getSha1DigestFromCertificate(*cert);

References

Avoid code duplication by reusing existing utility functions.

source/common/tls/connection_info_impl_base.cc (177-182)

This logic to compute the SHA1 digest is duplicated in Utility::getSha1DigestFromCertificate. Consider calling the utility function directly to avoid code duplication.

        return Utility::mapX509Stack(*cert_chain, [](X509& cert) -> std::string {
          std::vector<uint8_t> computed_hash(SHA_DIGEST_LENGTH);
          unsigned int n;
          X509_digest(&cert, EVP_sha1(), computed_hash.data(), &n);
          RELEASE_ASSERT(n == computed_hash.size(), "");
          return Hex::encode(computed_hash);
        });

References

Avoid code duplication by reusing existing utility functions.

ggreenway

This is looking much better; I like the approach of saving the validated chain. It removes that entire class of security issue.

/gemini review

/wait

ggreenway · 2026-03-10T20:51:59Z

configs/mtls_issuer_test.yaml

@@ -0,0 +1,94 @@
+static_resources:


This doesn't need to be added to the repo

ggreenway · 2026-03-10T20:52:31Z

docs/root/configuration/advanced/substitution_formatter.rst

+``%DOWNSTREAM_PEER_ISSUER_FINGERPRINT_256%``
+  HTTP/TCP/THRIFT
+    The hex-encoded SHA256 fingerprint of the direct issuer (CA) certificate from the downstream
+    TLS peer certificate chain. The issuer is identified by searching the chain for a certificate


Update docs; this is the verified issuer

ggreenway · 2026-03-10T21:07:09Z

source/common/tls/cert_validator/default_validator.cc

+        X509* cert = sk_X509_value(verified_chain, i);
+        X509_up_ref(cert);
+        validated_chain.emplace_back(cert);


Suggested change

X509* cert = sk_X509_value(verified_chain, i);

X509_up_ref(cert);

validated_chain.emplace_back(cert);

X509* cert = sk_X509_value(verified_chain, i);

validated_chain.emplace_back(bssl::UpRef(cert));

ggreenway · 2026-03-10T21:08:02Z

source/common/tls/cert_validator/default_validator.cc

    }
+
+    STACK_OF(X509)* verified_chain = X509_STORE_CTX_get0_chain(ctx.get());
+    if (verified_chain != nullptr) {


Remove this check; this cannot be nullptr if verification succeeded

ggreenway · 2026-03-10T21:09:41Z

source/common/tls/connection_info_impl_base.cc

 // There must be an version of this function for each type possible in variant `CachedValue`.
 bool shouldRecalculateCachedEntry(const std::string& str) { return str.empty(); }
 bool shouldRecalculateCachedEntry(const std::vector<std::string>& vec) { return vec.empty(); }
+


ggreenway · 2026-03-10T21:14:05Z

source/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator.cc

+      X509_up_ref(cert);
+      validated_chain.emplace_back(cert);


bssl::UpRef

ggreenway · 2026-03-10T21:16:54Z

test/common/tls/validated_chain_memory_benchmark.cc

+  return cert;
+}
+
+// Simulate what the implementation does: for each connection, store a


I think the bigger question is whether the X509 cert chain is kept in memory either way, or if this change causes the certificates to be kept in memory longer.

@ggreenway

This PR does not cause the certificates to live longer in memory. The verified chain from X509_STORE_CTX_get0_chain contains references to X509 objects that are already owned elsewhere, peer certs by BoringSSL's SSL session, and root CA certs by the X509_STORE. These objects already live for the connection lifetime or longer. The bssl::UpRef only bumps their reference counts. The only new per-connection cost is the std::vector of 3 pointers (~48-64 bytes for a typical leaf + intermediate + root chain).

…llptr check, update docs - Replace X509_up_ref + emplace_back with bssl::UpRef in default_validator.cc and spiffe_validator.cc - Remove unnecessary nullptr check on X509_STORE_CTX_get0_chain after successful X509_verify_cert in default_validator.cc - Update DOWNSTREAM_PEER_ISSUER_FINGERPRINT_256 and DOWNSTREAM_PEER_ISSUER_SERIAL docs to clarify these use the verified issuer from the validated chain - Remove stray blank line in connection_info_impl_base.cc Signed-off-by: stias <seokjun.yang@mycraft.kr>

stias requested review from mattklein123 and wbpcode as code owners March 3, 2026 10:02

stias had a problem deploying to external-contributors March 3, 2026 10:02 — with GitHub Actions Error

stias force-pushed the feat/downstream-peer-issuer-chain-info branch from ad698ca to db82740 Compare March 3, 2026 10:05

stias had a problem deploying to external-contributors March 3, 2026 10:05 — with GitHub Actions Error

ggreenway requested changes Mar 3, 2026

View reviewed changes

envoy/ssl/connection.h Outdated Show resolved Hide resolved

source/common/tls/connection_info_impl_base.cc Outdated Show resolved Hide resolved

source/common/tls/connection_info_impl_base.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Mar 3, 2026

ggreenway self-assigned this Mar 3, 2026

repokitteh-read-only bot removed the waiting label Mar 4, 2026

stias had a problem deploying to external-contributors March 4, 2026 03:19 — with GitHub Actions Error

stias had a problem deploying to external-contributors March 4, 2026 03:56 — with GitHub Actions Error

stias requested a review from ggreenway March 4, 2026 04:00

stias had a problem deploying to external-contributors March 4, 2026 06:26 — with GitHub Actions Error

stias had a problem deploying to external-contributors March 4, 2026 07:15 — with GitHub Actions Error

stias had a problem deploying to external-contributors March 4, 2026 07:44 — with GitHub Actions Error

ggreenway requested changes Mar 4, 2026

View reviewed changes

source/common/tls/connection_info_impl_base.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Mar 4, 2026

repokitteh-read-only bot removed the waiting label Mar 5, 2026

stias had a problem deploying to external-contributors March 5, 2026 14:30 — with GitHub Actions Error

gemini-code-assist bot reviewed Mar 6, 2026

View reviewed changes

stias requested a review from ggreenway March 6, 2026 11:57

ggreenway requested changes Mar 6, 2026

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 6, 2026

stias requested review from botengyao, mathetake and tyxia as code owners March 9, 2026 13:58

repokitteh-read-only bot removed the waiting label Mar 9, 2026

stias had a problem deploying to external-contributors March 9, 2026 13:59 — with GitHub Actions Error

stias requested a review from ggreenway March 9, 2026 14:20

stias had a problem deploying to external-contributors March 9, 2026 14:20 — with GitHub Actions Error

gemini-code-assist bot reviewed Mar 9, 2026

View reviewed changes

envoyproxy deleted a comment from gemini-code-assist bot Mar 10, 2026

ggreenway requested changes Mar 10, 2026

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 10, 2026

tls: Remove mtls_issuer_test.yaml

4692d83

stias had a problem deploying to external-contributors March 11, 2026 01:37 — with GitHub Actions Error

repokitteh-read-only bot removed the waiting label Mar 11, 2026

stias requested a deployment to external-contributors March 11, 2026 01:49 — with GitHub Actions Waiting

stias requested a review from ggreenway March 11, 2026 02:08

Conversation

stias commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Mar 3, 2026

Uh oh!

stias commented Mar 3, 2026

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stias commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stias commented Mar 4, 2026

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stias commented Mar 5, 2026

Uh oh!

stias commented Mar 6, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

stias commented Mar 9, 2026

Memory overhead measurement

Benchmark validation

Uh oh!

stias commented Mar 9, 2026

Uh oh!

stias commented Mar 9, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

source/common/tls/connection_info_impl_base.cc (125-130)

source/common/tls/connection_info_impl_base.cc (125)

source/common/tls/connection_info_impl_base.cc (150)

source/common/tls/connection_info_impl_base.cc (177-182)

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

stias commented Mar 3, 2026 •

edited

Loading

stias commented Mar 4, 2026 •

edited

Loading