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

(MODULES-9695) Debian: use modern APT keyring format #681

Merged
merged 4 commits into from
Jul 9, 2024

Conversation

kenyon
Copy link
Contributor

@kenyon kenyon commented Nov 21, 2023

This updates puppet_agent::osfamily::debian to use modern APT keyrings (puppetlabs/puppetlabs-apt#1128) instead of the deprecated apt-key method used by apt::key and apt::source.key without name.

For this to work properly, it needs a release of puppetlabs-apt which contains puppetlabs/puppetlabs-apt#1128, which should be the release after v9.1.0.

This also removes the legacy key, because keys not used for signing package sources aren't needed.

/etc/pki is not needed anymore (also this directory is a RedHatism) because keyrings are now stored in the default location of /etc/apt/keyrings. We don't clean it up though, in case people are using the files there for something else.

@kenyon kenyon requested review from bastelfreak and a team as code owners November 21, 2023 22:13
@kenyon kenyon changed the title Debian: use modern APT keyring format (MODULES-9695) Debian: use modern APT keyring format Nov 21, 2023
@kenyon
Copy link
Contributor Author

kenyon commented Nov 21, 2023

The commit message check wants only a PA jira issue ID, but the issue for this is in the MODULES jira project. Caused by #642.

.fixtures.yml Outdated Show resolved Hide resolved
@kenyon kenyon force-pushed the debian-keyring branch 2 times, most recently from 85dc25b to e48620a Compare November 22, 2023 23:23
@mhashizume
Copy link
Contributor

Thanks for this @kenyon , could you rebase to pick up the changes from #682 ?

@kenyon
Copy link
Contributor Author

kenyon commented Nov 27, 2023

@mhashizume already did that. That's what the last force push was. Commit e48620a is on top of current main.

@kenyon
Copy link
Contributor Author

kenyon commented Nov 27, 2023

FWIW I'm using this plus puppetlabs-apt at puppetlabs/puppetlabs-apt@cb21d0b on my infrastructure, and it does the right thing on Ubuntu 18.04, 20.04, and 22.04.

@kenyon
Copy link
Contributor Author

kenyon commented Nov 28, 2023

Modern APT keyring format also needs to be used in puppet_enterprise::repo::config, which doesn't use puppetlabs-apt. Bug report: puppetlabs/puppet-enterprise_issues#2

@kenyon
Copy link
Contributor Author

kenyon commented Dec 12, 2023

I'm not seeing why the Debian tests would be running on Windows agents, and why my changes would cause these test failures.

@janit42
Copy link

janit42 commented Feb 22, 2024

as we have nightly puppet-agent packages for Debian 12 now it would be great to have this merged if possible

@joshcooper
Copy link
Contributor

@kenyon can you resolve the test failures so we can get this merged?

@kenyon
Copy link
Contributor Author

kenyon commented Mar 15, 2024

@kenyon can you resolve the test failures so we can get this merged?

I'd like to, but like I mentioned in #681 (comment), I investigated and couldn't figure out why these changes have anything to do with Windows tests. So I'll need a little assistance.

@kenyon
Copy link
Contributor Author

kenyon commented Mar 15, 2024

My guess is that something more fundamental is wrong with the tests, like there are tests executing on platforms that they shouldn't be executing on.

@linuxdaemon
Copy link

My guess is that something more fundamental is wrong with the tests, like there are tests executing on platforms that they shouldn't be executing on.

Looking at the log, the current failures look to be dependency related. It is happening in main as well, so it seems to be an issue with the Gemfile.

@mhashizume
Copy link
Contributor

Thanks for all of your work on this @kenyon , would it make sense to bump the puppetlabs-apt requirement in metadata.json since this is only supported in >= 9.2.0?

@kenyon
Copy link
Contributor Author

kenyon commented Jun 27, 2024

Thanks for all of your work on this @kenyon , would it make sense to bump the puppetlabs-apt requirement in metadata.json since this is only supported in >= 9.2.0?

@mhashizume good catch (I started this PR before puppetlabs-apt 9.2.0 was released 😄). Updated metadata.json.

@mhashizume
Copy link
Contributor

mhashizume commented Jun 28, 2024

We'll also want to update puppetlab-apt in tests here:

on(host, puppet('module', 'install', 'puppetlabs-apt', '--version', '9.0.0', '--environment', environment), { acceptable_exit_codes: [0] })

Edit: And here

on host, '/opt/puppetlabs/bin/puppet module install puppetlabs-apt --version 9.0.0', { acceptable_exit_codes: [0, 1] }

puppetlabs-apt is also pinned to an older version in our various dockerfiles.

@kenyon
Copy link
Contributor Author

kenyon commented Jun 28, 2024

@mhashizume I updated the versions in those helpers. The Dockerfiles don't specify versions (there are some branch specifications, but those lines are commented out).

@mhashizume
Copy link
Contributor

@kenyon A few more things:

  • After bumping puppetlabs-apt in metadata.json and helpers, we should also bump its dependency, stdlib (to >= 9.0.0). Because inifile depends on stdlib, we should also bump it to >= 6.0.0 in metadata.json and helpers.
  • With your PR and the above changes, I ran into errors with our upgrade tests from Puppet 6 to Puppet 7. I believe the issue is that if there are multiple Puppet repos (like for different major version) and one of them does not include the signed-by option, apt errors:
E: Conflicting values set for option Signed-By regarding source http://nightlies.puppet.com/apt/ buster: /etc/apt/keyrings/GPG-KEY-puppet-20250406.asc != 

I'm not sure what the best solution is for the second issue, if we need to update anything on our end or if we just need to update documentation to let users know to update their repo configuration to point to the key.

Thoughts?

@kenyon
Copy link
Contributor Author

kenyon commented Jul 2, 2024

@mhashizume I updated the metadata. Without looking at it myself, I'm not sure either what to do about that upgrade issue. I still have one PE 2019 instance that I need to upgrade, but the puppet_agent and apt modules would remain on Puppet 6-compatible versions until after the upgrade to Puppet 7. So I don't think it would be a problem. Which brings up this line:

"requirements": [
{
"name": "puppet",
"version_requirement": ">= 5.0.0 < 9.0.0"

IMO this should be bumped to require Puppet 7 since the apt, stdlib, and inifile dependencies require it. Should I make that change too?

@kenyon
Copy link
Contributor Author

kenyon commented Jul 3, 2024

@mhashizume I added a commit that bumps the Puppet version requirement to 7. I think this is the only safe way to go if the dependent modules require Puppet 7. Otherwise there is no guarantee that the Ruby interpreter used in Puppet 5 or 6 would work with the Ruby code in these newer modules, for example.

@mhashizume
Copy link
Contributor

mhashizume commented Jul 9, 2024

The bump makes sense to me. I found a few more things that needed to be updated:

  • inifile needs to be a minimum of 6.1.0 to work with newer stdlib
  • a hacky change to the tests so there aren't conflicts that make apt sad

You can see what I did in addition to what you have here in this commit: 627d06c

I think after you incorporate those changes, this PR should be good to go.

kenyon and others added 4 commits July 8, 2024 21:29
This updates puppet_agent::osfamily::debian to use modern APT keyrings
instead of the deprecated apt-key method used by apt::key and
apt::source.key without `name`.

This also removes the legacy key, because keys not used for signing
package sources aren't needed.

/etc/pki is not needed anymore (also this directory is a RedHatism)
because keyrings are now stored in the default location of
/etc/apt/keyrings. We don't clean it up though, in case people are using
the files there for something else.
The module dependencies (apt, stdlib, inifile) require Puppet 7.
@kenyon
Copy link
Contributor Author

kenyon commented Jul 9, 2024

@mhashizume done.

Not sure where the proper fix would go for that acceptance helper, maybe around here: https://github.com/puppetlabs/beaker-puppet/blob/1146cb9783f74812421ebd52dc556f68710c57fe/lib/beaker-puppet/install_utils/foss_utils.rb#L1091

@mhashizume
Copy link
Contributor

Thanks again @kenyon ! Really appreciate all your work.

I'm not sure what the upstream fix is for the issue we had to work around in the acceptance helper. The fundamental issue is that we had two sources.list with the same URL, but only one had the signed-by option enabled. The sources.list without the signed-by option is our release package installed by beaker-puppet. Our release packages don't use apt-key (AFAIK) and doesn't otherwise cause any warnings or errors with apt, so I don't think we need to change anything with beaker-puppet or our release packages.

Maybe we should document this circumstance somewhere, but I'm not sure where makes the most sense (and I'm not sure how often users will run into it).

@mhashizume mhashizume merged commit 4680ee4 into puppetlabs:main Jul 9, 2024
16 checks passed
@kenyon
Copy link
Contributor Author

kenyon commented Sep 27, 2024

Well I've run into a problem with this. I set up the puppet-tools apt source, which uses the same keyring as the pc_repo apt source that puppetlabs-puppet_agent sets up, here: https://github.com/kenyon/puppet/blob/3196bea6a8e8f54acf814c5cb5a0fb65e01cb6db/data/os/family/Debian.yaml#L107-L116

apt complains about conflicting values set for signed-by if there are different values for the same location:

E: Conflicting values set for option Signed-By regarding source https://apt.puppet.com/ bullseye: /etc/apt/keyrings/GPG-KEY-puppet-20250406.asc != /etc/apt/keyrings/puppet.gpg
E: Couldn't read list of package sources

Because of the way apt::keyring is implemented, I don't think there is a way to use the same keyring for multiple sources without causing duplicate resource catalog compilation errors. I was able to work around this case by using apt.puppetlabs.com for puppet-tools instead of apt.puppet.com, but I think the real solution would be to add $name to the file resource title to allow for making the file resource unique, here: https://github.com/puppetlabs/puppetlabs-apt/blob/cd24e6dc8e654a38aef34b7138f40c3cd4a619ec/manifests/keyring.pp#L50

@kenyon
Copy link
Contributor Author

kenyon commented Sep 27, 2024

Nevermind, that wouldn't work, of course you can't manage the same file path multiple times with puppet even if they have different resource titles. So I'm not sure what the best solution is for this kind of situation.

@kenyon
Copy link
Contributor Author

kenyon commented Sep 27, 2024

Maybe puppetlabs-apt's apt::source type needs a way to specify the signed-by value without actually managing any file resources.

@kenyon
Copy link
Contributor Author

kenyon commented Sep 27, 2024

Maybe puppetlabs-apt's apt::source type needs a way to specify the signed-by value without actually managing any file resources.

Sigh, nevermind, I forgot that this already exists, the keyring parameter of apt::source. Here it is in use: https://github.com/kenyon/puppet/blob/8787920f3480365f5972072262a24ef973c8290e/data/os/family/Debian.yaml#L107-L114

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants