CLAassistant
commented
1 year ago
All committers have signed the CLA.
Closed jorhett closed 7 months ago
CLAassistant
commented
1 year ago
All committers have signed the CLA.
puppet-community-rangefinder[bot]
commented
1 year ago that may have no external impact to Forge modules.
Puppetfiles.These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.
Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.
dhs-rec
commented
1 year ago This allows a simple key download behavior:
# This will create /etc/apt/keyrings/puppetlabs.gpg apt::keyring { 'puppetlabs': source => 'https://apt.puppetlabs.com/keyring.gpg', }
Will there be an option to store the keyring in /usr/share/keyrings (in case it is later managed by a package)?
jorhett
commented
1 year ago Will there be an option to store the keyring in /usr/share/keyrings (in case it is later managed by a package)?
You can pass any filename you want via the filename parameter. However, /usr/share/keyrings should never be (directly) managed by Puppet. Everything in that directory should match to a package.
dhs-rec
commented
1 year ago Will there be an option to store the keyring in /usr/share/keyrings (in case it is later managed by a package)?
You can pass any filename you want via the
filenameparameter.
Ah, didn't see that parameter in the example.
However, /usr/share/keyrings should never be (directly) managed by Puppet.
Why not? How else should the initial setup, before the package containing the key is installed, be done, then? File resources have the replace parameter, exactly for this purpose. I'm aware this would require a bit more code, though...
wornet-mwo
commented
1 year ago /usr/share is for data which does not need to be modified (see the FHS). So in regular case, no changes in /usr/share/keyrings should be made by puppet, unless explicitely intended by the author of the host manifest. There is an use case for doing against the standard: installing a package which adds the same keyring again. But from my point of view the filename parameter is the right way to use it. Also the default in /etc/apt/keyrings is a good way to go.
dhs-rec
commented
1 year ago In this case, a change in /usr/share/keyrings IS intended. The key is needed initially to setup the repo, from which a package is installed that manages the key afterwards. Thus, Puppet SHOULD be able to initially install a key into /usr/share/keyrings, but don't touch it again if it's already there.
Please see https://wiki.debian.org/DebianRepository/UseThirdParty for the details.
jamesps-ebi
commented
1 year ago In this case, a change in
/usr/share/keyringsIS intended. The key is needed initially to setup the repo, from which a package is installed that manages the key afterwards. Thus, Puppet SHOULD be able to initially install a key into/usr/share/keyrings, but don't touch it again if it's already there.Please see https://wiki.debian.org/DebianRepository/UseThirdParty for the details.
I see this as a chicken and egg situation. I know that some Google packages like Google Chrome do exactly this.
If you install the package from .deb, it creates the repo configuration and GPG key under /usr/share/keyrings
But if you want to manage the repo and key via Puppet, how should that be done? There doesn't seem a clean solution.
Probably in situations like this, you can remove the repo created by the .deb and add a new Puppet managed one with the 'signed-by' parameter pointing to a GPG key under /etc/apt/keyrings
dhs-rec
commented
1 year ago But if you want to manage the repo and key via Puppet, how should that be done? There doesn't seem a clean solution.
Yes, there is: https://www.puppet.com/docs/puppet/7/types/file.html#file-attribute-replace
dploeger
commented
11 months ago @jorhett Are you still working on this? Need any help?
rwaffen
commented
10 months ago i'm also interested in this. any news here?
jorhett
commented
10 months ago In this case, a change in /usr/share/keyrings IS intended. The key is needed initially to setup the repo, from which a package is installed that manages the key afterwards. Thus, Puppet SHOULD be able to initially install a key into /usr/share/keyrings, but don't touch it again if it's already there. Please see https://wiki.debian.org/DebianRepository/UseThirdParty for the details.
@dhs-rec actually you need to go back and re-read that same document.
Let me quote that document for you:
If future updates to the certificate will be managed by an apt/dpkg package as recommended below, then it SHOULD be downloaded into /usr/share/keyrings using the same filename that will be provided by the package.
This is very direct and explicit. The only keys that should be in /usr/share/keyrings should be installed by a package, and have exactly the same filename as the package. They then go further to say:
If it will be managed locally , it SHOULD be downloaded into /etc/apt/keyrings instead.
So the only compliant way for Puppet to put a key in /usr/share/keyrings is to install a debian package that does it. Any manual additions (e.g. THIS MODULE) should be in /etc/apt/keyrings.
@jorhett Are you still working on this? Need any help?
TBH this was honestly my attempt to remind Puppet that their "supported" module doesn't support any non-EOL version of the operating system. Kind of like elbow, elbow... hey, if you're going to claim this module is supported, then you need to update to post-2012 Debian standards. Because it only works with loud warnings on anything since 2012, and it doesn't work at all on the last 3 releases of the operating system.
I might have some time to poke at this in the future, but waiting for someone to give you a fix doesn't quite fit the "supported" paradigm as I understand it.
dhs-rec
commented
10 months ago @dhs-rec actually you need to go back and re-read that same document.
@jorhett, believe me, I've read this multiple times.
Let me quote that document for you:
If future updates to the certificate will be managed by an apt/dpkg package as recommended below, then it SHOULD be downloaded into /usr/share/keyrings using the same filename that will be provided by the package.
This is very direct and explicit. The only keys that should be in
/usr/share/keyringsshould be installed by a package, and have exactly the same filename as the package. They then go further to say:
You seem to completely ignore the "If future updates..." part of this paragraph, as well as "...it SHOULD be downloaded into...". Tell me: Who should do this download if not Puppet (given that one has to manage tens or hundreds of nodes)?
So, what this really means, once again, is: The key should initially be stored in /usr/share/keyrings, but then not touched anymore, except by the package that manages it. If you think about it, how else should this work? Where should the key be stored before you can install the package that manages it? In another location? You would have two keys, then, right? Which one would you use? How would you manage the change in the sources.list file?
This only makes sense if Puppet manages keys in /usr/share/keyrings initially (which means: using a file resource with replace => false), and those in /etc/apt/keyrings all the time.
jorhett
commented
10 months ago You seem to completely ignore the "If future updates..." part of this paragraph, as well as "...it SHOULD be downloaded into...". Tell me: Who should do this download if not Puppet (given that one has to manage tens or hundreds of nodes)?
If you're downloading it (not installing a package), it should be in /etc/apt/keyrings -- end of subject. No exceptions. The doc is very clear.
So, what this really means, once again, is: The key should initially be stored in
/usr/share/keyrings, but then not touched anymore, except by the package that manages it.
... This only makes sense if Puppet manages keys in/usr/share/keyringsinitially (which means: using a file resource withreplace => false), and those in/etc/apt/keyringsall the time.
I'm really not trying to be rude when I say this, but these convoluted statements of your don't make any sense at all. There is no reason to have two different file resources. You can't point a repo at two different key files, why would you create two different ones? Having the same key twice will cause problems. Stop imagining some scenario that doesn't exist.
/usr/share/keyrings should contains keys installed by packages and should be named the same as the package which installed it/etc/apt/keyrings EVERYTHING ELSE, no exceptionswornet-mwo
commented
10 months ago No need to offend here, as there is no reason to. @jorhett has planned to implement a filename parameter which is perfectly fine. So @dhs-rec, you can place your keyring whereever you want even to /var/run if you think it's a great idea. No need to change defaults here, where /etc/apt/keyrings is a well documented default from debian side.
Let's focus on the implementation - which is somehow missing, at least i did not find the defined type in your commit - am I missing something here (can't find a keyring.pp or a define in your changes)? Also there are some requested changes by @bastelfreak.
dhs-rec
commented
10 months ago You seem to completely ignore the "If future updates..." part of this paragraph, as well as "...it SHOULD be downloaded into...". Tell me: Who should do this download if not Puppet (given that one has to manage tens or hundreds of nodes)?
If you're downloading it (not installing a package), it should be in
/etc/apt/keyrings-- end of subject. No exceptions. The doc is very clear.
@jorhett , yes, it is very clear, indeed. It says (as you already cited above):
If future updates to the certificate will be managed by an apt/dpkg package as recommended below, then it SHOULD be downloaded into /usr/share/keyrings...
That is to overcome the chicken and egg problem you have in this case, because you need the keyfile in order to be able to install the package that manages it in the future. Thus, it should initially be downloaded into /usr/share/keyrings.
I'm really not trying to be rude when I say this, but these convoluted statements of your don't make any sense at all. There is no reason to have two different file resources. You can't point a repo at two different key files, why would you create two different ones? Having the same key twice will cause problems. Stop imagining some scenario that doesn't exist.
But that's what you get if the the Puppet class installs the keyfile into /etc/apt/keyrings only. Then you install the package that manages the key in /usr/share/keyrings. And then? How to proceed from this? The keyfile from the package is the one that should be referred to in the sources.list file.
I didn't write anything about two file resources. Of course, we need just one. But if the path starts with /usr/share/keyrings/, it should contain a replace => false.
* `/usr/share/keyrings` should contains keys installed by packages and should be named the same as the package which installed it
Yes, of course. I didn't write anything else, except for the last words: "... package which will install it"
* `/etc/apt/keyrings` EVERYTHING ELSE, no exceptionsSure
Could be something like this:
...
file { 'keyring':
path => $destination,
...
replace => ($destination !~ /^\/usr\/share\/keyrings\/.*$/),
...
}
...Here are some sample installation instructions (from Element Desktop), which exactly reflect what I wrote above:
sudo wget -O /usr/share/keyrings/element-io-archive-keyring.gpg https://packages.element.io/debian/element-io-archive-keyring.gpg echo "deb [signed-by=/usr/share/keyrings/element-io-archive-keyring.gpg] https://packages.element.io/debian/ default main" | sudo tee /etc/apt/sources.list.d/element-io.list sudo apt update sudo apt install element-desktop
The element-desktop package has a dependency on element-io-archive-keyring, which is the package that manages the keyring file afterwards. But the keyring file needed to be downloaded once, manually, in order to be able to install that package.
jorhett
commented
10 months ago That is to overcome the chicken and egg problem you have in this case
There is no chicken and egg problem here. You can install release packages with keys without sticking a key file in /usr/share/keyrings. This whole conundrum lies entirely in your imagination. This entire discussion is a waste of time.
jorhett
commented
10 months ago Here are some sample installation instructions (from Element Desktop), which exactly reflect what I wrote above:
sudo wget -O /usr/share/keyrings/element-io-archive-keyring.gpg https://packages.element.io/debian/element-io-archive-keyring.gpg echo "deb [signed-by=/usr/share/keyrings/element-io-archive-keyring.gpg] https://packages.element.io/debian/ default main" | sudo tee /etc/apt/sources.list.d/element-io.list sudo apt update sudo apt install element-desktop
The
element-desktoppackage has a dependency onelement-io-archive-keyring, which is the package that manages the keyring file afterwards. But the keyring file needed to be downloaded once, manually, in order to be able to install that package.
Those instructions are flat out wrong, but hey -- you can do it a number of different ways. Say that you really wanted to do it this way, my PR offered that ability, no? So what is it that you keep arguing about? If you really want to violate the guidelines, nothing in my PR would prevent that.
Now, is Puppet going to take responsibility for fixing their SUPPOSEDLY "supported" module which works with correctly exactly zero non-EOL versions of Debian?
jorhett
commented
10 months ago Let's focus on the implementation
🤷♀️ Like I said, this was my poke to remind Puppet to fix their "supported" module. I might have been willing to do all the work before, but given how I'm getting treated here I have better things to do with my time than fix their problems so they can make money from it.
kenyon
commented
10 months ago @jorhett best just to block known bad community member @dhs-rec.
jamesps-ebi
commented
10 months ago In #1120 I submitted a draft for a new apt::keyring defined type that could be made to work with the changes proposed here.
There's some overlap between the two PRs though.
Since this PR already has most of the surrounding work done, maybe we can look at cherry-picking just the defined-type and adapt it to work with these changes.
kenyon
commented
7 months ago Should be closed, was done in #1128.
This PR attempts to get most of the work in place to handle modern apt gpg keyrings, without breaking existing behavior. resolves #1034
This allows a simple key download behavior:
Modifications to the allowed values for
keyparameter ofapt::sourceallows a unified definition:The goal of this PR is to lay down a lot of the logic and see if some of the directions are worth pursing, before I dump hours into creating tests, etc. I'm looking for feedback here.
Some alternate design choices
keyringparameter instead ofkey-- might read better?Some things that should be improved