launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #24596
[Merge] ~cjwatson/lp-signing:encrypted-private-keys into lp-signing:master
Colin Watson has proposed merging ~cjwatson/lp-signing:encrypted-private-keys into lp-signing:master.
Commit message:
Encrypt private keys at rest
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/lp-signing/+git/lp-signing/+merge/382413
The encrypted-container code is borrowed from Launchpad.
Changing the existing database migration is obviously a massive cheat, but as it happens there are no Key rows on staging right now and no production deployment at all yet; indeed, the charm doesn't actually run database migrations yet. We can thus get away with changing this in place and save some effort. Existing test deployments should probably run "make reset-db", noting that that will wipe any existing data.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/lp-signing:encrypted-private-keys into lp-signing:master.
diff --git a/README.rst b/README.rst
index 4d990f7..7662378 100644
--- a/README.rst
+++ b/README.rst
@@ -35,6 +35,11 @@ Generate a key pair, and add the private half to `service_private_keys`
$ env/bin/lp-signing generate-key-pair
+Generate another key pair, and add the private half to `private_keys`
+(JSON-encoded) in the `[key_storage]` section of `service.conf`::
+
+ $ env/bin/lp-signing generate-key-pair
+
Start the server::
$ make run
diff --git a/charm/Makefile b/charm/Makefile
index a9218f2..28de120 100644
--- a/charm/Makefile
+++ b/charm/Makefile
@@ -63,11 +63,18 @@ tmp/service-private-key: | $(TMPDIR)
--private-key-path tmp/service-private-key \
--public-key-path tmp/service-public-key
-bundle.yaml: bundle.yaml.in tmp/signing.launchpad.test.crt tmp/service-private-key
+tmp/key-storage-private-key: | $(TMPDIR)
+ @$(MAKE) -C ..
+ ../env/bin/lp-signing generate-key-pair \
+ --private-key-path tmp/key-storage-private-key \
+ --public-key-path tmp/key-storage-public-key
+
+bundle.yaml: bundle.yaml.in tmp/signing.launchpad.test.crt tmp/service-private-key tmp/key-storage-private-key
sed -e 's/%BUILD_LABEL%/$(BUILD_LABEL)/g' \
-e "s/%SSL_KEY%/$$(base64 -w 0 <tmp/signing.launchpad.test.key)/g" \
-e "s/%SSL_CERT%/$$(base64 -w 0 <tmp/signing.launchpad.test.crt)/g" \
-e "s/%SERVICE_PRIVATE_KEY%/$$(cat tmp/service-private-key)/g" \
+ -e "s/%KEY_STORAGE_PRIVATE_KEY%/$$(cat tmp/key-storage-private-key)/g" \
bundle.yaml.in >bundle.yaml
deploy: build payload bundle.yaml
diff --git a/charm/bundle.yaml.in b/charm/bundle.yaml.in
index b99a248..cf34c11 100644
--- a/charm/bundle.yaml.in
+++ b/charm/bundle.yaml.in
@@ -16,6 +16,7 @@ applications:
num_units: 1
options:
build_label: "%BUILD_LABEL%"
+ key_storage_private_keys: '["%KEY_STORAGE_PRIVATE_KEY%"]'
service_private_keys: '["%SERVICE_PRIVATE_KEY%"]'
resources:
lp-signing: "../build/%BUILD_LABEL%/lp-signing.tar.gz"
diff --git a/charm/lp-signing/config.yaml b/charm/lp-signing/config.yaml
index 384473d..bbe6b25 100644
--- a/charm/lp-signing/config.yaml
+++ b/charm/lp-signing/config.yaml
@@ -6,6 +6,13 @@ options:
A JSON-encoded list of base64-encoded private service keys. The first
key in the list is the preferred one; older keys may follow,
permitting rollover.
+ key_storage_private_keys:
+ type: string
+ default: "[]"
+ description: >
+ A JSON-encoded list of base64-encoded NaCl private keys for decrypting
+ private keys held in the database. Currently only the first one is
+ used, but this may change in future to permit rollover.
log_hosts_allow:
type: string
default: ""
diff --git a/charm/lp-signing/templates/service.conf.j2 b/charm/lp-signing/templates/service.conf.j2
index 044fbca..82d8b6b 100644
--- a/charm/lp-signing/templates/service.conf.j2
+++ b/charm/lp-signing/templates/service.conf.j2
@@ -7,3 +7,9 @@ standby_urls: [{% if standby_urls %}"{{ standby_urls|join('", "') }}"{% endif %}
# follow, permitting rollover.
service_private_keys: {{ service_private_keys }}
+[key_storage]
+# Base64-encoded NaCl private keys for decrypting private keys held in the
+# database. Currently only the first one is used, but this may change in
+# future to permit rollover.
+private_keys: {{ key_storage_private_keys }}
+
diff --git a/lp_signing/config.py b/lp_signing/config.py
index e6d1706..2c4036b 100644
--- a/lp_signing/config.py
+++ b/lp_signing/config.py
@@ -30,6 +30,12 @@ DEFAULT_TEST_CONFIG = {
"UTF-8"),
]),
},
+ "key_storage": {
+ "private_keys": json.dumps([
+ PrivateKey.generate().encode(encoder=Base64Encoder).decode(
+ "UTF-8"),
+ ]),
+ },
}
diff --git a/lp_signing/crypto.py b/lp_signing/crypto.py
new file mode 100644
index 0000000..671338c
--- /dev/null
+++ b/lp_signing/crypto.py
@@ -0,0 +1,130 @@
+# Copyright 2019-2020 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""A container for data encrypted at rest using configured keys."""
+
+__all__ = [
+ 'NaClEncryptedContainerBase',
+ ]
+
+import base64
+
+from nacl.exceptions import CryptoError as NaClCryptoError
+from nacl.public import (
+ PrivateKey,
+ PublicKey,
+ SealedBox,
+ )
+
+
+class CryptoError(Exception):
+ pass
+
+
+class NaClEncryptedContainerBase:
+ """A container that can encrypt and decrypt data using NaCl.
+
+ See `IEncryptedContainer`.
+ """
+
+ @property
+ def public_key_bytes(self):
+ """The serialised public key as a byte string.
+
+ Concrete implementations must provide this.
+ """
+ raise NotImplementedError
+
+ @property
+ def public_key(self):
+ """The public key as a L{nacl.public.PublicKey}."""
+ if self.public_key_bytes is not None:
+ try:
+ return PublicKey(self.public_key_bytes)
+ except NaClCryptoError as e:
+ raise CryptoError(str(e)) from e
+ else:
+ return None
+
+ @property
+ def can_encrypt(self):
+ """Is this container configured to encrypt data?"""
+ try:
+ return self.public_key is not None
+ except CryptoError:
+ return False
+
+ def encrypt(self, data):
+ """Encrypt a blob of data to a JSON-serialisable form.
+
+ This includes the public key to ease future key rotation.
+
+ :param data: An unencrypted byte string to encrypt.
+ :return: A tuple of (base64-encoded public key, base64-encoded
+ encrypted text string).
+ :raises RuntimeError: if no public key is configured for this
+ container.
+ """
+ if self.public_key is None:
+ raise RuntimeError("No public key configured")
+ try:
+ data_encrypted = SealedBox(self.public_key).encrypt(data)
+ except NaClCryptoError as e:
+ raise CryptoError(str(e)) from e
+ return (
+ base64.b64encode(self.public_key_bytes).decode("UTF-8"),
+ base64.b64encode(data_encrypted).decode("UTF-8"))
+
+ @property
+ def private_key_bytes(self):
+ """The serialised private key as a byte string.
+
+ Concrete implementations must provide this.
+ """
+ raise NotImplementedError
+
+ @property
+ def private_key(self):
+ """The private key as a L{nacl.public.PrivateKey}."""
+ if self.private_key_bytes is not None:
+ try:
+ return PrivateKey(self.private_key_bytes)
+ except NaClCryptoError as e:
+ raise CryptoError(str(e)) from e
+ else:
+ return None
+
+ @property
+ def can_decrypt(self):
+ """Is this container configured to decrypt data?"""
+ try:
+ return self.private_key is not None
+ except CryptoError:
+ return False
+
+ def decrypt(self, data):
+ """Decrypt data that was encrypted by L{encrypt}.
+
+ :param data: A tuple of (base64-encoded public key, base64-encoded
+ encrypted text string) to decrypt.
+ :return: An unencrypted byte string.
+ :raises ValueError: if no private key is configured for this container
+ that corresponds to the requested public key.
+ :raises CryptoError: if decryption failed.
+ """
+ public_key, encrypted = data
+ try:
+ public_key_bytes = base64.b64decode(public_key.encode("UTF-8"))
+ encrypted_bytes = base64.b64decode(encrypted.encode("UTF-8"))
+ except TypeError as e:
+ raise CryptoError(str(e)) from e
+ if public_key_bytes != self.public_key_bytes:
+ raise ValueError(
+ "Public key %r does not match configured public key %r" %
+ (public_key_bytes, self.public_key_bytes))
+ if self.private_key is None:
+ raise ValueError("No private key configured")
+ try:
+ return SealedBox(self.private_key).decrypt(encrypted_bytes)
+ except NaClCryptoError as e:
+ raise CryptoError(str(e)) from e
diff --git a/lp_signing/model/key.py b/lp_signing/model/key.py
index 22f02bd..84ddc46 100644
--- a/lp_signing/model/key.py
+++ b/lp_signing/model/key.py
@@ -7,7 +7,9 @@ __all__ = [
"Key",
]
+import base64
from contextlib import contextmanager
+import json
import logging
from pathlib import Path
import shutil
@@ -17,6 +19,7 @@ from tempfile import TemporaryDirectory
from textwrap import dedent
from flask_storm import store
+from nacl.public import PrivateKey
import pytz
from storm.locals import (
DateTime,
@@ -26,7 +29,10 @@ from storm.locals import (
Storm,
Unicode,
)
+from storm.databases.postgres import JSON
+from lp_signing.config import read_config
+from lp_signing.crypto import NaClEncryptedContainerBase
from lp_signing.database.constants import DEFAULT
from lp_signing.database.enumcol import DBEnum
from lp_signing.enums import (
@@ -104,6 +110,36 @@ def _log_subprocess_run(args, **kwargs):
return process
+class KeyStorageEncryptedContainer(NaClEncryptedContainerBase):
+
+ @property
+ def public_key_bytes(self):
+ private_key_bytes = self.private_key_bytes
+ if private_key_bytes:
+ return bytes(PrivateKey(private_key_bytes).public_key)
+ else:
+ return None
+
+ @property
+ def private_key_bytes(self):
+ config = read_config()
+ try:
+ private_keys = [
+ base64.b64decode(key.encode("UTF-8"))
+ for key in json.loads(config["key_storage"]["private_keys"])
+ ]
+ except Exception: # pragma: no cover
+ # Yes, this is uninformative, but we don't want to accidentally
+ # dump private keys in exceptions.
+ raise ValueError("Failed to load key_storage.private_keys")
+ if private_keys:
+ # XXX cjwatson 2020-04-16: Work out some scheme to permit
+ # rollover.
+ return private_keys[0]
+ else:
+ return None
+
+
class Key(Storm):
"""A signing key."""
@@ -114,7 +150,9 @@ class Key(Storm):
key_type = DBEnum(enum=KeyType, allow_none=False)
fingerprint = Unicode(name="fingerprint", allow_none=False)
- private_key = RawStr(name="private_key", allow_none=False)
+ # The actual private key material is encrypted at rest.
+ _private_key = JSON(name="private_key", allow_none=False)
+
public_key = RawStr(name="public_key", allow_none=False)
created_at = DateTime(name="created_at", tzinfo=pytz.UTC, allow_none=False)
@@ -124,7 +162,7 @@ class Key(Storm):
created_at=DEFAULT, updated_at=DEFAULT):
self.key_type = key_type
self.fingerprint = fingerprint
- self.private_key = private_key
+ self.setPrivateKey(private_key)
self.public_key = public_key
self.created_at = created_at
self.updated_at = updated_at
@@ -156,6 +194,22 @@ class Key(Storm):
__repr__ = __str__
+ def getPrivateKey(self):
+ """Get the private key.
+
+ :return: The bytes of the private key.
+ """
+ container = KeyStorageEncryptedContainer()
+ return container.decrypt(self._private_key)
+
+ def setPrivateKey(self, private_key):
+ """Set the private key.
+
+ :param private_key: The bytes of the private key.
+ """
+ container = KeyStorageEncryptedContainer()
+ self._private_key = container.encrypt(private_key)
+
@classmethod
def getByTypeAndFingerprint(cls, key_type, fingerprint):
"""Get a key by key type and fingerprint.
@@ -300,7 +354,7 @@ class Key(Storm):
_log.info("Injected new key with fingerprint %s", fingerprint)
try:
key = Key.getByTypeAndFingerprint(key_type, fingerprint)
- if (key.private_key == private_key) and (
+ if (key.getPrivateKey() == private_key) and (
key.public_key == public_key):
if key.created_at > created_at:
key.created_at = created_at
@@ -357,7 +411,7 @@ class Key(Storm):
"""
with _temporary_path() as tmp:
key = tmp / f"{self.key_type.name.lower()}.key"
- key.write_bytes(self.private_key)
+ key.write_bytes(self.getPrivateKey())
cert = tmp / f"{self.key_type.name.lower()}.crt"
cert.write_bytes(self.public_key)
message_path = tmp / message_name
diff --git a/lp_signing/model/tests/test_key.py b/lp_signing/model/tests/test_key.py
index f6a4e65..48bc737 100644
--- a/lp_signing/model/tests/test_key.py
+++ b/lp_signing/model/tests/test_key.py
@@ -32,6 +32,7 @@ from lp_signing.model.key import Key
from lp_signing.tests import factory
from lp_signing.tests.matchers import RanCommand
from lp_signing.tests.testfixtures import (
+ ConfigOverrideFixture,
DatabaseFixture,
FakeKmodSign,
FakeMkimage,
@@ -45,6 +46,7 @@ class TestKey(TestCase):
def setUp(self):
super().setUp()
+ self.useFixture(ConfigOverrideFixture({}))
self.useFixture(DatabaseFixture())
self.processes_fixture = self.useFixture(FakeProcesses())
@@ -107,10 +109,10 @@ class TestKey(TestCase):
self.assertThat(key, MatchesStructure.byEquality(
key_type=KeyType.UEFI,
fingerprint=fingerprint,
- private_key=private_key,
public_key=public_key,
created_at=now,
updated_at=now))
+ self.assertEqual(private_key, key.getPrivateKey())
self.assertEqual(
key, Key.getByTypeAndFingerprint(KeyType.UEFI, fingerprint))
req_args = [
@@ -144,10 +146,10 @@ class TestKey(TestCase):
self.assertThat(key, MatchesStructure.byEquality(
key_type=KeyType.KMOD,
fingerprint=fingerprint,
- private_key=private_key,
public_key=public_key,
created_at=now,
updated_at=now))
+ self.assertEqual(private_key, key.getPrivateKey())
self.assertEqual(
key, Key.getByTypeAndFingerprint(KeyType.KMOD, fingerprint))
self.assertIn("[ req ]", fake_openssl.keygen_text)
@@ -193,10 +195,10 @@ class TestKey(TestCase):
self.assertThat(key, MatchesStructure.byEquality(
key_type=KeyType.OPAL,
fingerprint=fingerprint,
- private_key=private_key,
public_key=public_key,
created_at=now,
updated_at=now))
+ self.assertEqual(private_key, key.getPrivateKey())
self.assertEqual(
key, Key.getByTypeAndFingerprint(KeyType.OPAL, fingerprint))
self.assertIn("[ req ]", fake_openssl.keygen_text)
@@ -240,10 +242,10 @@ class TestKey(TestCase):
self.assertThat(key, MatchesStructure.byEquality(
key_type=KeyType.SIPL,
fingerprint=fingerprint,
- private_key=private_key,
public_key=public_key,
created_at=now,
updated_at=now))
+ self.assertEqual(private_key, key.getPrivateKey())
self.assertEqual(
key, Key.getByTypeAndFingerprint(KeyType.SIPL, fingerprint))
self.assertIn("[ req ]", fake_openssl.keygen_text)
@@ -287,10 +289,10 @@ class TestKey(TestCase):
self.assertThat(key, MatchesStructure.byEquality(
key_type=KeyType.FIT,
fingerprint=fingerprint,
- private_key=private_key,
public_key=public_key,
created_at=now,
updated_at=now))
+ self.assertEqual(private_key, key.getPrivateKey())
self.assertEqual(
key, Key.getByTypeAndFingerprint(KeyType.FIT, fingerprint))
req_args = [
@@ -465,10 +467,10 @@ class TestKey(TestCase):
self.assertThat(key, MatchesStructure.byEquality(
key_type=KeyType.UEFI,
fingerprint=fingerprint,
- private_key=private_key,
public_key=public_key,
created_at=created_at,
updated_at=now))
+ self.assertEqual(private_key, key.getPrivateKey())
self.assertEqual(
key, Key.getByTypeAndFingerprint(KeyType.UEFI, fingerprint))
fingerprint_args = [
@@ -496,10 +498,10 @@ class TestKey(TestCase):
self.assertThat(key, MatchesStructure.byEquality(
key_type=KeyType.KMOD,
fingerprint=fingerprint,
- private_key=private_key,
public_key=public_key,
created_at=created_at,
updated_at=now))
+ self.assertEqual(private_key, key.getPrivateKey())
self.assertEqual(
key, Key.getByTypeAndFingerprint(KeyType.KMOD, fingerprint))
fingerprint_args = [
@@ -527,10 +529,10 @@ class TestKey(TestCase):
self.assertThat(key, MatchesStructure.byEquality(
key_type=KeyType.OPAL,
fingerprint=fingerprint,
- private_key=private_key,
public_key=public_key,
created_at=created_at,
updated_at=now))
+ self.assertEqual(private_key, key.getPrivateKey())
self.assertEqual(
key, Key.getByTypeAndFingerprint(KeyType.OPAL, fingerprint))
fingerprint_args = [
@@ -558,10 +560,10 @@ class TestKey(TestCase):
self.assertThat(key, MatchesStructure.byEquality(
key_type=KeyType.SIPL,
fingerprint=fingerprint,
- private_key=private_key,
public_key=public_key,
created_at=created_at,
updated_at=now))
+ self.assertEqual(private_key, key.getPrivateKey())
self.assertEqual(
key, Key.getByTypeAndFingerprint(KeyType.SIPL, fingerprint))
fingerprint_args = [
@@ -589,10 +591,10 @@ class TestKey(TestCase):
self.assertThat(key, MatchesStructure.byEquality(
key_type=KeyType.FIT,
fingerprint=fingerprint,
- private_key=private_key,
public_key=public_key,
created_at=created_at,
updated_at=now))
+ self.assertEqual(private_key, key.getPrivateKey())
self.assertEqual(
key, Key.getByTypeAndFingerprint(KeyType.FIT, fingerprint))
fingerprint_args = [
diff --git a/lp_signing/tests/test_crypto.py b/lp_signing/tests/test_crypto.py
new file mode 100644
index 0000000..4c1987e
--- /dev/null
+++ b/lp_signing/tests/test_crypto.py
@@ -0,0 +1,78 @@
+# Copyright 2019-2020 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for encrypted data containers."""
+
+from nacl.public import PrivateKey
+from testtools import TestCase
+
+from lp_signing.crypto import (
+ CryptoError,
+ NaClEncryptedContainerBase,
+ )
+
+
+class FakeEncryptedContainer(NaClEncryptedContainerBase):
+
+ def __init__(self, public_key_bytes, private_key_bytes=None):
+ self._public_key_bytes = public_key_bytes
+ self._private_key_bytes = private_key_bytes
+
+ @property
+ def public_key_bytes(self):
+ return self._public_key_bytes
+
+ @property
+ def private_key_bytes(self):
+ return self._private_key_bytes
+
+
+class TestNaClEncryptedContainerBase(TestCase):
+
+ def test_public_key_valid(self):
+ public_key = PrivateKey.generate().public_key
+ container = FakeEncryptedContainer(bytes(public_key))
+ self.assertEqual(public_key, container.public_key)
+ self.assertTrue(container.can_encrypt)
+
+ def test_public_key_invalid(self):
+ container = FakeEncryptedContainer(b"nonsense")
+ self.assertRaises(CryptoError, getattr, container, "public_key")
+ self.assertFalse(container.can_encrypt)
+
+ def test_public_key_unset(self):
+ container = FakeEncryptedContainer(None)
+ self.assertIsNone(container.public_key)
+ self.assertFalse(container.can_encrypt)
+
+ def test_encrypt_without_private_key(self):
+ # Encryption only requires the public key, not the private key.
+ public_key = PrivateKey.generate().public_key
+ container = FakeEncryptedContainer(bytes(public_key))
+ self.assertIsNotNone(container.encrypt(b"plaintext"))
+
+ def test_private_key_valid(self):
+ private_key = PrivateKey.generate()
+ container = FakeEncryptedContainer(
+ bytes(private_key.public_key), bytes(private_key))
+ self.assertEqual(private_key, container.private_key)
+ self.assertTrue(container.can_decrypt)
+
+ def test_private_key_invalid(self):
+ public_key = PrivateKey.generate().public_key
+ container = FakeEncryptedContainer(bytes(public_key), b"nonsense")
+ self.assertRaises(CryptoError, getattr, container, "private_key")
+ self.assertFalse(container.can_decrypt)
+
+ def test_private_key_unset(self):
+ public_key = PrivateKey.generate().public_key
+ container = FakeEncryptedContainer(bytes(public_key), None)
+ self.assertIsNone(container.private_key)
+ self.assertFalse(container.can_decrypt)
+
+ def test_encrypt_decrypt(self):
+ private_key = PrivateKey.generate()
+ container = FakeEncryptedContainer(
+ bytes(private_key.public_key), bytes(private_key))
+ self.assertEqual(
+ b"plaintext", container.decrypt(container.encrypt(b"plaintext")))
diff --git a/migrations/patch-0-1-0-std.sql b/migrations/patch-0-1-0-std.sql
index e77bfa2..b81fea8 100644
--- a/migrations/patch-0-1-0-std.sql
+++ b/migrations/patch-0-1-0-std.sql
@@ -38,7 +38,7 @@ CREATE TABLE key (
id serial PRIMARY KEY,
key_type integer NOT NULL,
fingerprint text NOT NULL,
- private_key bytea NOT NULL,
+ private_key jsonb NOT NULL,
public_key bytea NOT NULL,
created_at timestamp without time zone DEFAULT (CURRENT_TIMESTAMP AT TIME ZONE 'UTC') NOT NULL,
updated_at timestamp without time zone DEFAULT (CURRENT_TIMESTAMP AT TIME ZONE 'UTC') NOT NULL
diff --git a/service.conf b/service.conf
index 727cf41..ea0de24 100644
--- a/service.conf
+++ b/service.conf
@@ -6,3 +6,9 @@ standby_urls: []
# The first key in this sequence is the preferred one. Older keys may
# follow, permitting rollover.
service_private_keys: []
+
+[key_storage]
+# Base64-encoded NaCl private keys for decrypting private keys held in the
+# database. Currently only the first one is used, but this may change in
+# future to permit rollover.
+private_keys: []
