GPG2 signer #199

g-k · 2018-12-19T22:09:24Z

Changes:

Adds a new signer with type gpg2 that shells out to a gpg2 binary to handle the RelEng use case and as a reference for a pure golang implementation if we decide to do that in the future. Per the signer readme it expects a passphrase, key ID, and public key in addition to the armored private key.
Changes Travis CI to build a container and run tests inside it (we need gpg > 2.1 and Travis CI doesn't support the Ubuntu Bionic base image where it's the default tracked in https://travis-ci.community/t/ubuntu-18-04-1-lts-bionic-beaver/1270 I didn't see a 2.2 pkg in backports and continuing to mess around with aliasing or building from source seemed worse)
Adds an autograph config with just the APK signers for the CircleCI testsignapk job to workaround CircleCI only having gpg 2.1 in (which resulted in a permissions error creating the subkey at test gpg2 signer init)

NB: this will require a monitor deploy since it adds a new signer type

Implementation-wise:

at signer init / app start we
- create a temp dir /tmp/autograph_gpg2_<signer_id>
- write public and private keys to gpg2_publickey and gpg2_privatekey in that dir
- import the keys into gpg2 key and secrings
- this creates {autograph_gpg2_keyring.gpg,autograph_gpg2_secring.gpg} in that dir ~~and the keys are not deleted at app exit (since we're assuming we're running in a container, but someone running locally with sensitive keys might want to clean those up)~~
For each request input data is written to a tempfile in /tmp/autograph_gpg2_<signer_id>/ and removed at SignData fn exit
when that app is terminated with SIGTERM, or the platform-generalized os.Interrupt, os.Kill signals (which should translate to SIGINT and SIGKILL on unix) the signer's temp dir is cleaned up (edit: SIGKILL isn't caught)

Functional Test:

go through the steps in the readme to sign and verify a detached signature using the subkey

r? @jvehent

coveralls · 2018-12-19T22:37:55Z

Pull Request Test Coverage Report for Build 1322

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at ?%

Totals
Change from base Build 1312:	0.0%
Covered Lines:
Relevant Lines:	0

💛 - Coveralls

hwine · 2018-12-27T21:03:06Z

@g-k any reason not to go to ubuntu bionic? That has gpg 2.2

g-k · 2018-12-27T21:12:28Z

@hwine per https://travis-ci.community/t/ubuntu-18-04-1-lts-bionic-beaver/1270 Travis CI doesn't support it yet.

autograph.yaml

g-k · 2018-12-27T23:20:00Z

NB: The Travis CI jobs have a failing test, but the overall CI job is passing (maybe due to moving the test inside a container)

--- FAIL: TestStartMain (0.36s)
    main_test.go:296: Get http://localhost:8000/__heartbeat__: dial tcp 127.0.0.1:8000: connect: connection refused

jvehent

This looks good, modulo the concern on having the secring on disk.

jvehent · 2018-12-20T14:06:20Z

Dockerfile

-RUN addgroup --gid 10001 app && \
-
+RUN addgroup --gid 10001 app \
+    && \


fixes a docker warning

jvehent · 2018-12-28T15:04:34Z

autograph.testsignapk.yaml

@@ -0,0 +1,219 @@
+server:


why is this new config file needed?

Because creating the keyring on app start depends on gpg2 > 2.1 and this was a workaround for trying to get that installed on the test image

We can remove it when either the testapksigner image circle uses a gpg2 > 2.1 or we switch to running the tests against containerized autograph

jvehent · 2018-12-28T15:35:30Z

signer/gpg2/gpg2.go

+	// call gpg to load the private key in it
+	gpgLoadPrivateKey := exec.Command("gpg", "--no-default-keyring",
+		"--keyring", keyRingPath,
+		"--secret-keyring", secRingPath,


I'm concern that we're putting the secring on disk with no way to clean it up should autograph crash and the instance get recycled. Other signers keep their configurations in memory so we have no artifact left on disk that could eventually leak secrets. This is harder to do here, because I don't think Go can register a function to run on shutdown/crash. Maybe the passphrase protects the key sufficiently that we don't have to worry about it? Or maybe we can put the secring on a tmpfs?

It is in the tmp dir for the signer.

With Python I'd register an atexit handler. With golang we can handle interrupts and clean up on sigterm, sigint, and sigkill though there might be ways to have it not run and I'd expect docker to duplicate the clean up when it's run in container.

Alternatively, the initial implementation 5a4c7ac#diff-5c7805a5493d8e612e5a2c5e5974d363L70 creates and cleans up the key and sec rings for each run, so we could revert to that with the caveats that 1) we'd need to namespace and manage rings per request (e.g. by include the request ID in the files) and 2) it would increase per request latency since we're doing more IO for each request. That might be cleanest though.

You can't catch sigkill, so that leaves a uncovered corner case.

OK added a cleanup handler and checked that the temp dir is cleaned up.

For multiple gpg2 signers or if other signers add a cleanup handler that calls os.Exit we create a race condition. Assuming cleanup is the way we want to go we'll want to:

move interrupt handling to main

add a cleanup/teardown interface for signers

have the main interrupt handler call each that fn for each signer

OK went ahead and made the above change so we can support multiple gpg2 signers and confirmed that kill -SIGKILL did leave the rings around.

I think our advice should be to run autograph in the container in general and especially if you're testing dep or prod gpg keys, since we've expanded the app boundary to include the filesystem and gpg.

Workaround gpg 2.1 on CCI image

jvehent · 2019-01-03T20:37:54Z

main.go

+// signals and run signer AtExit functions
+func (a *autographer) startCleanupHandler() {
+	c := make(chan os.Signal, 1)
+	signal.Notify(c, os.Interrupt, os.Kill, syscall.SIGTERM)


would this catch autograph itself panicking?

It should just catch the int, kill, and term signals (though Hal pointed out that kill isn't catchable).

If there's a way to panic and trigger those signals I assume they'd be caught, but other signals
like SIGBUS, SIGFPE, and SIGSEGV get converted into runtime panics as usual.

This does mean we'd leave the sec and key rings on disk, but I think they'd be helpful for debugging and don't want to mask actual panics.

https://golang.org/pkg/os/signal/#hdr-Types_of_signals

jvehent · 2019-01-03T20:42:39Z

signer/gpg2/README.rst

+passphrases exported with the `unsupported and non-standard gnu-dummy
+S2K algorithm`_ and signing with subkeys. However, it does require
+that a gpg2 version > 2.1 be installed.
+


please add: gpg2 supports hesoteric key formats that the pgp signer doesn't understand, but it comes at the cost of writing private keys to disk in order to shell out to the gnupg binary. The pgp signer will keep keys in memory at all times, which is more secure. Try using it first and only switch to gpg2 if needed.

jvehent · 2019-01-03T20:55:35Z

signer/gpg2/README.rst

+    - id: some-pgp-key
+      type: gpg2
+      keyid: 0xE09F6B4F9E6FDCCB
+      passphrase: abcdef123


can the passphrase be omitted?

It can be the empty string, but we're assuming there's a passphrase and keyid for this signer and people will use the golang one otherwise.

jvehent · 2019-01-03T20:56:08Z

signer/gpg2/README.rst

+        -----END PGP PRIVATE KEY BLOCK-----
+      publickey: |
+        -----BEGIN PGP PUBLIC KEY BLOCK-----
+	...


nit: missing leading spaces

jvehent · 2019-01-03T21:00:13Z

signer/gpg2/gpg2.go

+
+	// Passphrase is the optional passphrase to use decrypt the
+	// gpg secret key
+	Passphrase string


nit: do those need to be exported?

jvehent · 2019-01-03T21:09:05Z

signer/gpg2/gpg2.go

+	gpgLoadPublicKey := exec.Command("gpg",
+		"--no-default-keyring",
+		"--keyring", keyRingPath,
+		"--secret-keyring", secRingPath,


should we care about command injection via malicious config files? I'm thinking not, but the paranoid in me still thinks enforcing alphanumerical here would be good.

Can't hurt. The paths are built from the signer Type (hardcoded) and KeyID (added alphanumeric validation in New), golang TempDir (can trust), and hardcoded file names. I suppose we could additionally validate them at each call site in case how they're constructed changes.

refs: #199 (review)

fixes: #199 (review)

jvehent · 2019-01-03T21:12:59Z

signer/gpg2/gpg2.go

+}
+
+func (s *GPG2Signer) AtExit() error {
+	os.RemoveAll(s.tmpDir)


removeall can return an error, so maybe just do return os.RemoveAll(s.tmpDir) ?

fixes: #199 (comment)

refs: #199 (review)

jvehent · 2019-01-03T21:21:05Z

signer/signer.go

+	Mode        string `json:"mode"`
+	PrivateKey  string `json:"privatekey,omitempty"`
+	PublicKey   string `json:"publickey,omitempty"`
+	Certificate string `json:"certificate,omitempty"`


gofmt -d signer/signer.go isn't giving me anything

webui quirk? the string of Certificate is misaligned: https://screenshots.firefox.com/lz6CVo8LSu2Rb7GH/github.com

oh that's bizarre I'm seeing : https://screenshots.firefox.com/aPYW8lY8SKavqYSE/github.com

jvehent

r+ w/ nits

refs: #199 (review)

g-k · 2019-01-03T21:46:43Z

huh we aren't getting coverage data https://coveralls.io/jobs/43793714

g-k force-pushed the gpg-cli branch from b5a91ca to 4bb3c4a Compare December 19, 2018 22:24

g-k force-pushed the gpg-cli branch from 02864e6 to 0355d98 Compare December 26, 2018 23:15

hwine reviewed Dec 27, 2018

View reviewed changes

autograph.yaml Show resolved Hide resolved

g-k force-pushed the gpg-cli branch 3 times, most recently from 5985202 to c0b0e1f Compare December 27, 2018 23:03

g-k requested a review from jvehent December 27, 2018 23:14

g-k changed the title ~~PGP CLI signer~~ GPG2 signer Dec 27, 2018

jvehent previously approved these changes Dec 28, 2018

View reviewed changes

g-k dismissed jvehent’s stale review via 8120f9f December 28, 2018 17:57

g-k force-pushed the gpg-cli branch from 522b404 to 4746170 Compare January 2, 2019 15:36

Greg Guthe added 15 commits January 3, 2019 14:03

travis: bump build env to xenial

b32fc8d

docker: install gpg pkg

0ff13d6

docker: fix line continuation warnings

777d029

signer: update docstring for pgp signer type

db84570

make: add pgpcli targets

6882261

signer: add docstrings for signer specific conf attrs

f4b5ebb

signer: gofmt signer.go

d5dfb35

signer: add KeyID and Passphrase to signer conf

48aad9e

signer: add skeleton for new pgpcli type

f01ce9e

add pgpcli test key to dev conf

e4827b0

add pgpcli signer type to main

a09cd95

gofmt main.go

73a2e91

tools: skip monitoring pgpcli signatures too

9b79651

signer: s/pgpcli/gpg2/g

f1e48e0

signer: rename pgpcli dir and files too

07b2f83

Greg Guthe added 9 commits January 3, 2019 14:03

signer: Add readme for gpg2 signer

b468534

signer: Add initial gpg2 implementation

4bbc497

run travis tests in container

e8c5309

signer: preserve and namespace gpg2 keyrings

48d9642

dev config: clarify that pgpsubkey is a test key

396ea77

circleci: add config for testapksign CCI job

444ee84

Workaround gpg 2.1 on CCI image

tools: add client support for gpg2 signer

4450912

signer: register cleanup handler for gpg2 signers

ae100e5

move signer cleanup to main

efd0960

g-k force-pushed the gpg-cli branch from 4746170 to efd0960 Compare January 3, 2019 19:03

jvehent reviewed Jan 3, 2019

View reviewed changes

Greg Guthe added 2 commits January 3, 2019 16:10

signer: clarify risks of gpg2 signer relative to pgp in readme

d6e77a8

refs: #199 (review)

signer: fix whitespace in gpg2 readme

81d4554

fixes: #199 (review)

jvehent reviewed Jan 3, 2019

View reviewed changes

Greg Guthe added 2 commits January 3, 2019 16:15

signer: don't export passphrase from gpg2 module

5461f99

fixes: #199 (comment)

signer: return os.RemoveAll err in gpg2 AtExit cleanup

d457918

refs: #199 (review)

jvehent reviewed Jan 3, 2019

View reviewed changes

jvehent previously approved these changes Jan 3, 2019

View reviewed changes

signer: check gpg2 KeyID is alphanumeric

af8f132

refs: #199 (review)

g-k dismissed jvehent’s stale review via af8f132 January 3, 2019 21:25

gofmt

8c76d99

g-k merged commit 2307a78 into master Jan 4, 2019

g-k mentioned this pull request Jan 14, 2019

code coverage is broken #213

Closed

g-k deleted the gpg-cli branch September 24, 2019 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GPG2 signer #199

GPG2 signer #199

g-k commented Dec 19, 2018 •

edited

coveralls commented Dec 19, 2018 •

edited

hwine commented Dec 27, 2018

g-k commented Dec 27, 2018

g-k commented Dec 27, 2018

jvehent left a comment

jvehent Dec 20, 2018

g-k Dec 28, 2018

jvehent Dec 28, 2018

g-k Dec 28, 2018

g-k Dec 28, 2018

jvehent Dec 28, 2018

g-k Dec 28, 2018

hwine Dec 28, 2018

g-k Dec 28, 2018

g-k Dec 28, 2018

jvehent Jan 3, 2019

g-k Jan 3, 2019

jvehent Jan 3, 2019

jvehent Jan 3, 2019

g-k Jan 3, 2019

jvehent Jan 3, 2019

jvehent Jan 3, 2019

jvehent Jan 3, 2019

g-k Jan 3, 2019

jvehent Jan 3, 2019

jvehent Jan 3, 2019

g-k Jan 3, 2019

jvehent Jan 3, 2019

g-k Jan 3, 2019

jvehent left a comment

g-k commented Jan 3, 2019

GPG2 signer #199

GPG2 signer #199

Conversation

g-k commented Dec 19, 2018 • edited

coveralls commented Dec 19, 2018 • edited

Pull Request Test Coverage Report for Build 1322

💛 - Coveralls

hwine commented Dec 27, 2018

g-k commented Dec 27, 2018

g-k commented Dec 27, 2018

jvehent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvehent left a comment

Choose a reason for hiding this comment

g-k commented Jan 3, 2019

g-k commented Dec 19, 2018 •

edited

coveralls commented Dec 19, 2018 •

edited