Repository: sigstore/fulcio
Pull request #1762 (2024-08-13 14:04:30)

fix: removing surplus slash, making logs richer
<!--
Thanks for opening a pull request! Please do not just delete this text.  The three fields below are mandatory.

Please remember to:
- This PR requires an issue. If it is a new feature, the issue should proceed the PR and will have allowed sufficent time for discussions to take place. Please use
issue tags such as "Closes #XYZ" or "Resolves sigstore/repo-name#XYZ".
  [documentation](https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword)
- ensure your commits are signed-off, as sigstore uses the [DCO](https://en.wikipedia.org/wiki/Developer_Certificate_of_Origin) using `git commit -s`, or `git commit -s --amend` if you want to amend already existing commits
- lastly, ensure there are no merge commits!

Thank you :)
-->
#### Summary
During the validations, we noticed that the the template for `source-repository-ref` for gitlab has a surplus slash.
We also noticed that the error with log `"value <> not present in either claims or defaults.` has spawned several times in production, but we wasn't able to detect the reason for it, then I decided to put at least the issuer that triggered the flow on the error log.
I also added a validation that the metadata should exist for ci-providers.
<!--
 Explain the **motivation** for making this change. What existing problem does the pull request solve? How can reviewers test this PR?
-->

#### Release Note
<!--
Add a release note for each of the following conditions:

* Config changes (additions, deletions, updates)
* API additions—new endpoint, new response fields, or newly accepted request parameters
* Database changes (any)
* Websocket additions or changes
* Anything noteworthy to an administrator running private sigstore instances (err on the side of over-communicating)
* New features and improvements, including behavioural changes, UI changes and CLI changes
* Bug fixes and fixes of previous known issues
* Deprecation warnings, breaking changes, or compatibility notes

If no release notes are required write NONE. Use past-tense.

-->

#### Documentation
<!--

Does this change require an update to documentation? How will users implement your new feature?

Please reference a PR within https://docs.sigstore.dev

-->
cc @haydentherapper 


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/sigstore/fulcio/pull/1762?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) Report
Attention: Patch coverage is `75.00000%` with `2 lines` in your changes missing coverage. Please review.
> Project coverage is 50.19%. Comparing base [(`cf238ac`)](https://app.codecov.io/gh/sigstore/fulcio/commit/cf238accd68ef13e1e3425a5d87db2f352de173c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) to head [(`a6c97c1`)](https://app.codecov.io/gh/sigstore/fulcio/commit/a6c97c13c3389bd1f24b55a8d2429439184f850d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore).
> Report is 172 commits behind head on main.

| [Files](https://app.codecov.io/gh/sigstore/fulcio/pull/1762?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) | Patch % | Lines |
|---|---|---|
| [pkg/identity/ciprovider/principal.go](https://app.codecov.io/gh/sigstore/fulcio/pull/1762?src=pr&el=tree&filepath=pkg%2Fidentity%2Fciprovider%2Fprincipal.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore#diff-cGtnL2lkZW50aXR5L2NpcHJvdmlkZXIvcHJpbmNpcGFsLmdv) | 75.00% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/sigstore/fulcio/pull/1762?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #1762      +/-   ##
==========================================
- Coverage   57.93%   50.19%   -7.75%     
==========================================
  Files          50       70      +20     
  Lines        3119     4164    +1045     
==========================================
+ Hits         1807     2090     +283     
- Misses       1154     1842     +688     
- Partials      158      232      +74     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/sigstore/fulcio/pull/1762?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore).


Patch:
diff --git a/config/identity/config.yaml b/config/identity/config.yaml
index e2650d7..77ef00f 100644
--- a/config/identity/config.yaml
+++ b/config/identity/config.yaml
@@ -178,7 +178,7 @@ ci-issuer-metadata:
       # ref_type: The type of the ref
       # E.g. "branch", "tag"
       # ref: Git ref being built
-      source-repository-ref: refs/{{if eq .ref_type "branch"}}heads/{{ else }}tags/{{end}}/{{ .ref }}
+      source-repository-ref: refs/{{if eq .ref_type "branch"}}heads/{{ else }}tags/{{end}}{{ .ref }}
       # project_id: ID to the source repo
       source-repository-identifier: "project_id"
       # namespace_path: Owner of the source repo (mutable)
diff --git a/pkg/identity/ciprovider/principal.go b/pkg/identity/ciprovider/principal.go
index dce9227..2066a04 100644
--- a/pkg/identity/ciprovider/principal.go
+++ b/pkg/identity/ciprovider/principal.go
@@ -18,6 +18,7 @@ import (
 	"bytes"
 	"context"
 	"crypto/x509"
+	"encoding/json"
 	"fmt"
 	"html/template"
 	"net/url"
@@ -47,7 +48,10 @@ func getTokenClaims(token *oidc.IDToken) (map[string]string, error) {
 
 // It makes string interpolation for a given string by using the
 // templates syntax https://pkg.go.dev/text/template
-func applyTemplateOrReplace(extValueTemplate string, tokenClaims map[string]string, issuerMetadata map[string]string) (string, error) {
+// logMetadata added as a parameter for having a richer log
+func applyTemplateOrReplace(
+	extValueTemplate string, tokenClaims map[string]string,
+	issuerMetadata map[string]string, logMetadata map[string]string) (string, error) {
 
 	// Here we merge the data from was claimed by the id token with the
 	// default data provided by the yaml file.
@@ -81,7 +85,10 @@ func applyTemplateOrReplace(extValueTemplate string, tokenClaims map[string]stri
 	}
 	claimValue, ok := mergedData[extValueTemplate]
 	if !ok {
-		return "", fmt.Errorf("value <%s> not present in either claims or defaults", extValueTemplate)
+		var jsonMetadata bytes.Buffer
+		inrec, _ := json.Marshal(logMetadata)
+		_ = json.Indent(&jsonMetadata, inrec, "", "\t")
+		return "", fmt.Errorf("value <%s> not present in either claims or defaults. %s", extValueTemplate, jsonMetadata.String())
 	}
 	return claimValue, nil
 }
@@ -97,9 +104,13 @@ func WorkflowPrincipalFromIDToken(ctx context.Context, token *oidc.IDToken) (ide
 	if !ok {
 		return nil, fmt.Errorf("configuration can not be loaded for issuer %v", token.Issuer)
 	}
+	metadata, ok := cfg.CIIssuerMetadata[issuerCfg.CIProvider]
+	if !ok {
+		return nil, fmt.Errorf("metadata not found for ci provider %s", issuerCfg.CIProvider)
+	}
 	return ciPrincipal{
 		token,
-		cfg.CIIssuerMetadata[issuerCfg.CIProvider],
+		metadata,
 	}, nil
 }
 
@@ -115,7 +126,15 @@ func (principal ciPrincipal) Embed(_ context.Context, cert *x509.Certificate) er
 	if err != nil {
 		return err
 	}
-	subjectAlternativeName, err := applyTemplateOrReplace(principal.ClaimsMetadata.SubjectAlternativeNameTemplate, claims, defaults)
+	if strings.TrimSpace(principal.ClaimsMetadata.SubjectAlternativeNameTemplate) == "" {
+		return fmt.Errorf("SubjectAlternativeNameTemplate should not be empty. Issuer: %s", principal.Token.Issuer)
+	}
+	subjectAlternativeName, err := applyTemplateOrReplace(
+		principal.ClaimsMetadata.SubjectAlternativeNameTemplate, claims, defaults,
+		map[string]string{
+			"Issuer":        principal.Token.Issuer,
+			"ExtensionName": "SubjectAlternativeName",
+		})
 	if err != nil {
 		return err
 	}
@@ -135,10 +154,14 @@ func (principal ciPrincipal) Embed(_ context.Context, cert *x509.Certificate) er
 		s := v.Field(i).String() // value of each field, e.g the template string
 		// We check the field name to avoid to apply the template for the Issuer
 		// Issuer field should always come from the token issuer
-		if s == "" || vType.Field(i).Name == "Issuer" {
+		if strings.TrimSpace(s) == "" || vType.Field(i).Name == "Issuer" {
 			continue
 		}
-		extValue, err := applyTemplateOrReplace(s, claims, defaults)
+		extValue, err := applyTemplateOrReplace(s, claims, defaults,
+			map[string]string{
+				"Issuer":        principal.Token.Issuer,
+				"ExtensionName": vType.Field(i).Name,
+			})
 		if err != nil {
 			return err
 		}


Test patch:
diff --git a/pkg/identity/ciprovider/issuer_test.go b/pkg/identity/ciprovider/issuer_test.go
index 1d8e605..c636fe5 100644
--- a/pkg/identity/ciprovider/issuer_test.go
+++ b/pkg/identity/ciprovider/issuer_test.go
@@ -20,6 +20,7 @@ import (
 	"testing"
 
 	"github.com/coreos/go-oidc/v3/oidc"
+	"github.com/sigstore/fulcio/pkg/certificate"
 	"github.com/sigstore/fulcio/pkg/config"
 	"github.com/sigstore/fulcio/pkg/identity"
 )
@@ -81,8 +82,16 @@ func TestIssuer(t *testing.T) {
 					ClientID:   "sigstore",
 				},
 			}
+		template := "{{.foobar}}"
+		ciissuerMetadata := make(map[string]config.IssuerMetadata)
+		ciissuerMetadata["github-workflow"] = config.IssuerMetadata{
+			ExtensionTemplates: certificate.Extensions{
+				BuildTrigger: template,
+			},
+		}
 		cfg := &config.FulcioConfig{
-			OIDCIssuers: OIDCIssuers,
+			OIDCIssuers:      OIDCIssuers,
+			CIIssuerMetadata: ciissuerMetadata,
 		}
 		ctx = config.With(ctx, cfg)
 		identity.Authorize = func(_ context.Context, _ string, _ ...config.InsecureOIDCConfigOption) (*oidc.IDToken, error) {
diff --git a/pkg/identity/ciprovider/principal_test.go b/pkg/identity/ciprovider/principal_test.go
index 4e51d06..aafae57 100644
--- a/pkg/identity/ciprovider/principal_test.go
+++ b/pkg/identity/ciprovider/principal_test.go
@@ -180,17 +180,25 @@ func TestName(t *testing.T) {
 			}
 			withClaims(token, claims)
 			ctx := context.TODO()
+			template := "{{.foobar}}"
+			ciissuerMetadata := make(map[string]config.IssuerMetadata)
+			ciissuerMetadata["github-workflow"] = config.IssuerMetadata{
+				ExtensionTemplates: certificate.Extensions{
+					BuildTrigger: template,
+				},
+			}
 			OIDCIssuers :=
 				map[string]config.OIDCIssuer{
 					token.Issuer: {
 						IssuerURL:  token.Issuer,
 						Type:       config.IssuerTypeCIProvider,
-						CIProvider: "ci-provider",
+						CIProvider: "github-workflow",
 						ClientID:   "sigstore",
 					},
 				}
 			cfg := &config.FulcioConfig{
-				OIDCIssuers: OIDCIssuers,
+				OIDCIssuers:      OIDCIssuers,
+				CIIssuerMetadata: ciissuerMetadata,
 			}
 			ctx = config.With(ctx, cfg)
 			principal, err := WorkflowPrincipalFromIDToken(ctx, token)
@@ -305,7 +313,10 @@ func TestApplyTemplateOrReplace(t *testing.T) {
 
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {
-			res, err := applyTemplateOrReplace(test.Template, tokenClaims, issuerMetadata)
+			res, err := applyTemplateOrReplace(test.Template, tokenClaims, issuerMetadata,
+				map[string]string{
+					"Issuer": "https://token.actions.githubusercontent.com",
+				})
 			if res != test.ExpectedResult {
 				t.Errorf("expected result don't matches: Expected %s, received: %s, error: %v",
 					test.ExpectedResult, res, err)