Merge lp:~stub/charm-helpers/fix-gpg into lp:charm-helpers

Proposed by Stuart Bishop on 2017-08-15
Status: Merged
Merged at revision: 783
Proposed branch: lp:~stub/charm-helpers/fix-gpg
Merge into: lp:charm-helpers
Diff against target: 83 lines (+38/-24)
1 file modified
charmhelpers/fetch/ (+38/-24)
To merge this branch: bzr merge lp:~stub/charm-helpers/fix-gpg
Reviewer Review Type Date Requested Status
Alex Kavanagh 2017-08-15 Approve on 2017-08-15
Review via email:

Description of the change

A feature of the PostgreSQL charm had stopped working, as charm-helpers was attempting to do more validation of GPG key formats and the PG charm happens to add comments to its keys so they don't get mixed up.

While fixing this, noticed that insecure usage still seems to be promoted. Clearly flag this cases in the docstring and add WARNING messages to logs when people open themselves up to attack (the key retrieval protocol is unencrypted for historical reasons and the same man-in-the-middle attack that poisons an archive can also make people trust keys retrieved this way).

To post a comment you must log in.
Alex Kavanagh (ajkavanagh) wrote :

Looks good and passes tests.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'charmhelpers/fetch/'
2--- charmhelpers/fetch/ 2017-07-20 08:17:20 +0000
3+++ charmhelpers/fetch/ 2017-08-15 07:09:24 +0000
4@@ -27,6 +27,7 @@
5 from charmhelpers.core.hookenv import (
6 log,
9 )
10 from charmhelpers.fetch import SourceConfigError, GPGKeyError
12@@ -261,34 +262,47 @@
13 return apt_mark(packages, 'unhold', fatal=fatal)
16-def import_key(keyid):
17- """Import a key in either ASCII Armor or Radix64 format.
19- `keyid` is either the keyid to fetch from a PGP server, or
20- the key in ASCII armor foramt.
22- :param keyid: String of key (or key id).
23+def import_key(key):
24+ """Import an ASCII Armor key.
26+ /!\ A Radix64 format keyid is also supported for backwards
27+ compatibility, but should never be used; the key retrieval
28+ mechanism is insecure and subject to man-in-the-middle attacks
29+ voiding all signature checks using that key.
31+ :param keyid: The key in ASCII armor format,
32+ including BEGIN and END markers.
33 :raises: GPGKeyError if the key could not be imported
34 """
35- key = keyid.strip()
36- if (key.startswith('-----BEGIN PGP PUBLIC KEY BLOCK-----') and
37- key.endswith('-----END PGP PUBLIC KEY BLOCK-----')):
38+ key = key.strip()
39+ if '-' in key or '\n' in key:
40+ # Send everything not obviously a keyid to GPG to import, as
41+ # we trust its validation better than our own. eg. handling
42+ # comments before the key.
43 log("PGP key found (looks like ASCII Armor format)", level=DEBUG)
44- log("Importing ASCII Armor PGP key", level=DEBUG)
45- with NamedTemporaryFile() as keyfile:
46- with open(, 'w') as fd:
47- fd.write(key)
48- fd.write("\n")
49- cmd = ['apt-key', 'add',]
50- try:
51- subprocess.check_call(cmd)
52- except subprocess.CalledProcessError:
53- error = "Error importing PGP key '{}'".format(key)
54- log(error)
55- raise GPGKeyError(error)
56+ if ('-----BEGIN PGP PUBLIC KEY BLOCK-----' in key and
57+ '-----END PGP PUBLIC KEY BLOCK-----' in key):
58+ log("Importing ASCII Armor PGP key", level=DEBUG)
59+ with NamedTemporaryFile() as keyfile:
60+ with open(, 'w') as fd:
61+ fd.write(key)
62+ fd.write("\n")
63+ cmd = ['apt-key', 'add',]
64+ try:
65+ subprocess.check_call(cmd)
66+ except subprocess.CalledProcessError:
67+ error = "Error importing PGP key '{}'".format(key)
68+ log(error)
69+ raise GPGKeyError(error)
70+ else:
71+ raise GPGKeyError("ASCII armor markers missing from GPG key")
72 else:
73- log("PGP key found (looks like Radix64 format)", level=DEBUG)
74- log("Importing PGP key from keyserver", level=DEBUG)
75+ # We should only send things obviously not a keyid offsite
76+ # via this unsecured protocol, as it may be a secret or part
77+ # of one.
78+ log("PGP key found (looks like Radix64 format)", level=WARNING)
79+ log("INSECURLY importing PGP key from keyserver; "
80+ "full key not provided.", level=WARNING)
81 cmd = ['apt-key', 'adv', '--keyserver',
82 'hkp://', '--recv-keys', key]
83 try:


People subscribed via source and target branches