cli: embed multiple reference values #752

msanft · 2024-07-19T15:31:24Z

This adds support for embedding a more versatile format of reference values (i.e. a structured type) into the Contrast binaries. This will allow us to embed all reference values at build-time from a single source (the Nix build file) rather than having SVNs in Go code and inserting trusted measurements via the go build commandline. It will now embed a JSON file containing the reference values, which is unmarshaled at first default manifest generation.

cli/main.go

internal/manifest/manifest.go

Freax13 · 2024-07-29T09:08:28Z

CI's failing.

katexochen

Ticket said

ReferenceValues section should be a list of the same type

Which isn't reflected in this PR. Will that be done in a follow-up?

cli/cmd/generate.go

katexochen · 2024-07-29T09:05:23Z

cli/main.go

@@ -43,11 +43,10 @@ func buildVersionString() string {
 			fmt.Fprintf(versionsWriter, "\t%s\n", image)
 		}
 	}
-	fmt.Fprint(versionsWriter, "\n")
-	fmt.Fprintf(versionsWriter, "reference values for %s platform:\n", platforms.AKSCloudHypervisorSNP.String())
-	fmt.Fprintf(versionsWriter, "\truntime handler:\tcontrast-cc-%s\n", manifest.TrustedMeasurement[:32])


Runtime handler isn't printed anymore.

Because there's no singular valid runtime handler anymore

That was already anticipated, that's why there was a platform-specific section in the output.

So do you want to roll back to how it was before, but showing everything for SNP and TDX?

I'm just saying you shouldn't throw away the work other developers put in there without a thought. Should users construct the runtime name themselves in the future and count the offset of the prefix we are using? Is the length of the offset documented anywhere?

I'm fine with changing it back, but then I'd also suggest we don't print the JSON at all. Are you fine with "reverting" that @burgerdev?

Sounds like we need to discuss the question of runtime handlers vs. reference values first - we want this to be a 1:1 mapping, right? So, should it go into ReferenceValues fields?

Implicitly, I'd say it's already there due to the presence of TrustedMeasurement in each of the ReferenceValues. But I think this is something so adjacent to the general discussion of how we want to handle these that we should discuss it in the sync today.

Let's fix the output in a follow up.

Created a ticket for this

katexochen · 2024-07-29T09:06:50Z

cli/main.go

-	fmt.Fprintf(versionsWriter, "\tlaunch digest:\t%s\n", manifest.TrustedMeasurement)
-	fmt.Fprintf(versionsWriter, "\tgenpolicy version:\t%s\n", constants.GenpolicyVersion)
+	if refValues, err := json.MarshalIndent(manifest.EmbeddedReferenceValues(), "\t", "  "); err == nil {
+		fmt.Fprintf(versionsWriter, "embedded reference values:\t%s\n", refValues)


This reduced readability of the output a lot. If we want to keep this as json output, we should move it to the end and start the json doc in a separate line so you can cut it and process it with jq if needed.

I'd rather have a --json for this use case.

That would mean a refactoring of the command structure, as we are currently using the builtin --version flag, not a command, right?

internal/kuberesource/resourcegen/main.go

internal/kuberesource/sets.go

internal/manifest/referencevalues.go

internal/manifest/constants.go

internal/manifest/manifest.go

internal/manifest/referencevalues.go

msanft · 2024-07-29T09:45:33Z

Which isn't reflected in this PR. Will that be done in a follow-up?

@katexochen With the current design, I don't think this is necessary anymore? Also, we discussed in-person that a structured format makes more sense, and you agreed

katexochen · 2024-07-29T09:54:36Z

Which isn't reflected in this PR. Will that be done in a follow-up?

With the current design, I don't think this is necessary anymore? Also, we discussed in-person that a structured format makes more sense, and you agreed

We discussed the embedding of the reference values in a structured format (json).

In the manifest, we still target a list of reference value configurations, as discussed in previous meetings and documented in the ticket. This will allow us to add multiple platforms of different configuration (heterogeneous clusters with different types of SNP CPUs, for example), which is something we must support but cannot be enabled by the manifest structure given in this PR.

msanft · 2024-07-29T09:58:46Z

@katexochen I think there was a misunderstanding of the tickets on my side. Consider this PR to handle 4459, while a follow-up PR should handle 4299

e2e/openssl/openssl_test.go

packages/by-name/contrast/package.nix

Freax13 · 2024-07-30T08:24:27Z

What's the status on this PR?

msanft · 2024-07-30T08:27:30Z

What's the status on this PR?

I'd like to merge it, but require a review from @katexochen

This adds support for embedding a more versatile format of reference values (i.e. a structured type) into the Contrast binaries. This will allow us to embed all reference values at build-time from a single source (the Nix build file) rather than having SVNs in Go code and inserting trusted measurements via the go build commandline. It will now embed a JSON file containing the reference values, which is unmarshaled at first default manifest generation.

This puts the Nix builds requiring a non-ext filesystem into a tmpfs. This is a workaround until the upstream bug is reported and resolved to / in fakeroot.

msanft added the no changelog PRs not listed in the release notes label Jul 19, 2024

Freax13 mentioned this pull request Jul 23, 2024

preparations for e2e tests on baremetal SNP #730

Merged

msanft force-pushed the msanft/tdx-trusted-measurement-embedding branch 28 times, most recently from dd5740c to 2fdb880 Compare July 24, 2024 13:45

msanft requested a review from burgerdev July 26, 2024 11:06

burgerdev reviewed Jul 26, 2024

View reviewed changes

cli/main.go Outdated Show resolved Hide resolved

msanft force-pushed the msanft/tdx-trusted-measurement-embedding branch from 77462e7 to 3948272 Compare July 26, 2024 12:45

burgerdev reviewed Jul 26, 2024

View reviewed changes

cli/main.go Show resolved Hide resolved

burgerdev approved these changes Jul 26, 2024

View reviewed changes

Freax13 reviewed Jul 26, 2024

View reviewed changes

internal/manifest/manifest.go Show resolved Hide resolved

Freax13 reviewed Jul 26, 2024

View reviewed changes

internal/manifest/manifest.go Show resolved Hide resolved

msanft force-pushed the msanft/tdx-trusted-measurement-embedding branch from 3948272 to f65e7e1 Compare July 29, 2024 06:59

katexochen requested changes Jul 29, 2024

View reviewed changes

msanft requested a review from katexochen July 29, 2024 12:20

msanft force-pushed the msanft/tdx-trusted-measurement-embedding branch from f65e7e1 to 9c0e2e2 Compare July 29, 2024 12:21

Freax13 reviewed Jul 29, 2024

View reviewed changes

e2e/openssl/openssl_test.go Outdated Show resolved Hide resolved

msanft force-pushed the msanft/tdx-trusted-measurement-embedding branch from 9c0e2e2 to 842d44d Compare July 29, 2024 12:52

Freax13 reviewed Jul 29, 2024

View reviewed changes

packages/by-name/contrast/package.nix Show resolved Hide resolved

msanft force-pushed the msanft/tdx-trusted-measurement-embedding branch 3 times, most recently from 7a0881b to 1b3d3c0 Compare July 30, 2024 13:18

msanft added 3 commits July 30, 2024 15:19

justfile: supply platform when generating deployment

de7d8fa

ci: run ext*-depending builds in tmpfs

fda28a4

This puts the Nix builds requiring a non-ext filesystem into a tmpfs. This is a workaround until the upstream bug is reported and resolved to / in fakeroot.

msanft force-pushed the msanft/tdx-trusted-measurement-embedding branch from 1b3d3c0 to fda28a4 Compare July 30, 2024 13:19

katexochen approved these changes Jul 30, 2024

View reviewed changes

msanft merged commit ac33991 into main Jul 30, 2024
11 checks passed

msanft deleted the msanft/tdx-trusted-measurement-embedding branch July 30, 2024 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: embed multiple reference values #752

cli: embed multiple reference values #752

msanft commented Jul 19, 2024

Freax13 commented Jul 29, 2024

katexochen left a comment

katexochen Jul 29, 2024

msanft Jul 29, 2024

katexochen Jul 29, 2024

msanft Jul 29, 2024

katexochen Jul 29, 2024

msanft Jul 29, 2024

burgerdev Jul 29, 2024

msanft Jul 30, 2024

katexochen Jul 30, 2024

msanft Jul 30, 2024

katexochen Jul 29, 2024

burgerdev Jul 29, 2024

katexochen Jul 29, 2024

msanft commented Jul 29, 2024

katexochen commented Jul 29, 2024 •

edited

Loading

msanft commented Jul 29, 2024

Freax13 commented Jul 30, 2024

msanft commented Jul 30, 2024 •

edited

Loading

cli: embed multiple reference values #752

cli: embed multiple reference values #752

Conversation

msanft commented Jul 19, 2024

Freax13 commented Jul 29, 2024

katexochen 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

msanft commented Jul 29, 2024

katexochen commented Jul 29, 2024 • edited Loading

msanft commented Jul 29, 2024

Freax13 commented Jul 30, 2024

msanft commented Jul 30, 2024 • edited Loading

katexochen commented Jul 29, 2024 •

edited

Loading

msanft commented Jul 30, 2024 •

edited

Loading