From a5455d35728d8308a11c808994103bcc9163c412 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 10 Jan 2022 15:49:37 +0100 Subject: [PATCH 1/4] 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) From 0475a4d26ffdb243bb66eeaf81f7ed317658294f Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Wed, 12 Jan 2022 10:41:36 +0100 Subject: [PATCH 2/4] Refactor extraction of JSON template syntax errors --- authority/ssh.go | 5 ++--- authority/tls.go | 22 ++++++++++++++++++---- authority/tls_test.go | 2 +- 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/authority/ssh.go b/authority/ssh.go index e582eddd..4a67b28c 100644 --- a/authority/ssh.go +++ b/authority/ssh.go @@ -200,10 +200,9 @@ func (a *Authority) SignSSH(ctx context.Context, key ssh.PublicKey, opts provisi } // 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), + return nil, errs.InternalServerErr(templatingError(err), errs.WithKeyVal("signOptions", signOpts), + errs.WithMessage("error applying certificate template"), ) } return nil, errs.Wrap(http.StatusInternalServerError, err, "authority.SignSSH") diff --git a/authority/tls.go b/authority/tls.go index 6affd15d..d4838342 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -7,7 +7,9 @@ import ( "crypto/x509" "encoding/asn1" "encoding/base64" + "encoding/json" "encoding/pem" + "fmt" "net/http" "strings" "time" @@ -127,13 +129,12 @@ 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 + // 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), + return nil, errs.InternalServerErr(templatingError(err), errs.WithKeyVal("csr", csr), errs.WithKeyVal("signOptions", signOpts), + errs.WithMessage("error applying certificate template"), ) } return nil, errs.Wrap(http.StatusInternalServerError, err, "authority.Sign", opts...) @@ -559,3 +560,16 @@ func (a *Authority) GetTLSCertificate() (*tls.Certificate, error) { tlsCrt.Leaf = resp.Certificate return &tlsCrt, nil } + +// templatingError tries to extract more information about the cause of +// an error related to (most probably) malformed template data and adds +// this to the error message. +func templatingError(err error) error { + cause := errors.Cause(err) + var syntaxError *json.SyntaxError + if errors.As(err, &syntaxError) { + // offset is arguably not super clear to the user, but it's the best we can do here + cause = fmt.Errorf("%s at offset %d", cause.Error(), syntaxError.Offset) + } + return errors.Wrap(cause, "error applying certificate template") +} diff --git a/authority/tls_test.go b/authority/tls_test.go index 1fd3d6cb..bc0f0526 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -421,7 +421,7 @@ ZYtQ9Ot36qc= csr: csr, extraOpts: testExtraOpts, signOpts: signOpts, - err: errors.New("authority.Sign: failed to apply certificate template"), + err: errors.New("error applying certificate template"), code: http.StatusInternalServerError, } }, From a3cf6bac36c07cb45b6aaafa9f84a53bf26e384b Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Wed, 12 Jan 2022 11:15:39 +0100 Subject: [PATCH 3/4] Add special handling for *json.UnmarshalTypeError --- authority/ssh_test.go | 11 ++++-- .../{badjson.tpl => badjsonsyntax.tpl} | 0 authority/testdata/templates/badjsonvalue.tpl | 10 ++++++ authority/tls.go | 9 ++++- authority/tls_test.go | 35 +++++++++++++++++-- 5 files changed, 58 insertions(+), 7 deletions(-) rename authority/testdata/templates/{badjson.tpl => badjsonsyntax.tpl} (100%) create mode 100644 authority/testdata/templates/badjsonvalue.tpl diff --git a/authority/ssh_test.go b/authority/ssh_test.go index 523cfc8d..2ea65352 100644 --- a/authority/ssh_test.go +++ b/authority/ssh_test.go @@ -172,8 +172,12 @@ 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"}, + userJSONSyntaxErrorTemplateFile, err := provisioner.TemplateSSHOptions(&provisioner.Options{ + SSH: &provisioner.SSHOptions{TemplateFile: "./testdata/templates/badjsonsyntax.tpl"}, + }, sshutil.CreateTemplateData(sshutil.UserCert, "key-id", []string{"user"})) + assert.FatalError(t, err) + userJSONValueErrorTemplateFile, err := provisioner.TemplateSSHOptions(&provisioner.Options{ + SSH: &provisioner.SSHOptions{TemplateFile: "./testdata/templates/badjsonvalue.tpl"}, }, sshutil.CreateTemplateData(sshutil.UserCert, "key-id", []string{"user"})) assert.FatalError(t, err) @@ -226,7 +230,8 @@ 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}, + {"fail-custom-template-syntax-error-file", fields{signer, signer}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userJSONSyntaxErrorTemplateFile, userOptions}}, want{}, true}, + {"fail-custom-template-syntax-value-file", fields{signer, signer}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userJSONValueErrorTemplateFile, 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/badjsonsyntax.tpl similarity index 100% rename from authority/testdata/templates/badjson.tpl rename to authority/testdata/templates/badjsonsyntax.tpl diff --git a/authority/testdata/templates/badjsonvalue.tpl b/authority/testdata/templates/badjsonvalue.tpl new file mode 100644 index 00000000..107cde02 --- /dev/null +++ b/authority/testdata/templates/badjsonvalue.tpl @@ -0,0 +1,10 @@ +{ + "subject": 1, + "sans": {{ toJson .SANs }}, +{{- if typeIs "*rsa.PublicKey" .Insecure.CR.PublicKey }} + "keyUsage": ["keyEncipherment", "digitalSignature"], +{{- else }} + "keyUsage": ["digitalSignature"], +{{- end }} + "extKeyUsage": ["serverAuth", "clientAuth"] +} \ No newline at end of file diff --git a/authority/tls.go b/authority/tls.go index d4838342..78f629e7 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -566,10 +566,17 @@ func (a *Authority) GetTLSCertificate() (*tls.Certificate, error) { // this to the error message. func templatingError(err error) error { cause := errors.Cause(err) - var syntaxError *json.SyntaxError + var ( + syntaxError *json.SyntaxError + typeError *json.UnmarshalTypeError + ) if errors.As(err, &syntaxError) { // offset is arguably not super clear to the user, but it's the best we can do here cause = fmt.Errorf("%s at offset %d", cause.Error(), syntaxError.Offset) } + if errors.As(err, &typeError) { + // slightly rewriting the default error message to include the offset + cause = fmt.Errorf("cannot unmarshal %s at offset %d into Go value of type %s", typeError.Value, typeError.Offset, typeError.Type) + } return errors.Wrap(cause, "error applying certificate template") } diff --git a/authority/tls_test.go b/authority/tls_test.go index bc0f0526..3a0c999e 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -396,7 +396,7 @@ ZYtQ9Ot36qc= code: http.StatusBadRequest, } }, - "fail bad JSON template file": func(t *testing.T) *signTest { + "fail bad JSON syntax template file": func(t *testing.T) *signTest { csr := getCSR(t, priv) testAuthority := testAuthority(t) p, ok := testAuthority.provisioners.Load("step-cli:4UELJx8e0aS9m0CH3fZ0EB7D5aUPICb759zALHFejvc") @@ -405,7 +405,7 @@ ZYtQ9Ot36qc= } p.(*provisioner.JWK).Options = &provisioner.Options{ X509: &provisioner.X509Options{ - TemplateFile: "./testdata/templates/badjson.tpl", + TemplateFile: "./testdata/templates/badjsonsyntax.tpl", }, } testExtraOpts, err := testAuthority.Authorize(ctx, token) @@ -421,7 +421,36 @@ ZYtQ9Ot36qc= csr: csr, extraOpts: testExtraOpts, signOpts: signOpts, - err: errors.New("error applying certificate template"), + err: errors.New("error applying certificate template: invalid character"), + code: http.StatusInternalServerError, + } + }, + "fail bad JSON value 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/badjsonvalue.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("error applying certificate template: cannot unmarshal"), code: http.StatusInternalServerError, } }, From 50c3bce98d230a7a682b4f1f9bb7f7307ae2adbb Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Wed, 12 Jan 2022 21:34:38 +0100 Subject: [PATCH 4/4] Change if/if to if/else-if when checking the type of JSON error --- authority/tls.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/authority/tls.go b/authority/tls.go index 78f629e7..cc049655 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -573,8 +573,7 @@ func templatingError(err error) error { if errors.As(err, &syntaxError) { // offset is arguably not super clear to the user, but it's the best we can do here cause = fmt.Errorf("%s at offset %d", cause.Error(), syntaxError.Offset) - } - if errors.As(err, &typeError) { + } else if errors.As(err, &typeError) { // slightly rewriting the default error message to include the offset cause = fmt.Errorf("cannot unmarshal %s at offset %d into Go value of type %s", typeError.Value, typeError.Offset, typeError.Type) }