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

Check certificate policy flags with only a certificate #1869

Merged
merged 1 commit into from
May 12, 2022
Merged

Check certificate policy flags with only a certificate #1869

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
4 changes: 4 additions & 0 deletions cmd/cosign/cli/verify/verify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,10 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) {
return err
}
if c.CertChain == "" {
err = cosign.CheckCertificatePolicy(cert, co)
if err != nil {
return err
}
pubKey, err = signature.LoadVerifier(cert.PublicKey, crypto.SHA256)
if err != nil {
return err
Expand Down
4 changes: 4 additions & 0 deletions cmd/cosign/cli/verify/verify_attestation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,10 @@ func (c *VerifyAttestationCommand) Exec(ctx context.Context, images []string) (e
return errors.Wrap(err, "loading certificate from reference")
}
if c.CertChain == "" {
err = cosign.CheckCertificatePolicy(cert, co)
if err != nil {
return err
}
co.SigVerifier, err = signature.LoadVerifier(cert.PublicKey, crypto.SHA256)
if err != nil {
return errors.Wrap(err, "creating certificate verifier")
Expand Down
14 changes: 9 additions & 5 deletions cmd/cosign/cli/verify/verify_blob.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,16 @@ func VerifyBlobCmd(ctx context.Context, ko options.KeyOpts, certRef, certEmail,
if err != nil {
return err
}
co := &cosign.CheckOpts{
CertEmail: certEmail,
CertOidcIssuer: certOidcIssuer,
EnforceSCT: enforceSCT,
}
if certChain == "" {
err = cosign.CheckCertificatePolicy(cert, co)
if err != nil {
return err
}
verifier, err = signature.LoadVerifier(cert.PublicKey, crypto.SHA256)
if err != nil {
return err
Expand All @@ -116,11 +125,6 @@ func VerifyBlobCmd(ctx context.Context, ko options.KeyOpts, certRef, certEmail,
if err != nil {
return err
}
co := &cosign.CheckOpts{
CertEmail: certEmail,
CertOidcIssuer: certOidcIssuer,
EnforceSCT: enforceSCT,
}
verifier, err = cosign.ValidateAndUnpackCertWithChain(cert, chain, co)
if err != nil {
return err
Expand Down
59 changes: 36 additions & 23 deletions pkg/cosign/verify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,35 @@ func ValidateAndUnpackCert(cert *x509.Certificate, co *CheckOpts) (signature.Ver
if err != nil {
return nil, err
}

err = CheckCertificatePolicy(cert, co)
if err != nil {
return nil, err
}

contains, err := ctl.ContainsSCT(cert.Raw)
if err != nil {
return nil, err
}
if co.EnforceSCT && !contains {
return nil, errors.New("certificate does not include required embedded SCT")
}
if contains {
// handle if chains has more than one chain - grab first and print message
if len(chains) > 1 {
fmt.Fprintf(os.Stderr, "**Info** Multiple valid certificate chains found. Selecting the first to verify the SCT.\n")
}
if err := ctl.VerifyEmbeddedSCT(context.Background(), chains[0]); err != nil {
return nil, err
}
}

return verifier, nil
}

// CheckCertificatePolicy checks that the certificate subject and issuer match
// the expected values.
func CheckCertificatePolicy(cert *x509.Certificate, co *CheckOpts) error {
if co.CertEmail != "" {
emailVerified := false
for _, em := range cert.EmailAddresses {
Expand All @@ -181,12 +210,12 @@ func ValidateAndUnpackCert(cert *x509.Certificate, co *CheckOpts) (signature.Ver
}
}
if !emailVerified {
return nil, errors.New("expected email not found in certificate")
return errors.New("expected email not found in certificate")
}
}
if co.CertOidcIssuer != "" {
if getIssuer(cert) != co.CertOidcIssuer {
return nil, errors.New("expected oidc issuer not found in certificate")
return errors.New("expected oidc issuer not found in certificate")
}
}
// If there are identities given, go through them and if one of them
Expand All @@ -198,7 +227,7 @@ func ValidateAndUnpackCert(cert *x509.Certificate, co *CheckOpts) (signature.Ver
if identity.Issuer != "" {
issuer := getIssuer(cert)
if regex, err := regexp.Compile(identity.Issuer); err != nil {
return nil, fmt.Errorf("malformed issuer in identity: %s : %w", identity.Issuer, err)
return fmt.Errorf("malformed issuer in identity: %s : %w", identity.Issuer, err)
} else if regex.MatchString(issuer) {
issuerMatches = true
}
Expand All @@ -212,7 +241,7 @@ func ValidateAndUnpackCert(cert *x509.Certificate, co *CheckOpts) (signature.Ver
if identity.Subject != "" {
regex, err := regexp.Compile(identity.Subject)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this PR but it's a regex??? That's a surprise

Does this open up an attack where someone passes zjn@mail.example.com and I go register mailqexample.com and can pass the check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vaikas - FYI, I think you added this originally

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I sure did add it. @znewman01, in your particular example, yeah, you'd have to use:
zjn@mail\.example\.com

Let's take this off the thread however, and if we want this to be a non-regexp, I'd be happy to make the change. Would you mind creating an issue, tagging me in it so we have a proper track record and I'll go change it so that it is what folks want.

Thanks for the flagging!!!

Copy link
Contributor

Choose a reason for hiding this comment

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

ACK will do :)

if err != nil {
return nil, fmt.Errorf("malformed subject in identity: %s : %w", identity.Subject, err)
return fmt.Errorf("malformed subject in identity: %s : %w", identity.Subject, err)
}
for _, san := range getSubjectAlternateNames(cert) {
if regex.MatchString(san) {
Expand All @@ -226,28 +255,12 @@ func ValidateAndUnpackCert(cert *x509.Certificate, co *CheckOpts) (signature.Ver
}
if subjectMatches && issuerMatches {
// If both issuer / subject match, return verifier
return verifier, nil
return nil
}
}
return nil, errors.New("none of the expected identities matched what was in the certificate")
return errors.New("none of the expected identities matched what was in the certificate")
}
contains, err := ctl.ContainsSCT(cert.Raw)
if err != nil {
return nil, err
}
if co.EnforceSCT && !contains {
return nil, errors.New("certificate does not include required embedded SCT")
}
if contains {
// handle if chains has more than one chain - grab first and print message
if len(chains) > 1 {
fmt.Fprintf(os.Stderr, "**Info** Multiple valid certificate chains found. Selecting the first to verify the SCT.\n")
}
if err := ctl.VerifyEmbeddedSCT(context.Background(), chains[0]); err != nil {
return nil, err
}
}
return verifier, nil
return nil
}

// getSubjectAlternateNames returns all of the following for a Certificate.
Expand Down
23 changes: 23 additions & 0 deletions pkg/cosign/verify_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,10 @@ func TestValidateAndUnpackCertSuccess(t *testing.T) {
if err != nil {
t.Errorf("ValidateAndUnpackCert expected no error, got err = %v", err)
}
err = CheckCertificatePolicy(leafCert, co)
if err != nil {
t.Errorf("CheckCertificatePolicy expected no error, got err = %v", err)
}
}

func TestValidateAndUnpackCertSuccessAllowAllValues(t *testing.T) {
Expand All @@ -422,6 +426,10 @@ func TestValidateAndUnpackCertSuccessAllowAllValues(t *testing.T) {
if err != nil {
t.Errorf("ValidateAndUnpackCert expected no error, got err = %v", err)
}
err = CheckCertificatePolicy(leafCert, co)
if err != nil {
t.Errorf("CheckCertificatePolicy expected no error, got err = %v", err)
}
}

func TestValidateAndUnpackCertWithSCT(t *testing.T) {
Expand Down Expand Up @@ -522,6 +530,8 @@ func TestValidateAndUnpackCertInvalidOidcIssuer(t *testing.T) {

_, err := ValidateAndUnpackCert(leafCert, co)
require.Contains(t, err.Error(), "expected oidc issuer not found in certificate")
err = CheckCertificatePolicy(leafCert, co)
require.Contains(t, err.Error(), "expected oidc issuer not found in certificate")
}

func TestValidateAndUnpackCertInvalidEmail(t *testing.T) {
Expand All @@ -542,6 +552,8 @@ func TestValidateAndUnpackCertInvalidEmail(t *testing.T) {

_, err := ValidateAndUnpackCert(leafCert, co)
require.Contains(t, err.Error(), "expected email not found in certificate")
err = CheckCertificatePolicy(leafCert, co)
require.Contains(t, err.Error(), "expected email not found in certificate")
}

func TestValidateAndUnpackCertWithChainSuccess(t *testing.T) {
Expand Down Expand Up @@ -705,6 +717,17 @@ func TestValidateAndUnpackCertWithIdentities(t *testing.T) {
t.Errorf("Did not get the expected error %s, got err = %v", tc.wantErrSubstring, err)
}
}
// Test CheckCertificatePolicy
err = CheckCertificatePolicy(leafCert, co)
if err == nil && tc.wantErrSubstring != "" {
t.Errorf("Expected error %s got none", tc.wantErrSubstring)
} else if err != nil {
if tc.wantErrSubstring == "" {
t.Errorf("Did not expect an error, got err = %v", err)
} else if !strings.Contains(err.Error(), tc.wantErrSubstring) {
t.Errorf("Did not get the expected error %s, got err = %v", tc.wantErrSubstring, err)
}
}
}
}
func TestCompareSigs(t *testing.T) {
Expand Down