Add support for forwarded Tls-Client-Cert

[JasonN3]

This is to allow cert auth through a reverse proxy that terminates the SSL connection.

The changes link off of the existing processes that are used to process X-Forwarded-For header and related headers. If X-Forwarded-* headers are accepted by the existing code, the existing request will be modified so the forwarded client certificate will be treated like it was the client certificate in the request. This way all existing checks still apply. This is the same process that was already being used for the remote address. Since the header that includes the forwarded certificate differs between reverse proxies, the header is configurable using the option x_forwarded_for_client_cert_header. This approach was chosen because it requires minimal changes and takes advantage of the existing code.

I was able to compile and test this using Traefik and the header X-Forwarded-Tls-Client-Cert.

Resolves: #12178

[hashicorp-cla]

All committers have signed the CLA.

[mpalmi]

Greetings, @JasonN3! Thanks for your submission.

Have you had a chance to look at our Contribution Guidelines? Please make sure to review and update the PR to be in compliance with these guidelines.

I will follow up with a more thorough review of the PR, but figured I'd flag the minor things to get the ball rolling on this.

In order to kick off CI, you'll need a changelog entry. The CATEGORY seems like improvement, since we'll be enabling a new usecase.

Other than that, a bit more description would help provide a bit more context for a review. Particularly interested in:

Your pull request should have a description of what it accomplishes, how it does so, and why you chose the approach you did. PRs should include unit tests that validate correctness and the existing tests must pass. Follow-up work to fix tests does not need a fresh issue filed.

Please let us know if you have any questions.

[cipherboy]

@JasonN3 To clarify @mpalmi questions a little (if I may...) I think I read #12178 as applying to the CertAuth plugin, whereas this appears to help Agents' mutual TLS.

I think the former only needs to have the mount tuning enabled, and then read the request headers rather than the connection TLS cert.

It'd be great to get clarity as to use case.

The other question I have is of precedence. Sometimes you'll want the LB to do mutual TLS on both ends of the connection (mTLS to client and mTLS to Vault)... But obviously there's issues with defaulting to the header by default over the mTLS value (if it is end-client supplied, this is bad). It might also warrant a discussion of if the header-provided cert should be re-verified by Vault rather than assumed to be already validated by the LB/...

[vercel]

Deployment failed with the following error:

The provided GitHub repository does not contain the requested branch or commit reference. Please ensure the repository is not empty.

[JasonN3]

@mpalmi, I hadn't read that. I've updated my description and added the changelog entry. Let me know if I need to add more detail to either one or if there's anything else I missed.

@cipherboy, the intent for that issue was to allow client certificate authentication through a reverse proxy. To expand on the use case and the reason I submitted that issue initially, I have a CA server that each of my servers gets a certificate from. I want my servers to be able to query Vault to get a secret automatically using that certificate as authentication. Prior to this change, I could not do that because my Vault server is behind a LB. The LB would terminate the SSL connection and start a new connection. This would cause the client cert to be in the header X-Forwarded-Tls-Client-Cert, which wasn't being read by the cert authentication method.
Do you have any examples where Vault can handle mTLS other than in cert auth? As for it being re-verified by Vault, it is. The changes I made do not modify the code that verifies the certificate. It just moves the certificate from a header to the root of the request. The rest of the processes that verify the certificate still apply. I did test this by testing the authentication using a certificate issued by a CA that Vault was not told to trust and authentication rightfully failed.

[cipherboy]

Ah, so I see, this does indeed do client cert now that I trace it. A test might be good in that regard ;-)

I was thinking https://www.vaultproject.io/docs/agent/autoauth/methods/cert was it, but I believe this just allows Agent to interpose and provide its own cert for cert auth, rather than using the client's (and passing that through for Vault).

[JasonN3]

I'm not sure what test can be added. In order to test just the header being read and moved, it would need a complete certificate since the certificate is being parsed and I'm not sure if there's one available during the testing.

Since this change enables cert auth to function with the Vault server behind a LB, it would allow for auto-auth using the cert method as well. Nothing would change on the client side. When the client makes the request to the LB, the LB rewrites the request. This change is just to allow the Vault server to rewrite the request back to what the client originally made before being processed.

[cipherboy]

@JasonN3 I'd probably add one in the cert auth tests:

https://github.com/hashicorp/vault/blob/main/builtin/credential/cert/backend_test.go

There's a lot of test certificates available to use, and when you call vault.NewTestCluster you can add a new option to the test config which enables this header and adds it to the listener:

vault/vault/testing.go

Line 1143 in 87801ec

type TestClusterOptions struct {

vault/vault/testing.go

Line 1472 in 87801ec

// Listener setup

You'll have to add a header to the client and invalidate the token, e.g.:

vault/builtin/logical/pki/backend_test.go

Lines 5214 to 5215 in 87801ec

	// Ensure that the CA is returned correctly if we give it the old time.
	client.AddHeader("If-Modified-Since", beforeOldCAGeneration.Format(time.RFC1123))

vault/api/client.go

Line 970 in 87801ec

func (c *Client) ClearToken() {

And then re-auth with the header, probably by direct Logical().Write(...) would be my idea.

My 2c. :-)

[JasonN3]

I ended up just copying the test certificate from those test and creating 2 tests that are purely focused on the modified code.
Test 1 (reject_bad_cert_in_header) will verify that a certificate that is not properly formatted will be rejected.
Test 2 (pass_cert) will verify that a certificate that is properly formatted will be moved in to r.TLS.PeerCertificates.

Verification that the certificate is valid was not part of this modification as that is done elsewhere using the certificates normally provided in r.TLS.PeerCertificates by the client.

[cipherboy]

Thanks @JasonN3! I'll take another look by Friday, feel free to ping me if I haven't gotten around to it...

[JasonN3]

I found a minor logic error that shouldn't affect anything, but I fixed it anyway.

Also, @cipherboy, ping

[cipherboy]

\o sorry about the delay @JasonN3.

I think we're going to need to discuss this a little more internally first. We've definitely talked about the idea before and would like to include the feature, but I think we need to have a few more conversations about implementation first. In particular, I think we'd worried about header use vs connection use, and what the final deployment architecture looks like (how would you ensure clients can't directly connect to the Vault system and provide header values, would you do a separate mTLS to the LB, &c).

I'll let you know what comes of those discussions, they'll hopefully occur in the next couple of weeks.

[JasonN3]

I understand the need to discuss it further. If there's any concerns, feel free to post them. I might have an answer for you.

As for a client directly connecting to the Vault server and providing the header, that's not possible unless the server owner specifies XForwardedForAuthorizedAddrs to include the client source addresses or the LB is configured to forward that header when received. This is the same limitations that exist for overriding the source address using X-Forwarded-For. Using the provided change, a TLS connection between the LB and the server is still required as the client certificate auth method mandates a TLS connection. It just won't use mTLS because the LB doesn't have a client certificate. Even if you were to add in mTLS between the LB and the Vault server, it wouldn't change anything the client could do. All that mTLS connection would do is verify the LB but then you still need to verify the client certificate that was passed in the header. The LB can't use the client certificate that was passed to it by the client because the LB doesn't have the private key.

The process it goes through:

1. The client makes a request to the LB using https and specifies the client certificate's public key
2. The LB accepts (either blindly or after verifying it against a CA cert) the client certificate and moves the certificate to a header. The client will have to have a valid private key regardless of whether the certificate is checked at this point because otherwise it won't be able to encrypt the data for the TLS connection and communication will fail. The LB should also be configured to override/erase the Tls-Client-Cert (or equivalent) header if it was passed in.
3. The LB makes a new connection to the Vault server using https and specifies the client certificate's public key in the header
4. The Vault server ensures the source (the LB) is in the XForwardedForAuthorizedAddrs list, makes sure the connection from the LB is using https, and validates the client certificate is valid and allowed to log in. It will then generate the token with the appropriate permissions and pass it back to the LB to be passed back to the client.

[JasonN3]

@cipherboy, how'd the internal discussions go? Are there any changes you guys would like me to make in order to better meet any requirements?

[cipherboy]

Sorry about the delay. We were initially worried about whether or not useful validation would occur on the chain, but we've been unable to reproduce the attack we were thinking of. (Could you send an unrelated chain in addition to a cert you control, wherein the unrelated chain was a valid trusted chain by Vault, but the cert you controlled would sign the TLS handshake).

I think ideally, we'd like to see the LB's credentials persisted rather than potentially overwritten here, to allow us to do LB credential pinning in the future. I think as such, it makes sense to put this directly on the request, rather than on the req.TLS.PeerCertificates object, to allow plugins and operators to decide which they want to use.

[cipherboy]

Note that, if no TLS was used between the LB and us, r.TLS will be nil.

[JasonN3]

Cert authentication still requires TLS to be used even between the LB and server. That case shouldn't occur, but I added I added a condition for that anyway in 7396430

[cipherboy]

Right, it does currently, but if we were to implement this, we wouldn't necessarily need to require it if the LB and Vault were colocated on the same machine (e.g., over localhost). This is a useful deployment scenario with Vault Enterprise's Seal Wrap FIPS compliance, wherein Vault's TLS isn't necessarily FIPS compliant, but putting a FIPS-validated LB in front of it would be useful.

Edit to add: as it is right now, this simply panics if HTTP is used, which is less than ideal. :-)

[JasonN3]

The condition I added in 7396430 should keep it from panicking. It will just return an error now.

Allowing non-secured traffic between the LB and the server (even if it is on localhost) would open it up to a MITM attack or traffic sniffing. Returning an error instead of allowing a potential insecure setup would be the preferred option in my opinion. Especially in situations where FIPS is involved.

[joecorall]

Allowing non-secured traffic between the LB and the server (even if it is on localhost) would open it up to a MITM attack or traffic sniffing.

Not sure this is true if traffic between the LB and server are encrypted in transit. e.g. if Vault is running in Cloud Run behind a serverless LB.

[cipherboy]

Same would also apply for unix: https://developer.hashicorp.com/vault/docs/configuration/listener/unix

Having root on the box is sufficient to dump Vault's memory anyways, and so there's more interesting attack vectors than intercepting (plaintext) unix socket traffic.

[JasonN3]

The link @joecorall referenced is for VM to VM or container to container traffic. Within a VM, no cloud is going to affect the traffic. The situation I was referencing was when you have your LB setup to offload SSL and not use SSL to the Vault process. At that point, the connection between the LB and the Vault server would be unencrypted. If either the LB or the server running Vault are compromised, you would be able to forward the traffic to your own server so you can manipulate the data or capture the unencrypted data.

As for unix sockets, that would be more difficult since you would be fighting against the Vault server to capture the data first since I'm not aware of a way for multiple processes to safely read from the same socket.

[JasonN3]

Sorry about the delay. We were initially worried about whether or not useful validation would occur on the chain, but we've been unable to reproduce the attack we were thinking of.

Glad to hear the attacks failed. This change shouldn't open up new forms of attacks, but it's definitely better to test it.

Could you send an unrelated chain in addition to a cert you control, wherein the unrelated chain was a valid trusted chain by Vault, but the cert you controlled would sign the TLS handshake

The certificate for the https communication is separate from the peer certificate that is used for authentication, so they can be and should be unrelated. It would be a bad idea to have a public CA trusted to validate your authentication since you can't control what certs they will sign, but you will want your https traffic to be signed by a public CA (or at least a CA the clients already trust) to validate your server. For the tests I've done, my Vault server is signed by a public CA and authentication is signed by an internal CA.

I think ideally, we'd like to see the LB's credentials persisted rather than potentially overwritten here, to allow us to do LB credential pinning in the future. I think as such, it makes sense to put this directly on the request, rather than on the req.TLS.PeerCertificates object, to allow plugins and operators to decide which they want to use.

I'm confused by this. What credentials would the LB normally pass? As far as I can find, a LB can't form a mTLS connection with a backend server. It can only pass along the mTLS connection that the client tried for form with the LB or form a new TLS connection to the backend server. Do you know of any LBs that can initiate a mTLS connection to the backend server? If not, req.TLS.PeerCertificates can only be used by the client and not a LB so nothing would be lost when it is updated. As for plugins or operators detecting if it was used, they can still check for the header. The header is never removed. It is just copied to where the certificate normally would be if there wasn't a LB so the rest of the cert authentication code doesn't need to be modified to support this situation and can behave the same regardless of whether a LB is used.

[cipherboy]

Could you send an unrelated chain in addition to a cert you control, wherein the unrelated chain was a valid trusted chain by Vault, but the cert you controlled would sign the TLS handshake

The certificate for the https communication is separate from the peer certificate that is used for authentication, so they can be and should be unrelated. It would be a bad idea to have a public CA trusted to validate your authentication since you can't control what certs they will sign, but you will want your https traffic to be signed by a public CA (or at least a CA the clients already trust) to validate your server. For the tests I've done, my Vault server is signed by a public CA and authentication is signed by an internal CA.

Right, this was the theoretical attack: can a client of the LB send a two part chain (valid client cert, unrelated attacker cert), with the LB validating the TLS connection is signed by the attacker cert, thus passing through to Vault, and with cert-auth succeeding but verifying and approving based on the valid client cert -- even though there was no signature by its corresponding private key. The answer is no, as TLS stacks will strip out unrelated certs from the validation (at least, from our testing, Go's and OpenSSL's will).

---

I'm confused by this. What credentials would the LB normally pass? As far as I can find, a LB can't form a mTLS connection with a backend server.

Why can't a LB have a separate mTLS to Vault? That's a common scenario, initiating a new mTLS from LB to a backend service with its own credential, to ensure the data is still protected and authenticated (preventing non-LB from contacting backend service directly). This is perhaps more common when the service is over the network, rather than a LB running on the same instance as the backend service. See e.g., https://docs.nginx.com/nginx/admin-guide/security-controls/securing-http-traffic-upstream/ .

[JasonN3]

I'm confused by this. What credentials would the LB normally pass? As far as I can find, a LB can't form a mTLS connection with a backend server.

Why can't a LB have a separate mTLS to Vault? That's a common scenario, initiating a new mTLS from LB to a backend service with its own credential, to ensure the data is still protected and authenticated (preventing non-LB from contacting backend service directly). This is perhaps more common when the service is over the network, rather than a LB running on the same instance as the backend service. See e.g., https://docs.nginx.com/nginx/admin-guide/security-controls/securing-http-traffic-upstream/ .

I hadn't come across that situation. Normally I just see TLS between the LB and server and not mTLS. In that case, yes, it would cause some information to be lost. However, it would depend on where you plan on verifying the mTLS cert. It should probably be verified before any of the X-Forwarded headers are processed. That way there is less code run before the LB client cert would be rejected. In that case it wouldn't matter if the LB client cert is lost. Alternatively, I could prepend the client cert to the list of PeerCertificates, which is the commit I just pushed (b7a1ffe). Only the first certificate is processed for login (builtin/credential/cert/path_login.go line 230) so any additional certificates would still be available, but the current code would ignore them.

[se-chris-thach]

Any update on when this might be included in a release?

[cwchristerw]

I'm looking forward to this and its good that there is support for using custom header 😄 I would like to know something, I have seen multiple guides like...

proxy_set_header X-SSL-CERT $ssl_client_escaped_cert;

$ssl_client_escaped_cert is client certificate in PEM format url encoded. Would this work in future? I have also added...

@JasonN3 Is it okay that, $ssl_client_escaped_cert was also including in the beginning -----BEGIN%20CERTIFICATE-----%0 and in the end %0A-----END%20CERTIFICATE-----%0A. I looked test, its why ask 😄

I used curl --request POST --cert cert.pem --key privkey.pem --http1.1 --data '{"name": "web"}' https://vault.example.com/v1/auth/cert/login --verbose command to get $ssl_client_escaped_cert as response header. I added add_header X-SSL-CERT $ssl_client_escaped_cert always; to Nginx config.

X-SSL-CERT: -----BEGIN%20CERTIFICATE-----%0AMIIESDCCAzCgAwIBAgIUWWQIzsdICMRWpzb2yPYRJZVHaYgwDQYJKoZIhvcNAQEL%0ABQAwDzENMAsGA1UEAxMEVGVzdDAeFw0yMzA4MDIwNDE3NTZaFw0yMzA4MDIwNTE5%0AMTZaMBsxGTAXBgNVBAMTEG5vZGUxLmRvbWFpbi5jb20wggIiMA0GCSqGSIb3DQEB%0AAQUAA4ICDwAwggIKAoICAQCswKCIqCoKTvAySZnAnPPV8iGoS59xJnyYmJ4TChK0%0A1%2BhB8htli6L34Kh6aZ52YRMCXpKpXJEDmSjK3DtdivqOLYFoHPSe%2FBgs64NMT1Vp%0ApoWirh4oVCMb6ojGKHpYMczH3BzP5xAUfNPGWFqK5%2BI7YVQv7oR7gy8yR28bJz%2Bm%0A3Qpt5uyzCol5cC2HOepfbF6x44a%2BjLSVdNUbQxtL0n1RcNQgwur7o4c6F59gc1Bc%0Ao%2FTfJSPsWZ62ejaYBOVnV9SxvjNsw0TirxOKJ38tyHba1pTSISeaG57DD6LDku%2Bl%0Ailj3dPb6Fq8KudrgUVz9dfFeQHDhH%2F7q9fz0aaFlVbnYY6y7yIhW%2B1DJQLXTSacX%0AMq2OVwUYjlmx7NUS8bCVqwDiB4zUjlrAjE%2BxKtQ0s4B4lRNLsSoXKBT0F7N6sBBf%0ANP4BJSq6Zt5E3qMvPTPgWRAfT99m0tI13DfMWhPnAvu7kmiFsAMUzmD4B%2BUlb7ak%0A6qeyRglas9CXuVYfw7Y3s16zI7a8GSm%2BJfHxvhX70OsyTSh3CUP9WBCAmzC%2FY%2F8x%0A8jiCj87DFTXZkClCbPTH2tWukNeKg2eY0f8eL7zu2wKeBskKjakBb6SHBDkvGMah%0AyS%2F76TFpO1W8KABoGl%2Bu%2B%2BOOrz2eeQbQYTbRyJHPStOkOY6V%2F%2BA0TvqNlzWu12tD%0AnQIDAQABo4GPMIGMMA4GA1UdDwEB%2FwQEAwIDqDAdBgNVHSUEFjAUBggrBgEFBQcD%0AAQYIKwYBBQUHAwIwHQYDVR0OBBYEFIk038Eim%2Bvy%2BDPpobgdCUygwDcdMB8GA1Ud%0AIwQYMBaAFBved0eZwAgo2x1d8vOoVw3dLdDwMBsGA1UdEQQUMBKCEG5vZGUxLmRv%0AbWFpbi5jb20wDQYJKoZIhvcNAQELBQADggEBAJ1Ci7iU9bCApACMP73idO7zxCf7%0AGEmTsrRU5PgbOfJ3WMf0YBu6ynWPovGg2rh4N4KUs9cO0YJCmuerrZUIIQjPF%2FSa%0A42NWoAO35yogcXX6YS3ujz8eYfplh9rrQDQvDopJCpX6YeEY5oM2uEslCickiRI%2B%0AlhnedZ9tOdsJiAfheEeyEI%2BaQiZoXYYopMtmPHbcQ9jPRf54LeNlw%2FgX6o4BJV83%0AQhHcGh%2FrRYwjPE%2Farj2pUqY01b0z7zRAvn8g6Ju5qXzw6D3t507imFwEbE9WMFFe%0A24ZJVFCPjSN842GmrR56Dh3gR7TKXxstaNoSA0tmul9unu6tw8T%2Bx%2FtMzXQ%3D%0A-----END%20CERTIFICATE-----%0A

Here is URL encoded character list

%0A New Line (\n)
%2B +
%2F /
%3D =

In addition of that I used also...

ssl_client_certificate /etc/nginx/certs/example.com/chain.pem;
ssl_verify_client optional_no_ca;

I use optional_no_ca because I dont want to validate client certificate in reverse proxy, I had to use some CA cert to pass requirement of having ssl_client_certificate when using ssl_verify_client.

https://nginx.org/en/docs/http/ngx_http_ssl_module.html#variables

This might be useful when someone might make tutorial, how to configure this with Nginx for example.

[Cajga]

We are also looking forward to get this merged and would assign some developer time if needed to get this rolled.

@JasonN3 nice work and thanks for keeping it up!

[se-chris-thach]

LGTM

[Gunni]

Please merge?

[sgmiller]

Finally getting back around to reviewing this, sorry for the delay. Could you make one final tweak and fix the spelling around delimited in the documentation?

[JasonN3]

Finally getting back around to reviewing this, sorry for the delay. Could you make one final tweak and fix the spelling around delimited in the documentation?

I just want to make sure I'm fixing what you're seeing. Is this the line you want fixed?

Comma delimeters list that specifies the decoders that will be used to decode the client certificate.

Do you just want it to say Comma delimited instead of Comma delimeters or is there something else I'm overlooking?

[JasonN3]

@sgmiller I think I fixed the typo you wanted. Let me know if you meant something else or if there's anything else you want changed

[cwchristerw]

I would like to make sure that this would be backported 1.14.x?

[sgmiller]

This would be considered a new feature, and would not be backported. We're just taking one more pass on the security of this, thanks for the change @JasonN3 .

[sgmiller]

Apologize to everyone interested in this feature for the time it's taken. Approved.

[JasonN3]

@sgmiller, do you know what next steps are left? It's been a couple of weeks since your approval.

[VioletHynes]

I'm going to hit Update Branch as there was previously an issue with our build / completed-successfully GHA that prevented it passing on community submitted PRs, as that seemed to be an issue blocking this PR, and that might have been an issue getting this merged. It has since been fixed, and the update should have everything passing as expected (except Vercel, unfortunately our hands are tied there).

I'll leave it up to @sgmiller as to when to merge this though as I've not been as close to this issue.

[sgmiller]

@JasonN3 Do you want the honor of merging? If not I'll merge it in a couple of days.

[JasonN3]

@sgmiller, do you know what next steps are left? It's been a couple of weeks since your approval.

@JasonN3 Do you want the honor of merging? If not I'll merge it in a couple of days.

The only merge I can do is merge the updates to main into my fork. I can't merge my fork into this repo.

[sgmiller]

@sgmiller, do you know what next steps are left? It's been a couple of weeks since your approval.

@JasonN3 Do you want the honor of merging? If not I'll merge it in a couple of days.

The only merge I can do is merge the updates to main into my fork. I can't merge my fork into this repo.

Doh, right. Okay, I'll merge.