(PUP-11429) Make split() sensitive-aware

[cocker-cc]

PUP-11429

[puppetlabs-jenkins]

Can one of the admins verify this patch?

[CLAassistant]

All committers have signed the CLA.

[joshcooper]

@cocker-cc can you rebase your PR on d2ae297, it has fixes for the unrelated Ruby 3 failures.

[joshcooper]

jenkins please test this with server tests

[joshcooper]

Sorry coming back to this after some time. I'm thinking we need to preserve the sensitive-ness of the returned values like we did in 10724d7, otherwise, the sensitive data may lose its sensitive-ness and end up in a log file.

I think we want to add return_type 'Sensitive[Array[String]]' for each dispatch and then for each split_*_sensitive method wrap the result back in Sensitive. For example something like:

def split_String_sensitive(sensitive, pattern)
  wrap(split_String(sensitive.unwrap, pattern))
end

def wrap(array)
  Puppet::Pops::Types::PSensitiveType::Sensitive.new(array)
end

[cocker-cc]

I'm thinking we need to preserve the sensitive-ness of the returned values like we did in [10724d7]

My main Goal with all my many PRs concearning Sensitive is, that the Enduser should have to deal with the Subject Sensitive as little as possible.
Enduser: “I have a String, which I want to split. It may be Sensitive, but I don't really know. So please, split, take care of it by yourself.”
Following your Example, the Result could be a Sensitive[Array[String]] (unusual enough) and the Enduser would not be able to f.e.:

$mystring2 = $mystring1.split(' ').join('_')

because join does not yet accept a Sensitive[Array[String]], and as long as this is the Case with join and many other Functions, I do not agree to your Proposal.

epp() returning a Sensitive[String] in your linked Commit works flawless, only because file already knows by itself how to deal with Sensitive[String], and the Enduser does not have to care. Perfect.

[joshcooper]

@cocker-cc I understand why you're not wanting to modify functions to accept Sensitive, but the current state is Sensitive is its own data type, it's not an attribute of a String, etc. See @hlindberg comments in https://puppet.atlassian.net/browse/PUP-10092?focusedCommentId=75281 about why this is true. Therefore, the function must be updated to explicitly accept a Sensitive value.

Also from a security standpoint, we don't want the split function to have the side effect of automatically typecasting the Sensitive value to Array[String]. That is a security footgun and it will leak into logs, reports and puppetdb.

I think it's reasonable for join to accept an array where some of the elements may be Sensitive, and if so, wrap the resulting array in Sensitive. That way Sensitive values round trip correctly.

[cocker-cc]

I think it's reasonable for join to accept an array where some of the elements may be Sensitive,
and if so, wrap the resulting array in Sensitive. That way Sensitive values round trip correctly.

This is exactly, what I am saying. But until then…
And apart from join there are several other Functions to educate 😃

In one of my PRs I introduced here an additional Parameter to let the Enduser decide, if the Returnvalue should be sensitive. Could this perhaps be an option?

[cocker-cc]

Okay – I am conviced. Returning Sensitive (like epp() does) hands over the Responsibility to handle Sensitive correctly to the next Function.
I adopted your suggestion.

[joshcooper]

That's great, thanks @cocker-cc! Also for the future, what built in functions do you think should accept Sensitive but don't currently? Sounds like join should accept it, any others?

[cocker-cc]

For regsubst() I have an open PR
Those related to Passwords, f.e. length, chomp, …

[joshcooper]

Backport to 7.x #9118