Skip to content

[fixes 254] Add mTLS support with client cert and key.#211

Merged
ansgarm merged 24 commits intohashicorp:mainfrom
scr-oath:add-mTLS
Mar 28, 2025
Merged

[fixes 254] Add mTLS support with client cert and key.#211
ansgarm merged 24 commits intohashicorp:mainfrom
scr-oath:add-mTLS

Conversation

@scr-oath
Copy link
Copy Markdown
Contributor

@scr-oath scr-oath commented Dec 16, 2022

This PR is to support mTLS support for the http data source, by allowing the client_cert and client_key to be passed.

@scr-oath scr-oath requested a review from a team as a code owner December 16, 2022 01:54
scr-oath added a commit to scr-oath/terraform that referenced this pull request Dec 16, 2022
kmoe added a commit to hashicorp/terraform that referenced this pull request Jan 26, 2023
… & key, as well as enterprise cacert. (#31699)

* Add mTLS support for http backend by way of client cert & key, as well as enterprise cacert.

* Fix style.

* Skip cert validation to be sure error is related to missing client cert; not untrusted server cert.

* Remove misplaced err check.

* Fix the size of test using http backend.

* Just for correctness, include all certs in the pem encoded cert - sometimes certs come with a chain of their signers.

* Adjusted names as recommended in PR comments.

* Adjusted names to be full-length and more descriptive.

* Added full-fledged testing with mTLS http server

* Fix goimports.

* Fix the names of the backend config.

* Exclusive lock for write and delete.

* Revert "Fix goimports."

This reverts commit 7d40f60.

* goimports just for server test.

* Added the go:generation for the mock.

* Move the TLS configuration out to make it more readable - don't replace the HTTPClient as the retryablehttp already creates one - just configure its TLS.

* Just switch the client/data params - felt more natural this way.

* Update internal/backend/remote-state/http/backend.go

Co-authored-by: kmoe <5575356+kmoe@users.noreply.github.com>

* Update internal/backend/remote-state/http/testdata/gencerts.sh

Co-authored-by: kmoe <5575356+kmoe@users.noreply.github.com>

* Update internal/backend/remote-state/http/backend.go

Co-authored-by: kmoe <5575356+kmoe@users.noreply.github.com>

* Update internal/backend/remote-state/http/backend.go

Co-authored-by: kmoe <5575356+kmoe@users.noreply.github.com>

* Update internal/backend/remote-state/http/backend.go

Co-authored-by: kmoe <5575356+kmoe@users.noreply.github.com>

* Update internal/backend/remote-state/http/backend.go

Co-authored-by: kmoe <5575356+kmoe@users.noreply.github.com>

* the location of the file name is not sensitive.

* Added error if only one of client_certificate_pem and client_private_key_pem are set.

* Remove testify from test cases; use t.Error* for assert and t.Fatal* for require.

* Fixed import consistency

* Just use default openssl.

* Since file(...) is so trivial to use, changed the client cert, key, and ca cert to be the data.

See also hashicorp/terraform-provider-http#211

Co-authored-by: Sheridan C Rawlins <scr@ouryahoo.com>
Co-authored-by: kmoe <5575356+kmoe@users.noreply.github.com>
@scr-oath
Copy link
Copy Markdown
Contributor Author

@kmoe can you help merge this one too? It's for non-backend http to use mTLS as well

@dlanoire
Copy link
Copy Markdown

Hello. Do you now why this merge request is pending because this is a very usefull feature?

@scr-oath scr-oath changed the title Add mTLS support with client cert and key. [fixes 254] Add mTLS support with client cert and key. Mar 23, 2023
@bendbennett
Copy link
Copy Markdown
Contributor

Hi @scr-oath and @dlanoire 👋

I have approved the workflow but I'm afraid that reviewing this PR is currently on-hold as we need to triage the changes and our team’s focus is currently elsewhere at the moment.

@scr-oath
Copy link
Copy Markdown
Contributor Author

scr-oath commented Mar 23, 2023

@bendbennett

Hi @scr-oath and @dlanoire 👋

I have approved the workflow but I'm afraid that reviewing this PR is currently on-hold as we need to triage the changes and our team’s focus is currently elsewhere at the moment.

I just ran make generate to appease the diff checker - do you know what the ETA might be for triage? Will this be looked at / considered even if lower priority than current attention?

@bendbennett
Copy link
Copy Markdown
Contributor

@scr-oath I have added this PR to our triage list and will endeavour to discuss this as a team in an upcoming triage meeting. The PR will be considered but it is hard to give a definitive answer on ETA.

@bendbennett
Copy link
Copy Markdown
Contributor

Hi @scr-oath 👋

Thank you for submitting this PR and opening the associated issue. We have just discussed this PR at our triage meeting and have come to the conclusion that we'd like to hold off on reviewing and potentially merging until we have a clearer idea on the level of community interest in these changes.

We have to consider each enhancement request (issue) or PR on the basis of community interest alongside the consequences of increasing the surface area of the provider. We will leave the issue and PR open and will revisit if there is community interest signalled by up votes.

@linde
Copy link
Copy Markdown

linde commented May 14, 2024

+1 that this is needed, definitely situations where you need to connect with client certs.

@ElfoLiNk
Copy link
Copy Markdown

Hi @scr-oath,

I see that this now has conflicts. It would be great to have it reviewed and merged.

@austinvalle, could you take a look?

Thanks!

@scr-oath
Copy link
Copy Markdown
Contributor Author

scr-oath commented Feb 20, 2025

Wow, almost two years old; sheesh… yeah, I'll try to dust this off and resolve conflicts.

DONE

@larryboymi
Copy link
Copy Markdown

Would love this to get merged... is it being considered?

@ansgarm
Copy link
Copy Markdown
Member

ansgarm commented Mar 25, 2025

Hi everyone 👋
Just wanted to give a short update that we're planning to accept / add this. We're going to look into a way to not include the private key in the repository to not trip our security scanners, but other than that this PR is already looking pretty good!

@github-actions github-actions Bot removed the size/XL label Mar 26, 2025
@scr-oath
Copy link
Copy Markdown
Contributor Author

ok - took a few tries but got all testify uses deleted resulting in ZERO changes to go.mod 😎

@scr-oath scr-oath requested a review from ansgarm March 26, 2025 17:31
@scr-oath
Copy link
Copy Markdown
Contributor Author

Whew… ok - done with self-reviews and style corrections… I think this is pretty close to minimal now

Copy link
Copy Markdown
Member

@ansgarm ansgarm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much, @scr-oath!

One small thing and we're good to go 🚀 (Sorry I missed that in my earlier review)

Comment thread internal/provider/data_source_http.go
Comment thread internal/provider/cert_test.go
@ansgarm ansgarm added this to the v3.5.0 milestone Mar 27, 2025
Co-authored-by: Ansgar Mertens <mertens.ansgar@gmail.com>
@scr-oath scr-oath requested a review from ansgarm March 27, 2025 14:58
Copy link
Copy Markdown
Member

@ansgarm ansgarm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ansgarm ansgarm merged commit 5d77f98 into hashicorp:main Mar 28, 2025
4 checks passed
@scr-oath scr-oath deleted the add-mTLS branch March 28, 2025 16:05
ansgarm added a commit that referenced this pull request Mar 31, 2025
ansgarm added a commit that referenced this pull request Mar 31, 2025
ansgarm added a commit that referenced this pull request Mar 31, 2025
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Apr 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants