trust: add flag --skip-trusted #3812

indradhanush · 2017-09-23T13:14:30Z

Work based on #3756

indradhanush · 2017-09-23T13:15:05Z

indradhanush · 2017-09-23T13:16:42Z

This is a work in progress.
I'm looking for some early feedback before writing a lot of code unnecessarily. Does this look like a step in the right direction? I'll proceed with writing tests if this looks good or make changes if suggested.

alban

I suggest to remove the "Fixes #3756" in the commitmsg since this PR is not about making the output parsable.

alban · 2017-09-25T14:48:34Z

rkt/pubkey/pubkey.go

@@ -110,6 +110,18 @@ func (m *Manager) AddKeys(pkls []string, prefix string, accept AcceptOption) err
 			return errwrap.Wrap(fmt.Errorf("error displaying the key %s", pkl), err)
 		}

+		if overwriteTrusted == false {


Usually we would write if !overwriteTrusted instead of == false.

alban · 2017-09-25T14:48:55Z

rkt/pubkey/pubkey.go

+			}
+
+			if trusted == true {
+				log.Printf("Already trusted %q for prefix %q.", pkl, prefix)


missing \n?

alban · 2017-09-25T14:53:10Z

rkt/trust.go

@@ -28,7 +28,7 @@ import (

 var (
 	cmdTrust = &cobra.Command{
-		Use:   "trust [--prefix=PREFIX] [--insecure-allow-http] [--skip-fingerprint-review] [--root] [PUBKEY ...]",
+		Use:   "trust [--prefix=PREFIX] [--insecure-allow-http] [--skip-fingerprint-review] [--root] [PUBKEY ...] [--overwrite-trusted]",


Shouldn't the --overwrite-trusted=... be before the [PUBKEY...]?

I find boolean parameter defaulting to true confusing because just writing --overwrite-trusted would have no effect.

I am not sure how to make things clearer. I overheard other suggestions but they're kind of long:

--disable-overwrite-trusted

--prevent-trusted-from-being-overwritten

Agreed. --disable-overwrite-trusted seems to do a better job of removing the confusion while also keeping the default to ~~true~~ false for backwards compatibility.

On second thoughts how does --dont-overwrite-trusted sound for the name of the flag?

Another thought, how about --skip-trusted ? It's more concise and also is similar to the --skip-fingerprint-review.

Ack, I think --skip-trusted is a better choice.

What about --skip-already-trusted? Too long?

--skip-already-trusted does feel long. I feel already does not appear to add any additional meaning on --skip-trusted.

Should we stick with --skip-trusted then?

alban · 2017-09-25T14:59:23Z

TODO:

Documentation
Tests

--skip-trusted is more concise and also similar to other flags that have alredy been implemented. The default for this is `false` ensuring that existing this does not break existing users' configurations.

The test for --skip-trusted=true without a trusted key already being present is failing at the moment. Saving progress to get feedback.

indradhanush · 2017-09-27T04:40:40Z

@alban I've made changes as suggested and have added tests. However one of the tests I wrote is failing with the error message:

/tmp/localdir-581577645 --system-config=/tmp/systemdir-483202344 --user-config=/tmp/userdir-884398695 trust --prefix rkt-prefix.com/my-app --skip-trusted key1.gpg
	rkt_tests.go:901: Expected but didn't find "Are you sure you want to trust this key" in pubkey: prefix: "rkt-prefix.com/my-app"
		key: "key1.gpg"
		gpg key fingerprint is: 36B2 87AD D109 5DB1 AFD9  CBF1 25D5 D9FA D9DC EF41
		    Subkey fingerprint: D567 C8E0 5E79 ACA5 9A32  7888 ECA3 3AFD 42E9 C212
			Key for testing only (Key for testing only) <key.for.testing.only@example.com>
		pubkey: Already trusted "key1.gpg" for prefix "rkt-prefix.com/my-app".

I've given this some thought but am unable to understand what could be going wrong. Is there something that I am missing?

alban · 2017-09-27T15:09:01Z

tests/rkt_trust_test.go

@@ -84,4 +84,16 @@ func TestTrust(t *testing.T) {
 	t.Logf("Now both images can be executed\n")
 	runImage(t, ctx, imageFile, "Hello", false)
 	runImage(t, ctx, imageFile2, "Hello", false)
+	t.Logf("Skip trusted key (rkt trust --skip-trusted) with trusted key absent\n")
+	runRktTrustSkipTrustedTrue(t, ctx, "rkt-prefix.com/my-app", 1, false)


You pass the parameter alreadyTrusted==false but the key is already trusted from the previous tests (line 67 and line 82). You could do your new test on a different image with a different prefix that is not yet trusted (e.g. imageFile3, see example line 36). The new test would have to be done before the "rkt trust --root".

alban · 2017-09-27T15:22:02Z

tests/rkt_tests.go

+			t.Fatalf("Expected but didn't find %q in %v", expected, err)
+		}
+	} else {
+		assertTrustPrompt(t, prefix, child)


In this 'else" branch, the key will already be trusted anyway because "rkt trust" is executed twice in this fuction:

line 916 with runRktTrust

line 922 with spawnOrFail

So assertTrustPrompt will fail

Use a new image file with a unique prefix to ensure that the key being tested is not trusted ahead of time. Also the test for `rkt trust --root` should be the last test.

alban · 2017-09-28T11:10:27Z

tests/rkt_trust_test.go

+	runRktTrustSkipTrustedFalse(t, ctx, "rkt-skip-trusted.com/my-app", 1, false)
+
+	t.Logf("Now the image can be executed\n")
+	runImage(t, ctx, imageFile3, "Hello", false)


Could you also try to run the image with runImage after each runRktTrustSkipTrusted* and check it has the correct behavior?

For --skip-trusted=false with trusted key not present, we should use a separate image as well.

indradhanush · 2017-10-13T19:21:54Z

@alban Ping. Updated with latest changes as per review comments.

trust: add flag --overwrite-trusted

f0a7d6b

Fixes rkt#3756

alban suggested changes Sep 25, 2017

View reviewed changes

indradhanush added 4 commits September 26, 2017 11:17

trust: rename --overwrite-trusted to --skip-trusted

af65cbe

--skip-trusted is more concise and also similar to other flags that have alredy been implemented. The default for this is `false` ensuring that existing this does not break existing users' configurations.

pubkey: add newline to log statement for already trusted key

4e0e2de

pubkey: idiomatic go

f362631

trust: add tests for --skip-trusted flag

a571ea0

The test for --skip-trusted=true without a trusted key already being present is failing at the moment. Saving progress to get feedback.

indradhanush changed the title ~~trust: add flag --overwrite-trusted~~ trust: add flag --skip-trusted Sep 27, 2017

alban reviewed Sep 27, 2017

View reviewed changes

indradhanush added 3 commits September 28, 2017 14:28

trust: fix tests for --skip-trusted

8ca5a54

Use a new image file with a unique prefix to ensure that the key being tested is not trusted ahead of time. Also the test for `rkt trust --root` should be the last test.

trust: add documentation for --skip-trusted

a569a3a

trust: minor spacing fixes

0c217e1

alban reviewed Sep 28, 2017

View reviewed changes

trust: test the image after each individual test for --skip-trusted

5405301

For --skip-trusted=false with trusted key not present, we should use a separate image as well.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

trust: add flag --skip-trusted #3812

trust: add flag --skip-trusted #3812

indradhanush commented Sep 23, 2017 •

edited

indradhanush commented Sep 23, 2017

indradhanush commented Sep 23, 2017

alban left a comment

alban Sep 25, 2017

alban Sep 25, 2017

alban Sep 25, 2017

indradhanush Sep 26, 2017 •

edited

indradhanush Sep 26, 2017

indradhanush Sep 26, 2017

lucab Sep 27, 2017

alban Sep 27, 2017

indradhanush Sep 28, 2017

indradhanush Sep 28, 2017

alban commented Sep 25, 2017

indradhanush commented Sep 27, 2017 •

edited

alban Sep 27, 2017

alban Sep 27, 2017

alban Sep 28, 2017

indradhanush commented Oct 13, 2017

trust: add flag --skip-trusted #3812

Are you sure you want to change the base?

trust: add flag --skip-trusted #3812

Conversation

indradhanush commented Sep 23, 2017 • edited

indradhanush commented Sep 23, 2017

indradhanush commented Sep 23, 2017

alban 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

indradhanush Sep 26, 2017 • edited

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

alban commented Sep 25, 2017

indradhanush commented Sep 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indradhanush commented Oct 13, 2017

indradhanush commented Sep 23, 2017 •

edited

indradhanush Sep 26, 2017 •

edited

indradhanush commented Sep 27, 2017 •

edited