8000 cli: read Azure SEV-SNP version values from launch TCB in `constellation verify` output by elchead · Pull Request #2357 · edgelesssys/constellation · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

cli: read Azure SEV-SNP version values from launch TCB in constellation verify output #2357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

cli: read Azure SEV-SNP version values from launch TCB in constellation verify output #2357

wants to merge 1 commit into from

Conversation

elchead
Copy link
Contributor
@elchead elchead commented Sep 25, 2023 •
edited
Loading

UPDATE waiting for #2391

Context

As described in #2343, the current version fetching is not reliable. Since it is not transparent how the Azure MAA values are deduced, we rather refer to the Launch TCB values from constellation verify

Proposed change(s)

  • read the verify output in the E2E verify test to read the version values from the LaunchTCB

Related issue

Checklist

  • Update docs
  • Add labels (e.g., for changelog category)
  • Is PR title adequate for changelog?
  • Link to Milestone

@elchead elchead requested a review from derpsteb as a code owner September 25, 2023 10:22
@netlify
Copy link
netlify bot commented Sep 25, 2023 •
edited
Loading

✅ Deploy Preview for constellation-docs canceled.

Name Link
🔨 Latest commit a2ed05b
🔍 Latest deploy log https://app.netlify.com/sites/constellation-docs/deploys/65116dc90cf65e0008f91ac5

@elchead elchead added the no changelog Change won't be listed in release changelog label Sep 25, 2023
@elchead elchead added this to the v2.12.0 milestone Sep 25, 2023
@github-actions
Copy link
Contributor
github-actions bot commented Sep 25, 2023 •
edited
Loading

Coverage report

Package Old New Trend
internal/api/attestationconfigapi/cli 6.80% 22.20% ↗️

@elchead elchead force-pushed the feat/config/read-from-snp-report branch from fb46fb1 to 7f22076 Compare September 25, 2023 10:32
3u13r
3u13r reviewed Sep 25, 2023
View reviewed changes
internal/api/attestationconfigapi/cli/parse_report.go
)

// ParseSNPReport parses the SNP report and returns the version information.
func ParseSNPReport(reader io.Reader) (attestationconfigapi.AzureSEVSNPVersion, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have no stability guarantees of Constellation's cli output format. Therefore, this way of parsing is very brittle.

My suggestion is that we should write the full json (before parsing) to disk on constellation verify and then use the exact same parsing here.

Copy link
Contributor Author
@elchead elchead Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a sensible default to write it to file at all times? or should we add a new flag to show the output as json? I can do that but was not sure this was worth the implementation effort since we also control the constellation verify output and the CI test for the attestationconfigapi would fail

Copy link
Contributor 8000 Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we use --raw to parse the content the instance info containing the report is base64 encoded this makes the file parsing yet more obscure in the e2e test setup. Not sure we should go this route. @derpsteb what do you think?
I don't see the verify output as very unstable.
The current parsing would only fail if we rename the affected fields in the report or move the signature field in the output. But the latter could be fixed too.

@elchead elchead force-pushed the feat/config/read-from-snp-report branch from 7f22076 to a2ed05b Compare September 25, 2023 11:23
@thomasten
Copy link
Member

Note that we changed this feature to also consider the Committed and Reported TCB. Please check the AB ticket again.

@3u13r
Copy link
Contributor
3u13r commented Sep 25, 2023

I discussed with @katexochen and we currently see the following way forward:

  1. Refactor the parsing of cmd/verify.go EITHER refactor out the parsing to a location that is used by the verify command and your tool OR implement a custom mashaller and unmarshaller which recursively parses the attestationDoc to a struct containing the parsed InstanceInfo (not as base64 but as e.g. azureInstanceInfo struct). We prefer the second option.
  2. If the second option was chosen: Use the refactored parsing to enable --json or --output json for the constellation verify command.

Both in one separate PR.

Then we can go ahead and implement this story. For the second option based on the json output which can simply be marshalled and unmarshalled again or fir the first option based on the raw output.

@derpsteb
Copy link
Contributor

I agree that having a --json/--output json flag on constellation verify would be ideal. What I would definitely aim to do is that verify and this cli share the parsing logic. Probably by the way that Leo and Paul suggest.

@elchead
Copy link
Contributor Author
elchead commented Sep 25, 2023

@3u13r @derpsteb so should the azureInstanceInfo be further parsed into a new format which contains version information as presented in the output (see

constellation/cli/internal/cmd/verify.go

Line 410 in 3ed001f

func (f *attestationDocFormatterImpl) parseSNPReport(b *strings.Builder, reportBytes []byte) error {
)?

The azureInstanceInfo still has raw data which is unreadable, but I think that we should have a readable file output.

@3u13r
Copy link
Contributor
3u13r commented Sep 25, 2023

The azureInstanceInfo still has raw data which is unreadable, but I think that we should have a readable file output.

Yes, the idea is to parse this recursively. All the way down to all struct we currently have and so that everything is readable.

8000
@elchead elchead mentioned this pull request Sep 28, 2023
Merged
4 tasks
@malt3 malt3 modified the milestones: v2.12.0, v2.13.0 Oct 9, 2023
@elchead
Copy link
Contributor Author
elchead commented Oct 16, 2023

closing in favor of new implementation in #2429

@elchead elchead closed this Oct 16, 2023
@daniel-weisse daniel-weisse deleted the feat/config/read-from-snp-report branch May 24, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@3u13r 3u13r 3u13r left review comments

@derpsteb derpsteb Awaiting requested review from derpsteb

Assignees
No one assigned
Labels
no changelog Change won't be listed in release changelog
Projects
None yet
Milestone
v2.13.0
Development

Successfully merging this pull request may close these issues.
5 participants
@elchead @thomasten @3u13r @derpsteb @malt3
0