[fixes 31700] Add mTLS support for http backend by way of client cert & key, as well as enterprise cacert.

[scr-oath]

For any http servers that require mTLS (client certificates), as well as using custom cacert rather than skipping cert validation altogether, add the following settings:

• cacert: verify server certs using this trust file
• cert: provide a public cert from the client to server
• key: sign the public cert and all TLS traffic with a private key

[hashicorp-cla]

All committers have signed the CLA.

[crw]

Thanks for this submission!

[kmoe]

Thank you for this PR. I'd appreciate if you could provide a guide for manually testing this change.

[scr-oath]

Thank you for this PR. I'd appreciate if you could provide a guide for manually testing this change.

Well… I did put a unit test in there, which describes its usage - I teased out the "self-signed" certificate used in httptest and wrote its pem files out to use for trust of the server (since it's self signed it is the CA as well), and to present to the server as client cert.

I guess to test this manually… one would create a cert that's used ONLY for the CA, then create a server cert and a client cert that's signed by that… then add that to the RootCAs of the client and the ClientCAs of the server as well as check assumptions about the principals' reported Common Name (CN field of subject).

This technique would thwart the chicken-and-egg issue I had with using the test cert - that is, in order to get the cert, I needed to fire up the server, but in order to add to the ClientCAs, one would need to know the signing cert up front… which could be more easily done if that cert were known ahead of time and configured in the server before starting it in the ClientCAs field as well as set the ClientAuth value of the servers tls.Config to RequireAndVerifyClientCert rather than the RequireAnyClientCert which I used due to not having this signing cert available before starting the server.

[Magnitus-]

Thank you for this PR. I'd appreciate if you could provide a guide for manually testing this change.

I'm going on vacation on the 21th and I have things to wrap up at work before I go, but on my return, I could publish a test setup with our etcd backend if you like. I want this change :)

[scr-oath]

Hi - we started using the s3+dynamo solution for a few needs so this lost urgency for me.

But... I thought I had added a test to show the use of it... I'll try to make that test more specifically about this, if not also document setting up outside of test, as well as address the naming convention comments so this can merge.

[scr-oath]

FWIW, I had to force push to make the license appear signed (I recall reading that commits made prior to adding a user address don't get tied to your account) so rebasing and pushing seemed to have solved that

[scr-oath]

Thank you for this PR. I'd appreciate if you could provide a guide for manually testing this change.

The test should now serve as the guide - I grew it a little from the original state to create the three separate certs:

1. ca.cert.pem
2. server.crt
3. client.crt This way, both client and server can share the same signing authority and use that to trust the other.

The test itself implements a rudimentary http backend server that just stuffs value in an in-memory map. I thought of making the whole thing be built with mock, but ended up letting the mock be attached, so that calls (or absence of them) to the server could be verified.

[scr-oath]

I'm not quite sure how to get Vercel to ✅ - if anyone can authorize that, please do, or if that's an ignorable check, please let me know.

[crw]

Thanks for this update! You can ignore the Vercel check. I'll bump this PR for review.

[scr-oath]

Just merged and resolved the conflict in go.sum via go mod tidy…

Any chance someone can review and merge this?

[crw]

Thanks! I'll bump this for review.

[Magnitus-]

Not an employee at Hashicorp, but I started looking at it if it can help in any way, otherwise just ignore this.

Slight readability nitpick here:

	var tlsConfig tls.Config
	if data.Get("skip_cert_verification").(bool) {
		// ignores TLS verification
		tlsConfig.InsecureSkipVerify = true
		client.Transport.(*http.Transport).TLSClientConfig = &tlsConfig
	}
	clientCACertificatePem := data.Get("client_ca_certificate_pem").(string)
	if clientCACertificatePem != "" {
		caCertificateData, err := os.ReadFile(clientCACertificatePem)
		if err != nil {
			return fmt.Errorf("failed to read file %s: %w", clientCACertificatePem, err)
		}
		tlsConfig.RootCAs = x509.NewCertPool()
		if !tlsConfig.RootCAs.AppendCertsFromPEM(caCertificateData) {
			return fmt.Errorf("failed to append certs from file %s: %w", clientCACertificatePem, err)
		}
		client.Transport.(*http.Transport).TLSClientConfig = &tlsConfig
	}
	clientCertificatePem := data.Get("client_certificate_pem").(string)
	clientPrivateKeyPem := data.Get("client_private_key_pem").(string)
	if clientCertificatePem != "" && clientPrivateKeyPem != "" {
		certificate, err := tls.LoadX509KeyPair(clientCertificatePem, clientPrivateKeyPem)
		if err != nil {
			return fmt.Errorf("cannot load certifcate from %s and %s: %w", clientCertificatePem, clientPrivateKeyPem, err)
		}
		tlsConfig.Certificates = []tls.Certificate{certificate}
		client.Transport.(*http.Transport).TLSClientConfig = &tlsConfig
	}

I think I'd change it to this:

	var tlsConfig tls.Config

	if data.Get("skip_cert_verification").(bool) {
		// ignores TLS verification
		tlsConfig.InsecureSkipVerify = true
	}

	clientCACertificatePem := data.Get("client_ca_certificate_pem").(string)
	if clientCACertificatePem != "" {
		caCertificateData, err := os.ReadFile(clientCACertificatePem)
		if err != nil {
			return fmt.Errorf("failed to read file %s: %w", clientCACertificatePem, err)
		}
		tlsConfig.RootCAs = x509.NewCertPool()
		if !tlsConfig.RootCAs.AppendCertsFromPEM(caCertificateData) {
			return fmt.Errorf("failed to append certs from file %s: %w", clientCACertificatePem, err)
		}
	}
	
	clientCertificatePem := data.Get("client_certificate_pem").(string)
	clientPrivateKeyPem := data.Get("client_private_key_pem").(string)
	if clientCertificatePem != "" && clientPrivateKeyPem != "" {
		certificate, err := tls.LoadX509KeyPair(clientCertificatePem, clientPrivateKeyPem)
		if err != nil {
			return fmt.Errorf("cannot load certifcate from %s and %s: %w", clientCertificatePem, clientPrivateKeyPem, err)
		}
		tlsConfig.Certificates = []tls.Certificate{certificate}
	}
	
	client.Transport.(*http.Transport).TLSClientConfig = &tlsConfig

I think you can probably just set the tls client config all the time (I know I do this in my own code, though I always use tls), the defaults seem sensible: https://pkg.go.dev/crypto/tls#Config

[scr-oath]

I think you can probably just set the tls client config all the time (I know I do this in my own code, though I always use tls), the defaults seem sensible: https://pkg.go.dev/crypto/tls#Config

Yeah, I thought about that too but didn't as I was focusing more on the desired change - i.e. to add the client's CA, cert + key.

As it stands today, if you don't need to add the insecure, ca, cert&key then you can just leave the default http transport. I feel more comfortable keeping that behavior if nothing else than because you only choose this customized tlsConfig when you're an advanced user, and I'd hate to find that I had inadvertently changed behavior for users that hadn't customized this at all as they're less apt to be able to or patient enough to debug.

Also… note the word probably in your comment 😄 - don't get me wrong - I'm confident that just using tlsConfig always would have a high probability of working too, but, again, it's not the focus of this PR and I'd rather leave the behavior as only clobber tlsConfig if configured to need to do so.

[Magnitus-]

I think you can probably just set the tls client config all the time (I know I do this in my own code, though I always use tls), the defaults seem sensible: https://pkg.go.dev/crypto/tls#Config

Yeah, I thought about that too but didn't as I was focusing more on the desired change - i.e. to add the client's CA, cert + key.

As it stands today, if you don't need to add the insecure, ca, cert&key then you can just leave the default http transport. I feel more comfortable keeping that behavior if nothing else than because you only choose this customized tlsConfig when you're an advanced user, and I'd hate to find that I had inadvertently changed behavior for users that hadn't customized this at all as they're less apt to be able to or patient enough to debug.

Also… note the word probably in your comment smile - don't get me wrong - I'm confident that just using tlsConfig always would have a high probability of working too, but, again, it's not the focus of this PR and I'd rather leave the behavior as only clobber tlsConfig if configured to need to do so.

My thought on the matter is that usually, when people use that config, they don't set every setting there is on it (usually just the cert settings and maybe the skip verify setting), so for the vast majority of that config, they go with the default and if the default wasn't sensible (ex: if you don't set the CA, it will default to the system's CA store), they'd need to set most of those parameters to desired value (and obviously the complexity of using it even for simple use cases at that point would explode), so barring mistakes in implementation, I think its reasonable to assume that the config's defaults are safe for tls use cases.

The main thing I haven't validated on my end in our own libraries (because I use tls almost everywhere) is what happens when you set that config and don't use tls. My guess is that it must be ok, but that I'd validate.

But I agree that at the risk of making the code less readable, the thing to do if you want to be ultra-safe for backward compatibility when you have lots of existing users would be not to set that config at all if you don't need it (even though I'm 95%+ sure its ok).

Alternatively, you could add an if guard at the end:

if (clientCertificatePem != "" && clientPrivateKeyPem != "") || data.Get("skip_cert_verification").(bool) {
    client.Transport.(*http.Transport).TLSClientConfig = &tlsConfig
}

You have the clumsiness of an extra if, but you do the assignment only once (for me, doing the assignment up to 3 times is less readable, though admittedly that's a subjective assessment).

[scr-oath]

You have the clumsiness of an extra if, but you do the assignment only once (for me, doing the assignment up to 3 times is less readable, though admittedly that's a subjective assessment).

Ok; makes sense. FWIW, I moved the TLS stuff out to its own method to focus on that task alone, and put the guard in the beginning (I felt, if subjectively, that this was more go-style to check/return up front, then continue with the work if needed)

In the process of moving to this style, I also noticed that we were replacing a fresh http client with another fresh http client, so, when refactoring this way, I saved a new/copy in the bargain 😄

[vercel[bot]]

@scr-oath is attempting to deploy a commit to the HashiCorp Team on Vercel.

A member of the Team first needs to authorize it.

[scr-oath]

Why not use testify - you're already bringing it in https://github.com/hashicorp/terraform/blob/main/go.sum#L605 it's much more readable and has IDE support - you can you see diffs, etc in the format of the errors that it outputs. It's painful to have to type up the fmt string when there's an excellent assertion library.

[eastebry]

👍 The mtls bits here look good from my perspective. Deferring to the TF core team regarding the questions on the testing framework and final approval.

[kmoe]

Thanks for making the requested changes. The testify library appears in go.sum because it is used by one of Terraform Core's dependencies, but it is not used in the Core codebase itself (go mod why github.com/stretchr/testify). We've found that having human-readable context in test failures is helpful when debugging..

[kmoe]

Please also rebase your branch against main.

[scr-oath]

Thanks for making the requested changes. The testify library appears in go.sum because it is used by one of Terraform Core's dependencies, but it is not used in the Core codebase itself (go mod why github.com/stretchr/testify). We've found that having human-readable context in test failures is helpful when debugging..

Ok - I removed use of testify and now go.mod does not contain reference to it again.

[scr-oath]

Please also rebase your branch against main.

Done - rebased and force pushed - now all changes appear directly from the merge-base (no merges)

[Magnitus-]

Any news?

[github-actions[bot]]

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

[github-actions[bot]]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.