Skip to content
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

Use modern APT keyrings on Debian family; require puppetlabs/apt 9.2 #1563

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

kenyon
Copy link
Contributor

@kenyon kenyon commented Jan 3, 2024

This makes use of puppetlabs/puppetlabs-apt#1128 to store the public key in /etc/apt/keyrings and add a signed-by option to the sources.list.d entry.

ekohl
ekohl previously approved these changes Jan 3, 2024
@ekohl
Copy link
Collaborator

ekohl commented Jan 3, 2024

How is the migration on this. Does it clean up the old entry? Mostly asking because I want to know if this is backwards incompatible or not.

@kenyon
Copy link
Contributor Author

kenyon commented Jan 3, 2024

How is the migration on this. Does it clean up the old entry? Mostly asking because I want to know if this is backwards incompatible or not.

This change adds the file for the keyring (https://github.com/puppetlabs/puppetlabs-apt/blob/0871cadcdcbc5f0e6540298fa11e9a3ebe884735/manifests/keyring.pp#L54-L61) and changes the line in the file in sources.list.d to add signed-by (https://github.com/puppetlabs/puppetlabs-apt/blob/0871cadcdcbc5f0e6540298fa11e9a3ebe884735/manifests/source.pp#L219).

@kenyon kenyon marked this pull request as draft January 3, 2024 19:01
@kenyon kenyon force-pushed the modern-apt-keyring branch from 039a422 to 4d28938 Compare January 7, 2024 22:44
@kenyon kenyon marked this pull request as ready for review January 7, 2024 23:16
@kenyon
Copy link
Contributor Author

kenyon commented Jan 7, 2024

Need someone to approve a workflow run. Unit tests pass locally for me.

bastelfreak
bastelfreak previously approved these changes Jan 7, 2024
smortex
smortex previously approved these changes Jan 8, 2024
@kenyon kenyon force-pushed the modern-apt-keyring branch from 4d28938 to 969c14a Compare January 11, 2024 19:23
Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One consideration: this will reach out to the internet on every Puppet run while I think previously it didn't. That also means the content of the GPG key may change at random and you wouldn't know, while previously the key ID would change. That begs the question: what is GPG key signing adding then?

Don't get me wrong, I do like this but I'd suggest to include the GPG key in this module and provide a parameter for the source (which would default to puppet://${module_name}/some_file.asc). If the key changes, users can easily change the source to something else but by default you don't put additional load on www.postgresql.org and get better security out of it.

@kenyon kenyon dismissed stale reviews from bastelfreak, ekohl, and smortex via 7131199 January 17, 2024 17:51
@kenyon kenyon force-pushed the modern-apt-keyring branch from 969c14a to 7131199 Compare January 17, 2024 17:51
@kenyon
Copy link
Contributor Author

kenyon commented Jan 17, 2024

@ekohl good idea, implemented. That also makes the APT code consistent with the yumrepo code which contains the key in the module.

@kenyon
Copy link
Contributor Author

kenyon commented Jan 17, 2024

Forgot the parameter to be able to override it, I'll do that.

@kenyon
Copy link
Contributor Author

kenyon commented Jan 17, 2024

Wellllll, this module is kind of crazy. The postgresql::repo::yum_postgresql_org class doesn't provide a way to override gpgkey, so I'm going to leave postgresql::repo::apt_postgresql_org as is. I think, if the user doesn't want to use the provided defaults for the package repo, they'll have to set $manage_package_repo = false and manage it themselves.

Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll kick off ci, but looks good. Overall I think the GPG keys can use more parameters. That was already the case before this and it doesn't make it worse.

We can ensure the old way is ensured absent, but that can cause problems. Perhaps something for the release notes?

@kenyon
Copy link
Contributor Author

kenyon commented Jan 17, 2024

We can ensure the old way is ensured absent, but that can cause problems. Perhaps something for the release notes?

Yeah, I don't think it's necessary to remove the key from /etc/apt/trusted.gpg because it won't be used (by this source anyway) when signed-by is added to the sources.list.d entry managed by this module.

This makes use of puppetlabs/puppetlabs-apt#1128
to store the public key in /etc/apt/keyrings and add a signed-by
option to the sources.list.d entry.
@kenyon kenyon force-pushed the modern-apt-keyring branch from 7131199 to 8aadd09 Compare April 4, 2024 20:12
@bastelfreak bastelfreak changed the title Use modern APT keyrings on Debian family Use modern APT keyrings on Debian family; require puppetlabs/apt 9.2 Apr 4, 2024
@ekohl ekohl added the feature label Apr 5, 2024
@ekohl ekohl merged commit 3904ab3 into puppetlabs:main Apr 5, 2024
38 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants