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.

Sign up for GitHub

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

Jump to bottom

Accept URI for Sigstore Signed Images #2235

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Accept URI for Sigstore Signed Images #2235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions signature/fulcio_cert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"errors"
"fmt"
"slices"
"strings"
"time"

"github.com/containers/image/v5/signature/internal"
Expand All @@ -24,14 +25,15 @@ type fulcioTrustRoot struct {
caCertificates *x509.CertPool
oidcIssuer string
subjectEmail string
URI string
}

func (f *fulcioTrustRoot) validate() error {
if f.oidcIssuer == "" {
return errors.New("Internal inconsistency: Fulcio use set up without OIDC issuer")
}
if f.subjectEmail == "" {
return errors.New("Internal inconsistency: Fulcio use set up without subject email")
if f.subjectEmail == "" && f.URI == "" {
return errors.New("Internal inconsistency: Fulcio use set up without subject email or URI")
}
return nil
}
Expand Down Expand Up @@ -177,10 +179,17 @@ func (f *fulcioTrustRoot) verifyFulcioCertificateAtTime(relevantTime time.Time,
}

// == Validate the OIDC subject
if !slices.Contains(untrustedCertificate.EmailAddresses, f.subjectEmail) {
return nil, internal.NewInvalidSignatureError(fmt.Sprintf("Required email %q not found (got %q)",
f.subjectEmail,
untrustedCertificate.EmailAddresses))
if !slices.Contains(untrustedCertificate.EmailAddresses, f.subjectEmail) && !strings.Contains(untrustedCertificate.URIs[0].String(), f.URI) {
if len(untrustedCertificate.EmailAddresses) > 0 {
return nil, internal.NewInvalidSignatureError(fmt.Sprintf("Required email %s not found (got %#v)",
f.subjectEmail,
untrustedCertificate.EmailAddresses))
}
if len(untrustedCertificate.URIs) > 0 {
return nil, internal.NewInvalidSignatureError(fmt.Sprintf("Required URI %s not found (got %#v)",
f.URI,
untrustedCertificate.URIs))
}
Comment on lines +182 to +192
Copy link
Author

Choose a reason for hiding this comment

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

  • ⚠️ If the policy requires both an email address and an URI, this must not accept signatures which only have one of them
  • This is invoking slices.Contains(…, "") if the field is not set. That’s at the very least unclean.
  • I suspect the error reporting logic should also be tightened. It must primarily decide based on the policy, not on attacker-set untrusted* fields.

Copy link
Author

Choose a reason for hiding this comment

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

@mtrmac Do you mind elaborating on:

I suspect the error reporting logic should also be tightened. It must primarily decide based on the policy, not on attacker-set untrusted* fields.

Copy link
Collaborator

Choose a reason for hiding this comment

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

E.g. if the policy requires an URI, this code could see len(untrustedCertificate.EmailAddresses) > 0 { and report something about a “required email”, when no email is required.

In general, treat all of the data in the signature as false and potentially malicious until it is explicitly validated against user-configured policy. A specific manifestation of that is that control flow should primarily be driven by that user-configured policy, and not by the signature data, if at all possible.

Copy link

@jmpolom jmpolom Jan 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In general, treat all of the data in the signature as false and potentially malicious until it is explicitly validated against user-configured policy. A specific manifestation of that is that control flow should primarily be driven by that user-configured policy, and not by the signature data, if at all possible.

Agreed. The logic seems backwards. If I specify in the policy to check for a signature with a URI in the SAN and do not specify an email, I do not care if the signature cert also has an email in the SAN at all. There is a question in that case though whether the presence of an email should mean the certificate is rejected (possibly malicious?) outright for having extra fields? I think you do not reject it for having too many SAN fields. If anything is done based only on the presence or absence of a field it should be configurable in the policy though.

But yes, only check for what the policy specifies and right now this seems to check for the presence of things based on what is defined in the cert (backwards part of the logic). Changing the logic to look based on what's defined in the policy would also probably allow you to eliminate both the slices.Contains and strings.Contains since you could directly compare those fields in the structs.

I would also agree that it's probably bad practice at a minimum to use something from golang.org/x/exp in anything destined to get merged into a main branch. Unfortunately this project seems to use go 1.19 and slices did not make it out of exp until 1.21.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In c/image we do prefer x/exp/{slices, maps} over open-coded loops or copy&pasted helpers. These subpackages exist in Go 1.21, so we have a clear path to migrating to the standard library versions, and in the meantime, using shared well-tested code and idiomatic utilities is already better than single-use code repetition.

}
// FIXME: Match more subject types? Cosign does:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This FIXME comment might eventually also need updating.

// - .DNSNames (can’t be issued by Fulcio)
Expand Down
25 changes: 21 additions & 4 deletions signature/policy_config_sigstore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,17 @@ func PRSigstoreSignedFulcioWithSubjectEmail(subjectEmail string) PRSigstoreSigne
}
}

// PRSigstoreSignedFulcioWithURI specifies a value for the "URI" field when calling NewPRSigstoreSignedFulcio
func PRSigstoreSignedFulcioWithURI(URI string) PRSigstoreSignedFulcioOption {
return func(f *prSigstoreSignedFulcio) error {
if f.URI != "" {
return errors.New(`"URI" already specified`)
}
f.URI = URI
return nil
}
}

// newPRSigstoreSignedFulcio is NewPRSigstoreSignedFulcio, except it returns the private type
func newPRSigstoreSignedFulcio(options ...PRSigstoreSignedFulcioOption) (*prSigstoreSignedFulcio, error) {
res := prSigstoreSignedFulcio{}
Expand All @@ -279,8 +290,8 @@ func newPRSigstoreSignedFulcio(options ...PRSigstoreSignedFulcioOption) (*prSigs
if res.OIDCIssuer == "" {
return nil, InvalidPolicyFormatError("oidcIssuer not specified")
}
if res.SubjectEmail == "" {
return nil, InvalidPolicyFormatError("subjectEmail not specified")
if res.SubjectEmail == "" && res.URI == "" {
return nil, InvalidPolicyFormatError("subjectEmail and URI not specified")
}

return &res, nil
Expand All @@ -297,7 +308,7 @@ var _ json.Unmarshaler = (*prSigstoreSignedFulcio)(nil)
func (f *prSigstoreSignedFulcio) UnmarshalJSON(data []byte) error {
*f = prSigstoreSignedFulcio{}
var tmp prSigstoreSignedFulcio
var gotCAPath, gotCAData, gotOIDCIssuer, gotSubjectEmail bool // = false...
var gotCAPath, gotCAData, gotOIDCIssuer, gotSubjectEmail, gotURI bool // = false...
if err := internal.ParanoidUnmarshalJSONObject(data, func(key string) any {
switch key {
case "caPath":
Expand All @@ -312,6 +323,9 @@ func (f *prSigstoreSignedFulcio) UnmarshalJSON(data []byte) error {
case "subjectEmail":
gotSubjectEmail = true
return &tmp.SubjectEmail
case "URI":
gotURI = true
return &tmp.URI
default:
return nil
}
Expand All @@ -329,9 +343,12 @@ func (f *prSigstoreSignedFulcio) UnmarshalJSON(data []byte) error {
if gotOIDCIssuer {
opts = append(opts, PRSigstoreSignedFulcioWithOIDCIssuer(tmp.OIDCIssuer))
}
if gotSubjectEmail {
if gotSubjectEmail && !gotURI {
opts = append(opts, PRSigstoreSignedFulcioWithSubjectEmail(tmp.SubjectEmail))
}
if !gotSubjectEmail && gotURI {
opts = append(opts, PRSigstoreSignedFulcioWithURI(tmp.URI))
}
Comment on lines +346 to +351
Copy link
Author

Choose a reason for hiding this comment

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

This is silently ignoring the options if both are specified?!


res, err := newPRSigstoreSignedFulcio(opts...)
if err != nil {
Expand Down
1 change: 1 addition & 0 deletions signature/policy_eval_sigstore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ func (f *prSigstoreSignedFulcio) prepareTrustRoot() (*fulcioTrustRoot, error) {
caCertificates: certs,
oidcIssuer: f.OIDCIssuer,
subjectEmail: f.SubjectEmail,
URI: f.URI,
}
if err := fulcio.validate(); err != nil {
return nil, err
Expand Down
2 changes: 2 additions & 0 deletions signature/policy_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,8 @@ type prSigstoreSignedFulcio struct {
OIDCIssuer string `json:"oidcIssuer,omitempty"`
// SubjectEmail specifies the expected email address of the authenticated OIDC identity, recorded by Fulcio into the generated certificates.
SubjectEmail string `json:"subjectEmail,omitempty"`
// URI specifies the expected URI of the authenticated OIDC identity, recorded by Fulcio into the generated certificates.
URI string `json:"URI,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably be something like subjectURI (if we decide to add the URI field, and not something else).

Copy link

Choose a reason for hiding this comment

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

This should probably be something like subjectURI (if we decide to add the URI field, and not something else).

Agreed for consistency. I will also add that adding URI support is required for compatibility with keyless cosign signatures which as far as I'm concerned are the only way anyone should be using cosign to sign container images. The test workflow that @lukewarmtemp used was flawed and did not sign the container using the github actions token.

}

// PolicyReferenceMatch specifies a set of image identities accepted in PolicyRequirement.
Expand Down