Skip to content

Commit

Permalink
Code review comments
Browse files Browse the repository at this point in the history
Signed-off-by: Priya Wadhwa <[email protected]>
  • Loading branch information
priyawadhwa committed Nov 22, 2022
1 parent 29c44b1 commit ecedc7b
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 12 deletions.
3 changes: 2 additions & 1 deletion cmd/app/serve.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ func newServeCmd() *cobra.Command {
cmd.Flags().String("log_type", "dev", "logger type to use (dev/prod)")
cmd.Flags().String("ca", "", "googleca | tinkca | pkcs11ca | fileca | kmsca | ephemeralca (for testing)")
cmd.Flags().String("aws-hsm-root-ca-path", "", "Path to root CA on disk (only used with AWS HSM)")
cmd.Flags().String("gcp_private_ca_parent", "", "private ca parent: projects/<project>/locations/<location>/caPools/<caPool> (only used with --ca googleca)")
cmd.Flags().String("gcp_private_ca_parent", "", "private ca parent: projects/<project>/locations/<location>/caPools/<caPool> (only used with --ca googleca)"+
"Optionally specify /certificateAuthorities/*, which will bypass CA pool load balancing.")
cmd.Flags().String("hsm-caroot-id", "", "HSM ID for Root CA (only used with --ca pkcs11ca)")
cmd.Flags().String("ct-log-url", "http://localhost:6962/test", "host and path (with log prefix at the end) to the ct log")
cmd.Flags().String("ct-log-public-key-path", "", "Path to a PEM-encoded public key of the CT log, used to verify SCTs")
Expand Down
34 changes: 24 additions & 10 deletions pkg/ca/googleca/v1/googleca.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import (
)

type CertAuthorityService struct {
certAuthority string
certAuthorityParent string
caPoolParent string
client *privateca.CertificateAuthorityClient
Expand All @@ -56,15 +57,22 @@ func NewCertAuthorityService(ctx context.Context, parent string, opts ...option.
c := CertAuthorityService{
client: client,
}

if !strings.Contains(parent, "certificateAuthorities") {
c.caPoolParent = parent
} else {
// parent should be in the form projects/*/locations/*/caPools/*/certificateAuthorities/*
// to create a cert, we only want projects/*/locations/*/caPools/*
caPoolParent := strings.Split(parent, "/certificateAuthorities")
c.caPoolParent = caPoolParent[0]
c.certAuthorityParent = parent
return &c, nil
}
// parent should be in the form projects/*/locations/*/caPools/*/certificateAuthorities/*
// to create a cert, we only want projects/*/locations/*/caPools/*
caPoolParent := strings.Split(parent, "/certificateAuthorities")
c.caPoolParent = caPoolParent[0]

s := strings.SplitAfter(parent, "certificateAuthorities/")
if len(s) != 2 {
return nil, fmt.Errorf("certificate authority should be specified in the format projects/*/locations/*/caPools/*/certificateAuthorities/*")
}
c.certAuthority = s[1]
c.certAuthorityParent = parent
return &c, nil
}

Expand Down Expand Up @@ -92,7 +100,7 @@ func convertID(id asn1.ObjectIdentifier) []int32 {
return nid
}

func Req(parent string, pemBytes []byte, cert *x509.Certificate) (*privatecapb.CreateCertificateRequest, error) {
func Req(parent, certAuthority string, pemBytes []byte, cert *x509.Certificate) (*privatecapb.CreateCertificateRequest, error) {
pubkeyFormat, err := getPubKeyFormat(pemBytes)
if err != nil {
return nil, err
Expand All @@ -119,7 +127,7 @@ func Req(parent string, pemBytes []byte, cert *x509.Certificate) (*privatecapb.C
})
}

return &privatecapb.CreateCertificateRequest{
req := &privatecapb.CreateCertificateRequest{
Parent: parent,
Certificate: &privatecapb.Certificate{
Lifetime: durationpb.New(time.Until(cert.NotAfter)),
Expand All @@ -144,7 +152,13 @@ func Req(parent string, pemBytes []byte, cert *x509.Certificate) (*privatecapb.C
},
},
},
}, nil
}

if certAuthority != "" {
req.IssuingCertificateAuthorityId = certAuthority
}

return req, nil
}

func (c *CertAuthorityService) TrustBundle(ctx context.Context) ([][]*x509.Certificate, error) {
Expand Down Expand Up @@ -228,7 +242,7 @@ func (c *CertAuthorityService) CreateCertificate(ctx context.Context, principal
return nil, ca.ValidationError(err)
}

req, err := Req(c.caPoolParent, pubKeyBytes, cert)
req, err := Req(c.caPoolParent, c.certAuthority, pubKeyBytes, cert)
if err != nil {
return nil, ca.ValidationError(err)
}
Expand Down
77 changes: 76 additions & 1 deletion pkg/ca/googleca/v1/googleca_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,82 @@ func TestReq(t *testing.T) {
},
}

req, err := Req(parent, pubKeyBytes, cert)
req, err := Req(parent, "", pubKeyBytes, cert)
// We must copy over this field because we don't inject a clock, so
// lifetime will always be different.
expectedReq.Certificate.Lifetime = req.Certificate.Lifetime

if err != nil {
t.Fatalf("unexpected error, got: %v", err)
}
if !proto.Equal(req, expectedReq) {
t.Fatalf("proto equality failed, expected: %v, got: %v", req, expectedReq)
}
}

func TestReqCertAuthority(t *testing.T) {
parent := "parent-ca"
priv, err := rsa.GenerateKey(rand.Reader, 2048)
failErr(t, err)

uri := "sigstore.dev"
parsedURI, err := url.Parse(uri)
failErr(t, err)

emailAddress := "[email protected]"
notAfter := time.Now().Add(time.Minute * 10)
pubKeyBytes, err := cryptoutils.MarshalPublicKeyToPEM(priv.Public())
failErr(t, err)
ext := pkix.Extension{Id: asn1.ObjectIdentifier{1, 2, 3}, Value: []byte{1, 2, 3}}

cert := &x509.Certificate{
NotAfter: notAfter,
EmailAddresses: []string{emailAddress},
URIs: []*url.URL{parsedURI},
ExtraExtensions: []pkix.Extension{ext},
}

expectedReq := &privatecapb.CreateCertificateRequest{
Parent: parent,
IssuingCertificateAuthorityId: "cert-authority",
Certificate: &privatecapb.Certificate{
CertificateConfig: &privatecapb.Certificate_Config{
Config: &privatecapb.CertificateConfig{
PublicKey: &privatecapb.PublicKey{
Format: privatecapb.PublicKey_PEM,
Key: pubKeyBytes,
},
X509Config: &privatecapb.X509Parameters{
KeyUsage: &privatecapb.KeyUsage{
BaseKeyUsage: &privatecapb.KeyUsage_KeyUsageOptions{
DigitalSignature: true,
},
ExtendedKeyUsage: &privatecapb.KeyUsage_ExtendedKeyUsageOptions{
CodeSigning: true,
},
},
AdditionalExtensions: []*privatecapb.X509Extension{
{
ObjectId: &privatecapb.ObjectId{
ObjectIdPath: convertID(ext.Id),
},
Value: ext.Value,
},
},
},
SubjectConfig: &privatecapb.CertificateConfig_SubjectConfig{
Subject: &privatecapb.Subject{},
SubjectAltName: &privatecapb.SubjectAltNames{
EmailAddresses: []string{emailAddress},
Uris: []string{uri},
},
},
},
},
},
}

req, err := Req(parent, "cert-authority", pubKeyBytes, cert)
// We must copy over this field because we don't inject a clock, so
// lifetime will always be different.
expectedReq.Certificate.Lifetime = req.Certificate.Lifetime
Expand Down

0 comments on commit ecedc7b

Please sign in to comment.