Skip to content
This repository has been archived by the owner. It is now read-only.

Only change to short IDs for delete #5353

Merged
merged 3 commits into from Oct 22, 2016

Conversation

Projects
None yet
3 participants
@abadger
Copy link
Member

abadger commented Oct 21, 2016

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

apt_key.py

ANSIBLE VERSION
devel stable-2.2
SUMMARY

If the user specifies long IDs, use them for all commands except for
deleting a key. Need to use short IDs there because of an upstream
apt_key bug. Fixed in apt_key 1.10 (fix is present in Ubuntu 16.04 but
not Ubuntu 14.0 or some Debians).

Fixes #5237

Also:

  • Some style cleanups
  • Return a nice user error message if the key was not found on the
    keyserver
  • Make file and keyring parameters type='path' so envars and tilde are
    expanded
Only change to short IDs for delete
If the user specifies long IDs, use them for all commands except for
deleting a key.  Need to use short IDs there because of an upstream
apt_key bug.  Fixed in apt_key 1.10 (fix is present in Ubuntu 16.04 but
not Ubuntu 14.0 or some Debians).

Fixes #5237

Also:

* Some style cleanups
* Return a nice user error message if the key was not found on the
  keyserver
* Make file and keyring parameters type='path' so envars and tilde are
  expanded
@gregdek

This comment has been minimized.

Copy link
Contributor

gregdek commented Oct 21, 2016

Thanks @abadger. To the current maintainers, @jvantuyl please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with text 'shipit', 'needs_revision' or 'close_me' as appropriate.

[This message brought to you by your friendly Ansibull-bot.]

Check that apt-key really erased the key
When erasing a key, apt-key does not understand how to process subkeys.
This update explicitly checks that the key_id is no longer present and
throws an error if it is.  It also hints at subkeys being a possible
problem in the error message and the documentation.

Fixes #5119
Fix apt_key check mode with long ids
* More cleanups
* apt-key can be given a key id longer than 16 chars to more accurately
  define what key to download.  However, we can use a maximum of 16
  chars to verify whether a key is installed or not.  So we need to use
  different lengths for the id depending on what we're doing with it.

Fixes #2622

@abadger abadger force-pushed the abadger:apt_key-fingerprint-fix branch from 2fd2e53 to 50e3ccf Oct 22, 2016

@abadger abadger merged commit 66d47c8 into ansible:devel Oct 22, 2016

1 check passed

Shippable Run 1970 status is SUCCESS.
Details

@abadger abadger deleted the abadger:apt_key-fingerprint-fix branch Oct 22, 2016

abadger added a commit that referenced this pull request Oct 22, 2016

Only change to short IDs for delete (#5353)
* Only change to short IDs for delete

If the user specifies long IDs, use them for all commands except for
deleting a key.  Need to use short IDs there because of an upstream
apt_key bug.  Fixed in apt_key 1.10 (fix is present in Ubuntu 16.04 but
not Ubuntu 14.0 or some Debians).

Fixes #5237

* Check that apt-key really erased the key

When erasing a key, apt-key does not understand how to process subkeys.
This update explicitly checks that the key_id is no longer present and
throws an error if it is.  It also hints at subkeys being a possible
problem in the error message and the documentation.

Fixes #5119

* Fix apt_key check mode with long ids

apt-key can be given a key id longer than 16 chars to more accurately
define what key to download.  However, we can use a maximum of 16
chars to verify whether a key is installed or not.  So we need to use
different lengths for the id depending on what we're doing with it.

Fixes #2622

Also:

* Some style cleanups
* Use get_bin_path to find the path to apt-key and then use that when
  invoking apt-key
* Return a nice user error message if the key was not found on the
  keyserver
* Make file and keyring parameters type='path' so envars and tilde are
  expanded
@abadger

This comment has been minimized.

Copy link
Member Author

abadger commented Oct 22, 2016

Merged to devel and stbale-2.2. This should be in the 2.2.0 release.

@ypid

This comment has been minimized.

Copy link

ypid commented Oct 22, 2016

@abadger Thanks but even after the return fix it still showed ok for this task when the key is already present (the short key id only obvious):

- apt_key:
    # id: 'DDA2C105C4B73A6649AD2BBD47AE7F72479BC94B'
    id: '00000000000000000000000047AE7F72479BC94B'
  keyserver: 'hkp://pool.sks-keyservers.net'

Can someone else also review this? cc @jvantuyl

if key_id.startswith('0x'):
key_id = key_id[2:]
key_id = key_id.upper()[-8:]
key_id, fingerprint, short_key_id = parse_key_id(key_id)

This comment has been minimized.

@ypid

ypid Oct 22, 2016

@abadger You mixed up the return order. It should be: short_key_id, fingerprint, key_id = parse_key_id(key_id)

@abadger

This comment has been minimized.

Copy link
Member Author

abadger commented Oct 22, 2016

Fixed the problem ypid identified here: #5357

If we decide to backport this to stable-2.1, we'll need to cherry-pick that commit as well.

karmab added a commit to karmab/ansible-modules-core that referenced this pull request Oct 24, 2016

Only change to short IDs for delete (ansible#5353)
* Only change to short IDs for delete

If the user specifies long IDs, use them for all commands except for
deleting a key.  Need to use short IDs there because of an upstream
apt_key bug.  Fixed in apt_key 1.10 (fix is present in Ubuntu 16.04 but
not Ubuntu 14.0 or some Debians).

Fixes ansible#5237

* Check that apt-key really erased the key

When erasing a key, apt-key does not understand how to process subkeys.
This update explicitly checks that the key_id is no longer present and
throws an error if it is.  It also hints at subkeys being a possible
problem in the error message and the documentation.

Fixes ansible#5119

* Fix apt_key check mode with long ids

apt-key can be given a key id longer than 16 chars to more accurately
define what key to download.  However, we can use a maximum of 16
chars to verify whether a key is installed or not.  So we need to use
different lengths for the id depending on what we're doing with it.

Fixes ansible#2622

Also:

* Some style cleanups
* Use get_bin_path to find the path to apt-key and then use that when
  invoking apt-key
* Return a nice user error message if the key was not found on the
  keyserver
* Make file and keyring parameters type='path' so envars and tilde are
  expanded

abadger added a commit that referenced this pull request Oct 24, 2016

Backport the newest revision of apt_key from 2.2
apt_key has some handling of key ids that can be security concerns.

See this issue:
#5237

and this pair of PRs:
#5353
#5357

abadger added a commit that referenced this pull request Oct 24, 2016

Backport 2.2 version of apt_key to 1.9 to fix security issues
apt_key has some handling of key ids that can be security concerns.

See this issue:
#5237

and this pair of PRs:
#5353
#5357

bdowling added a commit to bdowling/ansible-modules-core that referenced this pull request Oct 28, 2016

Only change to short IDs for delete (ansible#5353)
* Only change to short IDs for delete

If the user specifies long IDs, use them for all commands except for
deleting a key.  Need to use short IDs there because of an upstream
apt_key bug.  Fixed in apt_key 1.10 (fix is present in Ubuntu 16.04 but
not Ubuntu 14.0 or some Debians).

Fixes ansible#5237

* Check that apt-key really erased the key

When erasing a key, apt-key does not understand how to process subkeys.
This update explicitly checks that the key_id is no longer present and
throws an error if it is.  It also hints at subkeys being a possible
problem in the error message and the documentation.

Fixes ansible#5119

* Fix apt_key check mode with long ids

apt-key can be given a key id longer than 16 chars to more accurately
define what key to download.  However, we can use a maximum of 16
chars to verify whether a key is installed or not.  So we need to use
different lengths for the id depending on what we're doing with it.

Fixes ansible#2622

Also:

* Some style cleanups
* Use get_bin_path to find the path to apt-key and then use that when
  invoking apt-key
* Return a nice user error message if the key was not found on the
  keyserver
* Make file and keyring parameters type='path' so envars and tilde are
  expanded
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.