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

verify: verify active manifest at Coordinator #615

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

davidweisse
Copy link
Contributor

The verify command already takes the manifest file as an input. On verify, the CLI will now check if the local manifest matches the active manifest on the Coordinator.

@davidweisse davidweisse requested a review from burgerdev June 21, 2024 14:02
@davidweisse davidweisse requested a review from katexochen as a code owner June 21, 2024 14:02
@davidweisse davidweisse added the changelog PRs that should be part of the release notes label Jun 21, 2024
@katexochen katexochen requested a review from msanft June 24, 2024 06:50
cli/cmd/verify.go Outdated Show resolved Hide resolved
@davidweisse davidweisse force-pushed the dav/verify-current-manifest branch from 366d61a to bdd53fb Compare June 24, 2024 09:40
cli/cmd/verify.go Outdated Show resolved Hide resolved
@davidweisse davidweisse force-pushed the dav/verify-current-manifest branch from bdd53fb to 3c57cd6 Compare June 26, 2024 08:06
@katexochen
Copy link
Member

I'm still confused by this change. If we have a manifest we trust, then we also already audited the policies referenced in this manifest. Why should we still write all the files on disk then and ask the user to audit it again?

@burgerdev
Copy link
Contributor

The set command does not unpack the policies, so it is somewhat convenient to get them through verify. But I agree that we should not ask the user to audit them.

@katexochen
Copy link
Member

The set command does not unpack the policies, so it is somewhat convenient to get them through verify. But I agree that we should not ask the user to audit them.

Remember those two calls are (potentially) called from different entities! The data owner still has to review both the manifest and the policies. If the result of the verify should be "you definitely can trust this Coordinator, and no further steps are required" then we must assume the policies were also communicated out of band and the data owner already verified them (so there is no reason to output them). I think we should keep the output of files and state even more clear to the data owner what to do.

cli/cmd/verify.go Show resolved Hide resolved
cli/cmd/verify.go Show resolved Hide resolved
cli/cmd/verify.go Outdated Show resolved Hide resolved
cli/cmd/verify.go Outdated Show resolved Hide resolved
cli/cmd/verify.go Outdated Show resolved Hide resolved
@davidweisse davidweisse force-pushed the dav/verify-current-manifest branch from 3c57cd6 to 1f0217e Compare June 26, 2024 12:47
cli/cmd/verify.go Outdated Show resolved Hide resolved
Copy link
Member

@katexochen katexochen left a comment

Choose a reason for hiding this comment

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

otherwise lgtm

@davidweisse davidweisse force-pushed the dav/verify-current-manifest branch 2 times, most recently from b92368d to 178daf4 Compare June 26, 2024 14:02
@davidweisse davidweisse force-pushed the dav/verify-current-manifest branch from 178daf4 to 8a028da Compare June 26, 2024 14:20
@davidweisse davidweisse merged commit 3f5601c into main Jun 26, 2024
8 checks passed
@davidweisse davidweisse deleted the dav/verify-current-manifest branch June 26, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog PRs that should be part of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants