add ssh-keys, ssh-public-keys, gpg-keys and gpg-public keys interfaces #4100

Merged
merged 15 commits into from Dec 8, 2017

Conversation

Projects
None yet
5 participants
Contributor

jdstrand commented Oct 27, 2017

This is a preliminary implementation for discussion. This was prompted by https://forum.snapcraft.io/t/classic-confinement-for-mosh/2346. With these:

  • gpg-keys: I was able to decrypt/encrypt/sign/verify/list keys using 'gpg --homedir=`getent passwd $UID | cut -d ':' -f 6`/.gnupg ...'
  • gpg-public-keys: I was able to encrypt/verify/list keys using 'gpg --homedir=`getent passwd $UID | cut -d ':' -f 6`/.gnupg --list-keys'
  • ssh-keys: I was able to login to a remote server
  • ssh-public-keys: I was unable to determine a use for this interface

Importantly, these are read-only access. If we ever wanted to create write access, we would likely create ssh-keys-control and gpg-keys-control.

jdstrand added some commits Oct 27, 2017

codecov-io commented Oct 27, 2017

Codecov Report

Merging #4100 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4100      +/-   ##
==========================================
+ Coverage   78.01%   78.04%   +0.03%     
==========================================
  Files         446      450       +4     
  Lines       30856    30896      +40     
==========================================
+ Hits        24071    24113      +42     
+ Misses       4775     4773       -2     
  Partials     2010     2010
Impacted Files Coverage Δ
interfaces/builtin/gpg_keys.go 100% <100%> (ø)
interfaces/builtin/gpg_public_keys.go 100% <100%> (ø)
interfaces/builtin/ssh_public_keys.go 100% <100%> (ø)
interfaces/builtin/ssh_keys.go 100% <100%> (ø)
overlord/ifacestate/helpers.go 64.31% <0%> (+0.82%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1e24ae...3af0f57. Read the comment docs.

zyga approved these changes Oct 30, 2017

+1, one question about known_hosts.

interfaces/builtin/ssh_public_keys.go
+
+owner @{HOME}/.ssh/ r,
+owner @{HOME}/.ssh/environment r,
+owner @{HOME}/.ssh/*.pub r,
@zyga

zyga Oct 30, 2017

Contributor

Should this also allow reading known_hosts?

@niemeyer

niemeyer Nov 10, 2017

Contributor

Should this also allow reading known_hosts?

No, public keys are public. The hosts you connect to is very private information. If we mix the two the interface becomes misleading and dangerous.

@pedronis pedronis requested a review from niemeyer Nov 9, 2017

Thanks for these. Interface names are good, functionality seems good. Only questions are about the writable access into what otherwise looks like a read interface.

interfaces/builtin/gpg_keys.go
+
+/usr/share/gnupg/options.skel r,
+owner @{HOME}/.gnupg/{,**} r,
+owner @{HOME}/.gnupg/random_seed w,
@niemeyer

niemeyer Nov 10, 2017

Contributor

This feels slightly surprising as it disagrees with the "read" bit. Why is it necessary?

@jdstrand

jdstrand Dec 6, 2017

Contributor

From the man page: "~/.gnupg/random_seed - A file used to preserve the state of the internal random pool.'

Without write access, gpg encrypt/decrypt hangs

interfaces/builtin/gpg_public_keys.go
+owner @{HOME}/.gnupg/ r,
+owner @{HOME}/.gnupg/gpg.conf r,
+owner @{HOME}/.gnupg/pubring.gpg{,.lock} r,
+owner @{HOME}/.gnupg/pubring.gpg.lock k,
@niemeyer

niemeyer Nov 10, 2017

Contributor

Similarly, this can lock the user out of gpg usage, right? Not quite write, but a bit more than just read.

@jdstrand

jdstrand Dec 6, 2017

Contributor

These are all locks with read, which means we only allow opens with O_RDONLY and shared/read locks (eg LOCK_SH). The 'r' (O_RDONLY) with an exclusive/write lock (eg LOCK_EX) requires 'w' with the 'k'. As such, these rules do allow locking writes but not reads. I recall adding these since the man page referenced them, but neither gpg1 or gpg2 needs them for encrypt/verify/list/etc so I'll remove them.

@jdstrand

jdstrand Dec 6, 2017

Contributor

This is done.

interfaces/builtin/ssh_public_keys.go
+
+owner @{HOME}/.ssh/ r,
+owner @{HOME}/.ssh/environment r,
+owner @{HOME}/.ssh/*.pub r,
@zyga

zyga Oct 30, 2017

Contributor

Should this also allow reading known_hosts?

@niemeyer

niemeyer Nov 10, 2017

Contributor

Should this also allow reading known_hosts?

No, public keys are public. The hosts you connect to is very private information. If we mix the two the interface becomes misleading and dangerous.

Contributor

jdstrand commented Dec 6, 2017

@niemeyer - this should be ready for review again.

@mvo5 - once this is approved I'll create a branch for 2.30.

@jdstrand jdstrand added this to the 2.30 milestone Dec 6, 2017

mvo5 approved these changes Dec 8, 2017

LGTM

@mvo5 mvo5 merged commit e50b4af into snapcore:master Dec 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Contributor

jdstrand commented Dec 8, 2017

Thanks for merging this! I'll submit a PR for 2.30 shortly.

@jdstrand jdstrand deleted the jdstrand:ssh-gpg-keys-interfaces branch Dec 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment