-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
lang/funcs: Redact sensitive values from function errors #30067
lang/funcs: Redact sensitive values from function errors #30067
Conversation
internal/lang/funcs/encoding.go
Outdated
log.Printf("[DEBUG] the result of decoding the provided string is not valid UTF-8: %s", sDec) | ||
return cty.UnknownVal(cty.String), fmt.Errorf("the result of decoding the provided string is not valid UTF-8") | ||
log.Printf("[DEBUG] the result of decoding the provided string is not valid UTF-8: %s", redactIfSensitive(sDec, strMarks)) | ||
return cty.UnknownVal(cty.String).WithMarks(strMarks), fmt.Errorf("the result of decoding the provided string is not valid UTF-8") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a precedent for returning the arguments' marks on an unknown value with an error? The unknown value from the error shouldn't need to propagate the marks any further, and I don't think we're returning marks in all cases below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good point! This was the precedent I cargo-culted:
terraform/internal/lang/funcs/collection.go
Lines 314 to 315 in ba6a64e
return cty.UnknownVal(cty.DynamicPseudoType).WithMarks(markses...), fmt.Errorf( | |
"lookup failed to find '%s'", lookupKey) |
But having poked around inside HCL for a bit I'm really pretty sure that the value is unused if we return an error from a function. I'll remove the marks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
} | ||
|
||
if fi.Mode().IsRegular() { | ||
return cty.True, nil | ||
return cty.True.WithMarks(pathMarks), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me the marks on the input path seem logically distinct from the boolean existence of a file. While the input may clearly be sensitive, should using that cause the result to be sensitive? I guess it follows pretty closely to the fact that using the input in a conditional would cause the conditional result to retain the same marks, but just wanted to pose the question in case we don't really need cause the marks to propagate through here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing implementation (using the automatic re-marking from cty) does mark the result as sensitive:
$ tf console
> fileexists("main.tf")
false
> fileexists(sensitive("main.tf"))
(sensitive)
I don't think that's particularly useful, assuming that we don't intend the sensitive value implementation to protect against side-channel attacks. I'll plan to remove the marks from this function, but if anyone disagrees with that change I'm happy to leave them in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a small hazard here that someone was using nonsensitive(fileexists(...))
to achieve this effect before and would now need to remove the nonsensitive
. I don't know how likely it is, but I mention it in case you think it might affect the risk/reward tradeoff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does make me more nervous about removing the marks. I hadn't thought of this situation where it breaks compatibility, no matter how unlikely it is. Given that we don't have any users requesting this change or considering it a bug, I'll leave it as-is for now.
Some function errors include values derived from arguments. This commit is the result of a manual audit of these errors, which resulted in: - Adding a helper function to redact sensitive values; - Applying that helper function where errors include values derived from possibly-sensitive arguments; - Cleaning up other errors which need not include those values, or were otherwise incorrect.
1b0c35a
to
5d7cb81
Compare
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. |
Some function errors include values derived from arguments. This commit is the result of a manual audit of these errors, which resulted in:
Fixes #30033
Example