-
Notifications
You must be signed in to change notification settings - Fork 69
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
Merge, flatten, length, lookup functions: more precise handling of marks #98
Merge, flatten, length, lookup functions: more precise handling of marks #98
Conversation
cty/function/stdlib/collection.go
Outdated
mapVar, marks := args[0].Unmark() | ||
|
||
// disregard marks on the key | ||
keyVal, _ := args[1].Unmark() |
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.
My intuition here would be that we'd want to merge any marks on the key string into the result's marks so that we don't "lose" them. For example, in Terraform's case if I try to use a sensitive key to look into a non-sensitive map then the result it ends up selecting could imply what the sensitive key was. Does that make sense, or am I missing a subtlety here? 🤔
It's a shame that this function ends up having to conditionally use either GetAttr
or Index
below because Index
already handles its argument being marked because an index is a cty.Value
itself, but attributes are always just normal Go strings and so it ends up being the caller's responsibility in that case. I guess there's a possible restructure here wkere we only do the Unmark
and AsString
inside the mapVar.Type().IsObjectType()
branch below, and just let the marked keyVal
pass directly into the mapVar.Index
call, but I guess that's a tradeoff between making the map case definitely match how direct indexing would work (because it uses the same function) vs. potentially causing the object and map cases to diverge due to being implemented differently. So I think I'd lean towards leaving the structure as-is for now but to add the key marks into marks
to be represented in the return value, unless we decide against doing that latter part after discussion.
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.
For example, in Terraform's case if I try to use a sensitive key to look into a non-sensitive map then the result it ends up selecting could imply what the sensitive key was
This is true, but it's also not my understanding of the kind of threats we're trying to model in Terraform. We're only trying to prevent accidental disclosure in UI, and since the key is not visible in the output of Lookup
, its marks are irrelevant in that case.
I can imagine that a more conservative approach would make sense for other uses of marks, but I can't think of any examples where that makes sense. My rule of thumb in these cases has been to propagate marks on values which may be present in the output (e.g. string transformations, object subsets) but not on values which are intermediate (e.g. list indices, map keys).
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.
Just noting I'll happily leave the decision on this up to you—I've pushed a fixup commit
(b535a2a) which I think does what you originally suggested here. If you're okay with doing the cleanup, please either squash that down or drop it before merge, depending on which direction you want to go.
Thanks for working on these, @alisdair! I left some comments above about some details but overall this matches my intuitions for how I'd expect these things to behave and I'd like to merge this once we've figured out together the question about marked keys in |
MergeFunc will now only mark the resulting map or object with marks from the given maps or objects as a whole, and not aggregate individual element/attribute marks at the top-level result. The individual element/attribute marks will still be preserved, but they'll remain attached to the individual values they came from, avoiding the result as a whole becoming marked.
FlattenFunc will now propagate marks from lists/sets/tuples which are flattened to the top level result. Marks on non-flattened elements are retained and not assigned to the resulting collection. There is a significant change to the flattener helper function to ensure that marks are propagated from all nested lists/sets/tuples, even after first encountering an unknown collection. Previously finding an unknown collection allowed us to terminate early, but to preserve marks we need to continue to iterate through the rest of the arguments.
ac6b489
to
6d3de8f
Compare
When taking the length of a marked collection, those marks should be preserved. Marks on individual elements should not propagate to the length result.
6d3de8f
to
2329ef7
Compare
Several changes to Lookup to handle marks in a way that I think makes more sense: - If the entire collection is marked, preserve the marks on any result (whether successful or fallback) - If a returned value from the collection is marked, preserve the marks from only that value, combined with any overall collection marks - Retain marks on the fallback value when it is returned, combined with any overall collection marks - Disregard marks on the key value, as map keys and object attributes cannot be marked - Retain collection marks when returning an unknown value for a not wholly-known collection
b535a2a
to
b8d7a58
Compare
Several changes to Lookup to improve how we handle marked values: - If the entire collection is marked, preserve the marks on any result (whether successful or fallback) - If a returned value from the collection is marked, preserve the marks from only that value, combined with any overall collection marks - Retain marks on the fallback value when it is returned, combined with any overall collection marks - Include marks on the key in the result, as otherwise the result it ends up selecting could imply what the sensitive value was - Retain collection marks when returning an unknown value for a not wholly-known collection See also zclconf/go-cty#98
Several changes to lookup to improve how we handle marked values: - If the entire collection is marked, preserve the marks on any result (whether successful or fallback) - If a returned value from the collection is marked, preserve the marks from only that value, combined with any overall collection marks - Retain marks on the fallback value when it is returned, combined with any overall collection marks - Include marks on the key in the result, as otherwise the result it ends up selecting could imply what the sensitive value was - Retain collection marks when returning an unknown value for a not wholly-known collection See also zclconf/go-cty#98
function/stdlib:
MergeFunc
precise handling of marksMergeFunc
will now only mark the resulting map or object with marks from the given maps or objects as a whole, and not aggregate individual element/attribute marks at the top-level result.The individual element/attribute marks will still be preserved, but they'll remain attached to the individual values they came from, avoiding the result as a whole becoming marked.
function/stdlib:
FlattenFunc
precise handling of marksFlattenFunc
will now propagate marks from lists/sets/tuples which are flattened to the top level result. Marks on non-flattened elements are retained and not assigned to the resulting collection.There is a significant change to the flattener helper function to ensure that marks are propagated from all nested lists/sets/tuples, even after first encountering an unknown collection. Previously finding an unknown collection allowed us to terminate early, but to preserve marks we need to continue to iterate through the rest of the arguments.
function/stdlib:
LengthFunc
precise handling of marksWhen taking the length of a marked collection, those marks should be preserved. Marks on individual elements should not propagate to the length result.
function/stdlib:
LookupFunc
precise handling of marksSeveral changes to
Lookup
to handle marks in a way that I think makes more sense: