Skip to content
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.

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

treewide: allow multiple validators #783

Merged
merged 1 commit into from
Aug 14, 2024
Merged

treewide: allow multiple validators #783

merged 1 commit into from
Aug 14, 2024

Conversation

msanft
Copy link
Contributor

@msanft msanft commented Aug 2, 2024

This changes the attestation (as of now, only SEV-SNP) to be passed
multiple validators. The aTLS code already handles multiple validators,
but the code previously passed only one. This way, attestation will now
work by being handed a list of validators, and returning success as soon
as one can successfully validate a report. Furthermore, the
atls.NoValidator is now obsolete, and semantically represented by
passing an empty list of validators.

@msanft msanft changed the title wip treewide: allow multiple validators Aug 2, 2024
@msanft msanft force-pushed the msanft/attestation-list branch from 3010fed to a353ac8 Compare August 2, 2024 13:55
@msanft msanft added the no changelog PRs not listed in the release notes label Aug 2, 2024
coordinator/internal/authority/userapi.go Outdated Show resolved Hide resolved
coordinator/internal/authority/authority.go Outdated Show resolved Hide resolved
coordinator/internal/authority/authority.go Outdated Show resolved Hide resolved
coordinator/internal/authority/authority.go Outdated Show resolved Hide resolved
coordinator/internal/authority/authority.go Outdated Show resolved Hide resolved
coordinator/internal/authority/userapi.go Outdated Show resolved Hide resolved
internal/attestation/snp/validator.go Outdated Show resolved Hide resolved
@msanft msanft force-pushed the msanft/attestation-list branch 4 times, most recently from 8520ec6 to 590bf28 Compare August 9, 2024 14:01
@msanft msanft requested a review from burgerdev August 9, 2024 14:04
@msanft msanft force-pushed the msanft/attestation-list branch from 590bf28 to 9cab51c Compare August 9, 2024 14:15
Copy link
Contributor

@burgerdev burgerdev left a comment

Choose a reason for hiding this comment

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

lgtm

initializer/main.go Outdated Show resolved Hide resolved
internal/manifest/manifest.go Outdated Show resolved Hide resolved
@msanft msanft force-pushed the msanft/attestation-list branch from 9cab51c to 24e862b Compare August 12, 2024 11:31
@msanft msanft marked this pull request as ready for review August 12, 2024 11:31
@msanft msanft requested a review from katexochen as a code owner August 12, 2024 11:31
@msanft msanft requested a review from Freax13 August 12, 2024 11:31
Copy link
Contributor

@Freax13 Freax13 left a comment

Choose a reason for hiding this comment

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

I'm not quite done with the review yet, but here are some preliminary comments.

internal/manifest/manifest.go Outdated Show resolved Hide resolved
internal/manifest/manifest.go Outdated Show resolved Hide resolved
internal/manifest/manifest.go Outdated Show resolved Hide resolved
internal/manifest/manifest.go Outdated Show resolved Hide resolved
cli/cmd/recover.go Outdated Show resolved Hide resolved
cli/cmd/set.go Outdated Show resolved Hide resolved
internal/manifest/manifest.go Outdated Show resolved Hide resolved
internal/manifest/manifest.go Outdated Show resolved Hide resolved
@msanft msanft force-pushed the msanft/attestation-list branch 3 times, most recently from ff9bf81 to 95fc2fb Compare August 13, 2024 09:33
internal/manifest/manifest.go Outdated Show resolved Hide resolved
internal/manifest/referencevalues.go Outdated Show resolved Hide resolved
@msanft msanft force-pushed the msanft/attestation-list branch from 95fc2fb to e72e418 Compare August 13, 2024 09:38
@msanft msanft force-pushed the msanft/attestation-list branch 2 times, most recently from d5ca88e to 8537531 Compare August 13, 2024 11:37
@msanft msanft requested a review from Freax13 August 13, 2024 11:55
internal/attestation/snp/validator.go Outdated Show resolved Hide resolved
@Freax13
Copy link
Contributor

Freax13 commented Aug 13, 2024

Could you add some tests please?

@msanft
Copy link
Contributor Author

msanft commented Aug 13, 2024

Could you add some tests please?

I'm not exactly sure how we would go about testing the validators. There were no unit tests before, and implementing some would definitely require some considerations. I don't think this is something that should be done in this PR.

For end-to-end testing, we'd probably also want a scenario where we'd have multiple reference values. But with the current Azure offerings, I don't think it's possible to request such a platform.

What did you have in mind exactly @Freax13 ?

@Freax13
Copy link
Contributor

Freax13 commented Aug 13, 2024

For end-to-end testing, we'd probably also want a scenario where we'd have multiple reference values. But with the current Azure offerings, I don't think it's possible to request such a platform.

I was thinking that we could have a test where we have multiple reference values with only one of them being the correct one and then make sure that attestation still works. If we really wanted multiple different platforms, we could also generate such reports once and hard-code them for the tests.

@msanft
Copy link
Contributor Author

msanft commented Aug 13, 2024

For end-to-end testing, we'd probably also want a scenario where we'd have multiple reference values. But with the current Azure offerings, I don't think it's possible to request such a platform.

I was thinking that we could have a test where we have multiple reference values with only one of them being the correct one and then make sure that attestation still works. If we really wanted multiple different platforms, we could also generate such reports once and hard-code them for the tests.

Okay, I guess we could have some kind of testdata and put this into a unit test, or have an e2e test

@msanft msanft force-pushed the msanft/attestation-list branch from a5cf563 to 1479e93 Compare August 13, 2024 14:57
This changes the attestation (as of now, only SEV-SNP) to be passed
multiple validators. The aTLS code already handles multiple validators,
but the code previously passed only one. This way, attestation will now
work by being handed a list of validators, and returning success as soon
as one can successfully validate a report. Furthermore, the
`atls.NoValidator` is now obsolete, and semantically represented by
passing an empty list of validators.
@msanft msanft force-pushed the msanft/attestation-list branch from 1479e93 to 0700ff2 Compare August 13, 2024 15:11
@msanft msanft requested a review from Freax13 August 14, 2024 07:19
@msanft msanft merged commit be979c2 into main Aug 14, 2024
9 checks passed
@msanft msanft deleted the msanft/attestation-list branch August 14, 2024 07:20
@katexochen katexochen added feature Shiny new feature for our users and removed no changelog PRs not listed in the release notes labels Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Shiny new feature for our users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants