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-8583) Improve rpm importing of the puppet GPG key #386

Merged
merged 1 commit into from
Feb 20, 2019

Conversation

ScottGarman
Copy link
Contributor

SLES 15 uses gpg 2.2, which changed some of its key output formatting
and is noisier about missing commands. This change normalizes the text
parsing of gpg keys, using the --with-colons option, that is recommended
in the man page for use in scripts. The updated parsing pipelines work with
older and newer versions of gpg.

@ScottGarman
Copy link
Contributor Author

ScottGarman commented Feb 12, 2019

Apart from further updating some spec tests, I think I can simplify the parsing pipeline further with some careful use of sed - looking into that option now.

@ScottGarman ScottGarman force-pushed the modules-8583-fix branch 2 times, most recently from 8b6f7dd to 112155e Compare February 12, 2019 23:32
@ScottGarman
Copy link
Contributor Author

After some experimentation, I feel that getting clever with sed and grep will reduce readability and result in a much more brittle parsing of the gpg key id, so I'm going to stick with this approach. I've tested it manually to upgrade SLES 12 and SLES 15 agents and verified the bug reported in the ticket has been fixed.

@puppetcla
Copy link

CLA signed by all contributors.

exec { "import-${legacy_keyname}":
path => '/bin:/usr/bin:/sbin:/usr/sbin',
command => "rpm --import ${legacy_gpg_path}",
unless => "rpm -q gpg-pubkey-`echo $(gpg --throw-keyids < ${legacy_gpg_path}) | cut --characters=11-18 | tr '[:upper:]' '[:lower:]'`",
unless => "rpm -q ${legacy_gpg_pubkey}",
Copy link
Contributor

Choose a reason for hiding this comment

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

What does --throw-keyids do such that we no longer need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

--throw-keyids is actually an option that is used in the context of encrypting a message. In indicates whether they recipient key ids are included in the message metadata. Leaving them out (with --no-throw-keys) can provide better privacy when trying to communicate anonymously. My guess is it may have been an option with a side effect that worked in this situation, but it's no longer necessary.

That said, while doing some digging with git blame to answer this I noticed something that I should improve, so thanks for asking! :)

Copy link
Contributor

@speedofdark speedofdark left a comment

Choose a reason for hiding this comment

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

See inline commentary.

Copy link
Contributor

@speedofdark speedofdark left a comment

Choose a reason for hiding this comment

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

👍

@ScottGarman
Copy link
Contributor Author

In addition to SLES 12 and 15, I've also now manually tested this with CentOS 5 and 6 nodes.

@caseywilliams
Copy link
Contributor

I ran the tests in ./acceptance for several rpm-using platforms, and things were mostly fine, but I found that upgrading Fedora 27 from PC1 to puppet5 didn't work (no errors, just a failure to upgrade). A puppet5 to puppet6 upgrade did explicitly error for me on fedora 28, as well (centos, redhat, sles all worked fine for me everywhere).

@ScottGarman ScottGarman force-pushed the modules-8583-fix branch 2 times, most recently from 6811466 to 38b5b27 Compare February 19, 2019 19:36
SLES 15 uses gpg 2.2, which changed some of its key output formatting
and is noisier about missing commands. This change normalizes the text
parsing of gpg keys, using the --with-colons option, that is recommended
in the man page for use in scripts. The updated parsing pipelines work with
older and newer versions of gpg.
@ScottGarman
Copy link
Contributor Author

The reason the Fedora27 upgrade from PC1->puppet5 failed is because the beaker test is trying to upgrade the agent to 5.5.10, and that version is missing from http://yum.puppetlabs.com/puppet5/fedora/27/x86_64/. The latest version there is 5.5.8. I'm guessing this is because F27 has been EOL for a while and we've stopped shipping it.

Copy link
Contributor

@caseywilliams caseywilliams left a comment

Choose a reason for hiding this comment

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

Ok, apologies for the legwork! I'm very ready to merge this!

@ScottGarman
Copy link
Contributor Author

The puppet5->puppet6 upgrade of Fedora28 looks to be unrelated to the agent module tests. I'm investigating why there are cert signing/validation issues going on with agent 5.5.10 and puppetserver 6.2.0 on this platform that I'm investigating and will file a ticket on when I know more of what's going on. In the meantime, I think this PR is now ready for merging.

@speedofdark speedofdark merged commit 5b50678 into puppetlabs:master Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants