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

Allow trusted builders by repo or tag #2266

Merged

Conversation

modulo11
Copy link
Contributor

Summary

With this change, the tag part of a trusted builder is ignored. Trusting a builder my/registry/builder at the moment does not also trust my/registry/builder:latest, which should be the same image. Following up on a Slack discussion, it might be more intuitive for users to trust an image regardless of any tag.

I took the opportunity to extract trusted/known builder handling into a common location.

Documentation

  • Should this change be documented?
    • Yes, see #___
    • No

@modulo11 modulo11 requested review from a team as code owners September 25, 2024 12:29
@github-actions github-actions bot added this to the 0.36.0 milestone Sep 25, 2024
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Sep 25, 2024
@modulo11 modulo11 force-pushed the match-trusted-builders branch 3 times, most recently from a8f525c to cb8f978 Compare September 26, 2024 09:03
Copy link
Member

@natalieparellano natalieparellano left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution @modulo11 - the implementation looks solid.

I do want to circulate this a bit more to ensure that folks are on board with it - I've added it to the agenda for the next working group. There are two things to note:

  • We'll auto trust deprecated images (e.g., heroku/builder:22) in this case, but we do that already (because heroku/builder:22 is on the trusted list though it's not suggested)
  • We could end up auto-trusting "dev" images, which worries me a little. Folks might not be placing the same care into the creation of those images compared to ones that get promoted to production. But that seems more of a risk for bugs vs malicious content.

@modulo11
Copy link
Contributor Author

@natalieparellano any updates from the working group meeting?

@natalieparellano
Copy link
Member

@natalieparellano any updates from the working group meeting?

I missed it 🙀 - @jabrown85 do you recall if this was discussed?

@jabrown85
Copy link
Contributor

Ah yeah - we did discuss this one the other day. The consensus of the those in the Working Group was that we like the idea of being able to trust a repo. E.g. pack config trusted-builders add heroku/builders in addition to the pack config trusted-builders add heroku/builders:24. We want users to be able to continue trusting only a tag. Auto-stripping the tag and trusting the entire repository could lead to users trusting things like heroku/builders:24-rc3.dev which is unexpected.

@natalieparellano
Copy link
Member

Awesome - thank you @jabrown85! @modulo11 would you be able to update the implementation to reflect that?

@modulo11 modulo11 force-pushed the match-trusted-builders branch 2 times, most recently from 60de1ed to 6dc5051 Compare November 8, 2024 12:30
@modulo11 modulo11 force-pushed the match-trusted-builders branch from 6dc5051 to c639827 Compare November 8, 2024 12:31
@modulo11
Copy link
Contributor Author

modulo11 commented Nov 8, 2024

I updated the PR according to the discussion. The current behavior (also seen in the tests) is:

  • Trusting my/trusted/builder-jammy will trust any tag, e.g., my/trusted/builder-jammy:1.2.3 and my/trusted/builder-jammy:latest
  • Trusting my/trusted/builder-jammy:1.2.3 will only trust this specific tag
  • Trusting my/trusted/builder-jammy:latest will only trust the latest tag, so my/trusted/builder-jammy:1.2.3 will be untrusted

@modulo11 modulo11 changed the title Ignore tag when matching trusted builders Allow trusted builders by repo or tag Nov 8, 2024
Copy link
Member

@jjbustamante jjbustamante left a comment

Choose a reason for hiding this comment

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

Thank you very much @modulo11 for this contribution!

I tested locally and it is working as expected!

@jjbustamante
Copy link
Member

@modulo11 Could you update it to reflect the latest changes from the main branch?

Signed-off-by: Johannes Dillmann <j.dillmann@sap.com>
auto-merge was automatically disabled November 13, 2024 12:31

Head branch was pushed to by a user without write access

@modulo11 modulo11 force-pushed the match-trusted-builders branch from c639827 to 5718204 Compare November 13, 2024 12:31
@jjbustamante jjbustamante merged commit 466439d into buildpacks:main Nov 13, 2024
16 checks passed
@modulo11 modulo11 deleted the match-trusted-builders branch November 13, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants