Skip to content

Conversation

@ok-nick
Copy link
Contributor

@ok-nick ok-nick commented Nov 20, 2025

This PR changes our logic to apply the same conditions we do on the active manifest to the ingredients as well. If you have an untrusted ingredient, should you have a trusted active manifest? If the ingredient has an untrusted claim signature does it already affect the active manifests success codes?

This change breaks many of our test cases that do not report a trusted success code (and others) for ingredients. I'm opening this PR for further discussion on this topic.

@codspeed-hq
Copy link

codspeed-hq bot commented Nov 20, 2025

CodSpeed Performance Report

Merging #1624 will not alter performance

Comparing ok-nick/ingredient-validation-state (9fa5071) with main (629ab6e)

Summary

✅ 16 untouched
⏩ 2 skipped1

Footnotes

  1. 2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Copy link
Collaborator

@gpeacock gpeacock left a comment

Choose a reason for hiding this comment

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

Our logic has always been that if an ingredient was added as part of a signed manifest, the signer is accepting the ingredient as it was at the time it was signed. So an untrusted ingredient can exist inside a trusted manifest, just as an unsigned ingredient can be added or, an ingredient that has some other set of errors.

But: if the status of that ingredient has changed since it was signed, either through tampering or the signature no longer being valid due to OSCP or other issues, that should be caught as delta here and reported as such.

@ok-nick
Copy link
Contributor Author

ok-nick commented Nov 24, 2025

@gpeacock So instead of checking if the ingredient delta has, for example, signingCredential.trusted we would want to check that it does not contain signingCredential.untrusted and also apply the same rules for inside/outside validity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants