Fix XFCC format handling: Hash= support, empty header, and sanity checks by stokpop · Pull Request #16 · cloudfoundry/java-buildpack-client-certificate-mapper

stokpop · 2026-06-03T15:29:47Z

Fixes #11, #6

What changed

Bug fixes

Hash-only XFCC headers no longer warn (support for XFCC hashed client cert #11): Hash= is now recognised as a valid XFCC key, so a header like Hash=abc...;By=... without Cert= is silently skipped instead of logging "Unable to parse certificates".
Empty XFCC header values no longer warn (special case for null XFCC value #6): Null/empty header values are now skipped before any parsing attempt.

New: XFCC key parsing

Full Envoy XFCC key-value format support: By=, Hash=, Cert=, Subject=, URI=, DNS=
Keys matched case-insensitively
Quoted values (e.g. Subject="/C=US;L=SF") handled correctly — ; inside quotes is not treated as a field separator
Values from the XFCC hash-type header are put in the request attributes for application usage as String:
- org.cloudfoundry.router.xfcc.hash
- org.cloudfoundry.router.xfcc.subject
- org.cloudfoundry.router.xfcc.uri

Explicitly excluded from this PR

Chain= support — intentionally not supported. CertificateFactory.generateCertificate() (singular) reads only the first cert; intermediates are silently ignored. A proper fix requires generateCertificates() (plural) and populating the X509Certificate[] attribute per Servlet spec. Tracked separately.

Decisions for consideration

Decision	Choice	Rationale
Warn when `Chain=` is present without `Cert=`	✅ Yes	Operators need to know their setup sends unsupported headers
Warn when `Hash=` doesn't look like SHA-256 (64 hex chars)	✅ Yes	Lightweight sanity check; spec mandates SHA-256. Use case is hash forwarded by Envoy — we trust Envoy to validate the client cert, but a malformed hash suggests misconfiguration
Validate `Hash=` against actual cert fingerprint	❌ No	`Hash=` and `Cert=` are not expected to appear together — Envoy sends either the hash or the full cert depending on configuration. No cert is available to validate against
Log XFCC field names at DEBUG (`FINE`)	✅ Yes	Useful for diagnosing format issues; cert values are never logged
Guard `xfccFieldNames()` with `isLoggable(FINE)`	✅ Yes	Avoids scanning all XFCC fields on every request when debug logging is off
Parse other XFCC fields (`By=`, `Subject=`, `URI=`, `DNS=`)	❌ No	Only `Cert=` needed; others recognised for format detection only
Log XFCC field values (incl. `Subject=`) at DEBUG	❌ No	Field names only — avoids leaking identity data into logs. Open for discussion if specific field values (e.g. `Subject=`) would be useful
Map X509 attribute when only `Hash=` is present	❌ No	A hash is a fingerprint only; full certificate required to construct an `X509Certificate` object
Support full cert chain from `Chain=`	❌ No	Deferred to separate issue/PR if needed
Put the parsed `XfccEntry` as attribute in request for apps to use	❌ No	Avoid dependency of apps to the a class in jar of client-certificate-mapper, values are available as String instead

- Detect Envoy XFCC text format (key=value pairs) to avoid spurious 'Unable to parse certificates' warnings (fixes issue cloudfoundry#11) - Keys are matched case-insensitively per the Envoy XFCC spec - Add Chain= key support as fallback when Cert= is absent - Add Subject= key recognition for correct field detection - Handle double-quoted values (e.g. Subject="/C=US;L=SF") so that semicolons inside quotes do not break field parsing - Skip empty/blank header values silently (fixes issue cloudfoundry#6) - Add tests: xfccHashOnly, xfccWithCert, xfccCaseInsensitiveKeys, xfccCaseInsensitiveCert, xfccChainFallback, xfccSubjectWithSemicolonDoesNotBreakCert

# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

- Remove Chain= from XFCC_KEYS and parseCertificate(); Chain= support is deferred to a separate issue/PR (requires generateCertificates() plural to populate full X509Certificate[] per Servlet spec) - Warn when Chain= is present without Cert= (unsupported, cert not mapped) - Add SHA-256 format sanity check on Hash= value (must be 64 hex chars); warn if it does not look like a valid SHA-256 digest - Guard xfccFieldNames() with isLoggable(FINE) to avoid scanning all XFCC fields on every request when debug logging is off - Update test hash values to valid 64-char SHA-256 length - Update README: XFCC format section with spec references (Envoy, RFC 9110, Jakarta Servlet 6.0), debug logging section, restructured for end-users Fixes cloudfoundry#11 (Hash-only XFCC no longer triggers 'Unable to parse' warning) Fixes cloudfoundry#6 (empty XFCC header value no longer triggers warning)

Copilot

Pull request overview

This pull request improves handling of the X-Forwarded-Client-Cert (XFCC) header by recognizing Envoy-style key/value entries (including Hash=-only values) and skipping empty header values to avoid unnecessary warnings, with added parsing to correctly handle quoted fields containing semicolons.

Changes:

Add XFCC key/value parsing (case-insensitive keys, quoted values) and treat Hash=-only entries as valid but non-mappable.
Skip null/empty XFCC header values before parsing and avoid adding empty comma-split parts.
Add new unit tests for hash-only, mixed fields, case-insensitive keys, and quoted Subject= with semicolons.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
README.md	Documents supported XFCC formats/keys and debug logging behavior.
java-buildpack-client-certificate-mapper-javax/src/main/java/org/cloudfoundry/router/javax/ClientCertificateMapper.java	Implements XFCC key/value parsing, hash sanity check, and empty-header handling.
java-buildpack-client-certificate-mapper-javax/src/test/java/org/cloudfoundry/router/javax/ClientCertificateMapperTest.java	Adds tests covering new XFCC parsing scenarios.
java-buildpack-client-certificate-mapper-jakarta/src/main/java/org/cloudfoundry/router/jakarta/ClientCertificateMapper.java	Jakarta equivalent of the XFCC parsing and validation logic.
java-buildpack-client-certificate-mapper-jakarta/src/test/java/org/cloudfoundry/router/jakarta/ClientCertificateMapperTest.java	Jakarta equivalent tests for the new XFCC behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

…g log - Trim whitespace around comma-separated XFCC entries per RFC 9110; a header like "Hash=...;Cert=X , Hash=...;Cert=X" would leave a leading space on the second entry, causing XFCC format detection to fail and the value to be treated as a raw certificate - Remove hash value from warning log to avoid logging user-controlled input (log injection risk); the shape check (64 hex chars) is sufficient diagnostic information - Add xfccCommaDelimitedWithWhitespace test documenting correct behaviour

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

jcvrabo

just some notes, but it's overall ok

jcvrabo · 2026-06-04T09:36:26Z

+            }
+        }
+        return false;
+    }


since for the purpose of this mapper an xfcc format MUST have the hash= field, with an optional cert= and another test for chain=, I argue that this function should simply check for hash=

I think it is either hash= or (cert= or chain=)?

I either read it incorrectly or just assumed that it would make sense that if the xfcc would be filled with envoy then a hash would always be present, but I see that indeed it's either has or cert (chain is only checked if cert is present), so I assume that providing the hash with the cert is something that is configured.

yes I think I saw that envoy can be configured to sent only hash or also the cert... when that happens both the X509 object as the xfcc attributes will be available in the request

- Add XfccField enum for known XFCC header field names (BY, HASH, CERT, CHAIN, SUBJECT, URI, DNS) each carrying its key suffix (e.g. Hash=) - Add XfccHeaderParser utility class: splitHeaderValues(List<String>) - trim fix, skips blanks isXfccFormat, extractFieldFromXfcc(XfccField), xfccFieldNames, isValidSha256Hex; SHA256_HEX pattern is private - ClientCertificateMapper delegates all XFCC logic to XfccHeaderParser and uses XfccField constants; converts Enumeration to List via Collections.list before passing to splitHeaderValues - Add XfccHeaderParserTest: 6 unit tests with plain List args, no mock - Use Arrays.asList for Java 8 compatibility - Remove dead [e] link definition from README.md Applied to both javax and jakarta modules.

Add java-buildpack-client-certificate-mapper-core module containing XfccField (public enum) and XfccHeaderParser (public final class) with no servlet dependency. Both jakarta and javax modules depend on core and import from org.cloudfoundry.router. Duplicate XfccField, XfccHeaderParser and XfccHeaderParserTest removed from both servlet modules.

- XfccEntry replaces multi-pass XfccHeaderParser field extraction - Structural XFCC detection: short (<=20 chars) all-letter key before '=' distinguishes XFCC from raw base64/PEM certs; JSON format not supported - resemblesXfcc() requires at least one of Hash=, Cert=, or Chain= to be present, preserving existing external behavior for all previously-seen inputs - Unknown fields logged at FINE level for forward compatibility with future XFCC fields - ClientCertificateMapper updated to use XfccEntry directly - README updated with XFCC detection and fallback behaviour notes

- isXfccFormat now O(1): scans at most MAX_KEY_LENGTH+1 chars instead of using indexOf which scans the full cert string for raw base64 inputs - Use Collections.emptyMap() for non-XFCC entries to avoid EnumMap allocation

When the XFCC header is detected, set: org.cloudfoundry.router.xfcc.hash - from Hash= field org.cloudfoundry.router.xfcc.subject - from Subject= field (if present) org.cloudfoundry.router.xfcc.uri - from URI= field, e.g. SPIFFE ID (if present) First XFCC entry wins for multi-entry headers. Attributes are set regardless of whether Cert= is present, so applications can identify the caller even when only a Hash= is forwarded by the router.

Copilot

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.

- splitHeaderValues: respects double-quoted field values (e.g. Subject="/C=US,ST=CA") so commas inside quotes are not treated as entry delimiters - XfccEntry FINE log for unknown fields: log position only, not the full raw header value, to avoid leaking cert/identity data into logs

Copilot

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.

readValue and skipToNextField now cap index at len when consuming escape sequences, so trailing backslash or unclosed quotes in header values don't crash request processing with a RuntimeException.

…try XFCC headers

Copilot

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.

Copilot

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

java-buildpack-client-certificate-mapper-javax/src/main/java/org/cloudfoundry/router/javax/ClientCertificateMapper.java:46

The class-level Javadoc still references RFC 7230 for comma-delimited header values, but this PR updates parsing and documentation to RFC 9110 list syntax. Updating the reference avoids pointing readers at an obsoleted spec.

 * A Servlet {@link Filter} that translates the {@code X-Forwarded-Client} HTTP header to the {@code javax.servlet.request.X509Certificate} Servlet attribute.  This implementation handles both
 * multiple headers as well as the <a href=https://tools.ietf.org/html/rfc7230#section-3.2.2>RFC 7230</a> comma delimited equivalent.
 */

java-buildpack-client-certificate-mapper-jakarta/src/main/java/org/cloudfoundry/router/jakarta/ClientCertificateMapper.java:46

The class-level Javadoc still references RFC 7230 for comma-delimited header values, but this PR updates parsing and documentation to RFC 9110 list syntax. Updating the reference avoids pointing readers at an obsoleted spec.

 * A Servlet {@link Filter} that translates the {@code X-Forwarded-Client} HTTP header to the {@code jakarta.servlet.request.X509Certificate} Servlet attribute.  This implementation handles both
 * multiple headers as well as the <a href=https://tools.ietf.org/html/rfc7230#section-3.2.2>RFC 7230</a> comma delimited equivalent.
 */

stokpop added 3 commits May 7, 2026 15:18

rkoster requested a review from Copilot June 4, 2026 07:47

Copilot started reviewing on behalf of rkoster June 4, 2026 07:47 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

Apply suggestions from code review

3eb3759

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

stokpop requested a review from Copilot June 4, 2026 08:16

Copilot started reviewing on behalf of stokpop June 4, 2026 08:16 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

stokpop and others added 2 commits June 4, 2026 10:25

Apply suggestions from code review

287ec36

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

stokpop requested a review from Copilot June 4, 2026 08:55

Copilot started reviewing on behalf of stokpop June 4, 2026 08:56 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

Comment thread ...ficate-mapper-javax/src/main/java/org/cloudfoundry/router/javax/ClientCertificateMapper.java Outdated

Comment thread ...te-mapper-jakarta/src/main/java/org/cloudfoundry/router/jakarta/ClientCertificateMapper.java Outdated

Comment thread README.md

jcvrabo reviewed Jun 4, 2026

View reviewed changes

stokpop added 7 commits June 4, 2026 10:02

Remove stray .swp file and add *.swp to .gitignore

64658ad

Add hasField() to XfccEntry and use in place of null checks

1198c8e

Optimise per-request overhead in XfccEntry

61a4e7e

- isXfccFormat now O(1): scans at most MAX_KEY_LENGTH+1 chars instead of using indexOf which scans the full cert string for raw base64 inputs - Use Collections.emptyMap() for non-XFCC entries to avoid EnumMap allocation

stokpop requested a review from Copilot June 4, 2026 13:43

Copilot started reviewing on behalf of stokpop June 4, 2026 13:43 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

Comment thread ...k-client-certificate-mapper-core/src/main/java/org/cloudfoundry/router/XfccHeaderParser.java

Comment thread ...uildpack-client-certificate-mapper-core/src/main/java/org/cloudfoundry/router/XfccEntry.java

stokpop requested a review from Copilot June 4, 2026 14:43

Copilot started reviewing on behalf of stokpop June 4, 2026 14:43 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

Comment thread ...uildpack-client-certificate-mapper-core/src/main/java/org/cloudfoundry/router/XfccEntry.java

Comment thread README.md Outdated

stokpop added 2 commits June 4, 2026 15:20

Guard against StringIndexOutOfBoundsException in malformed quoted values

d534bee

readValue and skipToNextField now cap index at len when consuming escape sequences, so trailing backslash or unclosed quotes in header values don't crash request processing with a RuntimeException.

Clarify README: first entry that contains the field wins for multi-en…

7efd734

…try XFCC headers

stokpop requested a review from Copilot June 4, 2026 15:23

Copilot started reviewing on behalf of stokpop June 4, 2026 15:23 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

Comment thread ...k-client-certificate-mapper-core/src/main/java/org/cloudfoundry/router/XfccHeaderParser.java

Comment thread ...uildpack-client-certificate-mapper-core/src/main/java/org/cloudfoundry/router/XfccEntry.java

Xfcc related public calls should deal with null values

bf7c856

stokpop requested a review from Copilot June 5, 2026 10:01

Copilot started reviewing on behalf of stokpop June 5, 2026 10:01 View session

Copilot AI reviewed Jun 5, 2026

View reviewed changes

stokpop requested a review from jcvrabo June 5, 2026 11:01

Conversation

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

Uh oh!

What changed

Bug fixes

New: XFCC key parsing

Explicitly excluded from this PR

Decisions for consideration

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jcvrabo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jcvrabo Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

stokpop Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

jcvrabo Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

stokpop Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

stokpop commented Jun 3, 2026 •

edited

Loading