trust: only trust pubkeys on first use, prompt for https trust #2033
Conversation
|
cc @gtank @brianredbeard @pbx0 |
|
With this setup, trusting new keys replaces existing ones. Multiple keys can still be trusted, but they all need to be added in the same step. I don't know what the use case for multiple public keys being trusted for a given prefix is though, so if people think that's important I can add a flag to |
|
Thanks!
|
|
When using For example, if I had keys in several depths of prefixes:
|
|
Fixes #1880 |
| @@ -42,6 +42,14 @@ type Manager struct { | |||
| type AcceptOption int | |||
| type OverrideOption int | |||
|
|
|||
| type ErrKeyForPrefixExists struct { | |||
There was a problem hiding this comment.
why not an error variable instead of a type? (e.g. pkg/lock/file.go:24)
|
Is this making |
|
Nevermind, @alban already pointed that out. |
| for _, p := range pathNamesPrefix { | ||
| _, err := os.Stat(p) | ||
| if err == nil { | ||
| return true, nil |
There was a problem hiding this comment.
If the file exists, the key might still not be there: it could be an empty file in /etc to disable the key in /usr. Arguably, this is an existing bug, but since TrustedKeyPrefixExists is now used for rkt trust, this should be fixed...
|
|
I didn't consider making rkt run interactive, so instead of asking to trust a key to under I wonder if we even want to be checking for existing matching keys at all at this point. We can't use it to trigger an interactive question for Also, I agree that |
|
I have no clue why my rkt is building in Users should still be able to use empty files to disable keys installed in Just added a commit (I'll squash them at the end). Notable changes:
A log of the tty change: Thoughts? |
Strange... does running |
| @@ -106,6 +106,15 @@ func checkSignature(ks *Keystore, prefix string, signed, signature io.ReadSeeker | |||
| return entities, err | |||
| } | |||
|
|
|||
| // DeleteAllTrustedKeysForPrefix deletes all trusted keys for the given prefix | |||
| func (ks *Keystore) DeleteAllTrustedKeysForPrefix(prefix string) error { | |||
There was a problem hiding this comment.
DeleteAllTrustedKeysForPrefix can be deleted now that it is not used anymore.
|
Made the suggested fixes, rebased on master, squashed commits. Should be ready for another pass. |
Even stranger: they don't appear to fix this. I even cloned master to a different directory, and did |
|
I have no idea what's wrong in the build-system. Maybe you have the BUILDDIR env variable defined? What does the line in makelib/variables.mk with |
|
Oh wait, it looks like |
| continue | ||
| } | ||
| if err != nil { | ||
| return false, fmt.Errorf("cannot check dir %q: %v", p, err) |
There was a problem hiding this comment.
return false, errwrap.Wrap(fmt.Errorf("cannot check dir %q", p), err)
|
It would be nice if the commit message explained the current behavior a bit better. Including the examples in the PR. |
|
This LGTM (modulo some nits) but I'd like another pair of eyes on it. |
|
|
||
| if m.TrustKeysFromHTTPS && u.Scheme == "https" { | ||
| accept = AcceptForce | ||
| } | ||
|
|
||
| if accept == AcceptAsk { | ||
| if !terminal.IsTerminal(int(os.Stdin.Fd())) || !terminal.IsTerminal(int(os.Stderr.Fd())) { | ||
| stdout.Printf("To trust the key for %q, do one of the following:", prefix) |
There was a problem hiding this comment.
Shouldn't this be printed to log, instead of stdout?
There was a problem hiding this comment.
I was expecting to see a stderr in addition to a stdout. What's the difference between stdout and log?
There was a problem hiding this comment.
log is basically printing to stderr. See https://github.com/dgonyeo/rkt/blob/rkt-trust/rkt/pubkey/pubkey.go#L58
I guess the name log was chosen to minimize number of changed lines when switching from the log package to our rktlog.
This commit changes rkt's behavior such that when fetching an image through meta discovery, rkt will only download and trust the gpg public key for the image if rkt doesn't have any local keys with a matching prefix. If the key for an image has changed, the user must manually trust the new key with `rkt trust --prefix`. In addition, `--trust-keys-from-https` now defaults to false, so the user is prompted to approve new keys (unless rkt isn't running in a tty, in which case an error is printed).
|
@iaguis updated the commit message and the PR. Thoughts on the new description / examples? |
|
I'll be spending some time today updating the docs and investigating what tests I could write for this, but I won't be able to work too many hours today due to homework, and tomorrow's a full day of classes. If we want to rush to get this in, help finishing this up would be appreciated. |
|
Thanks! Setting the "needs rework" label for the TODO in #2033 (comment) but this new version of the code looks good to me. |
trust: only trust pubkeys on first use, prompt for https trust
Global option --trust-keys-from-https was changed to default to false in rkt#2033 Update the documentation accordingly.
Add an entry for rkt#2033
Follow up for #2033: documentation
New default behavior when fetching an image in a tty:
New default behavior when fetching and not in a tty: