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.

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

Fix bugs in googleca and update flag description #897

Merged
merged 5 commits into from
Nov 29, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
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>/<name> (only used with --ca googleca)")
haydentherapper marked this conversation as resolved.
Show resolved Hide resolved
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.")
haydentherapper marked this conversation as resolved.
Show resolved Hide resolved
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
82 changes: 70 additions & 12 deletions pkg/ca/googleca/v1/googleca.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,10 @@ import (
)

type CertAuthorityService struct {
parent string
client *privateca.CertificateAuthorityClient
certAuthority string
certAuthorityParent string
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd merge these two variables into one, caPool, since they represent the same resource. You can then check if certAuthority is set to decide whether or not to target a specific CA

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we do need both of them later on, the full certAuthorityParent in the GetCertificateAuthority request and just the name of the CA certAuthority when actually making the request for IssuingCertificateAuthorityId.

i think it's more understandable to do all the validation/string splitting logic at the beginning and then have everything we need later on when actually getting CA's/making requests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see my confusion, it's around terminology. I thought two referred to the same resource. I would use the terms from aip.dev to describe the resources and IDs here:

  • certAuthority -> certAuthorityID (since it represents just an ID)
  • certAuthorityParent -> certAuthorityResource (since it's the full resource string)
  • caPoolParent -> caPoolResource

Does that sound good? "parent" was what was confusing, since a parent of the a resource is a different resource.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that's fine. I'd still keep "parent" personally to follow the google naming standards for a googleca, but am fine with changing it too. It's updated now.

caPoolParent string
client *privateca.CertificateAuthorityClient

// protected by once
cachedRoots [][]*x509.Certificate
Expand All @@ -52,10 +54,26 @@ func NewCertAuthorityService(ctx context.Context, parent string, opts ...option.
if err != nil {
return nil, err
}
return &CertAuthorityService{
parent: parent,
c := CertAuthorityService{
client: client,
}, nil
}

if !strings.Contains(parent, "certificateAuthorities") {
c.caPoolParent = 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
haydentherapper marked this conversation as resolved.
Show resolved Hide resolved
}

// getPubKeyFormat Returns the PublicKey KeyFormat required by gcp privateca.
Expand All @@ -82,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 @@ -109,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 @@ -134,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 All @@ -143,10 +167,44 @@ func (c *CertAuthorityService) TrustBundle(ctx context.Context) ([][]*x509.Certi
return c.cachedRoots, nil
}

// fetch the latest values for the specified CA
// if a specific certificate authority was specified, use that one
if c.certAuthorityParent != "" {
return c.getCertificateAuthorityTrustBundle(ctx)
}
// otherwise, get certs from all of the CAs in the pool
return c.listCertificateAuthorityTrustBundle(ctx)
}

func (c *CertAuthorityService) getCertificateAuthorityTrustBundle(ctx context.Context) ([][]*x509.Certificate, error) {
var roots [][]*x509.Certificate
ca, err := c.client.GetCertificateAuthority(ctx, &privatecapb.GetCertificateAuthorityRequest{
Name: c.certAuthorityParent,
})
if err != nil {
return nil, err
}
// if we fail to parse the PEM content, return an error
caCerts, err := cryptoutils.LoadCertificatesFromPEM(strings.NewReader(strings.Join(ca.PemCaCertificates, "")))
if err != nil {
return [][]*x509.Certificate{}, fmt.Errorf("failed parsing PemCACertificates response: %w", err)
}
if len(caCerts) == 0 {
return [][]*x509.Certificate{}, fmt.Errorf("error fetching root certificates")
}
roots = append(roots, caCerts)

c.cachedRootsOnce.Do(func() {
c.cachedRoots = roots
})

return c.cachedRoots, nil
}

func (c *CertAuthorityService) listCertificateAuthorityTrustBundle(ctx context.Context) ([][]*x509.Certificate, error) {
// fetch the latest values for the specified CA pool
var roots [][]*x509.Certificate
cas := c.client.ListCertificateAuthorities(ctx, &privatecapb.ListCertificateAuthoritiesRequest{
Parent: c.parent,
Parent: c.caPoolParent,
})
for {
ca, done := cas.Next()
Expand All @@ -161,7 +219,7 @@ func (c *CertAuthorityService) TrustBundle(ctx context.Context) ([][]*x509.Certi
if err != nil {
return [][]*x509.Certificate{}, fmt.Errorf("failed parsing PemCACertificates response: %w", err)
}
if len(roots) == 0 {
if len(caCerts) == 0 {
return [][]*x509.Certificate{}, fmt.Errorf("error fetching root certificates")
}
roots = append(roots, caCerts)
Expand All @@ -184,7 +242,7 @@ func (c *CertAuthorityService) CreateCertificate(ctx context.Context, principal
return nil, ca.ValidationError(err)
}

req, err := Req(c.parent, 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