fix: extract builder ID from OIDC #264
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Leo Di Donato <120051+leodido@users.noreply.github.com> Co-authored-by: Ona <no-reply@ona.com>
kylos101
reviewed
Nov 10, 2025
…lder ID from OIDC sub claims Signed-off-by: Leo Di Donato <120051+leodido@users.noreply.github.com> Co-authored-by: Ona <no-reply@ona.com>
- Check json.Encoder.Encode errors in test mock servers - Explicitly ignore resp.Body.Close error in defer context Co-authored-by: Ona <no-reply@ona.com>
Add support for extracting job_workflow_ref from top-level JWT claim when not found embedded in the sub claim. This provides flexibility for different OIDC token formats while maintaining compatibility with Fulcio's certificate identity generation. - Try extracting job_workflow_ref from sub claim first (primary path) - Fall back to top-level job_workflow_ref claim if not in sub - Add test case for top-level claim scenario - Update documentation to clarify both extraction paths Co-authored-by: Ona <no-reply@ona.com>
Fix unchecked json.Encoder.Encode error in test helper function. Co-authored-by: Ona <no-reply@ona.com>
Contributor
Author
👋 @kylos101 yes we can test it from a branch workflow, but it requires considerably more changes than just releasing a new Leeway and try it from the main workflow. In the end the problem is the same: correctly identifying the reusable workflow My 2 cents |
Add strings.TrimSpace() check to reject whitespace-only sub claims, preventing confusing error messages later in the extraction process. - Trim whitespace before checking if sub claim is empty - Add test case for whitespace-only sub claim validation Co-authored-by: Ona <no-reply@ona.com>
Remove silent fallback to GITHUB_WORKFLOW_REF when OIDC extraction fails. This prevents creating attestations with incorrect builder IDs that will fail verification. Breaking Change: - Operations now fail fast with clear error when OIDC token extraction fails - No longer falls back to GITHUB_WORKFLOW_REF (which creates broken attestations) - Users must properly configure OIDC environment (id-token: write permission) Benefits: - Prevents wasted CI resources (fails before signing, not at verification) - Clear error messages guide users to fix OIDC configuration - Eliminates security risk of silent degradation to wrong builder ID - Ensures attestations match Fulcio certificate identity Updated tests to reflect new fail-fast behavior. Co-authored-by: Ona <no-reply@ona.com>
Add detailed comment explaining: - How Fulcio (Sigstore's CA) processes OIDC tokens - Why attestation builder ID must match certificate SAN - Uncertainty about GitHub's OIDC token structure - Rationale for trying both extraction approaches This helps future maintainers understand the critical relationship between OIDC token claims, Fulcio certificate generation, and SLSA attestation verification. Co-authored-by: Ona <no-reply@ona.com>
Replace manual environment variable management with t.Setenv() which: - Automatically restores original values after test completion - Provides test isolation (each subtest gets clean environment) - Reduces boilerplate code by 92 lines - Eliminates manual defer cleanup logic This modernizes the test code to use Go 1.17+ testing features. Co-authored-by: Ona <no-reply@ona.com>
Merge TestExtractJobWorkflowRef and TestExtractJobWorkflowRef_RealWorldExamples into a single test function. The 'real world' examples were just additional test cases, not fundamentally different scenarios. Benefits: - Single source of truth for extractJobWorkflowRef testing - Reduced duplication (removed 21 lines) - All 11 test cases still covered (basic + edge cases + real-world scenarios) Co-authored-by: Ona <no-reply@ona.com>
Replace assert.Equal with cmp.Diff for struct comparisons to get better error messages when tests fail. This makes it easier to see exactly which fields differ. Changes: - Add google/go-cmp import (already in go.mod) - Use cmp.Diff for GitHubContext comparisons - Use cmp.Diff for SigningError comparisons - Remove unused testEnv map variable Benefits: - Better test failure messages showing field-by-field diffs - More idiomatic Go testing pattern for complex types - Easier debugging when tests fail Co-authored-by: Ona <no-reply@ona.com>
Refactor TestExtractBuilderIDFromOIDC to use a nested 'want' struct for
expected values, separating test inputs from expected outputs.
Before:
expectError bool
expectedID string
errorMsg string
After:
want struct {
id string
err string
}
Benefits:
- Clear separation between inputs and expected outputs
- More idiomatic Go testing pattern
- Easier to understand test intent at a glance
- Follows go.mdc testing conventions
Co-authored-by: Ona <no-reply@ona.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Related Issue(s)
Fixes https://linear.app/ona-team/issue/CLC-2063/ensure-leeway-extracts-builder-id-from-oidc-token
How to test
Documentation
/hold