Create optional provider config setting for using or disabling the child token by tstraley · Pull Request #775 · hashicorp/terraform-provider-vault

tstraley · 2020-05-25T21:25:07Z

This adds a new (often requested) vault provider configuration setting which can allow users to disable the creation of the intermediate child token (strongly discouraged, but made available for the cases that need it).

provider "vault" {
    address = "https://vault.testdomain"
    create_intermediate_child_token = false
}

(Open to changing the name of this config item, or related env var, but I like to be verbose with settings that typically shouldn't be adjusted without reading the documentation).

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Closes #29
Closes #722
Relates #550
Relates #368
Relates #192

Relates to the following as well:
hashicorp/terraform#16457
hashicorp/terraform#14839
https://groups.google.com/forum/#!topic/terraform-tool/wtlLrKVQlAo

Release note for CHANGELOG:

provider: Add skip_child_token provider configuration setting to control the use of ephemeral child tokens.

Output from acceptance testing:

$ make testacc TESTARGS='-run TestAccChildToken'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccChildToken -timeout 120m
?   	github.com/terraform-providers/terraform-provider-vault	[no test files]
?   	github.com/terraform-providers/terraform-provider-vault/cmd/coverage	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-vault/util	0.012s [no tests to run]
=== RUN   TestAccChildToken
--- PASS: TestAccChildToken (1.15s)
PASS
ok  	github.com/terraform-providers/terraform-provider-vault/vault	1.168s

hashicorp-cla · 2020-11-25T00:03:38Z

All committers have signed the CLA.

nikolaishields · 2021-01-26T21:41:30Z

@tstraley Thankyou so much for the contribution! looks like the only thing holding this back is for you to sign the CLA as mentioned above by @hashicorp-cla. Don't want to be a bother, but me and my team could really use this once its merged upstream.

nikolaishields · 2021-01-26T22:31:52Z

After further investigation it appears that @tstraley has been largely inactive on github for 6+ months, with the only blocker being that they have not accepted the hashicorp CLA.

tstraley · 2021-02-09T15:44:57Z

@nikolaishields - thanks for the bumps, this had fallen way off my radar. I am now seeking approval from my employer's license review board before I can accept the hashicorp CLA and get this moved on.

tstraley · 2021-03-03T17:00:06Z

Can I please get reviews from the maintainers of this project? I believe this is ready for merge (long overdue). Thanks :)

tstraley · 2021-03-17T00:22:36Z

@catsby @tomhjp @tvoran @jasonodonnell - is this something any of you could look at?

matttrach · 2021-04-02T19:25:40Z

I think this could resolve several other feature requests, what do you think @tyrannosaurus-becks ?

matttrach · 2021-04-02T19:30:39Z

As for the security concern, in many use cases an encrypted Terraform backend could mitigate a lot of the risk.

tyrannosaurus-becks · 2021-04-02T22:10:55Z

Hi @matttrach , I'm no longer involved with this project and have moved on from HashiCorp.

matttrach · 2021-04-05T21:10:12Z

We will miss you @tyrannosaurus-becks !
@jasonodonnell would you mind taking a look?

surendarkumavat · 2021-05-21T06:06:27Z

@tstraley Many Thanks for your contribution on this. With the remote disk encrypted backend becoming the norm, this has become a much awaited feature. Maintainers(@catsby @tomhjp @tvoran @jasonodonnell ), any updates on the ETA?

rmgpinto · 2021-07-13T17:21:06Z

Can this be merged and released please?

rmgpinto · 2021-07-13T20:15:33Z

Or is there a way to merge this change locally and replace the hashicorp provider?

rmgpinto · 2021-07-21T09:56:31Z

@jasonodonnell can this be approved please? It's just a small change on 3 files.

jasonodonnell · 2021-07-21T14:05:35Z

Hi @rmgpinto, we're still discussing the security implications of this change internally and haven't reached a decision yet. This PR is high quality and it looks good from a code perspective, but the exposure of Vault tokens via Terraform state files and logs are definitely concerning (even when the defaults are security focused).

All that said this is on our immediate radar and we'll make a decision very soon. Thanks for your patience on this!

rmgpinto · 2021-07-21T14:17:42Z

Thanks for the quick feedback!

alan2wong · 2021-08-30T15:35:04Z

@jasonodonnell, could you provide some insight as to what the security implications are for this particular PR?

tstraley · 2021-10-09T18:30:30Z

@jasonodonnell giving this a bump.

I want to call out that all of the security considerations are well documented and this isn't something that users would use unless they need it and understand those risks. Having the option available has been a major request for many years now, and I'm not hearing any reason this should be held up further.

Please let me know if you'd like me to resolve the new merge conflicts and re-push so this can finally land.

ghost · 2021-11-08T10:54:04Z

@tstraley thank you for this. Just commenting here for an update, so I could just terraform apply --auto-approve all the way when approved.

benashz · 2021-11-18T21:34:15Z

@tstraley we are taking another look at this PR. Would you mind rebasing it off of main?

Thanks,

Ben

Adds setting and documentation for create_intermediate_child_token provider configuration item. This defaults to true and is strongly discouraged to disable. When disabled, it allows users to use the vault terraform provider without the provided token creating ephermeral child token.

Adds tests for the `create_intermediate_child_token` vault provider config setting. Also refactors a couple test helper functions to avoid duplicate code in common testing patterns.

tstraley · 2021-11-24T02:18:33Z

@benashz - just rebased, resolved conflicts, and pushed updated version up. Please have a look and let me know if you'd like any changes.

Happy 🦃

benashz

This is looking pretty good so far. I have provided some initial feedback.

Thanks,

Ben

@benashz

Per pull request review from @benashz This name change includes a flip in the logic: if skipping the creation of the child token is true, we do not create it and assign it, if false we follow the default behavior of using the child token. Tests and documentation were updated to match.

benashz

Looking good! Just a few minor suggestions.

tstraley · 2021-12-02T16:23:10Z

@benashz just giving this a gentle bump for re-review.

benashz

So we just need to address the documentation comment, then +1.

Thank you for your contribution to HashiCorp!

benashz · 2021-12-06T21:47:06Z

Thanks again @tstraley for all the effort on this PR. It's much appreciated!

Based on review of 7 highly-commented PRs (hashicorp#1815, hashicorp#1821, hashicorp#902, hashicorp#2128, hashicorp#1877, hashicorp#2548, hashicorp#775) on hashicorp/terraform-provider-vault: - Use d.Get() instead of d.GetOk() for boolean field validation per SDKv2 best practice (d.GetOk returns (false,false) for booleans set to false, a known limitation flagged in PRs hashicorp#1815 and hashicorp#2128) - Add grace_period validation when dual_account_mode is enabled (must be > 0; catches misconfiguration at provider level before Vault API) - Add standby field test assertions (standby_username, standby_dn, standby_password) to dual-account data source acceptance test per PR hashicorp#2128 reviewer feedback: 'make assertions about what we expect' - Clarify grace_period docs: required when dual_account_mode is true, must be >= 5s and < rotation_period - Fix data source docs section header consistency Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

ghost added size/L documentation labels May 25, 2020

jasonodonnell self-requested a review July 13, 2021 22:36

tvoran mentioned this pull request Sep 15, 2021

Vault provider is unable to access data with a non-root token #29

Closed

benashz self-requested a review November 18, 2021 19:19

tstraley added 2 commits November 23, 2021 19:04

Add parameterized tests for new child token option

e73ac1b

Adds tests for the `create_intermediate_child_token` vault provider config setting. Also refactors a couple test helper functions to avoid duplicate code in common testing patterns.

tstraley force-pushed the optional-child-token-config branch from 2a704f1 to e73ac1b Compare November 24, 2021 02:14

benashz added this to the 3.1.0 milestone Nov 24, 2021

benashz requested changes Nov 26, 2021

View reviewed changes

benashz requested changes Nov 29, 2021

View reviewed changes

Comment thread vault/provider.go Outdated

Comment thread vault/provider_test.go Outdated

Comment thread vault/provider_test.go

Comment thread vault/provider_test.go

Comment thread vault/provider_test.go Outdated

benashz self-requested a review November 29, 2021 16:10

Address review comments for code cleanup and naming convention

650ef8c

tstraley force-pushed the optional-child-token-config branch from c060b9b to 650ef8c Compare November 30, 2021 00:29

benashz requested changes Dec 6, 2021

View reviewed changes

Comment thread website/docs/index.html.markdown Outdated

benashz self-requested a review December 6, 2021 16:27

benashz approved these changes Dec 6, 2021

View reviewed changes

Update website/docs/index.html.markdown

8dee6cb

benashz merged commit cbfbceb into hashicorp:main Dec 6, 2021

This was referenced Dec 7, 2021

Encrypt logged HTTP secret header values. #1250

Merged

Update skip_child_token docs. #1252

Merged

This was referenced Dec 7, 2021

Feature Request: add provider argument use_child_token to control child token creation #722

Closed

[Feature Request] Skip creating child token if token used is itself an orphan #550

Closed

This was referenced Dec 7, 2021

add use_child_token to control child token creation #723

Closed

Cannot create child token without root priviliges #368

Closed

benashz mentioned this pull request Dec 16, 2021

Avoid token generation if max_ttl is zero #1174

Closed

Uh oh!

Conversation

tstraley commented May 25, 2020 • edited by benashz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Community Note

Uh oh!

hashicorp-cla commented Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikolaishields commented Jan 26, 2021

Uh oh!

nikolaishields commented Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tstraley commented Feb 9, 2021

Uh oh!

tstraley commented Mar 3, 2021

Uh oh!

tstraley commented Mar 17, 2021

Uh oh!

matttrach commented Apr 2, 2021

Uh oh!

matttrach commented Apr 2, 2021

Uh oh!

tyrannosaurus-becks commented Apr 2, 2021

Uh oh!

matttrach commented Apr 5, 2021

Uh oh!

surendarkumavat commented May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rmgpinto commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rmgpinto commented Jul 13, 2021

Uh oh!

rmgpinto commented Jul 21, 2021

Uh oh!

jasonodonnell commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rmgpinto commented Jul 21, 2021

Uh oh!

alan2wong commented Aug 30, 2021

Uh oh!

tstraley commented Oct 9, 2021

Uh oh!

ghost commented Nov 8, 2021

Uh oh!

benashz commented Nov 18, 2021

Uh oh!

tstraley commented Nov 24, 2021

Uh oh!

benashz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benashz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tstraley commented Dec 2, 2021

Uh oh!

Uh oh!

benashz left a comment

Choose a reason for hiding this comment

Uh oh!

tstraley commented May 25, 2020 •

edited by benashz

Loading

hashicorp-cla commented Nov 25, 2020 •

edited

Loading

nikolaishields commented Jan 26, 2021 •

edited

Loading

surendarkumavat commented May 21, 2021 •

edited

Loading

rmgpinto commented Jul 13, 2021 •

edited

Loading

jasonodonnell commented Jul 21, 2021 •

edited

Loading