Fix certificate inspect (#1153)

* Pull in updated go.step.sm/crypto with improved parsing for PEM
  formatted CSR and certificate files
  - Allows for ignoring extraneous data in PEM files

* go mod tidy

* Removed unused derToPemBlock helper function

* Update changelog | allow certificate inspect to output CSR in PEM format

* Add unit tests for inspectCertificateRequest

Author: dopey

CHANGELOG.md

@@ -26,6 +26,17 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ---
 
+## [unreleased] - aaaa-bb-cc
+
+### Added
+
+- Ability to output inspected CSR in PEM format (smallstep/cli#1153)
+
+### Fixed
+
+- Allow 'certificate inspect' to parse PEM files containig extraneous data (smallstep/cli#1153)
+
+
 ## [v0.26.0] - 2024-03-27
 
 ### Added

command/certificate/inspect.go

@@ -1,7 +1,6 @@
 package certificate
 
 import (
-	"bytes"
 	"crypto/x509"
 	"encoding/json"
 	"encoding/pem"
@@ -12,10 +11,10 @@ import (
 	"github.com/pkg/errors"
 	"github.com/smallstep/certinfo"
 	"github.com/smallstep/cli/flags"
-	"github.com/smallstep/cli/utils"
 	zx509 "github.com/smallstep/zcrypto/x509"
 	"github.com/urfave/cli"
 	"go.step.sm/cli-utils/errs"
+	"go.step.sm/crypto/pemutil"
 )
 
 func inspectCommand() cli.Command {
@@ -26,7 +25,7 @@ func inspectCommand() cli.Command {
 		UsageText: `**step certificate inspect** <crt-file>
 [**--bundle**] [**--short**] [**--format**=<format>] [**--roots**=<root-bundle>]
 [**--servername**=<servername>]`,
-		Description: `**step certificate inspect** prints the details of the 
+		Description: `**step certificate inspect** prints the details of the
 certificate or CSR in a human- or machine-readable format. Beware: Local certificates
 are never verified. Always verify a certificate (using **step certificate verify**)
 before relying on the output of this command.
@@ -206,9 +205,6 @@ func inspectAction(ctx *cli.Context) error {
 		return errs.IncompatibleFlagWithFlag(ctx, "short", "format json")
 	}
 
-	var block *pem.Block
-	var blocks []*pem.Block
-
 	switch addr, isURL, err := trimURL(crtFile); {
 	case err != nil:
 		return err
@@ -217,67 +213,35 @@ func inspectAction(ctx *cli.Context) error {
 		if err != nil {
 			return err
 		}
-		for _, crt := range peerCertificates {
-			blocks = append(blocks, &pem.Block{
-				Type:  "CERTIFICATE",
-				Bytes: crt.Raw,
-			})
-		}
+		return inspectCertificates(ctx, peerCertificates, os.Stdout)
 	default: // is not URL
-		crtBytes, err := utils.ReadFile(crtFile)
-		if err != nil {
-			return errs.FileError(err, crtFile)
-		}
-		if bytes.Contains(crtBytes, []byte("-----BEGIN ")) {
-			for len(crtBytes) > 0 {
-				block, crtBytes = pem.Decode(crtBytes)
-				if block == nil {
-					break
-				}
-				if bundle && block.Type != "CERTIFICATE" {
-					return errors.Errorf("certificate bundle %q contains an unexpected PEM block of type %q\n\n  expected type: CERTIFICATE",
-						crtFile, block.Type)
-				}
-				blocks = append(blocks, block)
+		var pemError *pemutil.InvalidPEMError
+		crts, err := pemutil.ReadCertificateBundle(crtFile)
+		switch {
+		case errors.As(err, &pemError) && pemError.Type == pemutil.PEMTypeCertificate:
+			csr, err := pemutil.ReadCertificateRequest(crtFile)
+			if err != nil {
+				return errors.Errorf("file %s does not contain any valid CERTIFICATE or CERTIFICATE REQUEST blocks", crtFile)
 			}
-		} else {
-			if block = derToPemBlock(crtBytes); block == nil {
-				return errors.Errorf("%q contains an invalid PEM block", crtFile)
+			return inspectCertificateRequest(ctx, csr, os.Stdout)
+		case err != nil:
+			return err
+		default:
+			if bundle {
+				return inspectCertificates(ctx, crts, os.Stdout)
 			}
-			blocks = append(blocks, block)
-		}
-
-		// prevent index out of range errors
-		if len(blocks) == 0 {
-			return fmt.Errorf("%q does not contain valid PEM blocks", crtFile)
+			return inspectCertificates(ctx, crts[:1], os.Stdout)
 		}
 	}
-
-	// Keep the first one if !bundle
-	if !bundle {
-		blocks = []*pem.Block{blocks[0]}
-	}
-
-	switch blocks[0].Type {
-	case "CERTIFICATE":
-		return inspectCertificates(ctx, blocks, os.Stdout)
-	case "CERTIFICATE REQUEST", "NEW CERTIFICATE REQUEST": // only one is supported
-		return inspectCertificateRequest(ctx, blocks[0])
-	default:
-		return errors.Errorf("Invalid PEM type in %q. Expected [CERTIFICATE|CERTIFICATE REQUEST] but got %q)", crtFile, block.Type)
-	}
 }
 
-func inspectCertificates(ctx *cli.Context, blocks []*pem.Block, w io.Writer) error {
+func inspectCertificates(ctx *cli.Context, crts []*x509.Certificate, w io.Writer) error {
+	var err error
 	format, short := ctx.String("format"), ctx.Bool("short")
 	switch format {
 	case "text":
 		var text string
-		for _, block := range blocks {
-			crt, err := x509.ParseCertificate(block.Bytes)
-			if err != nil {
-				return errors.WithStack(err)
-			}
+		for _, crt := range crts {
 			if short {
 				if text, err = certinfo.CertificateShortText(crt); err != nil {
 					return err
@@ -292,16 +256,16 @@ func inspectCertificates(ctx *cli.Context, blocks []*pem.Block, w io.Writer) err
 		return nil
 	case "json":
 		var v interface{}
-		if len(blocks) == 1 {
-			zcrt, err := zx509.ParseCertificate(blocks[0].Bytes)
+		if len(crts) == 1 {
+			zcrt, err := zx509.ParseCertificate(crts[0].Raw)
 			if err != nil {
 				return errors.WithStack(err)
 			}
 			v = struct{ *zx509.Certificate }{zcrt}
 		} else {
 			var zcrts []*zx509.Certificate
-			for _, block := range blocks {
-				zcrt, err := zx509.ParseCertificate(block.Bytes)
+			for _, crt := range crts {
+				zcrt, err := zx509.ParseCertificate(crt.Raw)
 				if err != nil {
 					return errors.WithStack(err)
 				}
@@ -317,8 +281,8 @@ func inspectCertificates(ctx *cli.Context, blocks []*pem.Block, w io.Writer) err
 		}
 		return nil
 	case "pem":
-		for _, block := range blocks {
-			err := pem.Encode(w, block)
+		for _, crt := range crts {
+			err := pem.Encode(w, &pem.Block{Type: "CERTIFICATE", Bytes: crt.Raw})
 			if err != nil {
 				return errors.WithStack(err)
 			}
@@ -329,15 +293,12 @@ func inspectCertificates(ctx *cli.Context, blocks []*pem.Block, w io.Writer) err
 	}
 }
 
-func inspectCertificateRequest(ctx *cli.Context, block *pem.Block) error {
+func inspectCertificateRequest(ctx *cli.Context, csr *x509.CertificateRequest, w io.Writer) error {
+	var err error
 	format, short := ctx.String("format"), ctx.Bool("short")
 	switch format {
 	case "text":
 		var text string
-		csr, err := x509.ParseCertificateRequest(block.Bytes)
-		if err != nil {
-			return errors.WithStack(err)
-		}
 		if short {
 			text, err = certinfo.CertificateRequestShortText(csr)
 			if err != nil {
@@ -349,35 +310,26 @@ func inspectCertificateRequest(ctx *cli.Context, block *pem.Block) error {
 				return err
 			}
 		}
-		fmt.Print(text)
+		fmt.Fprint(w, text)
 		return nil
 	case "json":
-		zcsr, err := zx509.ParseCertificateRequest(block.Bytes)
+		zcsr, err := zx509.ParseCertificateRequest(csr.Raw)
 		if err != nil {
 			return errors.WithStack(err)
 		}
-		b, err := json.MarshalIndent(struct {
-			*zx509.CertificateRequest
-		}{zcsr}, "", "  ")
+		enc := json.NewEncoder(w)
+		enc.SetIndent("", "  ")
+		if err := enc.Encode(zcsr); err != nil {
+			return errors.WithStack(err)
+		}
+		return nil
+	case "pem":
+		err := pem.Encode(w, &pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csr.Raw})
 		if err != nil {
 			return errors.WithStack(err)
 		}
-		os.Stdout.Write(b)
 		return nil
 	default:
 		return errs.InvalidFlagValue(ctx, "format", format, "text, json")
 	}
 }
-
-// derToPemBlock attempts to parse the ASN.1 data as a certificate or a
-// certificate request, returning a pem.Block of the one that succeeds. Returns
-// nil if it cannot parse the data.
-func derToPemBlock(b []byte) *pem.Block {
-	if _, err := x509.ParseCertificate(b); err == nil {
-		return &pem.Block{Type: "CERTIFICATE", Bytes: b}
-	}
-	if _, err := x509.ParseCertificateRequest(b); err == nil {
-		return &pem.Block{Type: "CERTIFICATE REQUEST", Bytes: b}
-	}
-	return nil
-}

command/certificate/inspect_test.go

@@ -3,12 +3,12 @@ package certificate
 import (
 	"bytes"
 	"encoding/json"
-	"encoding/pem"
 	"flag"
 	"testing"
 
 	"github.com/smallstep/assert"
 	"github.com/urfave/cli"
+	"go.step.sm/crypto/pemutil"
 )
 
 var pemData = []byte(`-----BEGIN CERTIFICATE-----
@@ -39,9 +39,8 @@ func TestInspectCertificates(t *testing.T) {
 	_ = set.String("format", "", "")
 	ctx := cli.NewContext(app, set, nil)
 
-	var blocks []*pem.Block
-	block, _ := pem.Decode(pemData)
-	blocks = append(blocks, block)
+	certs, err := pemutil.ParseCertificateBundle(pemData)
+	assert.FatalError(t, err)
 
 	type testCase struct {
 		format string
@@ -72,7 +71,65 @@ func TestInspectCertificates(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			var buf bytes.Buffer
 			ctx.Set("format", tc.format)
-			err := inspectCertificates(ctx, blocks, &buf)
+			err := inspectCertificates(ctx, certs, &buf)
+			assert.NoError(t, err)
+			if err == nil {
+				tc.verify(&buf)
+			}
+		})
+	}
+
+}
+
+var csrPEMData = []byte(`-----BEGIN CERTIFICATE REQUEST-----
+MIHmMIGNAgEAMAAwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAASGlyI2t5ibpcG+
+hGm0JMW0or/QphyTlc4GGAccapsz4BeXkNucKpeX3nupFbbABHLcN/bjxL87Ims8
+jz5sdl6xoCswKQYJKoZIhvcNAQkOMRwwGjAYBgNVHREEETAPggNmb2+CA2JhcoID
+YmF6MAoGCCqGSM49BAMCA0gAMEUCIEuWM0UdEeDfvWqssxyoY4cUuv++FrmA97j+
+Fbp7Kk6gAiEAuoyrBIvX28Spmeog9Jl4iBJYzceSNz8a7crRNGLTyjs=
+-----END CERTIFICATE REQUEST-----
+`)
+
+func TestInspectCertificateRequest(t *testing.T) {
+	// This is just to get a simple CLI context
+	app := &cli.App{}
+	set := flag.NewFlagSet("contrive", 0)
+	_ = set.String("format", "", "")
+	ctx := cli.NewContext(app, set, nil)
+
+	csr, err := pemutil.ParseCertificateRequest(csrPEMData)
+	assert.FatalError(t, err)
+
+	type testCase struct {
+		format string
+		verify func(buf *bytes.Buffer)
+	}
+
+	tests := map[string]testCase{
+		"format text": {"text",
+			func(buf *bytes.Buffer) {
+				assert.HasPrefix(t, buf.String(), "Certificate Request:")
+			},
+		},
+		"format json": {"json",
+			func(buf *bytes.Buffer) {
+				var v interface{}
+				err := json.Unmarshal(buf.Bytes(), &v)
+				assert.NoError(t, err)
+			},
+		},
+		"format pem": {"pem",
+			func(buf *bytes.Buffer) {
+				assert.Equals(t, string(csrPEMData), buf.String())
+			},
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			var buf bytes.Buffer
+			ctx.Set("format", tc.format)
+			err := inspectCertificateRequest(ctx, csr, &buf)
 			assert.NoError(t, err)
 			if err == nil {
 				tc.verify(&buf)

go.mod

@@ -26,7 +26,7 @@ require (
 	github.com/urfave/cli v1.22.14
 	go.mozilla.org/pkcs7 v0.0.0-20210826202110-33d05740a352
 	go.step.sm/cli-utils v0.9.0
-	go.step.sm/crypto v0.44.6
+	go.step.sm/crypto v0.44.7
 	go.step.sm/linkedca v0.20.1
 	golang.org/x/crypto v0.22.0
 	golang.org/x/sys v0.19.0

go.sum

@@ -476,8 +476,8 @@ go.opentelemetry.io/otel/trace v1.24.0 h1:CsKnnL4dUAr/0llH9FKuc698G04IrpWV0MQA/Y
 go.opentelemetry.io/otel/trace v1.24.0/go.mod h1:HPc3Xr/cOApsBI154IU0OI0HJexz+aw5uPdbs3UCjNU=
 go.step.sm/cli-utils v0.9.0 h1:55jYcsQbnArNqepZyAwcato6Zy2MoZDRkWW+jF+aPfQ=
 go.step.sm/cli-utils v0.9.0/go.mod h1:Y/CRoWl1FVR9j+7PnAewufAwKmBOTzR6l9+7EYGAnp8=
-go.step.sm/crypto v0.44.6 h1:vQg8ujce7fNXDO8EWdriSz+ZSJpYnNh22QrFtRjdyoY=
-go.step.sm/crypto v0.44.6/go.mod h1:oKRO4jaf2MaCohJDN+/8ShImkvIgUKfJxxy87gqsnXs=
+go.step.sm/crypto v0.44.7 h1:aJ7dVbkm5TxEtHbicgN6JEVzPxZlp9JW9RQQH5bpi/o=
+go.step.sm/crypto v0.44.7/go.mod h1:oKRO4jaf2MaCohJDN+/8ShImkvIgUKfJxxy87gqsnXs=
 go.step.sm/linkedca v0.20.1 h1:bHDn1+UG1NgRrERkWbbCiAIvv4lD5NOFaswPDTyO5vU=
 go.step.sm/linkedca v0.20.1/go.mod h1:Vaq4+Umtjh7DLFI1KuIxeo598vfBzgSYZUjgVJ7Syxw=
 go.uber.org/atomic v1.3.2/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE=