terraform-provider-http icon indicating copy to clipboard operation
terraform-provider-http copied to clipboard

add CA certificate verification and insecure option

Open jkroepke opened this issue 3 years ago • 28 comments

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 https://github.com/hashicorp/terraform-provider-aws/pull/11426 and have a lot of common usage.

Link for tests https://github.com/jkroepke/terraform-provider-http/pull/1

Closes: #29

  • [x] Create an issue that insecure should conflicts with ca_cert_pem.

jkroepke avatar Mar 27 '22 13:03 jkroepke

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.

barryib avatar Mar 29 '22 20:03 barryib

@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 avatar Mar 29 '22 23:03 jkroepke

@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 avatar Mar 30 '22 00:03 jkroepke

Rebased.

jkroepke avatar Jun 14 '22 14:06 jkroepke

Rebased on #142

@bendbennett Can we have an review/merge soon?

jkroepke avatar Jul 05 '22 17:07 jkroepke

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.

jkroepke avatar Jul 06 '22 20:07 jkroepke

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 avatar Jul 07 '22 08:07 jkroepke

I will add an additional test case for invalid certificates.

jkroepke avatar Jul 07 '22 08:07 jkroepke

CHANGELOG.md entry added.

jkroepke avatar Jul 07 '22 08:07 jkroepke

@bendbennett I hope, everything is fine now.

jkroepke avatar Jul 10 '22 08:07 jkroepke

@jkroepke looks like the linter is complaining. Are you happy to fix the linting issues?

bendbennett avatar Jul 13 '22 10:07 bendbennett

@bendbennett I hope I get all lint issues.

About the workflow approval condition. Consider to select Require approval for first-time contributors who are new to GitHub instead Require approval for first-time contributors.

Now I have to wait for an approval again to see the results from golintci.

See: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#controlling-changes-from-forks-to-workflows-in-public-repositories

jkroepke avatar Jul 13 '22 11:07 jkroepke

@jkroepke apologies for any inconvenience caused but our current security policies require that the workflow settings are configured to Require approval for first-time contributors.

bendbennett avatar Jul 13 '22 13:07 bendbennett

Alright, then lets see what the next runs says.

jkroepke avatar Jul 13 '22 14:07 jkroepke

Currently, I have no idea whats going wrong here. I'm unable to reproduce the error locally.

TF_ACC=1 go test -v -cover -timeout 120m ./...
?       github.com/terraform-providers/terraform-provider-http  [no test files]
=== RUN   TestDataSource_200
--- PASS: TestDataSource_200 (0.76s)
=== RUN   TestDataSource_404
--- PASS: TestDataSource_404 (0.74s)
=== RUN   TestDataSource_withAuthorizationRequestHeader_200
--- PASS: TestDataSource_withAuthorizationRequestHeader_200 (0.75s)
=== RUN   TestDataSource_withAuthorizationRequestHeader_403
--- PASS: TestDataSource_withAuthorizationRequestHeader_403 (0.74s)
=== RUN   TestDataSource_utf8_200
--- PASS: TestDataSource_utf8_200 (0.75s)
=== RUN   TestDataSource_utf16_200
--- PASS: TestDataSource_utf16_200 (0.78s)
=== RUN   TestDataSource_x509cert
--- PASS: TestDataSource_x509cert (0.74s)
=== RUN   TestDataSource_UpgradeFromVersion2_2_0
--- PASS: TestDataSource_UpgradeFromVersion2_2_0 (3.75s)
=== RUN   TestDataSource_WithCACertificate
=== PAUSE TestDataSource_WithCACertificate
=== RUN   TestDataSource_WithCACertificateFalse
=== PAUSE TestDataSource_WithCACertificateFalse
=== 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_WithCACertificateFalse
=== CONT  TestDataSource_InsecureUnconfigured
--- PASS: TestDataSource_WithCACertificateFalse (0.23s)
--- PASS: TestDataSource_InsecureFalse (0.25s)
--- PASS: TestDataSource_InsecureUnconfigured (0.25s)
--- PASS: TestDataSource_InsecureTrue (0.85s)
--- PASS: TestDataSource_WithCACertificate (0.90s)
PASS
coverage: 86.7% of statements
ok      github.com/terraform-providers/terraform-provider-http/internal/provider        (cached)        coverage: 86.7% of statements


Edit: It seem like the error messages depends on the used golang version.

go1.17:

Error making request: Get "https://127.0.0.1:49383/": x509: certificate signed
by unknown authority

go1.18:

Error making request: Get "https://127.0.0.1:49383/": x509: “Acme Co”
certificate is not trusted

jkroepke avatar Jul 13 '22 15:07 jkroepke

Based on the error

Error tls: crypto/x509: system root pool is not available on Windows

from windows systems, I not longer reference the system root pool

jkroepke avatar Jul 14 '22 13:07 jkroepke

@bendbennett Tests are green.

jkroepke avatar Jul 15 '22 08:07 jkroepke

@detro @bendbennett Is this PR fine? Can I assist here to move forward?

jkroepke avatar Jul 19 '22 08:07 jkroepke

Edit: It seem like the error messages depends on the used golang version.

go1.17:

Error making request: Get "https://127.0.0.1:49383/": x509: certificate signed
by unknown authority

go1.18:

Error making request: Get "https://127.0.0.1:49383/": x509: “Acme Co”
certificate is not trusted

@jkroepke sorry for the delay in getting back to you. Given that the README states that Go 1.17 is a requirement, it's probably best that the test is modified along the following lines so that the test will pass when using Go 1.17 and will not break when we upgrade to 1.18:

ExpectError: regexp.MustCompile(fmt.Sprintf(`Error making request: Get "%s": x509: `, svr.URL)),

bendbennett avatar Jul 25 '22 10:07 bendbennett

I hope, everything is fine now. :-)

jkroepke avatar Jul 25 '22 16:07 jkroepke

Branch rebased on main.

jkroepke avatar Jul 31 '22 12:07 jkroepke

Rebased.

@bendbennett can we merge this PR soon?

jkroepke avatar Aug 02 '22 07:08 jkroepke

@bendbennett Can I help here to move forward here?

jkroepke avatar Aug 23 '22 22:08 jkroepke

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

bendbennett avatar Aug 25 '22 13:08 bendbennett

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

jkroepke avatar Sep 09 '22 22:09 jkroepke

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.

bookshelfdave avatar Oct 13 '22 15:10 bookshelfdave

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

I hope, we are complete here now.

jkroepke avatar Oct 14 '22 08:10 jkroepke

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.

jkroepke avatar Oct 14 '22 19:10 jkroepke

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

bryantbiggs avatar Oct 24 '22 15:10 bryantbiggs

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[bot] avatar May 26 '24 15:05 github-actions[bot]