[Signature discovery] Integrate signature discovery client into attestation agent. #343

yawangwang · 2023-08-02T16:57:06Z

This PR includes the following changes:

Added containerImageSignaturesFetcher to the attestation agent such that the agent can fetch any new container image signatures on refreshToken.
Integrated the signature discovery client into signature fetcher and made corresponding changes to container_runner.
Add specific tests that need root permission to run to ci.yaml.
Integrate client experimental flag to gate the signed container feature.

alexmwu

IMO, this is the crucial bit of this feature and should go behind an experiment. Please do not submit until we have that functionality.

alexmwu · 2023-08-17T00:00:48Z

launcher/container_runner.go

+		if err != nil {
+			logger.Printf("failed to retrieve auth token: %v, using empty auth for image pulling\n", err)
+		}
+		updatedSpec, err := spec.GetLaunchSpec(mdsClient)


Why do you need to update the spec? The launcher should only ever get the spec at startup. Refetching it can lead to weird states and potential security issues.

The rational behind updating the spec is to enable operators don't have to restart VM when new docker repos added. Even though ADMC team is ok with restarting VM when new repos added, I feel it might be nice to have this feature. But if refetching it will lead to any potential security issues, we should definitely avoid that.

alexmwu · 2023-08-17T00:01:46Z

launcher/container_runner.go

@@ -236,14 +266,38 @@ func NewRunner(ctx context.Context, cdClient *containerd.Client, token oauth2.To
 		return nil, fmt.Errorf("failed to create REST verifier client: %v", err)
 	}

+	// Fetch container image signatures by using signaturediscovery client.


If you don't update the spec, this func should be unnecessary. Which is good because this is tough to follow.

alexmwu · 2023-08-17T00:02:56Z

launcher/container_runner.go

@@ -79,6 +82,33 @@ const (
 	defaultRefreshJitter = 0.1
 )

+func fetchContainerImageSignatures(ctx context.Context, sdClient *signaturediscovery.Client, targetRepos []string, logger *log.Logger) []internal.Signature {


not required, but it would be nice to be able to cache signatures so we don't have to fetch every time. This complicates things, so maybe add a TODO with an issue.

yawangwang · 2023-08-17T01:24:27Z

IMO, this is the crucial bit of this feature and should go behind an experiment. Please do not submit until we have that functionality.

Agree, this PR should also go behind this PR for updating v1 SDK #305

yawangwang · 2023-09-19T19:39:03Z

IMO, this is the crucial bit of this feature and should go behind an experiment. Please do not submit until we have that functionality.

Agree, this PR should also go behind this PR for updating v1 SDK #305

Now since V1 SDK has been merged into master, this PR is now ready for review again with client experimental flag integration.

alexmwu · 2023-09-27T00:31:12Z

launcher/container_runner_test.go

@@ -414,6 +417,71 @@ type idTokenResp struct {
 	Token string `json:"token"`
 }

+func TestFetchContainerImageSignatures(t *testing.T) {
+	ctx := namespaces.WithNamespace(context.Background(), "test")


We should consider using something like https://pkg.go.dev/github.com/docker/docker/testutil/registry#V2 or the mock https://pkg.go.dev/github.com/docker/docker/testutil/registry#Mock instead of actually making a network call in our unit tests

alexmwu · 2023-09-27T00:32:05Z

launcher/container_runner_test.go

+			},
+		},
+		{
+			name:        "fetchContainerImageSignatures success with nil target repos",


Can we add a case for a nonexistent repo and one that exists but doesn't have signatures?

alexmwu · 2023-09-27T00:48:46Z

launcher/container_runner.go

@@ -230,15 +259,29 @@ func NewRunner(ctx context.Context, cdClient *containerd.Client, token oauth2.To
 		return nil, fmt.Errorf("failed to create REST verifier client: %v", err)
 	}

+	// Fetch container image signatures with specific repositories.
+	containerSignaturesFetcher := func(targetRepos []string) []internal.Signature {
+		sdClient := getSignatureDiscoveryClient(cdClient, token, image.Target())


are we getting a new signaturediscovery Client every time we call agent.Attest? Can we make this a closure to avoid this? Alternatively, we can pass the signature discovery client in the agent and use a fake for testing.

alexmwu · 2023-09-27T00:51:24Z

launcher/agent/agent.go

+	if a.launchSpec.Experiments.EnableSignedContainerImage {
+		signatures := a.sigsFetcher(a.launchSpec.SignedImageRepos)
+		if len(signatures) > 0 {
+			req.ContainerImageSignatures = signatures
+			a.logger.Printf("Found container image signatures: %v\n", signatures)
+		}
+	}
+
+	resp, err := a.client.VerifyAttestation(ctx, req)


Can you add unit test coverage here? The fake verifier client should expect to see signatures.

alexmwu · 2023-10-03T17:59:01Z

launcher/agent/agent_test.go

+		t.Run(tc.name, func(t *testing.T) {
+			sdClient := signaturediscovery.NewFakeClient()
+			gotSigs := fetchContainerImageSignatures(ctx, sdClient, tc.targetRepos, log.Default())
+			if len(gotSigs) != tc.wantLen {


nit: For these test cases: why not len(gotSigs) != len(tc.WantBase64Sigs)? The only reason I can think of to have wantLen is in case we have a partial error. Which, if you can add a test case for, would be great to have! (e.g., add a repo with all invalid signatures and one with partial invalid signatures and have the fake verifier handle that).

New Features: [launcher] Add experiment support google#352 [launcher] Integrate signature discovery client into attestation agent google#343 Bug Fixes: Make launcher host tmp directory before experiment fetch google#363 Other Changes: [launcher] Print kernel cmdline on builds google#268 Import latest version of go-tdx-guest google#373 [launcher] Print signature details instead of signature object google#374 [launcher] Add image tests for the experiments binary google#378 Update go-sev-guest to v0.9.3 google#381

New Features: [launcher] Add experiment support #352 [launcher] Integrate signature discovery client into attestation agent #343 Bug Fixes: Make launcher host tmp directory before experiment fetch #363 Other Changes: [launcher] Print kernel cmdline on builds #268 Import latest version of go-tdx-guest #373 [launcher] Print signature details instead of signature object #374 [launcher] Add image tests for the experiments binary #378 Update go-sev-guest to v0.9.3 #381

yawangwang force-pushed the attestation-agent branch 3 times, most recently from 758515c to c78c5b1 Compare August 2, 2023 18:00

yawangwang force-pushed the attestation-agent branch from c78c5b1 to 777c2c2 Compare August 16, 2023 21:02

yawangwang marked this pull request as ready for review August 16, 2023 21:26

yawangwang requested review from alexmwu and jkl73 August 16, 2023 21:26

alexmwu reviewed Aug 17, 2023

View reviewed changes

yawangwang force-pushed the attestation-agent branch from e3d6b05 to d806aa7 Compare August 17, 2023 01:11

yawangwang requested a review from alexmwu August 17, 2023 01:24

yawangwang force-pushed the attestation-agent branch 3 times, most recently from cc98bcd to 62d378c Compare August 23, 2023 19:13

yawangwang force-pushed the attestation-agent branch 5 times, most recently from 1652b48 to 042dc40 Compare September 19, 2023 19:32

alexmwu reviewed Sep 27, 2023

View reviewed changes

yawangwang force-pushed the attestation-agent branch 3 times, most recently from 2fcfaef to d6519ea Compare September 28, 2023 01:23

yawangwang requested a review from alexmwu September 28, 2023 01:32

alexmwu approved these changes Oct 3, 2023

View reviewed changes

yawangwang force-pushed the attestation-agent branch from d6519ea to 8c1388c Compare October 3, 2023 21:34

integrate signature discovery client into attestation agent

634e79c

yawangwang force-pushed the attestation-agent branch from 8c1388c to 634e79c Compare October 3, 2023 21:47

yawangwang merged commit 7f60076 into google:master Oct 3, 2023
11 checks passed

alexmwu mentioned this pull request Oct 27, 2023

Release v0.4.2 #385

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Signature discovery] Integrate signature discovery client into attestation agent. #343

[Signature discovery] Integrate signature discovery client into attestation agent. #343

yawangwang commented Aug 2, 2023 •

edited

Loading

alexmwu left a comment

alexmwu Aug 17, 2023

yawangwang Aug 17, 2023

alexmwu Aug 17, 2023

alexmwu Aug 17, 2023

yawangwang commented Aug 17, 2023

yawangwang commented Sep 19, 2023

alexmwu Sep 27, 2023

alexmwu Sep 27, 2023

alexmwu Sep 27, 2023

alexmwu Sep 27, 2023

alexmwu Oct 3, 2023

[Signature discovery] Integrate signature discovery client into attestation agent. #343

[Signature discovery] Integrate signature discovery client into attestation agent. #343

Conversation

yawangwang commented Aug 2, 2023 • edited Loading

alexmwu 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

yawangwang commented Aug 17, 2023

yawangwang commented Sep 19, 2023

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

yawangwang commented Aug 2, 2023 •

edited

Loading