Add some content trust tests #924

vdemeester · 2018-03-07T17:06:27Z

Importing from moby's DockerTrustSuite tests.

This is still wip since there is some tests that are failing and some empty tests I did not yet migrate.
Also, there has been some slight changes between 17.06 and current docker/cli, so one some behavior, I'm not sure if it's supposed to fail or not (cc @n4ss @endophage)

See moby/moby#36515 for original tests that are supposed to be migrated.

Probably require #929 to be able to write unit tests for content trust..

Signed-off-by: Vincent Demeester vincent@sbr.pm

dnephin · 2018-03-07T17:09:07Z

e2e/compose-env.yaml

@@ -17,5 +17,14 @@ services:
       - notary-fixtures:/fixtures
      command: ['notary-server', '-config=/fixtures/notary-config.json']

+  evil-notary-server:


why is it evil?

See https://github.com/moby/moby/pull/36515/files#diff-4b1e56bb77ac16f2ccf956fc24cf0a82L350 😛
Definitely open to name it something else, but I'll need to re-generate the certs 😅

Maybe malicious? I'm not sure what the intent is here.

codecov-io · 2018-03-07T17:11:29Z

Codecov Report

Merging #924 into master will decrease coverage by 0.14%.
The diff coverage is 35.29%.

@@            Coverage Diff             @@
##           master     #924      +/-   ##
==========================================
- Coverage   53.92%   53.78%   -0.15%     
==========================================
  Files         262      262              
  Lines       16604    16601       -3     
==========================================
- Hits         8954     8929      -25     
- Misses       7049     7084      +35     
+ Partials      601      588      -13

dnephin

This is a ton of work. Thanks for porting these!

dnephin · 2018-03-07T22:48:05Z

dockerfiles/Dockerfile.dev

+ARG     NOTARY_VERSION=v0.6.0
+RUN     export URL=https://github.com/theupdateframework/notary/releases/download; \
+        curl -Ls $URL/${NOTARY_VERSION}/notary-Linux-amd64 -o /usr/local/bin/notary && \
+        chmod +x /usr/local/bin/notary


Why do we need this in the dev image? Aren't we using a docker image for the notary server?

For some tests (to come), we need to use the notary client to set up the server in some state.

Maybe add a TODO to move to a separate stage when we make this multi-stage? and document that it's only used from the e2e runner?

dnephin · 2018-03-07T22:48:59Z

e2e/compose-env.yaml

@@ -17,5 +17,14 @@ services:
       - notary-fixtures:/fixtures
      command: ['notary-server', '-config=/fixtures/notary-config.json']

+  evil-notary-server:


Maybe malicious? I'm not sure what the intent is here.

dnephin · 2018-03-07T22:49:44Z

e2e/container/create_test.go

+	)
+	result.Assert(t, icmd.Expected{
+		ExitCode: 1,
+		Err:      "does not have trust data for",


Can we test error cases with unit tests?

I guess this is running now, so maybe just add a TODO?

yes, and we probably should, that's a good point. I only worked moving from the moby's integration to cli's e2e tests but those could definitely be unit tests instead 👼 (with just one e2e tests that validates the behavior against a real server)

dnephin · 2018-03-07T22:52:28Z

e2e/image/pull_trust_test.go

@@ -0,0 +1,78 @@
+package image


Can we keep this as pull_test.go ? We shouldn't have so many e2e tests that we need more than a single file per command.

dnephin · 2018-03-07T22:54:33Z

e2e/image/pull_trust_test.go

+	})
+}
+
+func TestPullWithContentTrustUnreachableServer(t *testing.T) {


TestPullWithContentTrustUsesCacheWhenNotaryUnavailable ?

dnephin · 2018-03-07T23:01:02Z

e2e/container/create_test.go

+	"github.com/gotestyourself/gotestyourself/icmd"
+)
+
+func TestCreateWithContentTrust(t *testing.T) {


If both create and run use exactly the same code, do we need e2e tests for both?

I personally prefer too much tests than the opposite. Today they run the same code but we can catch an issue the day this is not true anymore (extremely unlikely I admit).

dnephin · 2018-03-07T23:06:36Z

e2e/image/push_trust_test.go

+		Err:      "error contacting notary server",
+	})
+
+	// FIXME(vdemeester) doesn't work


ya, this is weird. I don't understand why it would hit notary if content trust is disabled.

Yep, but somehow, in moby, with 17.06, that's something it was doing 😅

dnephin · 2018-03-07T23:07:21Z

e2e/image/push_trust_test.go

@@ -0,0 +1,160 @@
+package image


push_test.go ?

dnephin · 2018-03-07T23:07:48Z

e2e/image/push_trust_test.go

+
+func TestPushWithContentTrustSignsForRolesWithKeysAndValidPaths(t *testing.T) {}
+
+func TestPullWithContentTrustSignsForRolesWithKeysAndValidPaths(t *testing.T) {}


There are way too many e2e tests here. It must be possible for some of these to be unit tests.

dnephin · 2018-03-07T23:11:23Z

e2e/plugin/trust_test.go

+
+const registryPrefix = "registry:5000"
+
+func TestPluginInstallWithContentTrust(t *testing.T) {


TestInstallWithContentTrust

vdemeester · 2018-03-12T16:53:12Z

e2e/container/create_test.go

+
+	// FIXME(vdemeester) doesn't work
+	/*
+		// Now, try running with the original client from this new trust server. This should fail because the new root is invalid.


@endophage @n4ss what's the expected behavior here ? (the commented code comes from initial moby test suite, but fails on current docker/cli)

vdemeester · 2018-03-12T16:54:17Z

e2e/image/push_test.go

+		Err:      "error contacting notary server",
+	})
+
+	// FIXME(vdemeester) doesn't work


@endophage @n4ss what's the expected behavior here ? pretty sure it works as expected and the commented code should be gone, but somehow, in 17.06 it did act that way (i.e. even with --disable-content-trust it would fail)

I don't know.. If you disable content trust, it shouldn't try to reach the notary server, right?

@endophage @cyli if you have more guesses?

Apologies for not getting to this sooner - if it was this test: https://github.com/moby/moby/pull/36515/files#diff-cf31c649a9e77c869ff8c6cdcec05f60L328, it looks like the intention was to test the --disable-content-trust flag in combination with the content trust environment variable to say that even with the environment variable set, if --disable-content-trust is passed, it doesn't use content trust. So the push should work, because it doesn't contact the notary server, hence it seem to be checking that there is no comment about the notary server being invalid (e.g. if it were going the path of content trust, the push should have failed with that error message).

vdemeester · 2018-03-12T16:55:00Z

e2e/image/push_test.go

+	// FIXME(vdemeester) doesn't work
+	/*
+		// With a wrong password
+		result = icmd.RunCmd(icmd.Command("docker", "push", image),


@endophage @n4ss what's the expected behavior here ? (the commented code comes from initial moby test suite, but fails on current docker/cli)

Is that https://github.com/moby/moby/pull/36515/files#diff-cf31c649a9e77c869ff8c6cdcec05f60L368? If so, it's possible that the error message has changed - not sure what the failure on docker/cli was.

dnephin

LGTM when the commented code is removed (if it's not necessary anymore)

vdemeester · 2018-03-14T16:52:59Z

Updated with removing commented code (and cleaning some stuff not required for now)

vdemeester · 2018-03-14T17:25:23Z

The failure is weird 😱

17:18:22 --- FAIL: TestPushWithContentTrust (0.87s)
17:18:22 	push_test.go:43: assertion failed: 
17:18:22 		--- expected
17:18:22 		+++ actual
17:18:22 		@@ -1,6 +1,6 @@
17:18:22 		 The push refers to repository [registry:5000/trust-push]
17:18:22 		 5bef08742407: Preparing
17:18:22 		-5bef08742407: Mounted from trust-create
17:18:22 		+5bef08742407: Mounted from trust-pull-unreachable
17:18:22 		 latest: digest: sha256:641b95ddb2ea9dc2af1a0113b6b348ebc20872ba615204fbe12148e98fd6f23d size: 528
17:18:22 		 Signing and pushing trust metadata
17:18:22 		 Finished initializing "registry:5000/trust-push"

PS: I have another Mounted from on my laptop..

vdemeester · 2018-03-14T17:26:54Z

We either need to clean the registry between each test or.. something else 😓

dnephin · 2018-03-14T18:02:23Z

We could use the line matching system from the image build test (https://github.com/docker/cli/blob/master/e2e/image/build_test.go#L31). Just needs to be renamed to assertLines or something like that.

cyli · 2018-03-14T18:25:49Z

Thank you so much for porting these @vdemeester! Apologies I didn't respond earlier. This all LGTM - I can attempt to add the TestTrustedPushWithoutServerAndUntrusted test back in another PR - that seems useful to test that the flag and the environment variable interact in the correct way, or perhaps that should just be a unit test.

TestTrustedPushWithIncorrectPassphraseForNonRoot possibly does not need to exist in docker/cli unless we translate the error messaging. I feel like this should be part of the notary tests instead. We have a test in notary for not being able to add data when the root passphrase is invalid (https://github.com/theupdateframework/notary/blob/master/client/client_test.go#L722) but not the repo passphrase. I've added an issue here: notaryproject/notary#1317

vdemeester · 2018-03-16T13:49:49Z

@dnephin updated, using assertBuildOuptut (move the thing in a separate package)

dnephin

lint failure on unused code.

dnephin · 2018-03-16T15:07:02Z

e2e/service/trust_test.go

+
+func TestServiceCreateWithContentTrust(t *testing.T) {}
+
+func TestServiceUpdateWithContentTrust(t *testing.T) {}


remove for now and open an issue for these?

Importing from moby's DockerTrustSuite tests. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

thaJeztah

LGTM

but left a comment 👍

thaJeztah · 2018-03-19T10:17:34Z

cli/command/container/run_test.go

@@ -23,3 +26,48 @@ func TestRunLabel(t *testing.T) {
 	cmd.SetArgs([]string{"--label", "foo", "busybox"})
 	assert.NilError(t, cmd.Execute())
 }
+
+func TestRunCommandWithContentTrustErrors(t *testing.T) {


This is basically the same test as for "create" (TestNewCreateCommandWithContentTrustErrors); wonder (for a follow-up) if we can share some of the code, so that we don't have to maintain it twice, but also so that we make sure they're kept in sync

vdemeester requested review from dnephin, endophage, thaJeztah and n4ss March 7, 2018 17:06

GordonTheTurtle added the status/0-triage label Mar 7, 2018

vdemeester mentioned this pull request Mar 7, 2018

Remove DockerTrustSuite to docker/cli e2e tests moby/moby#36515

Merged

vdemeester added status/2-code-review and removed status/0-triage labels Mar 7, 2018

dnephin reviewed Mar 7, 2018

View reviewed changes

vdemeester force-pushed the trust-suite-tests branch from ada7260 to 67572da Compare March 7, 2018 17:11

vdemeester force-pushed the trust-suite-tests branch from 67572da to 6542b98 Compare March 7, 2018 18:28

dnephin reviewed Mar 7, 2018

View reviewed changes

vdemeester force-pushed the trust-suite-tests branch from 6542b98 to d3ac738 Compare March 8, 2018 10:02

vdemeester mentioned this pull request Mar 8, 2018

Refactor content_trust cli/flags handling #929

Merged

vdemeester force-pushed the trust-suite-tests branch 5 times, most recently from e0e8479 to c786830 Compare March 12, 2018 16:28

vdemeester commented Mar 12, 2018

View reviewed changes

vdemeester force-pushed the trust-suite-tests branch 6 times, most recently from 931123c to 922cb69 Compare March 13, 2018 15:53

vdemeester changed the title ~~[wip] Add some content trust tests~~ Add some content trust tests Mar 13, 2018

dnephin reviewed Mar 14, 2018

View reviewed changes

vdemeester force-pushed the trust-suite-tests branch from 07722ba to 70bfe54 Compare March 14, 2018 16:52

dnephin approved these changes Mar 14, 2018

View reviewed changes

vdemeester force-pushed the trust-suite-tests branch from 70bfe54 to 492b480 Compare March 14, 2018 17:15

vdemeester force-pushed the trust-suite-tests branch from 492b480 to 54669f5 Compare March 14, 2018 17:26

vdemeester force-pushed the trust-suite-tests branch from 54669f5 to 84bdbfe Compare March 16, 2018 13:49

dnephin reviewed Mar 16, 2018

View reviewed changes

vdemeester force-pushed the trust-suite-tests branch 3 times, most recently from bd1b2a3 to ef5db75 Compare March 16, 2018 15:32

Add more content trust tests

8b00c5c

Importing from moby's DockerTrustSuite tests. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

vdemeester force-pushed the trust-suite-tests branch from ef5db75 to 8b00c5c Compare March 19, 2018 09:02

vdemeester mentioned this pull request Mar 19, 2018

[quality] content trust tests coverage #951

Closed

5 tasks

thaJeztah approved these changes Mar 19, 2018

View reviewed changes

thaJeztah added area/trust area/testing labels Mar 19, 2018

thaJeztah merged commit c0ffb94 into docker:master Mar 19, 2018

GordonTheTurtle added this to the 18.04.0 milestone Mar 19, 2018

vdemeester deleted the trust-suite-tests branch March 19, 2018 10:25

This was referenced Apr 6, 2018

[18.03] backport Fix error with merge composefile with networks… docker-archive/docker-ce#495

Merged

[18.03] backport content trust tests, windows, test-refactoring docker-archive/docker-ce#504

Merged

thaJeztah added the process/cherry-picked label Apr 30, 2018

thaJeztah mentioned this pull request Apr 9, 2020

CI: test-certificates in e2e tests have expired #2412

Closed

vdemeester mentioned this pull request Apr 9, 2020

Clean some integration-cli/fixtures package/files moby/moby#36838

Merged


		func TestPushWithContentTrustSignsForRolesWithKeysAndValidPaths(t *testing.T) {}

		func TestPullWithContentTrustSignsForRolesWithKeysAndValidPaths(t *testing.T) {}


		const registryPrefix = "registry:5000"

		func TestPluginInstallWithContentTrust(t *testing.T) {


		func TestServiceCreateWithContentTrust(t *testing.T) {}

		func TestServiceUpdateWithContentTrust(t *testing.T) {}

Add some content trust tests #924

Add some content trust tests #924

Conversation

vdemeester commented Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 7, 2018 • edited Loading

Codecov Report

dnephin 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

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

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

dnephin left a comment

Choose a reason for hiding this comment

vdemeester commented Mar 14, 2018

vdemeester commented Mar 14, 2018

vdemeester commented Mar 14, 2018

dnephin commented Mar 14, 2018 • edited Loading

cyli commented Mar 14, 2018

vdemeester commented Mar 16, 2018

dnephin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester commented Mar 7, 2018 •

edited

Loading

codecov-io commented Mar 7, 2018 •

edited

Loading

dnephin commented Mar 14, 2018 •

edited

Loading

dnephin left a comment •

edited

Loading