Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

stage0/trust: change error message if prefix/root flag missing #2661

Merged
merged 1 commit into from Jun 9, 2016

Conversation

matthaias
Copy link
Contributor

@matthaias matthaias commented May 19, 2016

Error message is not helpful at the moment for the user

Fixes #480

@jonboulle can you please take a look at this. I am not sure if this is ok so, this is my first code change in a golang project and rkt :)

The result is:

$ ./build-rkt-1.6.0+git/bin/rkt trust coreos.com
trust: Prevented potentially dangerous unbounded trust of all containers signed by keys from coreos.com
To continue trusting this root domain:
    rkt trust --root PUBKEY (local key file, no discovery)
Or, establish a smaller scope with a prefix:
    rkt trust --prefix "example.com/hello"
    rkt trust --prefix "example.com/foo/*"

To continue trusting this root domain:
rkt %s --root %s PUBKEY (local key file, no discovery)
Or, establish a smaller scope with a prefix:
rkt %s --prefix "example.com/hello"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonboulle I deleted the PUBKEY in the lines with the prefix. I hope you meant this in #480 (comment). But i am not sure if this is correct according to https://github.com/coreos/rkt/blob/master/Documentation/subcommands/trust.md#trust-a-key-from-specific-location

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also suspect this is not correct. From a quick look at the code, PUBKEY can be always specified. If omitted, it will be auto-discovered only if a prefix is specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested it with a fresh build of the latest master:

matthiasjahn@ubuntu:~/rkt$ ./build-rkt-1.7.0+git/bin/rkt trust --prefix coreos.com/etcd
pubkey: prefix: "coreos.com/etcd"
key: "https://coreos.com/dist/pubkeys/aci-pubkeys.gpg"
gpg key fingerprint is: 8B86 DE38 890D DB72 9186  7B02 5210 BD88 8818 2190
    CoreOS ACI Builder <release@coreos.com>
Are you sure you want to trust this key (yes/no)?
^C
matthiasjahn@ubuntu:~/rkt$ ./build-rkt-1.7.0+git/bin/rkt trust --prefix coreos.com/etcd https://coreos.com/dist/pubkeys/aci-pubkeys.gpg
pubkey: prefix: "coreos.com/etcd"
key: "https://coreos.com/dist/pubkeys/aci-pubkeys.gpg"
gpg key fingerprint is: 8B86 DE38 890D DB72 9186  7B02 5210 BD88 8818 2190
    CoreOS ACI Builder <release@coreos.com>
Are you sure you want to trust this key (yes/no)?
^C
matthiasjahn@ubuntu:~/rkt$ ./build-rkt-1.7.0+git/bin/rkt trust --prefix coreos.com/etcd aci-pubkeys.gpg
pubkey: prefix: "coreos.com/etcd"
key: "aci-pubkeys.gpg"
gpg key fingerprint is: 8B86 DE38 890D DB72 9186  7B02 5210 BD88 8818 2190
    CoreOS ACI Builder <release@coreos.com>
Are you sure you want to trust this key (yes/no)?
^C
matthiasjahn@ubuntu:~/rkt$ ./build-rkt-1.7.0+git/bin/rkt trust --root aci-pubkeys.gpg
pubkey: prefix: ""
key: "aci-pubkeys.gpg"
gpg key fingerprint is: 8B86 DE38 890D DB72 9186  7B02 5210 BD88 8818 2190
    CoreOS ACI Builder <release@coreos.com>
Are you sure you want to trust this key (yes/no)?
^C
matthiasjahn@ubuntu:~/rkt$ ./build-rkt-1.7.0+git/bin/rkt trust --root https://coreos.com/dist/pubkeys/aci-pubkeys.gpg
pubkey: prefix: ""
key: "https://coreos.com/dist/pubkeys/aci-pubkeys.gpg"
gpg key fingerprint is: 8B86 DE38 890D DB72 9186  7B02 5210 BD88 8818 2190
    CoreOS ACI Builder <release@coreos.com>
Are you sure you want to trust this key (yes/no)?
^C

So it seems, with --prefix it is possible without a PUBKEY (auto-discovery) or with a local PUBKEY-file or with a PUBKEY-file via URL. With --root it has to be a PUBKEY - either local or via URL. Does it mean that the last line: To trust a key for an entire root domain, you must use the --root flag, with a path to a local key file (no discovery) in https://github.com/coreos/rkt/blob/master/Documentation/subcommands/trust.md#rkt-trust should be changed, since it says with a path to a local key file but a URL is also possible? Then i can change that also in this PR if you want.

My suggestion now:

trust: Prevented potentially dangerous unbounded trust of all containers signed by keys from coreos.com
To continue trusting a specific prefix of this domain:
    rkt trust --prefix "coreos.com/foo" [PUBKEY]
    rkt trust --prefix "coreos.com/foo/*" [PUBKEY]
Or, trust the root domain:
    rkt trust --root PUBKEY

Well, i hope i am right. @lucab what do you think?

Copy link
Member

@lucab lucab Jun 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matthaias that's also my understanding.

I was double checking with the code, and I think the remote keyfile option is there on purpose. Thus I guess doc should just mention with a path to a key file (no discovery) (:+1: for spotting it).

Regarding the message, I would try to avoid guessing what the user was doing (ie. rkt trust my_key.pub.gpg is a whole different story, but will be shown your error). It is ok if you hardcode example.com and keep wording similar to your current message, like:

trust: aborting due to implicit unbounded trust (root domain)
Please provide a specific domain prefix to trust:
    rkt trust --prefix "example.com/foo" [PUBKEY]
    rkt trust --prefix "example.com/foo/*" [PUBKEY]
Otherwise, trust at the root domain (not recommended) must be explicitly requested:
    rkt trust --root PUBKEY

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback @lucab, i will change the error message to your suggestion, change the doc and squash it to one commit.

@matthaias
Copy link
Contributor Author

Woops, i didn't expect to brake the build. I hope i can fix this tomorrow.

@lucab
Copy link
Member

lucab commented Jun 6, 2016

@matthaias sorry for the delay in giving feedback on this. It looks like there are some spurious commits/merges on this branch, can you please squash them in a single commit?

@matthaias
Copy link
Contributor Author

No worries, take your time.

[...], can you please squash them in a single commit?

Of course, i just wanted to be sure that this change is correct. So if it's ok, i wait for a approve of somebody?

@@ -59,7 +59,15 @@ func init() {
func runTrust(cmd *cobra.Command, args []string) (exit int) {
if flagPrefix == "" && !flagRoot {
if len(args) != 0 {
stderr.Print("--root required for non-prefixed (root) keys")
trustName := args[0]
cmdName := "trust"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is effectively already hardcoded, you can put it directly into the error message.

@lucab
Copy link
Member

lucab commented Jun 6, 2016

Of course, i just wanted to be sure that this change is correct. So if it's ok, i wait for a approve of somebody?

Ah sorry, I just had a quick look at it and saw that the history was a bit muddy.
I've left some comments inline now, but I'd like to have a second pass once squashed.

Error message is not helpful at the moment for the user

Fixes rkt#480
@lucab
Copy link
Member

lucab commented Jun 9, 2016

@matthaias looks great, thanks!

@lucab lucab added this to the v1.9.0 milestone Jun 9, 2016
@lucab lucab changed the title trust: change error message if prefix/root flag missing stage0/trust: change error message if prefix/root flag missing Jun 9, 2016
@lucab lucab merged commit a55f8ac into rkt:master Jun 9, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rkt trust: Improve UX for root keys
2 participants