Allow file content that looks like a checksum.#170
Allow file content that looks like a checksum.#170jeremie-pierson wants to merge 28 commits intoOpenVoxProject:mainfrom
Conversation
|
I'll correct the new rubocop offense. |
|
@bastelfreak Should I add some tests ? |
Currently, if a file has a content attribute set to something that looks like a checksum, it will display a deprecation warning and then probably throw an error, as the checksum-link string won't match anything in filebuckets. This code is apparently intended to allow a checksum to be passed instead of actual content, with the effect of replacing file content if it doesn't match the checksum. It appears that this mecanism is replaced by "static catalogs" and was scheduled for removal in Puppet 7. As it is no longer documented (because deprecated), it is surprising to stumble upon the behavior by just having file content that looks like a checksum. I had to work around this in a real usecase involving some proprietary software that uses the same syntax for value to be encrypted at startup. This commit introduces a new setting "use_checksum_in_file_content" that default to true, preserving current behavior. If set to false, it will never look for checksums in file contents. This setting should probably be set to false by default in the next major release.
This is a bit less nice because the nested if allowed another comment, but hey, Rubocop.
jeremie-pierson
force-pushed
the
filecontentchecksumbug
branch
from
f8bdf9a to
a9ca544
Compare
|
in all honesty, this was deprecated seven years ago and I've never seen anyone actually use it. I vote we just remove the functionality. |
OK, I'll update this PR to remove the functionality instead of adding another config item :-) |
It was possible, in the distant past, to set a file's content to a checksum and have Puppet automagically fetch file content from filebucket. This functionality is long deprecated, and was once scheduled for removal in Puppet 7. I'm not even sure it's working today. This commit disables the functionality entirely.
|
Let's hope I didn't let tests for the functionality slip through... We'll see soon enough I guess 😅 |
| # Our argument looks like a checksum. Is it the value of the content | ||
| # attribute in Puppet code, that happens to look like a checksum, or is | ||
| # it an actual checksum computed on the actual content? | ||
| if @actual_content || resource.parameter(:source) | ||
| # Actual content is already set, value contains it's checksum | ||
| value | ||
| else | ||
| # The content only happens to look like a checksum by chance. | ||
| @actual_content = value.is_a?(Puppet::Pops::Types::PBinaryType::Binary) ? value.binary_buffer : value | ||
| resource.parameter(:checksum).sum(@actual_content) | ||
| end |
There was a problem hiding this comment.
I tried without it but got obscure errors. I think the problem was that the checksum mechanism is not only used for the functionality we're trying to remove, but also for other purposes in the file handling code. Probably has to do with filebuckets.
It seems like "value" can legitimately contain a checksum if this attribute ("content") was already set. I'm not sure about this, but as I understand it, since we are in a "munge" block, we're transforming the value of the attribute prior to storing it. In the standard case, when user sets the "content" attribute to something in Puppet code, we store the actual content in @actual_content and attribute value holds the checksum.
As I recall, when I first removed theses lines, I had loads of errors in filebucket-related code elsewhere because, presumably, other code assumes that file content attribute value is in fact a checksum...
If you think I'm mistaken, I'll try to delete those lines again and see what CI says this time.
| elsif actual_content.nil? | ||
| yield read_file_from_filebucket |
There was a problem hiding this comment.
This could be related to above discussion (or I could very well be mistaken about all this :-) ).
|
Removing this code will also fix #335. |
|
@jeremie-pierson please rebase against our main branch to get rid of the merge commit. And please sign your commit with a gpg or ssh key. |
Change systemd unit description as man:puppet-agent
style: systemd unit branding
Credit to @slippycheeze for the original
Restore the legend of help help help help help
…renew Add a renew_cert subcommand to puppet ssl
f1e77c2 "(PUP-3634) Hide password hash from process list for useradd" introduced `chpasswd -e` which does not exist on OpenBSD, thus `user` resources managing `password` would always fail: ``` Notice: Compiled catalog for atar in environment production in 0.02 seconds rror: Could not set password on user[test]: No command chpasswd defined for provider openbsd Error: /Stage[main]/Main/User[test]/password: change from [redacted] to [redacted] failed: Could not set password on user[test]: No command chpasswd defined for provider openbsd Notice: Applied catalog in 0.01 seconds ``` Use https://man.openbsd.org/usermod.8#p instead: ``` Notice: Compiled catalog for atar in environment production in 0.01 seconds Notice: /Stage[main]/Main/User[test]/password: changed [redacted] to [redacted] Notice: Applied catalog in 0.21 seconds ``` `password` values now do show up briefly in the process list, but given they must be encrypted in order to work, this does not seem critical.
…nbsd Use usermod(8) on OpenBSD to unbreak password management
Currently, if a file has a content attribute set to something that looks like a checksum, it will display a deprecation warning and then probably throw an error, as the checksum-link string won't match anything in filebuckets. This code is apparently intended to allow a checksum to be passed instead of actual content, with the effect of replacing file content if it doesn't match the checksum. It appears that this mecanism is replaced by "static catalogs" and was scheduled for removal in Puppet 7. As it is no longer documented (because deprecated), it is surprising to stumble upon the behavior by just having file content that looks like a checksum. I had to work around this in a real usecase involving some proprietary software that uses the same syntax for value to be encrypted at startup. This commit introduces a new setting "use_checksum_in_file_content" that default to true, preserving current behavior. If set to false, it will never look for checksums in file contents. This setting should probably be set to false by default in the next major release.
This is a bit less nice because the nested if allowed another comment, but hey, Rubocop.
It was possible, in the distant past, to set a file's content to a checksum and have Puppet automagically fetch file content from filebucket. This functionality is long deprecated, and was once scheduled for removal in Puppet 7. I'm not even sure it's working today. This commit disables the functionality entirely.
…penvox into filecontentchecksumbug
9 participants
This pull request is intended to fix #169 . Also this is my first pull request on github.
Currently, if a file has a content attribute set to something that looks like a checksum, it will display a deprecation warning and then probably throw an error, as the checksum-like string won't match anything in filebuckets.
This code is apparently intended to allow a checksum to be passed instead of actual content, with the effect of replacing file content if it doesn't match the checksum. It appears that this mecanism is replaced by "static catalogs" and was scheduled for removal in Puppet 7.
As it is no longer documented (because deprecated), it is surprising to stumble upon the behavior by just having file content that looks like a checksum. I had to work around this in a real usecase involving some proprietary software that uses the same syntax for values to be encrypted at startup.
This commit introduces a new setting "use_checksum_in_file_content" that default to true, preserving current behavior. If set to false, it will never look for checksums in file contents.
This setting should probably be set to false by default in the next major release.