Skip to content

fix(core/http): fix AWS ALB cert decoding and nil error logging#31740

Closed
Abhishek00810 wants to merge 2 commits into
hashicorp:mainfrom
Abhishek00810:fix/31728-aws-alb-cert-parsing
Closed

fix(core/http): fix AWS ALB cert decoding and nil error logging#31740
Abhishek00810 wants to merge 2 commits into
hashicorp:mainfrom
Abhishek00810:fix/31728-aws-alb-cert-parsing

Conversation

@Abhishek00810

Copy link
Copy Markdown
Contributor

Description

What does this PR do?

This PR fixes a regression where client certificates forwarded by AWS ALB failed to parse due to incorrect URL decoding, and fixes a nil pointer error in the associated logging.

1. The Parsing Bug (AWS ALB)

  • Issue: AWS ALB sends the X-Amzn-Mtls-Clientcert header URL-encoded but leaves + characters unescaped (literal +). Vault was using url.QueryUnescape, which converts + to space, corrupting the Base64 payload (e.g., abc+def becomes abc def).
  • Fix: Switched to url.PathUnescape. This correctly decodes percent-encoding while preserving literal + characters, matching AWS ALB behavior.

2. The Logging Bug (Nil Error)

  • Issue: The existing code tried to wrap err when pem.Decode failed. However, the signature func Decode(data []byte) (p *Block, rest []byte) does not return an error.
  • Result: The variable err was nil in that scope, causing logs to print failed to convert ...: %!w(<nil>).
  • Fix: Added an explicit errors.New("pem decode failed") to provide a readable error message when the block is nil.

Verification:

  • Added a reproduction test case in vault/http/forwarded_for_test.go that manually constructs a certificate string with literal + characters to simulate AWS ALB.
  • Verified the test fails with QueryUnescape and passes with PathUnescape.

Fixes #31728

TODO only if you're a HashiCorp employee

  • Backport Labels: If this fix needs to be backported, use the appropriate backport/ label that matches the desired release branch.
    • LTS: If this fixes a critical security vulnerability or severity 1 bug, it will also need to be backported to the current LTS versions of Vault. To ensure this, use all available enterprise labels.
  • Jira: If this change has an associated Jira, it's referenced either in the PR description, commit message, or branch name.
  • RFC: If this change has an associated RFC, please link it in the description.

PCI review checklist

  • I have documented a clear reason for, and description of, the change I am making.
  • If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
  • If applicable, I've documented the impact of any changes to security controls.

@Abhishek00810
Abhishek00810 requested a review from a team as a code owner February 4, 2026 20:58
@vercel

vercel Bot commented Feb 4, 2026

Copy link
Copy Markdown

@Abhishek00810 is attempting to deploy a commit to the HashiCorp Team on Vercel.

A member of the Team first needs to authorize it.

@Abhishek00810

Copy link
Copy Markdown
Contributor Author

@stevendpclark can you look into this, I think that can break in production if issue is valid, let me know what you think.

@hc-github-team-secure-vault-core

Copy link
Copy Markdown
Collaborator

Copy workflow completed!

From To Skipped Merge Commits Error
hashicorp/vault#31740 hashicorp/vault-enterprise#16587 0

@hc-github-team-secure-vault-core

Copy link
Copy Markdown
Collaborator

Copy pull request failed!

Origin Pull Request Copied Pull Request Commit SHA Error
#31740 https://github.com/hashicorp/vault-enterprise/pull/16587 https://github.com/hashicorp/vault-enterprise/commit/46ba6d5f1fb5b700225765627dfce3d3ed88d440 PATCH https://api.github.com/repos/hashicorp/vault/issues/31728: 403 Resource not accessible by personal access token []
Closed Issues #31728
Error: PATCH https://api.github.com/repos/hashicorp/vault/issues/31728: 403 Resource not accessible by personal access token []

@ncabatoff

Copy link
Copy Markdown
Collaborator

Hi @Abhishek00810, thanks for the contribution and for the detailed explanation of the issue. I've merged it as part of a PR in the enterprise version of this repo, with some small changes. It will be available in not the upcoming release, but the one after.

apurvagandhi pushed a commit to fidelity-contributions/hashicorp-vault that referenced this pull request Jul 16, 2026
… logging into main into ce/main (hashicorp#16602)

* Copy hashicorp#31740 into main



* fix(core/http): use PathUnescape for AWS ALB client cert decoding (fixes hashicorp#31728)

* fix(core/http): use PathUnescape for AWS ALB client cert decoding (fixes hashicorp#31728)

* Fix the upstream contributor's test (cluster.Start doesn't exist on main anymore).  

* Instead of just switching to PathEscape, try both QueryEscape and PathEscape to maintain backwards compat.  

* Add support for using x-forwarded-for without configuring a TLS listener.

---------

Co-authored-by: Abhishek Dadwal <dadwalabhishek10@gmail.com>
Co-authored-by: ncabatoff <ncabatoff@hashicorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Certificate auth headers from AWS ALBs cannot be correctly parsed

4 participants