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

add CA certificate verification and insecure option #125

Merged
merged 1 commit into from
Oct 24, 2022

Conversation

jkroepke
Copy link
Contributor

@jkroepke jkroepke commented Mar 27, 2022

Hi,

this PR is 1:1 copy of #29. The original author looks inactive and is not able to sign the CLA. To unblock the process, I'm create new PR here.

This PR will let user validate TLS certificate with a provided root CA or choose an insecure option with no validation at all.

May solves hashicorp/terraform-provider-aws#11426 and have a lot of common usage.

Link for tests jkroepke#1

Closes: #29

  • Create an issue that insecure should conflicts with ca_cert_pem.

@jkroepke jkroepke marked this pull request as ready for review March 27, 2022 13:17
@jkroepke jkroepke requested a review from a team as a code owner March 27, 2022 13:17
@barryib
Copy link

barryib commented Mar 29, 2022

I'm active. But the way Hashicorp is dealing with this repository make me feel that they don't need further contributions on this. #29 is 2 years old with lot of effort of keeping the PR up to date. I think I've already signed this CLA twice since I opened this PR. So I ended by forking this into https://github.com/terraform-aws-modules/terraform-provider-http.

BTW, feel free to maintain this one and I hope that someone at Hashicorp will get this merged.

@jkroepke
Copy link
Contributor Author

@detro I would like to pull you for a review here, since I saw some recent activity on your side. In case you are too busy, please ping others.

Thanks.

@jkroepke
Copy link
Contributor Author

@barryib I saw your fork and currently l‘m using it.

Since you are remove the forked provider from the eks module, there is some risk that the fork will be deprecated.

@jkroepke
Copy link
Contributor Author

Rebased.

@jkroepke jkroepke force-pushed the add-server-cert-verification branch 2 times, most recently from 18a2037 to 3df1515 Compare June 14, 2022 14:28
@jkroepke jkroepke force-pushed the add-server-cert-verification branch from 3df1515 to e189244 Compare July 5, 2022 17:49
@jkroepke
Copy link
Contributor Author

jkroepke commented Jul 5, 2022

Rebased on #142

@bendbennett Can we have an review/merge soon?

Copy link
Contributor

@bendbennett bendbennett left a comment

Choose a reason for hiding this comment

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

@jkroepke thank you for submitting the PR.

I've left a couple of comments about PlanModifiers and some suggested refactoring to bring the structure of the tests into line with the structure used in #151

internal/provider/data_source_http.go Outdated Show resolved Hide resolved
internal/planmodifiers/planmodifiers.go Outdated Show resolved Hide resolved
internal/provider/data_source_http_test.go Outdated Show resolved Hide resolved
internal/provider/data_source_http_test.go Outdated Show resolved Hide resolved
internal/provider/data_source_http_test.go Outdated Show resolved Hide resolved
internal/provider/data_source_http_test.go Outdated Show resolved Hide resolved
internal/provider/data_source_http_test.go Outdated Show resolved Hide resolved
internal/provider/data_source_http_test.go Outdated Show resolved Hide resolved
internal/provider/data_source_http_test.go Outdated Show resolved Hide resolved
internal/provider/data_source_http_test.go Outdated Show resolved Hide resolved
@jkroepke jkroepke force-pushed the add-server-cert-verification branch from e189244 to 492c2a2 Compare July 6, 2022 20:22
@jkroepke
Copy link
Contributor Author

jkroepke commented Jul 6, 2022

Hi @bendbennett

thanks for the review. I apply all the suggestion except on the ExceptError. since giving the URL would not work without any escape here.

Copy link
Contributor

@bendbennett bendbennett left a comment

Choose a reason for hiding this comment

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

I apply all the suggestion except on the ExceptError. since giving the URL would not work without any escape here.

I've added suggestions for using the URL in ExpectError.

Can you also add a CHANGELOG.md entry for 3.1.0 (unreleased)

internal/provider/data_source_http_test.go Outdated Show resolved Hide resolved
internal/provider/data_source_http_test.go Outdated Show resolved Hide resolved
internal/provider/data_source_http_test.go Outdated Show resolved Hide resolved
internal/provider/data_source_http_test.go Outdated Show resolved Hide resolved
internal/provider/data_source_http_test.go Outdated Show resolved Hide resolved
internal/provider/data_source_http_test.go Outdated Show resolved Hide resolved
@jkroepke jkroepke force-pushed the add-server-cert-verification branch from 492c2a2 to f676eab Compare July 7, 2022 08:26
@jkroepke jkroepke force-pushed the add-server-cert-verification branch from f676eab to eeb9bb1 Compare July 7, 2022 08:28
@jkroepke
Copy link
Contributor Author

jkroepke commented Jul 7, 2022

Changed applied. make test is fine.

jkr@joe-nb terraform-provider-http % make test
go test -v -cover -timeout=120s -parallel=4 ./...
?       github.com/terraform-providers/terraform-provider-http  [no test files]
=== RUN   TestDataSource_200
--- PASS: TestDataSource_200 (0.99s)
=== RUN   TestDataSource_404
--- PASS: TestDataSource_404 (0.90s)
=== RUN   TestDataSource_withAuthorizationRequestHeader_200
--- PASS: TestDataSource_withAuthorizationRequestHeader_200 (0.77s)
=== RUN   TestDataSource_withAuthorizationRequestHeader_403
--- PASS: TestDataSource_withAuthorizationRequestHeader_403 (0.80s)
=== RUN   TestDataSource_utf8_200
--- PASS: TestDataSource_utf8_200 (0.74s)
=== RUN   TestDataSource_utf16_200
--- PASS: TestDataSource_utf16_200 (0.73s)
=== RUN   TestDataSource_x509cert
--- PASS: TestDataSource_x509cert (0.73s)
=== RUN   TestDataSource_UpgradeFromVersion2_2_0
    data_source_http_test.go:177: Acceptance tests skipped unless env 'TF_ACC' set
--- SKIP: TestDataSource_UpgradeFromVersion2_2_0 (0.00s)
=== RUN   TestDataSource_WithCACertificate
=== PAUSE TestDataSource_WithCACertificate
=== RUN   TestDataSource_InsecureTrue
=== PAUSE TestDataSource_InsecureTrue
=== RUN   TestDataSource_InsecureFalse
=== PAUSE TestDataSource_InsecureFalse
=== RUN   TestDataSource_InsecureUnconfigured
=== PAUSE TestDataSource_InsecureUnconfigured
=== CONT  TestDataSource_WithCACertificate
=== CONT  TestDataSource_InsecureFalse
=== CONT  TestDataSource_InsecureTrue
=== CONT  TestDataSource_InsecureUnconfigured
--- PASS: TestDataSource_InsecureUnconfigured (0.22s)
--- PASS: TestDataSource_InsecureFalse (0.22s)
--- PASS: TestDataSource_InsecureTrue (0.86s)
--- PASS: TestDataSource_WithCACertificate (0.86s)
PASS
coverage: 85.1% of statements
ok      github.com/terraform-providers/terraform-provider-http/internal/provider        6.845s  coverage: 85.1% of statements

@jkroepke
Copy link
Contributor Author

jkroepke commented Jul 7, 2022

I will add an additional test case for invalid certificates.

@jkroepke jkroepke force-pushed the add-server-cert-verification branch from eeb9bb1 to 0d63144 Compare July 7, 2022 08:33
@bendbennett bendbennett added this to the v3.1.0 milestone Jul 7, 2022
@jkroepke jkroepke force-pushed the add-server-cert-verification branch from 0d63144 to 1892d8a Compare July 7, 2022 08:38
@jkroepke
Copy link
Contributor Author

jkroepke commented Jul 7, 2022

CHANGELOG.md entry added.

@jkroepke jkroepke requested a review from bendbennett July 7, 2022 09:12
docs/data-sources/http.md Outdated Show resolved Hide resolved
internal/provider/data_source_http.go Show resolved Hide resolved
internal/provider/data_source_http.go Outdated Show resolved Hide resolved
internal/provider/data_source_http.go Outdated Show resolved Hide resolved
internal/provider/data_source_http_test.go Outdated Show resolved Hide resolved
@jkroepke
Copy link
Contributor Author

Branch rebased on main.

@jkroepke jkroepke force-pushed the add-server-cert-verification branch from 23bd0a9 to 1afbe6b Compare August 2, 2022 07:03
@jkroepke
Copy link
Contributor Author

jkroepke commented Aug 2, 2022

Rebased.

@bendbennett can we merge this PR soon?

@bendbennett bendbennett modified the milestones: v3.1.0, v3.2.0 Aug 22, 2022
@jkroepke
Copy link
Contributor Author

@bendbennett Can I help here to move forward here?

@bendbennett
Copy link
Contributor

@jkroepke apologies for the delay in responding. We are just finalising some other changes to the http provider. We will get back to you as soon as we can.

@jkroepke
Copy link
Contributor Author

jkroepke commented Sep 9, 2022

I would much appreciate it, if this can. get merged soon.

@jkroepke jkroepke requested review from bendbennett and removed request for detro September 29, 2022 15:31
@bookshelfdave
Copy link
Contributor

hello @jkroepke - thank you for your contribution and apologies for the delay, we'll be able to put more energy into this over the next few weeks.

Copy link
Contributor

@bendbennett bendbennett left a comment

Choose a reason for hiding this comment

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

Apologies for the delay in getting back to you on this PR @jkroepke.

CHANGELOG.md Outdated Show resolved Hide resolved
internal/provider/data_source_http.go Show resolved Hide resolved
@jkroepke
Copy link
Contributor Author

jkroepke commented Oct 14, 2022

Rebased. all comments addressed. Tests for the Validators added.

I hope, we are complete here now.

@jkroepke
Copy link
Contributor Author

jkroepke commented Oct 14, 2022

It feels like the schemavalidator.ConflictsWith results into problems on 0.14?

No idea, whats wrong here...


edit: It should work now. On my local machine. 0.14 is green now.

@github-actions github-actions bot added size/XL and removed size/L labels Oct 15, 2022
@bendbennett bendbennett merged commit 09a48c1 into hashicorp:main Oct 24, 2022
@bryantbiggs
Copy link

woohoo, it made it! thanks @jkroepke and maintainers 🎉

@jkroepke jkroepke deleted the add-server-cert-verification branch October 24, 2022 15:56
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 26, 2024
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