-
Notifications
You must be signed in to change notification settings - Fork 462
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
Add apt::keyring defined type which creates modern-style keyrings #1105
Conversation
@@ -351,6 +355,9 @@ | |||
if $keys { | |||
create_resources('apt::key', $keys) | |||
} | |||
if $keyrings { | |||
create_resources('apt::keyring', $keyrings) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't use create_resources()
. That's a deprecated pattern. Instead, iterate on $keyrings
:
$keyrings.each |$key, $data| {
apt::keyring { $key:
* => $data,
}
}
@@ -159,6 +162,7 @@ | |||
Apt::Proxy $proxy = $apt::params::proxy, | |||
Hash $sources = $apt::params::sources, | |||
Hash $keys = $apt::params::keys, | |||
Hash $keyrings = $apt::params::keyrings, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please directly assign the data here:
Hash $keyrings = $apt::params::keyrings, | |
Hash $keyrings = {}, |
And please replace the Hash
datatype with something that's a bit stricter.
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 |
Ah, didn't see that parameter in the example.
Why not? How else should the initial setup, before the package containing the key is installed, be done, then? File resources have the |
|
In this case, a change in 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. 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 |
Yes, there is: https://www.puppet.com/docs/puppet/7/types/file.html#file-attribute-replace |
@jorhett Are you still working on this? Need any help? |
i'm also interested in this. any news here? |
@dhs-rec actually you need to go back and re-read that same document. Let me quote that document for you:
This is very direct and explicit. The only keys that should be in
So the only compliant way for Puppet to put a key in
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. |
@jorhett, believe me, I've read this multiple times.
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 This only makes sense if Puppet manages keys in |
If you're downloading it (not installing a package), it should be in
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.
|
No need to offend here, as there is no reason to. @jorhett has planned to implement a 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 |
@jorhett , yes, it is very clear, indeed. It says (as you already cited above):
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
But that's what you get if the the Puppet class installs the keyfile into I didn't write anything about two file resources. Of course, we need just one. But if the path starts with
Yes, of course. I didn't write anything else, except for the last words: "... package which will install it"
Sure Could be something like this:
|
Here are some sample installation instructions (from Element Desktop), which exactly reflect what I wrote above:
The |
There is no chicken and egg problem here. You can install release packages with keys without sticking a key file in |
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? |
🤷♀️ 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. |
In #1120 I submitted a draft for a new 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. |
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
key
parameter ofapt::source
allows a unifieddefinition:
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
keyring
parameter instead ofkey
-- might read better?Some things that should be improved