Warning for invalid authentication on Cloud #1890
Conversation
|
Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement Learn more about why HashiCorp requires a CLA and what the CLA includes Christian Doucette seems not to be a GitHub user. Have you signed the CLA already but the status is still pending? Recheck it. |
internal/client/config.go
Outdated
| func providerRunningInCloud() bool { | ||
| _, present := os.LookupEnv("TFC_AGENT_VERSION") | ||
| return present | ||
| } |
There was a problem hiding this comment.
Might be a stupid edge case, but if someone sets this variable themselves locally it will run as if on cloud. Shouldn't be a problem in this case because there are no security implications to having the additional warning
internal/client/config.go
Outdated
| // configure accepts the provider-level configuration values and creates a | ||
| // clientConfiguration using fallback values from the environment or CLI configuration. | ||
| func configure(tfeHost, token string, insecure bool) (*ClientConfiguration, error) { | ||
| func configure(tfeHost, token string, insecure bool) (*ClientConfiguration, bool, error) { |
There was a problem hiding this comment.
a lot of the lines of the PR are just for adding this new sendCredentialDeprecationWarning boolean that's passed from the client config to the client to the provider and then used to know whether to send the warning. if it would make more sense I could instead add an additional field to the ClientConfiguration and tfe.Client in go-tfe to accomplish the same thing
internal/provider/provider.go
Outdated
| Summary: "Authentication method invalid for TFE Provider with HCP Terraform and Terraform Enterprise", | ||
| Detail: "Use a TFE_TOKEN variable in the workspace or the token argument for the provider. This authentication method will be deprecated in a future version.", |
There was a problem hiding this comment.
let me know if this warning message makes sense
There was a problem hiding this comment.
Looks good though I think you should be explicit about which authentication method is invalid.
There was a problem hiding this comment.
It's called Autogenerated API Tokens in the docs
internal/provider/provider_next.go
Outdated
| tfeClient, sendCredentialDeprecationWarning, err := client.GetClient(data.Hostname.ValueString(), data.Token.ValueString(), data.SSLSkipVerify.ValueBool()) | ||
|
|
||
| if err != nil { | ||
| res.Diagnostics.AddError("Failed to initialize HTTP client", err.Error()) | ||
| return | ||
| } | ||
|
|
||
| if sendCredentialDeprecationWarning { | ||
| res.Diagnostics.AddWarning("Authentication method invalid for TFE Provider with HCP Terraform and Terraform Enterprise", "Use a TFE_TOKEN variable in the workspace or the token argument for the provider. This authentication method will be deprecated in a future version.") |
There was a problem hiding this comment.
I wasn't sure of the difference between provider.go and provider_next.go, should this warning be happening in both of them?
There was a problem hiding this comment.
I believe you should keep it in both
internal/client/client.go
Outdated
| func GetClient(tfeHost, token string, insecure bool) (*tfe.Client, error) { | ||
| config, err := configure(tfeHost, token, insecure) | ||
| func GetClient(tfeHost, token string, insecure bool) (*tfe.Client, bool, error) { | ||
| config, sendCredentialDeprecationWarning, err := configure(tfeHost, token, insecure) |
There was a problem hiding this comment.
Instead of drilling a third return value through to configure(), I'd probably add a new field to ClientConfiguration... something like, CredentialSource... then combine that outcome with providerRunningInCloud in the provider code, closer to the diagnostic
There was a problem hiding this comment.
I think that would make it much more flexible/readable as well as moving the different concerns together
christian-doucette
force-pushed
the
add_diagnostic_warning_when_using_config_authentication_on_terraform_cloud
branch
from
cf4b372 to
bec6278
Compare
christian-doucette
force-pushed
the
add_diagnostic_warning_when_using_config_authentication_on_terraform_cloud
branch
from
bec6278 to
5cc65fb
Compare
4 participants
Description
-Using CLI or Config file authentication for the TFE provider is valid on local but not cloud
-This causes a confusing error message due to the TFE provider trying to use the autogenerated single-use run token which is often not authorized for what its trying to do
-The new warning should make it more clear for users to resolve
Remember to:
Testing plan
External links
Include any links here that might be helpful for people reviewing your PR. If there are none, feel free to delete this section.
Output from acceptance tests
Please run applicable acceptance tests locally and include the output here. See testing.md to learn how to run acceptance tests.
If you are an external contributor, your contribution(s) will first be reviewed before running them against the project's CI pipeline.
Rollback Plan
Changes to Security Controls