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 #29156: Failing sensitive variables values are not logged #30552

Merged
merged 2 commits into from May 30, 2022

Conversation

gbataille
Copy link
Contributor

No description provided.

@hashicorp-cla
Copy link

hashicorp-cla commented Feb 18, 2022

CLA assistant check
All committers have signed the CLA.

@gbataille
Copy link
Contributor Author

#29156 looked good as a first issue for me. And I agree very much with the problem stated.

@gbataille
Copy link
Contributor Author

by the way,

before
Screenshot 2022-02-19 at 09 11 31

After
After

@crw
Copy link
Collaborator

crw commented Feb 25, 2022

Hi @gbataille, thanks for the contribution! We are looking into it, but I cannot commit to when this can be reviewed.

Copy link
Member

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

I think the general idea here is a good solution to this problem. There are a couple of improvements I'd like to make before going ahead.

Whenever possible, we should leave in the non-file source information (e.g. set using -var="foo=..."). I left an inline note about this.

For sensitive variables set from a file, I agree that the best option we have is to replace the source range. However, we need to inform the user where the variable value comes from, by including the .tfvars filename and line number in the detail string.

In this case, we can also follow the existing pattern for non-file sourced values of using the variable declaration as the source range, to give the user extra context.

Here are some example diagnostic designs based on these thoughts:

╷
│ Error: Invalid value for input variable
│
│   on main.tf line 1:
│    1: variable "foo" {
│
│ Unsuitable value for var.foo set using -var="foo=...": a bool is required.
╵
╷
│ Error: Invalid value for input variable
│
│   on main.tf line 1:
│    1: variable "foo" {
│
│ The given value is not suitable for var.foo, which is sensitive: a bool is required. Value
| declared at terraform.tfvars line 1.
╵

(Note the trailing full stop at the end of the description string, which is Terraform house style.)

Diagnostics for non-sensitive variable values should not change, so I've omitted these.

Let me know if anything is unclear here, or if you'd like me to review alternative designs. Thanks again!

// and not by the operator directly. In such a case, the failing secret value should not be
// displayed to the operator
if cfg.Sensitive {
subject = nil
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there's any need to remove the subject here when it comes from a non-file source, and we probably also need to treat the logic for appending to the diagnostic detail message separately too. I think therefore we probably want an if cfg.Sensitive { … } block inside both branches of the if nonFileSource != "" { … } statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's how I coded it originally and changed it back to that.

But I read all your comments and they make sense to me. I'll try to take some time this week to update the PR

@alisdair alisdair self-assigned this Mar 2, 2022
@crw crw added the enhancement label Mar 2, 2022
@gbataille gbataille force-pushed the 29156_do_not_log_sensitive_values branch from 00417a6 to e6a918d Compare April 24, 2022 07:29
@vercel
Copy link

vercel bot commented Apr 24, 2022

@gbataille is attempting to deploy a commit to the HashiCorp Team on Vercel.

A member of the Team first needs to authorize it.

@gbataille
Copy link
Contributor Author

sorry for not having come back sooner. Did not take the time.
So I agree with your comments

  • the var= message is already such that it does not show the variable value, so no reason to change it.
  • for the error coming from a variable file, I agree that it sounds better to have some snippets. I have however looked at how those "errors" are brough up, as Diagnostics (and actually multiple Diagnostics struct from different packages), all the way up.

--> I did not find yet a "smart" way to do it.

  • we could say that subject is not an hcl.Range anymore but a Range + a sensitive information. If sensitive is true, then we display location but not snippet when rendering a Range.
  • we could bring a sensitive flag in the Diagnostic(s) struct. In such a case, we would display source location information but not source snippet.
    Both of those are quite involved and I don't want to start on that without your opinion. Any thoughts, advices, options I failed to see?

I have in the meantime added the full stop at the end of the message and moved the logic to just one branch of the code

Copy link
Member

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

Thanks for the update! I think this is really close.

While I like your ideas about adding sensitive value redaction to diagnostics, I agree that it would be a lot of work. What I'd propose for this PR is to adjust one diagnostic to include a reference to the origin of the invalid variable value in the detail text, while keeping the code snippet pointing at the variable definition. See inline suggestion.

There are also some failing tests to fix up, but once those two things are done I think this is good to go!

// displayed to the operator
if cfg.Sensitive {
subject = nil
detail += fmt.Sprintf("\n\n%s is marked as sensitive. The failing variable assignment is not printed.", addr)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
detail += fmt.Sprintf("\n\n%s is marked as sensitive. The failing variable assignment is not printed.", addr)
detail += fmt.Sprintf("\n\n%s is marked as sensitive. Invalid value defined at %s.", addr, sourceRange.ToHCL())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I see. feels a bit hackish in the sense that diagnostics and subject handling are managed in common way up there, but I definitely can make that work (don't know when though, sorry :| )

@gbataille gbataille force-pushed the 29156_do_not_log_sensitive_values branch from e6a918d to 378ee6a Compare May 7, 2022 11:25
@gbataille
Copy link
Contributor Author

I feel stupid for not even thinking about the "local" solution. It is much simpler, although indeed a bit redundant.

Here is the result

image

@alisdair
Copy link
Member

I pushed a tiny tweak to add back the config declaration as the source parameter in both cases, resulting in diags like this:

diags

With that in place this looks good to me. We're in the freeze for 1.2.0 at the moment, but I'm marking this approved and plan to merge it once that freeze is over. Thanks again for working on this!

@alisdair alisdair added the 1.2-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label May 30, 2022
@alisdair alisdair merged commit a658797 into hashicorp:main May 30, 2022
@github-actions
Copy link

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@alisdair
Copy link
Member

This change will go out in the upcoming 1.2.2 release. Thanks once more!

@gbataille
Copy link
Contributor Author

gbataille commented May 31, 2022 via email

@github-actions
Copy link

github-actions bot commented Jul 1, 2022

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 Jul 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.2-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants