From 4607acedb3dfc22ea832dcfa1bddfbb5f800d3e6 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 20 Nov 2022 12:18:23 +0100 Subject: [PATCH 1/4] transport/ca/localca: New(): return error instead of calling os.Exit(1) This code was added in 56dfed7c82597110116d549dc1c990a5e24d1db7, 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 --- transport/ca/localca/signer.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/transport/ca/localca/signer.go b/transport/ca/localca/signer.go index b08e8d82a..96202f52d 100644 --- a/transport/ca/localca/signer.go +++ b/transport/ca/localca/signer.go @@ -7,6 +7,7 @@ import ( "crypto/x509" "encoding/pem" "errors" + "fmt" "time" "github.com/cloudflare/cfssl/config" @@ -15,7 +16,6 @@ import ( "github.com/cloudflare/cfssl/initca" "github.com/cloudflare/cfssl/signer" "github.com/cloudflare/cfssl/signer/local" - "github.com/kisom/goutils/assert" ) // CA is a local transport CertificateAuthority that is useful for @@ -146,13 +146,19 @@ func New(req *csr.CertificateRequest, profiles *config.Signing) (*CA, error) { // CFSSL has become inconsistent, and it can't be trusted. priv, err := helpers.ParsePrivateKeyPEM(keyPEM) - assert.NoError(err, "CFSSL-generated private key can't be parsed") + if err != nil { + return nil, fmt.Errorf("CFSSL-generated private key can't be parsed: %w", err) + } cert, err := helpers.ParseCertificatePEM(certPEM) - assert.NoError(err, "CFSSL-generated certificate can't be parsed") + if err != nil { + return nil, fmt.Errorf("CFSSL-generated private key can't be parsed: %w", err) + } s, err := local.NewSigner(priv, cert, helpers.SignerAlgo(priv), profiles) - assert.NoError(err, "a signer could not be constructed") + if err != nil { + return nil, fmt.Errorf("a signer could not be constructed: %w", err) + } return NewFromSigner(s), nil } From 5937a3f33738b1be432168d46b8dc818fe141724 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 20 Nov 2022 12:27:14 +0100 Subject: [PATCH 2/4] transport/ca/localca: remove uses of github.com/kisom/goutils/assert This package was the only package using this assertion library. Looking for a replacement, all packages (except for one) in this repository were not using an assertion library, so replacing it with standard checks. Signed-off-by: Sebastiaan van Stijn --- go.mod | 1 - go.sum | 2 - transport/ca/localca/signer_test.go | 75 ++++++-- vendor/github.com/kisom/goutils/LICENSE | 13 -- .../github.com/kisom/goutils/assert/assert.go | 174 ------------------ vendor/modules.txt | 3 - 6 files changed, 55 insertions(+), 213 deletions(-) delete mode 100644 vendor/github.com/kisom/goutils/LICENSE delete mode 100644 vendor/github.com/kisom/goutils/assert/assert.go diff --git a/go.mod b/go.mod index ae6de1b78..56e806e96 100644 --- a/go.mod +++ b/go.mod @@ -11,7 +11,6 @@ require ( github.com/jmhodges/clock v1.2.0 github.com/jmoiron/sqlx v1.3.3 github.com/kisielk/sqlstruct v0.0.0-20201105191214-5f3e10d3ab46 - github.com/kisom/goutils v1.4.3 github.com/lib/pq v1.10.1 github.com/mattn/go-sqlite3 v1.14.15 github.com/prometheus/client_golang v1.13.0 diff --git a/go.sum b/go.sum index 45aea313e..bd921ca0a 100644 --- a/go.sum +++ b/go.sum @@ -214,8 +214,6 @@ github.com/kataras/sitemap v0.0.5/go.mod h1:KY2eugMKiPwsJgx7+U103YZehfvNGOXURubc github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck= github.com/kisielk/sqlstruct v0.0.0-20201105191214-5f3e10d3ab46 h1:veS9QfglfvqAw2e+eeNT/SbGySq8ajECXJ9e4fPoLhY= github.com/kisielk/sqlstruct v0.0.0-20201105191214-5f3e10d3ab46/go.mod h1:yyMNCyc/Ib3bDTKd379tNMpB/7/H5TjM2Y9QJ5THLbE= -github.com/kisom/goutils v1.4.3 h1:N81mTXtO2LCpoqVtOrKthH5Abm0MknjX54QS8DmpQIk= -github.com/kisom/goutils v1.4.3/go.mod h1:Lp5qrquG7yhYnWzZCI/68Pa/GpFynw//od6EkGnWpac= github.com/klauspost/compress v1.8.2/go.mod h1:RyIbtBH6LamlWaDj8nUwkbUhJ87Yi3uG0guNDohfE1A= github.com/klauspost/compress v1.9.7/go.mod h1:RyIbtBH6LamlWaDj8nUwkbUhJ87Yi3uG0guNDohfE1A= github.com/klauspost/cpuid v1.2.1/go.mod h1:Pj4uuM528wm8OyEC2QMXAi2YiTZ96dNQPGgoMS4s3ek= diff --git a/transport/ca/localca/signer_test.go b/transport/ca/localca/signer_test.go index 8ac346375..142d912c7 100644 --- a/transport/ca/localca/signer_test.go +++ b/transport/ca/localca/signer_test.go @@ -2,6 +2,7 @@ package localca import ( "encoding/pem" + "errors" "io/ioutil" "os" "testing" @@ -11,7 +12,6 @@ import ( "github.com/cloudflare/cfssl/helpers" "github.com/cloudflare/cfssl/initca" "github.com/cloudflare/cfssl/selfsign" - "github.com/kisom/goutils/assert" ) func tempName() (string, error) { @@ -83,30 +83,46 @@ func TestEncodePEM(t *testing.T) { func TestLoadSigner(t *testing.T) { lca := &CA{} certPEM, csrPEM, keyPEM, err := initca.New(ExampleRequest()) - assert.NoErrorT(t, err) + if err != nil { + t.Fatal(err) + } _, err = lca.CACertificate() - assert.ErrorEqT(t, errNotSetup, err) + if !errors.Is(err, errNotSetup) { + t.Fatalf("expected an errNotSetup (%v), got: %v", errNotSetup, err) + } _, err = lca.SignCSR(csrPEM) - assert.ErrorEqT(t, errNotSetup, err) + if !errors.Is(err, errNotSetup) { + t.Fatalf("expected an errNotSetup (%v), got: %v", errNotSetup, err) + } lca.KeyFile, err = tempName() - assert.NoErrorT(t, err) + if err != nil { + t.Fatal(err) + } defer os.Remove(lca.KeyFile) lca.CertFile, err = tempName() - assert.NoErrorT(t, err) + if err != nil { + t.Fatal(err) + } defer os.Remove(lca.CertFile) err = ioutil.WriteFile(lca.KeyFile, keyPEM, 0644) - assert.NoErrorT(t, err) + if err != nil { + t.Fatal(err) + } err = ioutil.WriteFile(lca.CertFile, certPEM, 0644) - assert.NoErrorT(t, err) + if err != nil { + t.Fatal(err) + } err = Load(lca, ExampleSigningConfig()) - assert.NoErrorT(t, err) + if err != nil { + t.Fatal(err) + } } var testRequest = &csr.CertificateRequest{ @@ -121,33 +137,50 @@ var testRequest = &csr.CertificateRequest{ func TestNewSigner(t *testing.T) { req := ExampleRequest() lca, err := New(req, ExampleSigningConfig()) - assert.NoErrorT(t, err) + if err != nil { + t.Fatal(err) + } csrPEM, _, err := csr.ParseRequest(testRequest) - assert.NoErrorT(t, err) + if err != nil { + t.Fatal(err) + } certPEM, err := lca.SignCSR(csrPEM) - assert.NoErrorT(t, err) + if err != nil { + t.Fatal(err) + } _, err = helpers.ParseCertificatePEM(certPEM) - assert.NoErrorT(t, err) + if err != nil { + t.Fatal(err) + } certPEM, err = lca.CACertificate() - assert.NoErrorT(t, err) + if err != nil { + t.Fatal(err) + } cert, err := helpers.ParseCertificatePEM(certPEM) - assert.NoErrorT(t, err) + if err != nil { + t.Fatal(err) + } - assert.BoolT(t, cert.Subject.CommonName == req.CN, - "common names don't match") + if cert.Subject.CommonName != req.CN { + t.Fatalf("common names don't match: '%s' != '%s'", cert.Subject.CommonName, req.CN) + } lca.Toggle() _, err = lca.SignCSR(csrPEM) - assert.ErrorEqT(t, errDisabled, err) + if !errors.Is(err, errDisabled) { + t.Fatalf("expected an errDisabled (%v), got: %v", errDisabled, err) + } lca.Toggle() _, err = lca.SignCSR(certPEM) - assert.ErrorT(t, err, "shouldn't be able to sign non-CSRs") + if err == nil { + t.Fatal("shouldn't be able to sign non-CSRs") + } p := &pem.Block{ Type: "CERTIFICATE REQUEST", @@ -156,6 +189,8 @@ func TestNewSigner(t *testing.T) { junkCSR := pem.EncodeToMemory(p) _, err = lca.SignCSR(junkCSR) - assert.ErrorT(t, err, "signing a junk CSR should fail") + if err == nil { + t.Fatal("signing a junk CSR should fail") + } t.Logf("error: %s", err) } diff --git a/vendor/github.com/kisom/goutils/LICENSE b/vendor/github.com/kisom/goutils/LICENSE deleted file mode 100644 index fd546f9b6..000000000 --- a/vendor/github.com/kisom/goutils/LICENSE +++ /dev/null @@ -1,13 +0,0 @@ -Copyright (c) 2015 Kyle Isom - -Permission to use, copy, modify, and distribute this software for any -purpose with or without fee is hereby granted, provided that the above -copyright notice and this permission notice appear in all copies. - -THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES -WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF -MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR -ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES -WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN -ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF -OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. diff --git a/vendor/github.com/kisom/goutils/assert/assert.go b/vendor/github.com/kisom/goutils/assert/assert.go deleted file mode 100644 index aee0ee3f8..000000000 --- a/vendor/github.com/kisom/goutils/assert/assert.go +++ /dev/null @@ -1,174 +0,0 @@ -// Package assert provides C-like assertions for Go. For more -// information, see assert(3) (e.g. `man 3 assert`). -// -// The T variants operating on *testing.T values; instead of killing -// the program, they call the Fatal method. -// -// If GOTRACEBACK is set to enable coredumps, assertions will generate -// coredumps. -package assert - -import ( - "fmt" - "os" - "runtime" - "strings" - "testing" -) - -// NoDebug can be set to true to cause all asserts to be ignored. -var NoDebug bool - -func die(what string, a ...string) { - _, file, line, ok := runtime.Caller(2) - if !ok { - panic(what) - } - - if os.Getenv("GOTRACEBACK") == "crash" { - s := strings.Join(a, ", ") - if len(s) > 0 { - s = ": " + s - } - panic(what + s) - } else { - fmt.Fprintf(os.Stderr, "%s", what) - if len(a) > 0 { - s := strings.Join(a, ", ") - fmt.Fprintln(os.Stderr, ": "+s) - } else { - fmt.Fprintf(os.Stderr, "\n") - } - - fmt.Fprintf(os.Stderr, "\t%s line %d\n", file, line) - - os.Exit(1) - } -} - -// Bool asserts that cond is false. -// -// For example, this would replace -// if x < 0 { -// log.Fatal("x is subzero") -// } -// -// The same assertion would be -// assert.Bool(x, "x is subzero") -func Bool(cond bool, s ...string) { - if NoDebug { - return - } - - if !cond { - die("assert.Bool failed", s...) - } -} - -// Error asserts that err is not nil, e.g. that an error has occurred. -// -// For example, -// if err == nil { -// log.Fatal("call to should have failed") -// } -// // becomes -// assert.Error(err, "call to should have failed") -func Error(err error, s ...string) { - if NoDebug { - return - } else if nil != err { - return - } - - if len(s) == 0 { - die("error expected, but no error returned") - } else { - die(strings.Join(s, ", ")) - } -} - -// NoError asserts that err is nil, e.g. that no error has occurred. -func NoError(err error, s ...string) { - if NoDebug { - return - } - - if nil != err { - die(err.Error()) - } -} - -// ErrorEq asserts that the actual error is the expected error. -func ErrorEq(expected, actual error) { - if NoDebug || (expected == actual) { - return - } - - if expected == nil { - die(fmt.Sprintf("assert.ErrorEq: %s", actual.Error())) - } - - var should string - if actual == nil { - should = "no error was returned" - } else { - should = fmt.Sprintf("have '%s'", actual) - } - - die(fmt.Sprintf("assert.ErrorEq: expected '%s', but %s", expected, should)) -} - -// BoolT checks a boolean condition, calling Fatal on t if it is -// false. -func BoolT(t *testing.T, cond bool, s ...string) { - if !cond { - what := strings.Join(s, ", ") - if len(what) > 0 { - what = ": " + what - } - t.Fatalf("assert.Bool failed%s", what) - } -} - -// ErrorT asserts that err is not nil, e.g. asserting that an error -// has occurred. See also NoErrorT. -func ErrorT(t *testing.T, err error, s ...string) { - if nil != err { - return - } - - if len(s) == 0 { - t.Fatal("error expected, but no error returned") - } else { - t.Fatal(strings.Join(s, ", ")) - } -} - -// NoErrorT asserts that err is nil, e.g. asserting that no error has -// occurred. See also ErrorT. -func NoErrorT(t *testing.T, err error) { - if nil != err { - t.Fatalf("%s", err) - } -} - -// ErrorEqT compares a pair of errors, calling Fatal on it if they -// don't match. -func ErrorEqT(t *testing.T, expected, actual error) { - if NoDebug || (expected == actual) { - return - } - - if expected == nil { - die(fmt.Sprintf("assert.Error2: %s", actual.Error())) - } - - var should string - if actual == nil { - should = "no error was returned" - } else { - should = fmt.Sprintf("have '%s'", actual) - } - - die(fmt.Sprintf("assert.Error2: expected '%s', but %s", expected, should)) -} diff --git a/vendor/modules.txt b/vendor/modules.txt index 796dcba36..2744a3079 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -67,9 +67,6 @@ github.com/jmoiron/sqlx/types # github.com/kisielk/sqlstruct v0.0.0-20201105191214-5f3e10d3ab46 ## explicit github.com/kisielk/sqlstruct -# github.com/kisom/goutils v1.4.3 -## explicit; go 1.13 -github.com/kisom/goutils/assert # github.com/kr/text v0.2.0 ## explicit # github.com/kylelemons/go-gypsy v1.0.0 From b069c865435fd29bc93168a6832c3364b17a2147 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 20 Nov 2022 12:34:12 +0100 Subject: [PATCH 3/4] transport/ca/localca: remove unused testGenerateKeypair utility This was added in 56dfed7c82597110116d549dc1c990a5e24d1db7, but never used. Signed-off-by: Sebastiaan van Stijn --- transport/ca/localca/signer_test.go | 49 ----------------------------- 1 file changed, 49 deletions(-) diff --git a/transport/ca/localca/signer_test.go b/transport/ca/localca/signer_test.go index 142d912c7..dfd58b980 100644 --- a/transport/ca/localca/signer_test.go +++ b/transport/ca/localca/signer_test.go @@ -7,11 +7,9 @@ import ( "os" "testing" - "github.com/cloudflare/cfssl/config" "github.com/cloudflare/cfssl/csr" "github.com/cloudflare/cfssl/helpers" "github.com/cloudflare/cfssl/initca" - "github.com/cloudflare/cfssl/selfsign" ) func tempName() (string, error) { @@ -25,53 +23,6 @@ func tempName() (string, error) { return name, nil } -func testGenerateKeypair(req *csr.CertificateRequest) (keyFile, certFile string, err error) { - fail := func(err error) (string, string, error) { - if keyFile != "" { - os.Remove(keyFile) - } - if certFile != "" { - os.Remove(certFile) - } - return "", "", err - } - - keyFile, err = tempName() - if err != nil { - return fail(err) - } - - certFile, err = tempName() - if err != nil { - return fail(err) - } - - csrPEM, keyPEM, err := csr.ParseRequest(req) - if err != nil { - return fail(err) - } - - if err = ioutil.WriteFile(keyFile, keyPEM, 0644); err != nil { - return fail(err) - } - - priv, err := helpers.ParsePrivateKeyPEM(keyPEM) - if err != nil { - return fail(err) - } - - cert, err := selfsign.Sign(priv, csrPEM, config.DefaultConfig()) - if err != nil { - return fail(err) - } - - if err = ioutil.WriteFile(certFile, cert, 0644); err != nil { - return fail(err) - } - - return -} - func TestEncodePEM(t *testing.T) { p := &pem.Block{ Type: "CERTIFICATE REQUEST", From 0eecfe207f46635217e1e543e5638c9e88cad3db Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 20 Nov 2022 12:41:54 +0100 Subject: [PATCH 4/4] transport/ca/localca: remove uses of deprecated io/ioutil Using their replacements instead. Also making use of t.TempDir(), to let Go's testing take care of cleaning up. Signed-off-by: Sebastiaan van Stijn --- transport/ca/localca/signer_test.go | 31 ++++++----------------------- 1 file changed, 6 insertions(+), 25 deletions(-) diff --git a/transport/ca/localca/signer_test.go b/transport/ca/localca/signer_test.go index dfd58b980..754092cce 100644 --- a/transport/ca/localca/signer_test.go +++ b/transport/ca/localca/signer_test.go @@ -3,8 +3,8 @@ package localca import ( "encoding/pem" "errors" - "io/ioutil" "os" + "path/filepath" "testing" "github.com/cloudflare/cfssl/csr" @@ -12,17 +12,6 @@ import ( "github.com/cloudflare/cfssl/initca" ) -func tempName() (string, error) { - tmpf, err := ioutil.TempFile("", "transport_cachedkp_") - if err != nil { - return "", err - } - - name := tmpf.Name() - tmpf.Close() - return name, nil -} - func TestEncodePEM(t *testing.T) { p := &pem.Block{ Type: "CERTIFICATE REQUEST", @@ -48,24 +37,16 @@ func TestLoadSigner(t *testing.T) { t.Fatalf("expected an errNotSetup (%v), got: %v", errNotSetup, err) } - lca.KeyFile, err = tempName() - if err != nil { - t.Fatal(err) - } - defer os.Remove(lca.KeyFile) - - lca.CertFile, err = tempName() - if err != nil { - t.Fatal(err) - } - defer os.Remove(lca.CertFile) + tmpDir := t.TempDir() + lca.KeyFile = filepath.Join(tmpDir, "KeyFile") + lca.CertFile = filepath.Join(tmpDir, "CertFile") - err = ioutil.WriteFile(lca.KeyFile, keyPEM, 0644) + err = os.WriteFile(lca.KeyFile, keyPEM, 0644) if err != nil { t.Fatal(err) } - err = ioutil.WriteFile(lca.CertFile, certPEM, 0644) + err = os.WriteFile(lca.CertFile, certPEM, 0644) if err != nil { t.Fatal(err) }