Skip to content

Commit 4f84cef

Browse files
marainomaraino
authored
Merge pull request smallstep#752 from smallstep/errors-bad-request
Bad request errors
2 parents d925bc6 + aa3fdf8 commit 4f84cef

25 files changed

+165
-124
lines changed

‎api/api.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -318,7 +318,7 @@ func certChainToPEM(certChain []*x509.Certificate) []Certificate {
318318
func (h *caHandler) Provisioners(w http.ResponseWriter, r *http.Request) {
319319
cursor, limit, err := ParseCursor(r)
320320
if err != nil {
321-
WriteError(w, errs.BadRequestErr(err))
321+
WriteError(w, err)
322322
return
323323
}
324324

@@ -435,7 +435,7 @@ func ParseCursor(r *http.Request) (cursor string, limit int, err error) {
435435
if v := q.Get("limit"); len(v) > 0 {
436436
limit, err = strconv.Atoi(v)
437437
if err != nil {
438-
return "", 0, errors.Wrapf(err, "error converting %s to integer", v)
438+
return "", 0, errs.BadRequestErr(err, "limit '%s' is not an integer", v)
439439
}
440440
}
441441
return

‎api/api_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1087,7 +1087,7 @@ func Test_caHandler_Provisioners(t *testing.T) {
10871087
t.Fatal(err)
10881088
}
10891089

1090-
expectedError400 := errs.BadRequest("force")
1090+
expectedError400 := errs.BadRequest("limit 'abc' is not an integer")
10911091
expectedError400Bytes, err := json.Marshal(expectedError400)
10921092
assert.FatalError(t, err)
10931093
expectedError500 := errs.InternalServer("force")

‎api/rekey.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,23 +18,22 @@ func (s *RekeyRequest) Validate() error {
1818
return errs.BadRequest("missing csr")
1919
}
2020
if err := s.CsrPEM.CertificateRequest.CheckSignature(); err != nil {
21-
return errs.Wrap(http.StatusBadRequest, err, "invalid csr")
21+
return errs.BadRequestErr(err, "invalid csr")
2222
}
2323

2424
return nil
2525
}
2626

2727
// Rekey is similar to renew except that the certificate will be renewed with new key from csr.
2828
func (h *caHandler) Rekey(w http.ResponseWriter, r *http.Request) {
29-
3029
if r.TLS == nil || len(r.TLS.PeerCertificates) == 0 {
31-
WriteError(w, errs.BadRequest("missing peer certificate"))
30+
WriteError(w, errs.BadRequest("missing client certificate"))
3231
return
3332
}
3433

3534
var body RekeyRequest
3635
if err := ReadJSON(r.Body, &body); err != nil {
37-
WriteError(w, errs.Wrap(http.StatusBadRequest, err, "error reading request body"))
36+
WriteError(w, errs.BadRequestErr(err, "error reading request body"))
3837
return
3938
}
4039

‎api/renew.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import (
1010
// new one.
1111
func (h *caHandler) Renew(w http.ResponseWriter, r *http.Request) {
1212
if r.TLS == nil || len(r.TLS.PeerCertificates) == 0 {
13-
WriteError(w, errs.BadRequest("missing peer certificate"))
13+
WriteError(w, errs.BadRequest("missing client certificate"))
1414
return
1515
}
1616

‎api/revoke.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ func (r *RevokeRequest) Validate() (err error) {
4949
func (h *caHandler) Revoke(w http.ResponseWriter, r *http.Request) {
5050
var body RevokeRequest
5151
if err := ReadJSON(r.Body, &body); err != nil {
52-
WriteError(w, errs.Wrap(http.StatusBadRequest, err, "error reading request body"))
52+
WriteError(w, errs.BadRequestErr(err, "error reading request body"))
5353
return
5454
}
5555

@@ -80,12 +80,12 @@ func (h *caHandler) Revoke(w http.ResponseWriter, r *http.Request) {
8080
// the client certificate Serial Number must match the serial number
8181
// being revoked.
8282
if r.TLS == nil || len(r.TLS.PeerCertificates) == 0 {
83-
WriteError(w, errs.BadRequest("missing ott or peer certificate"))
83+
WriteError(w, errs.BadRequest("missing ott or client certificate"))
8484
return
8585
}
8686
opts.Crt = r.TLS.PeerCertificates[0]
8787
if opts.Crt.SerialNumber.String() != opts.Serial {
88-
WriteError(w, errs.BadRequest("revoke: serial number in mtls certificate different than body"))
88+
WriteError(w, errs.BadRequest("serial number in client certificate different than body"))
8989
return
9090
}
9191
// TODO: should probably be checking if the certificate was revoked here.

‎api/sign.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ func (s *SignRequest) Validate() error {
2626
return errs.BadRequest("missing csr")
2727
}
2828
if err := s.CsrPEM.CertificateRequest.CheckSignature(); err != nil {
29-
return errs.Wrap(http.StatusBadRequest, err, "invalid csr")
29+
return errs.BadRequestErr(err, "invalid csr")
3030
}
3131
if s.OTT == "" {
3232
return errs.BadRequest("missing ott")
@@ -50,7 +50,7 @@ type SignResponse struct {
5050
func (h *caHandler) Sign(w http.ResponseWriter, r *http.Request) {
5151
var body SignRequest
5252
if err := ReadJSON(r.Body, &body); err != nil {
53-
WriteError(w, errs.Wrap(http.StatusBadRequest, err, "error reading request body"))
53+
WriteError(w, errs.BadRequestErr(err, "error reading request body"))
5454
return
5555
}
5656

‎api/ssh.go

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -49,16 +49,16 @@ type SSHSignRequest struct {
4949
func (s *SSHSignRequest) Validate() error {
5050
switch {
5151
case s.CertType != "" && s.CertType != provisioner.SSHUserCert && s.CertType != provisioner.SSHHostCert:
52-
return errors.Errorf("unknown certType %s", s.CertType)
52+
return errs.BadRequest("invalid certType '%s'", s.CertType)
5353
case len(s.PublicKey) == 0:
54-
return errors.New("missing or empty publicKey")
54+
return errs.BadRequest("missing or empty publicKey")
5555
case s.OTT == "":
56-
return errors.New("missing or empty ott")
56+
return errs.BadRequest("missing or empty ott")
5757
default:
5858
// Validate identity signature if provided
5959
if s.IdentityCSR.CertificateRequest != nil {
6060
if err := s.IdentityCSR.CertificateRequest.CheckSignature(); err != nil {
61-
return errors.Wrap(err, "invalid identityCSR")
61+
return errs.BadRequestErr(err, "invalid identityCSR")
6262
}
6363
}
6464
return nil
@@ -185,7 +185,7 @@ func (r *SSHConfigRequest) Validate() error {
185185
case provisioner.SSHUserCert, provisioner.SSHHostCert:
186186
return nil
187187
default:
188-
return errors.Errorf("unsupported type %s", r.Type)
188+
return errs.BadRequest("invalid type '%s'", r.Type)
189189
}
190190
}
191191

@@ -208,9 +208,9 @@ type SSHCheckPrincipalRequest struct {
208208
func (r *SSHCheckPrincipalRequest) Validate() error {
209209
switch {
210210
case r.Type != provisioner.SSHHostCert:
211-
return errors.Errorf("unsupported type %s", r.Type)
211+
return errs.BadRequest("unsupported type '%s'", r.Type)
212212
case r.Principal == "":
213-
return errors.New("missing or empty principal")
213+
return errs.BadRequest("missing or empty principal")
214214
default:
215215
return nil
216216
}
@@ -232,7 +232,7 @@ type SSHBastionRequest struct {
232232
// Validate checks the values of the SSHBastionRequest.
233233
func (r *SSHBastionRequest) Validate() error {
234234
if r.Hostname == "" {
235-
return errors.New("missing or empty hostname")
235+
return errs.BadRequest("missing or empty hostname")
236236
}
237237
return nil
238238
}
@@ -250,27 +250,27 @@ type SSHBastionResponse struct {
250250
func (h *caHandler) SSHSign(w http.ResponseWriter, r *http.Request) {
251251
var body SSHSignRequest
252252
if err := ReadJSON(r.Body, &body); err != nil {
253-
WriteError(w, errs.Wrap(http.StatusBadRequest, err, "error reading request body"))
253+
WriteError(w, errs.BadRequestErr(err, "error reading request body"))
254254
return
255255
}
256256

257257
logOtt(w, body.OTT)
258258
if err := body.Validate(); err != nil {
259-
WriteError(w, errs.BadRequestErr(err))
259+
WriteError(w, err)
260260
return
261261
}
262262

263263
publicKey, err := ssh.ParsePublicKey(body.PublicKey)
264264
if err != nil {
265-
WriteError(w, errs.Wrap(http.StatusBadRequest, err, "error parsing publicKey"))
265+
WriteError(w, errs.BadRequestErr(err, "error parsing publicKey"))
266266
return
267267
}
268268

269269
var addUserPublicKey ssh.PublicKey
270270
if body.AddUserPublicKey != nil {
271271
addUserPublicKey, err = ssh.ParsePublicKey(body.AddUserPublicKey)
272272
if err != nil {
273-
WriteError(w, errs.Wrap(http.StatusBadRequest, err, "error parsing addUserPublicKey"))
273+
WriteError(w, errs.BadRequestErr(err, "error parsing addUserPublicKey"))
274274
return
275275
}
276276
}
@@ -394,11 +394,11 @@ func (h *caHandler) SSHFederation(w http.ResponseWriter, r *http.Request) {
394394
func (h *caHandler) SSHConfig(w http.ResponseWriter, r *http.Request) {
395395
var body SSHConfigRequest
396396
if err := ReadJSON(r.Body, &body); err != nil {
397-
WriteError(w, errs.Wrap(http.StatusBadRequest, err, "error reading request body"))
397+
WriteError(w, errs.BadRequestErr(err, "error reading request body"))
398398
return
399399
}
400400
if err := body.Validate(); err != nil {
401-
WriteError(w, errs.BadRequestErr(err))
401+
WriteError(w, err)
402402
return
403403
}
404404

@@ -426,11 +426,11 @@ func (h *caHandler) SSHConfig(w http.ResponseWriter, r *http.Request) {
426426
func (h *caHandler) SSHCheckHost(w http.ResponseWriter, r *http.Request) {
427427
var body SSHCheckPrincipalRequest
428428
if err := ReadJSON(r.Body, &body); err != nil {
429-
WriteError(w, errs.Wrap(http.StatusBadRequest, err, "error reading request body"))
429+
WriteError(w, errs.BadRequestErr(err, "error reading request body"))
430430
return
431431
}
432432
if err := body.Validate(); err != nil {
433-
WriteError(w, errs.BadRequestErr(err))
433+
WriteError(w, err)
434434
return
435435
}
436436

@@ -465,11 +465,11 @@ func (h *caHandler) SSHGetHosts(w http.ResponseWriter, r *http.Request) {
465465
func (h *caHandler) SSHBastion(w http.ResponseWriter, r *http.Request) {
466466
var body SSHBastionRequest
467467
if err := ReadJSON(r.Body, &body); err != nil {
468-
WriteError(w, errs.Wrap(http.StatusBadRequest, err, "error reading request body"))
468+
WriteError(w, errs.BadRequestErr(err, "error reading request body"))
469469
return
470470
}
471471
if err := body.Validate(); err != nil {
472-
WriteError(w, errs.BadRequestErr(err))
472+
WriteError(w, err)
473473
return
474474
}
475475

‎api/sshRekey.go

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"net/http"
55
"time"
66

7-
"github.com/pkg/errors"
87
"github.com/smallstep/certificates/authority/provisioner"
98
"github.com/smallstep/certificates/errs"
109
"golang.org/x/crypto/ssh"
@@ -20,9 +19,9 @@ type SSHRekeyRequest struct {
2019
func (s *SSHRekeyRequest) Validate() error {
2120
switch {
2221
case s.OTT == "":
23-
return errors.New("missing or empty ott")
22+
return errs.BadRequest("missing or empty ott")
2423
case len(s.PublicKey) == 0:
25-
return errors.New("missing or empty public key")
24+
return errs.BadRequest("missing or empty public key")
2625
default:
2726
return nil
2827
}
@@ -40,19 +39,19 @@ type SSHRekeyResponse struct {
4039
func (h *caHandler) SSHRekey(w http.ResponseWriter, r *http.Request) {
4140
var body SSHRekeyRequest
4241
if err := ReadJSON(r.Body, &body); err != nil {
43-
WriteError(w, errs.Wrap(http.StatusBadRequest, err, "error reading request body"))
42+
WriteError(w, errs.BadRequestErr(err, "error reading request body"))
4443
return
4544
}
4645

4746
logOtt(w, body.OTT)
4847
if err := body.Validate(); err != nil {
49-
WriteError(w, errs.BadRequestErr(err))
48+
WriteError(w, err)
5049
return
5150
}
5251

5352
publicKey, err := ssh.ParsePublicKey(body.PublicKey)
5453
if err != nil {
55-
WriteError(w, errs.Wrap(http.StatusBadRequest, err, "error parsing publicKey"))
54+
WriteError(w, errs.BadRequestErr(err, "error parsing publicKey"))
5655
return
5756
}
5857

‎api/sshRenew.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ type SSHRenewRequest struct {
1919
func (s *SSHRenewRequest) Validate() error {
2020
switch {
2121
case s.OTT == "":
22-
return errors.New("missing or empty ott")
22+
return errs.BadRequest("missing or empty ott")
2323
default:
2424
return nil
2525
}
@@ -37,13 +37,13 @@ type SSHRenewResponse struct {
3737
func (h *caHandler) SSHRenew(w http.ResponseWriter, r *http.Request) {
3838
var body SSHRenewRequest
3939
if err := ReadJSON(r.Body, &body); err != nil {
40-
WriteError(w, errs.Wrap(http.StatusBadRequest, err, "error reading request body"))
40+
WriteError(w, errs.BadRequestErr(err, "error reading request body"))
4141
return
4242
}
4343

4444
logOtt(w, body.OTT)
4545
if err := body.Validate(); err != nil {
46-
WriteError(w, errs.BadRequestErr(err))
46+
WriteError(w, err)
4747
return
4848
}
4949

‎api/sshRevoke.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ func (r *SSHRevokeRequest) Validate() (err error) {
4848
func (h *caHandler) SSHRevoke(w http.ResponseWriter, r *http.Request) {
4949
var body SSHRevokeRequest
5050
if err := ReadJSON(r.Body, &body); err != nil {
51-
WriteError(w, errs.Wrap(http.StatusBadRequest, err, "error reading request body"))
51+
WriteError(w, errs.BadRequestErr(err, "error reading request body"))
5252
return
5353
}
5454

‎api/utils.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ func ProtoJSONStatus(w http.ResponseWriter, m proto.Message, status int) {
9393
// pointed by v.
9494
func ReadJSON(r io.Reader, v interface{}) error {
9595
if err := json.NewDecoder(r).Decode(v); err != nil {
96-
return errs.Wrap(http.StatusBadRequest, err, "error decoding json")
96+
return errs.BadRequestErr(err, "error decoding json")
9797
}
9898
return nil
9999
}
@@ -103,7 +103,7 @@ func ReadJSON(r io.Reader, v interface{}) error {
103103
func ReadProtoJSON(r io.Reader, m proto.Message) error {
104104
data, err := io.ReadAll(r)
105105
if err != nil {
106-
return errs.Wrap(http.StatusBadRequest, err, "error reading request body")
106+
return errs.BadRequestErr(err, "error reading request body")
107107
}
108108
return protojson.Unmarshal(data, m)
109109
}

‎authority/provisioner/jwk.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,7 @@ func (p *JWK) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption,
228228
// Use options in the token.
229229
if opts.CertType != "" {
230230
if certType, err = sshutil.CertTypeFromString(opts.CertType); err != nil {
231-
return nil, errs.Wrap(http.StatusBadRequest, err, "jwk.AuthorizeSSHSign")
231+
return nil, errs.BadRequestErr(err, err.Error())
232232
}
233233
}
234234
if opts.KeyID != "" {

‎authority/provisioner/sign_options.go

Lines changed: 4 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,7 @@ import (
88
"crypto/x509/pkix"
99
"encoding/asn1"
1010
"encoding/json"
11-
"fmt"
1211
"net"
13-
"net/http"
1412
"net/url"
1513
"reflect"
1614
"time"
@@ -372,17 +370,6 @@ func newValidityValidator(min, max time.Duration) *validityValidator {
372370
return &validityValidator{min: min, max: max}
373371
}
374372

375-
// TODO(mariano): refactor errs package to allow sending real errors to the
376-
// user.
377-
func badRequest(format string, args ...interface{}) error {
378-
msg := fmt.Sprintf(format, args...)
379-
return &errs.Error{
380-
Status: http.StatusBadRequest,
381-
Msg: msg,
382-
Err: errors.New(msg),
383-
}
384-
}
385-
386373
// Valid validates the certificate validity settings (notBefore/notAfter) and
387374
// total duration.
388375
func (v *validityValidator) Valid(cert *x509.Certificate, o SignOptions) error {
@@ -395,20 +382,20 @@ func (v *validityValidator) Valid(cert *x509.Certificate, o SignOptions) error {
395382
d := na.Sub(nb)
396383

397384
if na.Before(now) {
398-
return badRequest("notAfter cannot be in the past; na=%v", na)
385+
return errs.BadRequest("notAfter cannot be in the past; na=%v", na)
399386
}
400387
if na.Before(nb) {
401-
return badRequest("notAfter cannot be before notBefore; na=%v, nb=%v", na, nb)
388+
return errs.BadRequest("notAfter cannot be before notBefore; na=%v, nb=%v", na, nb)
402389
}
403390
if d < v.min {
404-
return badRequest("requested duration of %v is less than the authorized minimum certificate duration of %v", d, v.min)
391+
return errs.BadRequest("requested duration of %v is less than the authorized minimum certificate duration of %v", d, v.min)
405392
}
406393
// NOTE: this check is not "technically correct". We're allowing the max
407394
// duration of a cert to be "max + backdate" and not all certificates will
408395
// be backdated (e.g. if a user passes the NotBefore value then we do not
409396
// apply a backdate). This is good enough.
410397
if d > v.max+o.Backdate {
411-
return badRequest("requested duration of %v is more than the authorized maximum certificate duration of %v", d, v.max+o.Backdate)
398+
return errs.BadRequest("requested duration of %v is more than the authorized maximum certificate duration of %v", d, v.max+o.Backdate)
412399
}
413400
return nil
414401
}

0 commit comments

Comments
 (0)