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

googleca: close certificate authority client when done #930

Merged
merged 3 commits into from
Dec 19, 2022
Merged

googleca: close certificate authority client when done #930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3bc466f
googleca: close connections when done
hectorj2f Dec 19, 2022
a830a39
chore: close the client after the switch
hectorj2f Dec 19, 2022
179acd2
implement close in baseca
hectorj2f Dec 19, 2022
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/app/http_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,3 +119,7 @@ func (tca *TrivialCertificateAuthority) CreateCertificate(context.Context, ident
func (tca *TrivialCertificateAuthority) TrustBundle(ctx context.Context) ([][]*x509.Certificate, error) {
return [][]*x509.Certificate{}, nil
}

func (tca *TrivialCertificateAuthority) Close() error {
return nil
}
1 change: 1 addition & 0 deletions cmd/app/serve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ func runServeCmd(cmd *cobra.Command, args []string) {
if err != nil {
log.Logger.Fatal(err)
}
defer baseca.Close()

var ctClient *ctclient.LogClient
if logURL := viper.GetString("ct-log-url"); logURL != "" {
Expand Down
5 changes: 5 additions & 0 deletions cmd/fetch_ca_cert/fetch_ca_cert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,19 +188,24 @@ func main() {

client, err := privateca.NewCertificateAuthorityClient(context.Background())
if err != nil {
client.Close()
hectorj2f marked this conversation as resolved.
Show resolved Hide resolved
log.Fatal(err)
}
parsedCerts, err := fetchCACertificate(context.Background(), *gcpCaParent, *kmsKey, *tinkKeysetPath, *tinkKmsKey, client)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can probably just put the defer client.Close() call here after the error check once and then remove all the other calls to client.Close()

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC the problem is that log.Fatal ends execution immediately so the deferral doesn't happen. I think the linter complains about the lack of closure in that case. I ran into this with closing files in other tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is due to the nature of log.Fatal. It won't run the defer.

if err != nil {
client.Close()
log.Fatal(err)
}
pemCerts, err := cryptoutils.MarshalCertificatesToPEM(parsedCerts)
if err != nil {
client.Close()
log.Fatal(err)
}

err = os.WriteFile(*outputPath, pemCerts, 0600)
if err != nil {
client.Close()
log.Fatal(err)
}
defer client.Close()
}
4 changes: 4 additions & 0 deletions pkg/ca/baseca/baseca.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,3 +143,7 @@ func (bca *BaseCA) TrustBundle(ctx context.Context) ([][]*x509.Certificate, erro
certs, _ := bca.GetSignerWithChain()
return [][]*x509.Certificate{certs}, nil
}

func (bca *BaseCA) Close() error {
return nil
}
1 change: 1 addition & 0 deletions pkg/ca/ca.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,5 @@ import (
type CertificateAuthority interface {
CreateCertificate(context.Context, identity.Principal, crypto.PublicKey) (*CodeSigningCertificate, error)
TrustBundle(ctx context.Context) ([][]*x509.Certificate, error)
Close() error
}
4 changes: 4 additions & 0 deletions pkg/ca/googleca/v1/googleca.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ func NewCertAuthorityService(ctx context.Context, parent string, opts ...option.
return &c, nil
}

func (c *CertAuthorityService) Close() error {
return c.client.Close()
}

// getPubKeyFormat Returns the PublicKey KeyFormat required by gcp privateca.
// https://pkg.go.dev/google.golang.org/genproto/googleapis/cloud/security/privateca/v1#PublicKey_KeyType
func getPubKeyFormat(pemBytes []byte) (privatecapb.PublicKey_KeyFormat, error) {
Expand Down
4 changes: 4 additions & 0 deletions pkg/server/grpc_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1542,3 +1542,7 @@ func (fca *FailingCertificateAuthority) CreateCertificate(context.Context, ident
func (fca *FailingCertificateAuthority) TrustBundle(ctx context.Context) ([][]*x509.Certificate, error) {
return nil, errors.New("TrustBundle always fails for testing")
}

func (fca *FailingCertificateAuthority) Close() error {
return nil
}