Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

scr-oath
Copy link

@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
Author

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

@dlanoire
Copy link

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
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
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
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
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

linde commented May 14, 2024

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

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.

None yet

4 participants