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

sanitize: allow for the sanitization of sensitive values #34

Merged
merged 1 commit into from May 7, 2021

Conversation

vancluever
Copy link
Contributor

This adds a new package and functions for sanitization of values marked
as sensitive in the plan, where we can get particular data to do it.

This data is derived in a number of ways, also documented in the
top-level SanitizePlan function:

  • ResourceChanges are sanitized based on BeforeSensitive and
    AfterSensitive fields.

  • Variables are sanitized based on variable config data found in the
    root module of the Config.

  • PlannedValues are sanitized based on the values found in
    AfterSensitive in ResourceChanges. Outputs are sanitized according to
    the appropriate sensitivity flags provided for the output.

  • PriorState is sanitized based on the values found in BeforeSensitive
    in ResourceChanges. Outputs are sanitized according to the appropriate
    sensitivity flags provided for the output.

  • OutputChanges are sanitized based on the values found in
    BeforeSensitive and AfterSensitive. This generally means that any
    sensitive output will have OutputChange fully obfuscated as the
    BeforeSensitive and AfterSensitive in outputs are opaquely the same.

@vancluever
Copy link
Contributor Author

Converting to draft while we wait on dependent PRs (mitchellh/reflectwalk#25 and mitchellh/copystructure#36).

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Aside from some minor in-line comments, I looked at the public API overall https://pkg.go.dev/github.com/hashicorp/terraform-json@v0.10.1-0.20210503192259-b4ee84fc1086/sanitize and it made me wonder what's the use case for accepting replaceWith as an argument to some functions, as opposed to having a single instance of Sanitizer hold the "magic token" that would be then used in all methods internally?

Wouldn't that make the API simpler?

Do you anticipate the need for values to be replaced differently in each function call in Sentinel?

sanitize/sanitize_plan_test.go Show resolved Hide resolved
sanitize/copy.go Show resolved Hide resolved
sanitize/sanitize_plan.go Outdated Show resolved Hide resolved
sanitize/sanitize_state.go Show resolved Hide resolved
@vancluever
Copy link
Contributor Author

vancluever commented May 4, 2021

Do you anticipate the need for values to be replaced differently in each function call in Sentinel?

Yeah, this is exactly it. I want to leave it open-ended to possibly add things like a first-class sensitive value in the future.

A single instance of Sanitizer hold the "magic token" that would be then used in all methods internally?

The package does not present its functionality as an object. There's not really any state that I can think of that would be necessary here, and I think it should stay that way, unless you can think of any reason why there should be state?

@vancluever vancluever force-pushed the sensitive-values branch 4 times, most recently from 1a30f72 to 1f5f42f Compare May 4, 2021 16:42
@radeksimko
Copy link
Member

There's not really any state that I can think of that would be necessary here, and I think it should stay that way, unless you can think of any reason why there should be state?

I didn't see a strong reason aside from a simpler API where number of arguments is reduced, but as you said that likely wouldn't play well with how you plan to use it, so never mind! 😄

This adds a new package and functions for sanitization of values marked
as sensitive in the plan, where we can get particular data to do it.

This data is derived in a number of ways, also documented in the
top-level SanitizePlan function:

* ResourceChanges are sanitized based on BeforeSensitive and
AfterSensitive fields.

* Variables are sanitized based on variable config data found in the
root module of the Config.

* PlannedValues are sanitized based on the values found in
AfterSensitive in ResourceChanges. Outputs are sanitized according to
the appropriate sensitivity flags provided for the output.

* PriorState is sanitized based on the values found in BeforeSensitive
in ResourceChanges. Outputs are sanitized according to the appropriate
sensitivity flags provided for the output.

* OutputChanges are sanitized based on the values found in
BeforeSensitive and AfterSensitive. This generally means that any
sensitive output will have OutputChange fully obfuscated as the
BeforeSensitive and AfterSensitive in outputs are opaquely the same.
@vancluever vancluever marked this pull request as ready for review May 6, 2021 18:17
@vancluever
Copy link
Contributor Author

@radeksimko should be good for a full review now. 🙂

@vancluever
Copy link
Contributor Author

Thank you 🙂

@vancluever vancluever merged commit 990dae7 into main May 7, 2021
@vancluever vancluever deleted the sensitive-values branch May 7, 2021 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants