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

Author: abadger

packaging/os/apt_key.py

@@ -113,26 +113,63 @@
 from ansible.module_utils.urls import fetch_url
 
 
-gpg_bin = None
-grep_bin = None
 apt_key_bin = None
 
 
 def find_needed_binaries(module):
-    global gpg_bin
-    global grep_bin
     global apt_key_bin
 
-    gpg_bin = module.get_bin_path('gpg', required=True)
-    grep_bin = module.get_bin_path('grep', required=True)
     apt_key_bin = module.get_bin_path('apt-key', required=True)
 
+    ### FIXME: Is there a reason that gpg and grep are checked?  Is it just
+    # cruft or does the apt .deb package not require them (and if they're not
+    # installed, /usr/bin/apt-key fails?)
+    module.get_bin_path('gpg', required=True)
+    module.get_bin_path('grep', required=True)
+
+
+def parse_key_id(key_id):
+    """validate the key_id and break it into segments
+
+    :arg key_id: The key_id as supplied by the user.  A valid key_id will be
+        8, 16, or more hexadecimal chars with an optional leading ``0x``.
+    :returns: The portion of key_id suitable for apt-key del, the portion
+        suitable for comparisons with --list-public-keys, and the portion that
+        can be used with --recv-key.  If key_id is long enough, these will be
+        the last 8 characters of key_id, the last 16 characters, and all of
+        key_id.  If key_id is not long enough, some of the values will be the
+        same.
+
+    * apt-key del <= 1.10 has a bug with key_id != 8 chars
+    * apt-key adv --list-public-keys prints 16 chars
+    * apt-key adv --recv-key can take more chars
+
+    """
+    # Make sure the key_id is valid hexadecimal
+    int(key_id, 16)
+
+    key_id = key_id.upper()
+    if key_id.startswith('0X'):
+        key_id = key_id[2:]
+
+    key_id_len = len(key_id)
+    if (key_id_len != 8 or key_id_len != 16) and key_id_len <= 16:
+        raise ValueError('key_id must be 8, 16, or 16+ hexadecimal characters in length')
+
+    short_key_id = key_id[-8:]
+
+    fingerprint = key_id
+    if key_id_len > 16:
+        fingerprint = key_id[-16:]
+
+    return short_key_id, fingerprint, key_id
+
 
 def all_keys(module, keyring, short_format):
     if keyring:
-        cmd = "apt-key --keyring %s adv --list-public-keys --keyid-format=long" % keyring
+        cmd = "%s --keyring %s adv --list-public-keys --keyid-format=long" % (apt_key_bin, keyring)
     else:
-        cmd = "apt-key adv --list-public-keys --keyid-format=long"
+        cmd = "%s adv --list-public-keys --keyid-format=long" % apt_key_bin
     (rc, out, err) = module.run_command(cmd)
     results = []
     lines = to_native(out).split('\n')
@@ -176,9 +213,9 @@ def download_key(module, url):
 
 def import_key(module, keyring, keyserver, key_id):
     if keyring:
-        cmd = "apt-key --keyring %s adv --keyserver %s --recv %s" % (keyring, keyserver, key_id)
+        cmd = "%s --keyring %s adv --keyserver %s --recv %s" % (apt_key_bin, keyring, keyserver, key_id)
     else:
-        cmd = "apt-key adv --keyserver %s --recv %s" % (keyserver, key_id)
+        cmd = "%s adv --keyserver %s --recv %s" % (apt_key_bin, keyserver, key_id)
     for retry in range(5):
         lang_env = dict(LANG='C', LC_ALL='C', LC_MESSAGES='C')
         (rc, out, err) = module.run_command(cmd, environ_update=lang_env)
@@ -198,25 +235,25 @@ def import_key(module, keyring, keyserver, key_id):
 def add_key(module, keyfile, keyring, data=None):
     if data is not None:
         if keyring:
-            cmd = "apt-key --keyring %s add -" % keyring
+            cmd = "%s --keyring %s add -" % (apt_key_bin, keyring)
         else:
-            cmd = "apt-key add -"
+            cmd = "%s add -" % apt_key_bin
         (rc, out, err) = module.run_command(cmd, data=data, check_rc=True, binary_data=True)
     else:
         if keyring:
-            cmd = "apt-key --keyring %s add %s" % (keyring, keyfile)
+            cmd = "%s --keyring %s add %s" % (apt_key_bin, keyring, keyfile)
         else:
-            cmd = "apt-key add %s" % (keyfile)
+            cmd = "%s add %s" % (apt_key_bin, keyfile)
         (rc, out, err) = module.run_command(cmd, check_rc=True)
     return True
 
 
 def remove_key(module, key_id, keyring):
     # FIXME: use module.run_command, fail at point of error and don't discard useful stdin/stdout
     if keyring:
-        cmd = 'apt-key --keyring %s del %s' % (keyring, key_id)
+        cmd = '%s --keyring %s del %s' % (apt_key_bin, keyring, key_id)
     else:
-        cmd = 'apt-key del %s' % key_id
+        cmd = '%s del %s' % (apt_key_bin, key_id)
     (rc, out, err) = module.run_command(cmd, check_rc=True)
     return True
 
@@ -234,7 +271,8 @@ def main():
             keyserver=dict(required=False),
             state=dict(required=False, choices=['present', 'absent'], default='present')
         ),
-        supports_check_mode=True
+        supports_check_mode=True,
+        mutually_exclusive=(('filename', 'keyserver', 'data', 'url'),),
     )
 
     key_id          = module.params['id']
@@ -246,61 +284,62 @@ def main():
     keyserver       = module.params['keyserver']
     changed         = False
 
+    fingerprint = short_key_id = key_id
+    short_format = False
     if key_id:
         try:
-            int(key_id, 16)
-            if key_id.startswith('0x'):
-                key_id = key_id[2:]
-            key_id = key_id.upper()
-            short_key_id = key_id[-8:]
+            key_id, fingerprint, short_key_id = parse_key_id(key_id)
         except ValueError:
-            module.fail_json(msg="Invalid key_id", id=key_id)
+            module.fail_json(msg='Invalid key_id', id=key_id)
 
-    find_needed_binaries(module)
+        if len(fingerprint) == 8:
+            short_format = True
 
-    short_format = False
-    if key_id == short_key_id:
-        short_format = True
+    find_needed_binaries(module)
 
     keys = all_keys(module, keyring, short_format)
     return_values = {}
 
     if state == 'present':
-        if key_id and key_id in keys:
+        if fingerprint and fingerprint in keys:
             module.exit_json(changed=False)
+        elif fingerprint and fingerprint not in keys and module.check_mode:
+            ### TODO: Someday we could go further -- write keys out to
+            # a temporary file and then extract the key id from there via gpg
+            # to decide if the key is installed or not.
+            module.exit_json(changed=True)
         else:
             if not filename and not data and not keyserver:
                 data = download_key(module, url)
-            if key_id and key_id in keys:
-                module.exit_json(changed=False)
+
+            if filename:
+                add_key(module, filename, keyring)
+            elif keyserver:
+                import_key(module, keyring, keyserver, key_id)
             else:
-                if module.check_mode:
-                    module.exit_json(changed=True)
-                if filename:
-                    add_key(module, filename, keyring)
-                elif keyserver:
-                    import_key(module, keyring, keyserver, key_id)
-                else:
-                    add_key(module, "-", keyring, data)
-                changed=False
-                keys2 = all_keys(module, keyring, short_format)
-                if len(keys) != len(keys2):
-                    changed=True
-                if key_id and key_id not in keys2:
-                    module.fail_json(msg="key does not seem to have been added", id=key_id)
-                module.exit_json(changed=changed)
+                add_key(module, "-", keyring, data)
+
+            changed = False
+            keys2 = all_keys(module, keyring, short_format)
+            if len(keys) != len(keys2):
+                changed=True
+
+            if fingerprint and fingerprint not in keys2:
+                module.fail_json(msg="key does not seem to have been added", id=key_id)
+            module.exit_json(changed=changed)
+
     elif state == 'absent':
         if not key_id:
             module.fail_json(msg="key is required")
-        if key_id in keys:
+        if fingerprint in keys:
             if module.check_mode:
                 module.exit_json(changed=True)
 
             # we use the "short" id: key_id[-8:], short_format=True
             # it's a workaround for https://bugs.launchpad.net/ubuntu/+source/apt/+bug/1481871
             if remove_key(module, short_key_id, keyring):
                 keys = all_keys(module, keyring, short_format)
-                if key_id in keys:
+                if fingerprint in keys:
                     module.fail_json(msg="apt-key del did not return an error but the key was not removed (check that the id is correct and *not* a subkey)", id=key_id)
                 changed = True
             else: