Skip to content

Consolidate trust logic into IsTrustedBuilder#2573

Open
runesoerensen wants to merge 6 commits into
buildpacks:mainfrom
runesoerensen:runesoerensen/consolidate-trusted-builder-logic
Open

Consolidate trust logic into IsTrustedBuilder#2573
runesoerensen wants to merge 6 commits into
buildpacks:mainfrom
runesoerensen:runesoerensen/consolidate-trusted-builder-logic

Conversation

@runesoerensen

@runesoerensen runesoerensen commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

Summary

Consolidate all trust checking into IsTrustedBuilder so callers don't need to separately check IsKnownTrustedBuilder(), and use name.ParseReference for consistent reference matching across both known and user-configured builders.

  • IsTrustedBuilder now checks both known trusted builders and user-configured trusted builders
  • Uses name.ParseReference for consistent reference matching across both sources - entries without a tag match any tag in the repository, entries with a tag require an exact match
  • Remove now-unnecessary || IsKnownTrustedBuilder() calls from build.go, config_trusted_builder.go, and builder_inspect.go

Output

Before

$ pack builder inspect paketobuildpacks/builder-jammy-base:latest
...
Trusted: No

After

$ pack builder inspect paketobuildpacks/builder-jammy-base:latest
...
Trusted: Yes

Documentation

  • Should this change be documented?
    • No

Related

Resolves #2572

@github-actions github-actions Bot added this to the 0.41.0 milestone Apr 15, 2026
@github-actions github-actions Bot added the type/enhancement Issue that requests a new feature or improvement. label Apr 15, 2026
…ilders

Signed-off-by: Rune Soerensen <rsoerensen@salesforce.com>
Signed-off-by: Rune Soerensen <rsoerensen@salesforce.com>
@runesoerensen runesoerensen force-pushed the runesoerensen/consolidate-trusted-builder-logic branch from ad1df62 to 85e15fc Compare May 15, 2026 14:00
@runesoerensen runesoerensen marked this pull request as ready for review May 15, 2026 14:15
@runesoerensen runesoerensen requested review from a team as code owners May 15, 2026 14:15
@runesoerensen

runesoerensen commented May 16, 2026

Copy link
Copy Markdown
Contributor Author

@jjbustamante thanks for reviewing/merging #2571! This branch was based on that work, so I've rebased and marked the PR ready for review :)

h.AssertTrue(t, isTrusted)

// Known builder without tag should match any tag
isTrusted, err = bldr.IsTrustedBuilder(config.Config{}, "paketobuildpacks/builder-jammy-base:latest")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this test case reference: paketobuildpacks/builder-jammy-base?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The :latest here is intentional - this case exercises the "known entry has no tag, match any tag in the same repository" branch of the new name.ParseReference logic, which is what #2572 is about and is also the example shown in the Before/After in the PR description.

The heroku/builder:24 case above covers the exact-tag branch. Switching to a bare paketobuildpacks/builder-jammy-base would collapse this into a bare-vs-bare repository match and stop exercising the "any tag" path.

Happy to expand the inline comment if it'd read more clearly, or add a second assertion with a different tag (e.g. :anything) to make the "any tag" intent more obvious - let me know what you'd prefer (or if I'm missing something obvious :)).

@TIR44

TIR44 commented May 31, 2026

Copy link
Copy Markdown

I accidentally opened duplicate PR #2619 after auditing #2572, then noticed this PR already covers the core fix. I closed #2619 in favor of this one.

While doing that audit, I found one extra test angle that may be useful here: caller coverage in addition to internal/builder unit tests. The branch in #2619 added checks that:

  • build treats paketobuildpacks/builder-jammy-base:latest as trusted;
  • builder inspect reports the same tagged known builder as trusted;
  • config trusted-builders add paketobuildpacks/builder-jammy-base:latest is a no-op because it is already trusted through the tagless known entry.

The reason I think caller coverage is useful is that pkg/client/build.go defaults opts.TrustBuilder to builder.IsKnownTrustedBuilder, while command code used to compose IsTrustedBuilder(...) || IsKnownTrustedBuilder(...). So the regression is not just the helper result; it is also keeping the command and client paths aligned.

Make IsKnownTrustedBuilder use the same name.ParseReference matching as
IsTrustedBuilder so tagless known entries match any tag in the same
repository. This fixes the SDK default at pkg/client/build.go and the
"can't untrust a known builder" guard in config trusted-builders remove,
both of which still relied on exact-string matching.

Signed-off-by: Rune Soerensen <rsoerensen@salesforce.com>
Add regression coverage at the command layer to guard against future
refactors that bypass the smart-matching helpers: build, builder
inspect, and config trusted-builders add now each verify that a tagged
reference (paketobuildpacks/builder-jammy-base:latest) is treated as
trusted via the tagless known entry.

Signed-off-by: Rune Soerensen <rsoerensen@salesforce.com>
@runesoerensen

runesoerensen commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @TIR44 - good catch on the SDK default.

You're right that pkg/client/build.go:453 still pointed at IsKnownTrustedBuilder, which kept exact-string matching. The CLI worked because internal/commands/build.go overrides TrustBuilder with a closure built from IsTrustedBuilder, but a programmatic caller passing paketobuildpacks/builder-jammy-base:latest without setting TrustBuilder would still hit the original regression.

Pushed two follow-up commits:

  • 6bb75db: IsKnownTrustedBuilder now uses the same name.ParseReference matching as IsTrustedBuilder, sharing a referencesMatch helper. Tagless known entries match any tag in the repo; tagged entries still require an exact tag match. This also closes the same hole in the "can't untrust a known builder" guard in removeTrustedBuilder.
  • 3dc1da2: Caller-level tests for build, builder inspect, and config trusted-builders add against paketobuildpacks/builder-jammy-base:latest.

Could you take a look and let me know if this covers what you had in mind?

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.

Trust checks are fragmented and known builders without a tag don't match tagged queries

3 participants