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

Improvements for APT keys management #698

Merged
merged 3 commits into from
May 13, 2021

Conversation

bkabrda
Copy link
Contributor

@bkabrda bkabrda commented May 13, 2021

What does this PR do?

  • By default, get keys from keys.datadoghq.com, not Ubuntu keyserver
  • Always add the DATADOG_APT_KEY_CURRENT.public key (contains key used to sign current repodata)
  • Add 'signed-by' option to all sources list lines
  • On Debian >= 9 and Ubuntu >= 16, only add keys to /usr/share/keyrings/datadog-archive-keyring.gpg
  • On older systems, also add the same keyring to /etc/apt/trusted.gpg.d

Motivation

Additional Notes

Describe your test plan

Slavek Kabrda added 2 commits May 13, 2021 11:32
* By default, get keys from keys.datadoghq.com, not Ubuntu keyserver
* Always add the DATADOG_APT_KEY_CURRENT.public key (contains key used to sign current repodata)
* Add 'signed-by' option to all sources list lines
* On Debian >= 9 and Ubuntu >= 16, only add keys to /usr/share/keyrings/datadog-archive-keyring.gpg
* On older systems, also add the same keyring to /etc/apt/trusted.gpg.d
@bkabrda bkabrda requested a review from a team as a code owner May 13, 2021 09:48
Copy link
Contributor

@albertvaka albertvaka left a comment

Choose a reason for hiding this comment

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

Looks great to me!

Comment on lines -345 to -347
Optional[Boolean] $use_apt_backup_keyserver = $datadog_agent::params::use_apt_backup_keyserver,
String $apt_backup_keyserver = $datadog_agent::params::apt_backup_keyserver,
String $apt_keyserver = $datadog_agent::params::apt_keyserver,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep the options here and just ignore them (or print a warning saying they are deprecated if they are set). Otherwise, if someone was specifying them they will get an error because the argument no longer exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note you can also remove the defaults from params.pp and make them default to undef.

Copy link
Contributor

@albertvaka albertvaka left a comment

Choose a reason for hiding this comment

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

Looks good to merge if tests agree ✅

@bkabrda bkabrda merged commit ea4d9b1 into master May 13, 2021
@bkabrda bkabrda deleted the slavek.kabrda/apt-keys-improvements branch May 13, 2021 12:54
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.

2 participants