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: strip white-space from manifest before comparing #770

Closed
wants to merge 1 commit into from

Conversation

Freax13
Copy link
Contributor

@Freax13 Freax13 commented Jul 30, 2024

Previously we failed verification when the white-space between the two manifest files didn't match.

Previously we failed verification when the white-space between the two
manifest files didn't match.
@Freax13 Freax13 requested a review from burgerdev July 30, 2024 10:08
@Freax13 Freax13 requested a review from katexochen as a code owner July 30, 2024 10:08
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.

If we wanted to introduce canonicalization, I would rather use e.g. RFC 8785, but we'd need to use that everywhere the manifest is interpreted.

However, I don't understand the use case:

  1. The workload owner has the manifest sitting in their workspace.
  2. The data owner should have received the expected manifest from the workload owner.

In both cases, I don't see why the manifest would have changed in whitespace.

We had a discussion about a similar proposal a few weeks back: #615 (comment).

@Freax13 Freax13 added the no changelog PRs not listed in the release notes label Jul 30, 2024
@Freax13 Freax13 requested a review from msanft July 30, 2024 10:55
@msanft
Copy link
Contributor

msanft commented Jul 30, 2024

In both cases, I don't see why the manifest would have changed in whitespace.

Without knowing any other context, I can think of a scenario where one editor appends a newline at the end, or uses spaces for tabs, and the other doesn't.

I generally think comparisons of structured data should happen in a canonical format, but wold also favor RFC-canonicalization over stripping.

@Freax13 Freax13 closed this Jul 30, 2024
@Freax13
Copy link
Contributor Author

Freax13 commented Jul 30, 2024

Closed for the following reasons:

  • There's no demand for this/there's no real use case.
  • Comparing the manifests byte-for-byte is very simple and therefor secure. Canonicalizations can potentially be a source for bugs, so we should avoid them unless there's a good reason.

@Freax13 Freax13 deleted the tom/verify-canonical-json branch August 22, 2024 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog PRs not listed in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants