Skip to content

Commit

Permalink
Add support for Fulcio username identity in SAN (#2291)
Browse files Browse the repository at this point in the history
* Add support for Fulcio username identity in SAN

We have changed the format of the username identity to not look
like an email, and so we also needed to change which SAN the username
was set in. Now using OtherName, I've added some custom unmarshalling
to extract the OtherName SAN, because Go doesn't support this SAN type.

Note in verify.go, I had to handle when the extension was critical.
Since Go doesn't handle the extension, but it must be marked critical
according to RFC5280, the cert will fail verification. We can simply
remove the extension from the list of unhandled extensions before
verifying.

Signed-off-by: Hayden Blauzvern <[email protected]>

* fix lint

Signed-off-by: Hayden Blauzvern <[email protected]>

* address comments

Signed-off-by: Hayden Blauzvern <[email protected]>

Signed-off-by: Hayden Blauzvern <[email protected]>
  • Loading branch information
haydentherapper authored Sep 30, 2022
1 parent 727e3e1 commit 983c364
Show file tree
Hide file tree
Showing 6 changed files with 394 additions and 2 deletions.
113 changes: 112 additions & 1 deletion pkg/cosign/certextensions.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,13 @@

package cosign

import "crypto/x509"
import (
"crypto/x509"
"crypto/x509/pkix"
"encoding/asn1"
"errors"
"fmt"
)

type CertExtensions struct {
Cert *x509.Certificate
Expand All @@ -30,6 +36,8 @@ var (
CertExtensionGithubWorkflowRepository = "1.3.6.1.4.1.57264.1.5"
CertExtensionGithubWorkflowRef = "1.3.6.1.4.1.57264.1.6"

OIDOtherName = asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 57264, 1, 7}

CertExtensionMap = map[string]string{
CertExtensionOIDCIssuer: "oidcIssuer",
CertExtensionGithubWorkflowTrigger: "githubWorkflowTrigger",
Expand All @@ -38,6 +46,9 @@ var (
CertExtensionGithubWorkflowRepository: "githubWorkflowRepository",
CertExtensionGithubWorkflowRef: "githubWorkflowRef",
}

// OID for Subject Alternative Name
SANOID = asn1.ObjectIdentifier{2, 5, 29, 17}
)

func (ce *CertExtensions) certExtensions() map[string]string {
Expand Down Expand Up @@ -82,3 +93,103 @@ func (ce *CertExtensions) GetCertExtensionGithubWorkflowRepository() string {
func (ce *CertExtensions) GetCertExtensionGithubWorkflowRef() string {
return ce.certExtensions()["githubWorkflowRef"]
}

// TODO: Move (un)marshalling to sigstore/sigstore
// OtherName describes a name related to a certificate which is not in one
// of the standard name formats. RFC 5280, 4.2.1.6:
//
// OtherName ::= SEQUENCE {
// type-id OBJECT IDENTIFIER,
// value [0] EXPLICIT ANY DEFINED BY type-id }
//
// OtherName for Fulcio-issued certificates only supports UTF-8 strings as values.
type OtherName struct {
ID asn1.ObjectIdentifier
Value string `asn1:"utf8,explicit,tag:0"`
}

// MarshalOtherNameSAN creates a Subject Alternative Name extension
// with an OtherName sequence. RFC 5280, 4.2.1.6:
//
// SubjectAltName ::= GeneralNames
// GeneralNames ::= SEQUENCE SIZE (1..MAX) OF GeneralName
// GeneralName ::= CHOICE {
//
// otherName [0] OtherName,
// ... }
func MarshalOtherNameSAN(name string, critical bool) (*pkix.Extension, error) {
o := OtherName{
ID: OIDOtherName,
Value: name,
}
bytes, err := asn1.MarshalWithParams(o, "tag:0")
if err != nil {
return nil, err
}

sans, err := asn1.Marshal([]asn1.RawValue{{FullBytes: bytes}})
if err != nil {
return nil, err
}
return &pkix.Extension{
Id: SANOID,
Critical: critical,
Value: sans,
}, nil
}

// UnmarshalOtherNameSAN extracts a UTF-8 string from the OtherName
// field in the Subject Alternative Name extension.
func UnmarshalOtherNameSAN(exts []pkix.Extension) (string, error) {
var otherNames []string

for _, e := range exts {
if !e.Id.Equal(SANOID) {
continue
}

var seq asn1.RawValue
rest, err := asn1.Unmarshal(e.Value, &seq)
if err != nil {
return "", err
} else if len(rest) != 0 {
return "", fmt.Errorf("trailing data after X.509 extension")
}
if !seq.IsCompound || seq.Tag != asn1.TagSequence || seq.Class != asn1.ClassUniversal {
return "", asn1.StructuralError{Msg: "bad SAN sequence"}
}

rest = seq.Bytes
for len(rest) > 0 {
var v asn1.RawValue
rest, err = asn1.Unmarshal(rest, &v)
if err != nil {
return "", err
}

// skip all GeneralName fields except OtherName
if v.Tag != 0 {
continue
}

var other OtherName
_, err := asn1.UnmarshalWithParams(v.FullBytes, &other, "tag:0")
if err != nil {
return "", fmt.Errorf("could not parse requested OtherName SAN: %w", err)
}
if !other.ID.Equal(OIDOtherName) {
return "", fmt.Errorf("unexpected OID for OtherName, expected %v, got %v", OIDOtherName, other.ID)
}
otherNames = append(otherNames, other.Value)
}
}

if len(otherNames) == 0 {
return "", errors.New("no OtherName found")
}
if len(otherNames) != 1 {
return "", errors.New("expected only one OtherName")
}

return otherNames[0], nil
}
165 changes: 165 additions & 0 deletions pkg/cosign/certextensions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import (
"crypto/x509"
"crypto/x509/pkix"
"encoding/asn1"
"encoding/hex"
"strings"
"testing"
)

Expand Down Expand Up @@ -64,3 +66,166 @@ func TestCertExtensions(t *testing.T) {
t.Fatal("CertExtension does not extract field 'githubWorkflowRef' correctly")
}
}

func TestMarshalAndUnmarshalOtherNameSAN(t *testing.T) {
otherName := "foo!example.com"
critical := true

ext, err := MarshalOtherNameSAN(otherName, critical)
if err != nil {
t.Fatalf("unexpected error for MarshalOtherNameSAN: %v", err)
}
if ext.Critical != critical {
t.Fatalf("expected extension to be critical")
}
if !ext.Id.Equal(SANOID) {
t.Fatalf("expected extension's OID to be SANs OID")
}
// https://lapo.it/asn1js/#MCGgHwYKKwYBBAGDvzABB6ARDA9mb28hZXhhbXBsZS5jb20
// 30 - Constructed sequence
// 21 - length of sequence
// A0 - Context-specific (class 2) (bits 8,7) with Constructed bit (bit 6) and 0 tag
// 1F - length of context-specific field (OID)
// 06 - OID tag
// 0A - length of OID
// 2B 06 01 04 01 83 BF 30 01 07 - OID
// A0 - Context-specific (class 2) with Constructed bit and 0 tag
// (needed for EXPLICIT encoding, which wraps field in outer encoding)
// 11 - length of context-specific field (string)
// 0C - UTF8String tag
// 0F - length of string
// 66 6F 6F 21 65 78 61 6D 70 6C 65 2E 63 6F 6D - string
if hex.EncodeToString(ext.Value) != "3021a01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6d" {
t.Fatalf("unexpected ASN.1 encoding")
}

on, err := UnmarshalOtherNameSAN([]pkix.Extension{*ext})
if err != nil {
t.Fatalf("unexpected error for UnmarshalOtherNameSAN: %v", err)
}
if on != otherName {
t.Fatalf("unexpected OtherName, expected %s, got %s", otherName, on)
}
}

func TestUnmarshalOtherNameSANFailures(t *testing.T) {
var err error

// failure: no SANs extension
ext := &pkix.Extension{
Id: asn1.ObjectIdentifier{},
Critical: true,
Value: []byte{},
}
_, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext})
if err == nil || !strings.Contains(err.Error(), "no OtherName found") {
t.Fatalf("expected error finding no OtherName, got %v", err)
}

// failure: bad sequence
ext = &pkix.Extension{
Id: SANOID,
Critical: true,
Value: []byte{},
}
_, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext})
if err == nil || !strings.Contains(err.Error(), "sequence truncated") {
t.Fatalf("expected error with invalid ASN.1, got %v", err)
}

// failure: extra data after valid sequence
b, _ := hex.DecodeString("3021a01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6d" + "30")
ext = &pkix.Extension{
Id: SANOID,
Critical: true,
Value: b,
}
_, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext})
if err == nil || !strings.Contains(err.Error(), "trailing data after X.509 extension") {
t.Fatalf("expected error with extra data, got %v", err)
}

// failure: non-universal class (Change last two bits: 30 = 00110000 => 10110000 -> B0)
b, _ = hex.DecodeString("B021a01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6d")
ext = &pkix.Extension{
Id: SANOID,
Critical: true,
Value: b,
}
_, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext})
if err == nil || !strings.Contains(err.Error(), "bad SAN sequence") {
t.Fatalf("expected error with non-universal class, got %v", err)
}

// failure: not compound sequence (Change 6th bit: 30 = 00110000 => 00010000 -> 10)
b, _ = hex.DecodeString("1021a01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6d")
ext = &pkix.Extension{
Id: SANOID,
Critical: true,
Value: b,
}
_, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext})
if err == nil || !strings.Contains(err.Error(), "bad SAN sequence") {
t.Fatalf("expected error with non-compound sequence, got %v", err)
}

// failure: non-sequence tag (Change lower 5 bits: 30 = 00110000 => 00100010 -> 12)
b, _ = hex.DecodeString("1221a01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6d")
ext = &pkix.Extension{
Id: SANOID,
Critical: true,
Value: b,
}
_, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext})
if err == nil || !strings.Contains(err.Error(), "bad SAN sequence") {
t.Fatalf("expected error with non-sequence tag, got %v", err)
}

// failure: no GeneralName with tag=0 (Change lower 5 bits of first sequence field: 3021a01f -> 3021a11f)
b, _ = hex.DecodeString("3021a11f060a2b0601040183bf300108a0110c0f666f6f216578616d706c652e636f6d")
ext = &pkix.Extension{
Id: SANOID,
Critical: true,
Value: b,
}
_, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext})
if err == nil || !strings.Contains(err.Error(), "no OtherName found") {
t.Fatalf("expected error with no GeneralName, got %v", err)
}

// failure: invalid OtherName (Change tag of UTF8String field to 1: a0110c0f -> a1110c0f)
b, _ = hex.DecodeString("3021a01f060a2b0601040183bf300108a1110c0f666f6f216578616d706c652e636f6d")
ext = &pkix.Extension{
Id: SANOID,
Critical: true,
Value: b,
}
_, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext})
if err == nil || !strings.Contains(err.Error(), "could not parse requested OtherName SAN") {
t.Fatalf("expected error with invalid OtherName, got %v", err)
}

// failure: OtherName has wrong OID (2b0601040183bf300107 -> 2b0601040183bf300108)
b, _ = hex.DecodeString("3021a01f060a2b0601040183bf300108a0110c0f666f6f216578616d706c652e636f6d")
ext = &pkix.Extension{
Id: SANOID,
Critical: true,
Value: b,
}
_, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext})
if err == nil || !strings.Contains(err.Error(), "unexpected OID for OtherName") {
t.Fatalf("expected error with wrong OID, got %v", err)
}

// failure: multiple OtherName fields (Increase sequence size from 0x21 -> 0x42, duplicate OtherName)
b, _ = hex.DecodeString("3042a01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6da01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6d")
ext = &pkix.Extension{
Id: SANOID,
Critical: true,
Value: b,
}
_, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext})
if err == nil || !strings.Contains(err.Error(), "expected only one OtherName") {
t.Fatalf("expected error with multiple OtherName fields, got %v", err)
}
}
20 changes: 20 additions & 0 deletions pkg/cosign/verify.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"crypto/ecdsa"
"crypto/sha256"
"crypto/x509"
"encoding/asn1"
"encoding/base64"
"encoding/hex"
"encoding/json"
Expand Down Expand Up @@ -169,6 +170,20 @@ func ValidateAndUnpackCert(cert *x509.Certificate, co *CheckOpts) (signature.Ver
return nil, fmt.Errorf("invalid certificate found on signature: %w", err)
}

// Handle certificates where the Subject Alternative Name is not set to a supported
// GeneralName (RFC 5280 4.2.1.6). Go only supports DNS, IP addresses, email addresses,
// or URIs as SANs. Fulcio can issue a certificate with an OtherName GeneralName, so
// remove the unhandled critical SAN extension before verifying.
if len(cert.UnhandledCriticalExtensions) > 0 {
var unhandledExts []asn1.ObjectIdentifier
for _, oid := range cert.UnhandledCriticalExtensions {
if !oid.Equal(SANOID) {
unhandledExts = append(unhandledExts, oid)
}
}
cert.UnhandledCriticalExtensions = unhandledExts
}

// Now verify the cert, then the signature.
chains, err := TrustedCert(cert, co.RootCerts, co.IntermediateCerts)
if err != nil {
Expand Down Expand Up @@ -332,6 +347,11 @@ func getSubjectAlternateNames(cert *x509.Certificate) []string {
for _, uri := range cert.URIs {
sans = append(sans, uri.String())
}
// ignore error if there's no OtherName SAN
otherName, _ := UnmarshalOtherNameSAN(cert.Extensions)
if len(otherName) > 0 {
sans = append(sans, otherName)
}
return sans
}

Expand Down
Loading

0 comments on commit 983c364

Please sign in to comment.