Skip to content

Commit 4607ace

Browse files
committed
transport/ca/localca: New(): return error instead of calling os.Exit(1)
This code was added in 56dfed7, but now has become the only use of github.com/kisom/goutils/assert, which previously was used in tests, and now is archived (moved to a new module). There were a couple of issues with this code; The `assert.NoError` appears to have a bug; it accepts optional arguments, but those are ignored; https://github.com/kisom/goutils/blob/v1.4.3/assert/assert.go#L90-L99 In this case, it meant that the additional information to describe the error won't be printed. Looking at the code (https://github.com/kisom/goutils/blob/v1.4.3/assert/assert.go#L35-L45), it defaults (`GOTRACEBACK` anything other than "crash") using `os.Exit(1)`. While (from the description), program execution MUST be terminated, there are some downsides to using `os.Exit` here, as it terminates execution immediately (which is desirable), but has no way to recover. While users should NOT use the result in this case, they still may want to catch this error (without terminating the program as a whole, which may be problematic if this module is used as part of a service). `os.Exit` also does not execute pending `defer` statements, which may still be desirable to handle state cleanup. This patch changes the function to return an error instead, allowing the caller to handle the error. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
1 parent cfd0e9a commit 4607ace

1 file changed

Lines changed: 10 additions & 4 deletions

File tree

transport/ca/localca/signer.go

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"crypto/x509"
88
"encoding/pem"
99
"errors"
10+
"fmt"
1011
"time"
1112

1213
"github.com/cloudflare/cfssl/config"
@@ -15,7 +16,6 @@ import (
1516
"github.com/cloudflare/cfssl/initca"
1617
"github.com/cloudflare/cfssl/signer"
1718
"github.com/cloudflare/cfssl/signer/local"
18-
"github.com/kisom/goutils/assert"
1919
)
2020

2121
// CA is a local transport CertificateAuthority that is useful for
@@ -146,13 +146,19 @@ func New(req *csr.CertificateRequest, profiles *config.Signing) (*CA, error) {
146146
// CFSSL has become inconsistent, and it can't be trusted.
147147

148148
priv, err := helpers.ParsePrivateKeyPEM(keyPEM)
149-
assert.NoError(err, "CFSSL-generated private key can't be parsed")
149+
if err != nil {
150+
return nil, fmt.Errorf("CFSSL-generated private key can't be parsed: %w", err)
151+
}
150152

151153
cert, err := helpers.ParseCertificatePEM(certPEM)
152-
assert.NoError(err, "CFSSL-generated certificate can't be parsed")
154+
if err != nil {
155+
return nil, fmt.Errorf("CFSSL-generated private key can't be parsed: %w", err)
156+
}
153157

154158
s, err := local.NewSigner(priv, cert, helpers.SignerAlgo(priv), profiles)
155-
assert.NoError(err, "a signer could not be constructed")
159+
if err != nil {
160+
return nil, fmt.Errorf("a signer could not be constructed: %w", err)
161+
}
156162

157163
return NewFromSigner(s), nil
158164
}

0 commit comments

Comments
 (0)