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 digest of artifact in storage #1088

Merged
merged 3 commits into from
May 10, 2023
Merged

Verify digest of artifact in storage #1088

merged 3 commits into from
May 10, 2023

Conversation

hiddeco
Copy link
Member

@hiddeco hiddeco commented May 10, 2023

This adds verification of the digest of the artifact in storage to all
reconcilers which manage artifacts.

When the artifact does not have a digest or if it mismatches with the
file in storage, the file is removed from the storage and status of the
object.

This hardens the storage against potential tampering, in addition to
resolving an issue where users upgrading from a (much) older version of
the controller would run into an error after the checksum field was
removed from the API.

This would cause the controller to not advertise any checksum at all,
while not producing a new one until a new revision was detected.
Resulting in fetch failures for consumers while they would try to
verify the digest of the advertised artifact.

Fixes: fluxcd/flux2#3861

@hiddeco hiddeco added enhancement New feature or request area/storage Storage related issues and pull requests labels May 10, 2023
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @hiddeco 🏅

Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

Manually tested and verified the behavior. LGTM!

Signed-off-by: Hidde Beydals <hidde@hhh.computer>
This commits adds verification of the digest of the artifact in storage
to all reconcilers which manage artifacts.

When the artifact does not have a digest or if it mismatches with the
file in storage, the file is removed from the storage and status of the
object.

This hardens the storage against potential tampering, in addition to
resolving an issue where users upgrading from a (much) older version of
the controller would run into an error after the checksum field was
removed from the API.

This would cause the controller to not advertise any checksum at all,
while not producing a new one until a new revision was detected.
Resulting in fetch failures for consumers while they would try to
verify the digest of the advertised artifact.

While not strictly part of this exercise, some of the tests were
altered to prepare the storage used in test cases to become isolated
by strictly using the `storage` provided via the callback. Actually
isolating this has however been left as a task at a later moment.

Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Given:

- None of the methods of the `Storage` are mutating the storage
  itself.
- It must be instantiated to be usable, as there is a strict
  reliance on values.
- The struct itself is light.

This seems to be more fitting.

Signed-off-by: Hidde Beydals <hidde@hhh.computer>
@hiddeco hiddeco merged commit 5c5b822 into main May 10, 2023
10 checks passed
@hiddeco hiddeco deleted the verify-storage-digest branch May 10, 2023 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/storage Storage related issues and pull requests enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kustomization status "failed to parse digest: invalid checksum digest format"
3 participants