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 #965

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

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.

This replaces #885 by using puppetlabs-apt rather than implementing keyring handling here in the docker module.

Fixes #884.

@kenyon
Copy link
Contributor Author

kenyon commented Jan 7, 2024

Acceptance tests are failing in setup of the machines under test, not related to this PR.

@kenyon kenyon marked this pull request as ready for review January 7, 2024 23:47
@kenyon kenyon requested a review from a team as a code owner January 7, 2024 23:47
@saz
Copy link

saz commented Mar 6, 2024

@kenyon What about adding the key to the module, just as it has been suggested in puppetlabs/puppetlabs-postgresql#1563 (review) for the same change?

@kenyon
Copy link
Contributor Author

kenyon commented Mar 7, 2024

@saz yes, that could be done. It means potentially more maintenance work for this module when the key needs to be updated. I'll leave it up to this module's @puppetlabs maintainers whether to do this.

@saz
Copy link

saz commented Mar 11, 2024

Looking at this module again, it's possible to set a custom URL for the key source. As puppet:///... will be a valid URL, it's easy to use a custom key.

@bastelfreak
Copy link
Collaborator

I think we should do a minor release before we merge this: https://github.com/puppetlabs/puppetlabs-docker/pull/978/files (and there are some other non-breaking changes that should be merged first)

@psaintemarie
Copy link

Is there any update on when that PR will be merged?

@gdlx
Copy link

gdlx commented Sep 26, 2024

@kenyon I just tried your fork and it works fine but I get the following warnings that I didn't have before:

Warning: Private key for 'my_server.fqdn.com' does not exist
Warning: Client certificate for 'my_server.fqdn.com' does not exist

Is it expected ? Theres's not much info even in debug so I don't know what kind of key/certificate I should set and where.

I've seen the apt mod requirement has changed but I was already using 9.4.0 so it shouldn't be related.

@kenyon
Copy link
Contributor Author

kenyon commented Sep 27, 2024

@kenyon I just tried your fork and it works fine but I get the following warnings that I didn't have before:

Warning: Private key for 'my_server.fqdn.com' does not exist
Warning: Client certificate for 'my_server.fqdn.com' does not exist

Is it expected ? Theres's not much info even in debug so I don't know what kind of key/certificate I should set and where.

I've seen the apt mod requirement has changed but I was already using 9.4.0 so it shouldn't be related.

Those warnings sound unrelated to the changes in this pull request. I would try running puppet agent with --debug to see if that gives more context on where those warnings are coming from.

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.
@gdlx
Copy link

gdlx commented Oct 3, 2024

Those warnings sound unrelated to the changes in this pull request. I would try running puppet agent with --debug to see if that gives more context on where those warnings are coming from.

@kenyon As I said, I already ran in debug mode but it didn't give much information except helping me to find it was in the middle of docker related stuff.

I've just disabled all my puppet code except this:

  class { 'docker':
  }

And I still get this in debug mode:

Debug: /Stage[main]/Docker::Repos/before: before to Class[Docker::Install]
Debug: /Stage[main]/Docker::Repos/Apt::Source[docker]/before: before to Package[docker]
Debug: /Stage[main]/Apt::Update/Exec[apt_update]/before: before to Package[cgroupfs-mount]
Debug: /Stage[main]/Apt/File[sources.list]/notify: notify to Class[Apt::Update]
Debug: /Stage[main]/Apt/File[sources.list.d]/notify: notify to Class[Apt::Update]
Debug: /Stage[main]/Apt/File[preferences]/notify: notify to Class[Apt::Update]
Debug: /Stage[main]/Apt/File[preferences.d]/notify: notify to Class[Apt::Update]
Debug: /Stage[main]/Apt/File[apt.conf.d]/notify: notify to Class[Apt::Update]
Debug: /Stage[main]/Apt/File[/etc/apt/auth.conf]/notify: notify to Class[Apt::Update]
Debug: /Stage[main]/Docker::Install/before: before to Class[Docker::Config]
Debug: /Stage[main]/Docker::Config/before: before to Class[Docker::Service]
Debug: /Stage[main]/Docker::Service/File[/etc/systemd/system/docker.service.d/service-overrides.conf]/before: before to Service[docker]
Debug: /Stage[main]/Docker::Service/File[/etc/systemd/system/docker.service.d/service-overrides.conf]/notify: notify to Exec[docker-systemd-reload-before-service]
Debug: /Stage[main]/Docker::Service/Exec[docker-systemd-reload-before-service]/notify: notify to Service[docker]
Debug: /Stage[main]/Docker::Service/File[/etc/default/docker-storage]/notify: notify to Service[docker]
Debug: /Stage[main]/Docker::Service/File[/etc/default/docker]/notify: notify to Service[docker]
Debug: /Stage[main]/Docker::Repos/Apt::Source[docker]/Apt::Keyring[docker.asc]/before: before to Apt::Setting[list-docker]
Debug: /Stage[main]/Apt/Apt::Setting[conf-update-stamp]/File[/etc/apt/apt.conf.d/15update-stamp]/notify: notify to Class[Apt::Update]
Debug: /Stage[main]/Docker::Repos/Apt::Source[docker]/Apt::Setting[list-docker]/File[/etc/apt/sources.list.d/docker.list]/notify: notify to Class[Apt::Update]
Debug: /Stage[main]/Docker::Service/File[/etc/systemd/system/docker.service.d/service-overrides.conf]: Adding autorequire relationship with File[/etc/systemd/system/docker.service.d]
Debug: /Stage[main]/Apt/Apt::Setting[conf-update-stamp]/File[/etc/apt/apt.conf.d/15update-stamp]: Adding autorequire relationship with File[apt.conf.d]
Debug: /Stage[main]/Docker::Repos/Apt::Source[docker]/Apt::Keyring[docker.asc]/File[/etc/apt/keyrings/docker.asc]: Adding autorequire relationship with File[/etc/apt/keyrings]
Debug: /Stage[main]/Docker::Repos/Apt::Source[docker]/Apt::Setting[list-docker]/File[/etc/apt/sources.list.d/docker.list]: Adding autorequire relationship with File[sources.list.d]
Debug: /Stage[main]/Docker::Repos/Apt::Pin[docker]/Apt::Setting[pref-docker]/File[/etc/apt/preferences.d/docker.pref]: Adding autorequire relationship with File[preferences.d]
Debug: /Stage[main]/Docker::Systemd_reload/Exec[docker-systemd-reload]: 'systemctl daemon-reload' won't be executed because of failed check 'refreshonly'
Debug: /Stage[main]/Apt/File[/etc/apt/auth.conf]: Nothing to manage: no ensure and the resource doesn't exist
Debug: Prefetching apt resources for package
Debug: Executing '/usr/bin/dpkg-query -W --showformat '${Status} ${Package} ${Version}\n''
Debug: Executing: '/usr/bin/apt-mark showmanual'
Warning: Private key for 'my_server.fqdn.com' does not exist
Warning: Client certificate for 'my_server.fqdn.com' does not exist
Debug: Creating new connection for https://download.docker.com:443
Debug: Starting connection for https://download.docker.com:443
Debug: Using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256
Debug: HTTP HEAD https://download.docker.com/linux/debian/gpg returned 200 OK
Debug: Caching connection for https://download.docker.com:443
Debug: /Stage[main]/Apt::Update/Exec[apt_update]: '/usr/bin/apt-get update' won't be executed because of failed check 'refreshonly'
Debug: /Stage[main]/Docker::Service/File[/etc/systemd/system/docker.service.d/service-overrides.conf]/seltype: SELinux bindings not found. Ignoring parameter.
Debug: /Stage[main]/Docker::Service/Exec[docker-systemd-reload-before-service]: 'systemctl daemon-reload > /dev/null' won't be executed because of failed check 'refreshonly'
Debug: Executing: '/usr/bin/systemctl is-active -- docker'
Debug: Executing: '/usr/bin/systemctl is-enabled -- docker'
Debug: Finishing transaction 17800
Debug: Storing state
Debug: Pruned old state cache entries in 0.00 seconds
Debug: Stored state in 0.01 seconds
Notice: Applied catalog in 0.41 seconds
Debug: Applying settings catalog for sections reporting, metrics
Debug: Finishing transaction 43380
Debug: Received report to process from my_server.fqdn.com
Debug: Closing connection for https://download.docker.com:443

@kenyon
Copy link
Contributor Author

kenyon commented Oct 4, 2024

@gdlx strange. I'm using this PR in my puppet code, and those warnings don't appear. I'm pretty sure they have to be coming from some other puppet module.

@gdlx
Copy link

gdlx commented Oct 4, 2024

@gdlx strange. I'm using this PR in my puppet code, and those warnings don't appear. I'm pretty sure they have to be coming from some other puppet module.

Maybe it depends on the OS ?
I'm running Debian 12.

@tdb
Copy link

tdb commented Oct 4, 2024

They're coming out of Puppet's code, rather than this module:

https://github.com/puppetlabs/puppet/blob/c8684e60453cbea539459e4721364957fa4dfc3c/lib/puppet/ssl/ssl_provider.rb#L100

@gdlx
Copy link

gdlx commented Oct 8, 2024

They're coming out of Puppet's code, rather than this module:

https://github.com/puppetlabs/puppet/blob/c8684e60453cbea539459e4721364957fa4dfc3c/lib/puppet/ssl/ssl_provider.rb#L100

Sure, yet reverting from the fork back to puppetlabs-docker 10.0.1 release removes the warnings (but restores the one in apt update).

It may be relevant to specify I'm using Puppet 8.9.0 in puppet-apply mode.

@gdlx
Copy link

gdlx commented Oct 8, 2024

I fixed the issue by generating requested key and certificate:

openssl genrsa -out /etc/puppetlabs/puppet/ssl/private_keys/myserver.domain.com.pem 4096
openssl req -new -x509 -key /etc/puppetlabs/puppet/ssl/private_keys/myserver.domain.com.pem -out /etc/puppetlabs/puppet/ssl/certs/myserver.domain.com.pem -days 3650 -subj "/CN=myserver.domain.com"
cp /etc/puppetlabs/puppet/ssl/certs/myserver.domain.com.pem /usr/local/share/ca-certificates/myserver.domain.com.crt
update-ca-certificates

It's probably related to the way puppetlabs-apt downloads the GPG key when using a name instead of an ID.

I haven't found a way to avoid creating this cert I don't actually need.

@kenyon
Copy link
Contributor Author

kenyon commented Oct 9, 2024

@gdlx I think most puppet users are using puppet agent, so that key and certificate must already exist, which means we won't get those warnings. I don't know why this particular change would trigger those warnings though. Looks like it has to do with making a TLS connection to download the GPG key. Maybe you could come up with the minimal puppet code needed to cause the warnings, and then raise an issue in https://github.com/puppetlabs/puppet.

@gdlx
Copy link

gdlx commented Oct 9, 2024

The doc says the cert should be provided by the puppet-agent package: https://github.com/puppetlabs/puppet/blob/6923b0edccc61ebc23cdd2c4c430cfe6d2b6d8dc/lib/puppet/ssl/ssl_provider.rb#L41-L42

I don't know why it's not my case. I'll investigate about that, but this change in the docker module obviously only reveals this problem.

Thanks !

@kenyon
Copy link
Contributor Author

kenyon commented Oct 9, 2024

@gdlx I suspect the cert is missing because it only gets generated if you run puppet in agent mode. Since you use apply mode, it never gets created.

@psaintemarie
Copy link

We are just waiting on Puppet to review/approve here if I understand correctly?

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.

[Bug] debian 11/ubuntu 22.04 need to handle apt-source differently
8 participants