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
8000
 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 67E6 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

trust: add flag --skip-trusted #3812

Are you sure you want to change the base?

trust: add flag --skip-trusted #3812

Conversation

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