From a5455d35728d8308a11c808994103bcc9163c412 Mon Sep 17 00:00:00 2001
From: Herman Slatman <hermanslatman@hotmail.com>
Date: Mon, 10 Jan 2022 15:49:37 +0100
Subject: [PATCH] Improve errors related to template execution failures
 (slightly)

---
 authority/ssh.go                         |  8 +++++++
 authority/ssh_test.go                    |  5 ++++
 authority/testdata/templates/badjson.tpl |  3 +++
 authority/tls.go                         | 10 ++++++++
 authority/tls_test.go                    | 29 ++++++++++++++++++++++++
 5 files changed, 55 insertions(+)
 create mode 100644 authority/testdata/templates/badjson.tpl

diff --git a/authority/ssh.go b/authority/ssh.go
index 7b8f26e2..e582eddd 100644
--- a/authority/ssh.go
+++ b/authority/ssh.go
@@ -198,6 +198,14 @@ func (a *Authority) SignSSH(ctx context.Context, key ssh.PublicKey, opts provisi
 				errs.WithKeyVal("signOptions", signOpts),
 			)
 		}
+		// explicitly check for unmarshaling errors, which are most probably caused by JSON template syntax errors
+		if strings.HasPrefix(err.Error(), "error unmarshaling certificate") {
+			msg := strings.TrimSpace(strings.TrimPrefix(err.Error(), "error unmarshaling certificate:"))
+			return nil, errs.ApplyOptions(
+				errs.InternalServer("authority.Sign: failed to apply certificate template: %s", msg),
+				errs.WithKeyVal("signOptions", signOpts),
+			)
+		}
 		return nil, errs.Wrap(http.StatusInternalServerError, err, "authority.SignSSH")
 	}
 
diff --git a/authority/ssh_test.go b/authority/ssh_test.go
index b0907a79..523cfc8d 100644
--- a/authority/ssh_test.go
+++ b/authority/ssh_test.go
@@ -172,6 +172,10 @@ func TestAuthority_SignSSH(t *testing.T) {
 		SSH: &provisioner.SSHOptions{Template: `{{ fail "an error"}}`},
 	}, sshutil.CreateTemplateData(sshutil.UserCert, "key-id", []string{"user"}))
 	assert.FatalError(t, err)
+	userFailTemplateFile, err := provisioner.TemplateSSHOptions(&provisioner.Options{
+		SSH: &provisioner.SSHOptions{TemplateFile: "./testdata/templates/badjson.tpl"},
+	}, sshutil.CreateTemplateData(sshutil.UserCert, "key-id", []string{"user"}))
+	assert.FatalError(t, err)
 
 	now := time.Now()
 
@@ -222,6 +226,7 @@ func TestAuthority_SignSSH(t *testing.T) {
 		{"fail-no-host-key", fields{signer, nil}, args{pub, provisioner.SignSSHOptions{CertType: "host"}, []provisioner.SignOption{hostTemplate}}, want{}, true},
 		{"fail-bad-type", fields{signer, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, sshTestModifier{CertType: 100}}}, want{}, true},
 		{"fail-custom-template", fields{signer, signer}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userFailTemplate, userOptions}}, want{}, true},
+		{"fail-custom-template-file", fields{signer, signer}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userFailTemplateFile, userOptions}}, want{}, true},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
diff --git a/authority/testdata/templates/badjson.tpl b/authority/testdata/templates/badjson.tpl
new file mode 100644
index 00000000..1088f142
--- /dev/null
+++ b/authority/testdata/templates/badjson.tpl
@@ -0,0 +1,3 @@
+{
+    "subject": "badjson.localhost,
+}
\ No newline at end of file
diff --git a/authority/tls.go b/authority/tls.go
index dfa88ac3..6affd15d 100644
--- a/authority/tls.go
+++ b/authority/tls.go
@@ -9,6 +9,7 @@ import (
 	"encoding/base64"
 	"encoding/pem"
 	"net/http"
+	"strings"
 	"time"
 
 	"github.com/pkg/errors"
@@ -126,6 +127,15 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Sign
 				errs.WithKeyVal("signOptions", signOpts),
 			)
 		}
+		// explicitly check for unmarshaling errors, which are most probably caused by JSON template syntax errors
+		if strings.HasPrefix(err.Error(), "error unmarshaling certificate") {
+			msg := strings.TrimSpace(strings.TrimPrefix(err.Error(), "error unmarshaling certificate:"))
+			return nil, errs.ApplyOptions(
+				errs.InternalServer("authority.Sign: failed to apply certificate template: %s", msg),
+				errs.WithKeyVal("csr", csr),
+				errs.WithKeyVal("signOptions", signOpts),
+			)
+		}
 		return nil, errs.Wrap(http.StatusInternalServerError, err, "authority.Sign", opts...)
 	}
 
diff --git a/authority/tls_test.go b/authority/tls_test.go
index 3acf05f5..1fd3d6cb 100644
--- a/authority/tls_test.go
+++ b/authority/tls_test.go
@@ -396,6 +396,35 @@ ZYtQ9Ot36qc=
 				code:      http.StatusBadRequest,
 			}
 		},
+		"fail bad JSON template file": func(t *testing.T) *signTest {
+			csr := getCSR(t, priv)
+			testAuthority := testAuthority(t)
+			p, ok := testAuthority.provisioners.Load("step-cli:4UELJx8e0aS9m0CH3fZ0EB7D5aUPICb759zALHFejvc")
+			if !ok {
+				t.Fatal("provisioner not found")
+			}
+			p.(*provisioner.JWK).Options = &provisioner.Options{
+				X509: &provisioner.X509Options{
+					TemplateFile: "./testdata/templates/badjson.tpl",
+				},
+			}
+			testExtraOpts, err := testAuthority.Authorize(ctx, token)
+			assert.FatalError(t, err)
+			testAuthority.db = &db.MockAuthDB{
+				MStoreCertificate: func(crt *x509.Certificate) error {
+					assert.Equals(t, crt.Subject.CommonName, "smallstep test")
+					return nil
+				},
+			}
+			return &signTest{
+				auth:      testAuthority,
+				csr:       csr,
+				extraOpts: testExtraOpts,
+				signOpts:  signOpts,
+				err:       errors.New("authority.Sign: failed to apply certificate template"),
+				code:      http.StatusInternalServerError,
+			}
+		},
 		"ok": func(t *testing.T) *signTest {
 			csr := getCSR(t, priv)
 			_a := testAuthority(t)