(PA-8346) Harden sensitive parameter preservation against regressions - #390
Closed
gavindidrichsen wants to merge 1 commit into
Closed
(PA-8346) Harden sensitive parameter preservation against regressions#390gavindidrichsen wants to merge 1 commit into
gavindidrichsen wants to merge 1 commit into
Conversation
PR #388 fixed the cleartext leak of sensitive parameters by injecting :sensitive_parameters into the attributes hash before super. This follow-up hardens that fix in three ways: 1. Replace the fragile attributes[:sensitive_parameters] injection (which relies on hash2resource's undocumented key-deletion) with an explicit post-super set_sensitive_parameters call using the public Puppet::Type API. 2. Add a re-wrap guard after canonicalize to catch providers that may unwrap Sensitive values during canonicalization. 3. Add the missing regression test that directly asserts parameter(:secret).sensitive == true when a Puppet::Resource carries sensitive_parameters -- the test that would catch a reintroduction of the original bug.
Contributor
Author
|
Closing this. Can possibly re-open at a later date. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to PR #388 which fixed the cleartext leak of sensitive parameters during
Puppet::Resource→ Hash conversion. This PR hardens that fix in three ways:Replace fragile
attributes[:sensitive_parameters]injection withset_sensitive_parameters— PR (PA-8346) Preserve sensitive parameters #388 injected:sensitive_parametersinto the attributes hash beforesuper, relying onhash2resource's undocumented internal key-deletion to clean up the extra key. This works today but couples to an implementation detail of Puppet core. This PR uses the publicPuppet::Type#set_sensitive_parametersAPI aftersuperinstead, which is both the intended API and resilient to upstream changes inhash2resource.Add a re-wrap guard after
canonicalize— Providers that implementcanonicalizereceive unwrapped attribute values. If a provider returns a plain string where aSensitive-wrapped value was expected, the sensitive wrapping is silently lost. This PR adds a post-canonicalize re-wrap pass (identical to the existing pre-canonicalize one) so that theSensitivewrapper survives regardless of provider behavior.Add the missing regression test — PR (PA-8346) Preserve sensitive parameters #388's tests covered the "no sensitive params" case and the Sensitive-wrapping case, but did not test the primary fix path: a
Puppet::Resourceinput carrying a non-emptysensitive_parameterslist with unwrapped values. This PR adds that test, directly assertingparameter(:secret).sensitive == trueand that the value is re-wrapped asSensitive— the test that would catch a reintroduction of the original bug.Test Plan
when loading from a Puppet::Resource with sensitive parameters listedverifies:parameter(:secret).sensitivereturnstruePuppet::Pops::Types::PSensitiveType::SensitiveRelated