From 1df21b9b6a5f86df54cba905110212b583b5a1b7 Mon Sep 17 00:00:00 2001 From: max furman Date: Tue, 6 Jul 2021 17:14:13 -0700 Subject: [PATCH] Addressing comments in PR review - added a bit of validation to admin create and update - using protojson where possible in admin api - fixing a few instances of admin -> acme in errors --- authority/admin/api/admin.go | 27 ++++++++++++++++++++++++--- authority/admin/api/middleware.go | 2 +- authority/admin/api/provisioner.go | 2 +- authority/admin/db/nosql/nosql.go | 4 ++-- authority/admin/errors.go | 12 ++++++------ authority/authority.go | 6 ++++++ ca/adminClient.go | 6 +++--- 7 files changed, 43 insertions(+), 16 deletions(-) diff --git a/authority/admin/api/admin.go b/authority/admin/api/admin.go index d92af5b2..bf79ebcf 100644 --- a/authority/admin/api/admin.go +++ b/authority/admin/api/admin.go @@ -18,6 +18,17 @@ type CreateAdminRequest struct { // Validate validates a new-admin request body. func (car *CreateAdminRequest) Validate() error { + if car.Subject == "" { + return admin.NewError(admin.ErrorBadRequestType, "subject cannot be empty") + } + if car.Provisioner == "" { + return admin.NewError(admin.ErrorBadRequestType, "provisioner cannot be empty") + } + switch car.Type { + case linkedca.Admin_SUPER_ADMIN, linkedca.Admin_ADMIN: + default: + return admin.NewError(admin.ErrorBadRequestType, "invalid value for admin type") + } return nil } @@ -34,6 +45,11 @@ type UpdateAdminRequest struct { // Validate validates a new-admin request body. func (uar *UpdateAdminRequest) Validate() error { + switch uar.Type { + case linkedca.Admin_SUPER_ADMIN, linkedca.Admin_ADMIN: + default: + return admin.NewError(admin.ErrorBadRequestType, "invalid value for admin type") + } return nil } @@ -52,7 +68,7 @@ func (h *Handler) GetAdmin(w http.ResponseWriter, r *http.Request) { "admin %s not found", id)) return } - api.JSON(w, adm) + api.ProtoJSON(w, adm) } // GetAdmins returns a segment of admins associated with the authority. @@ -104,7 +120,7 @@ func (h *Handler) CreateAdmin(w http.ResponseWriter, r *http.Request) { return } - api.JSON(w, adm) + api.ProtoJSONStatus(w, adm, http.StatusCreated) } // DeleteAdmin deletes admin. @@ -127,6 +143,11 @@ func (h *Handler) UpdateAdmin(w http.ResponseWriter, r *http.Request) { return } + if err := body.Validate(); err != nil { + api.WriteError(w, err) + return + } + id := chi.URLParam(r, "id") adm, err := h.auth.UpdateAdmin(r.Context(), id, &linkedca.Admin{Type: body.Type}) @@ -135,5 +156,5 @@ func (h *Handler) UpdateAdmin(w http.ResponseWriter, r *http.Request) { return } - api.JSON(w, adm) + api.ProtoJSON(w, adm) } diff --git a/authority/admin/api/middleware.go b/authority/admin/api/middleware.go index 0261047a..90289f85 100644 --- a/authority/admin/api/middleware.go +++ b/authority/admin/api/middleware.go @@ -14,7 +14,7 @@ type nextHTTP = func(http.ResponseWriter, *http.Request) // is enabled before servicing requests. func (h *Handler) requireAPIEnabled(next nextHTTP) nextHTTP { return func(w http.ResponseWriter, r *http.Request) { - if h.db == nil { + if !h.auth.IsAdminAPIEnabled() { api.WriteError(w, admin.NewError(admin.ErrorNotImplementedType, "administration API not enabled")) return diff --git a/authority/admin/api/provisioner.go b/authority/admin/api/provisioner.go index e63a4417..fd1a02d5 100644 --- a/authority/admin/api/provisioner.go +++ b/authority/admin/api/provisioner.go @@ -171,5 +171,5 @@ func (h *Handler) UpdateProvisioner(w http.ResponseWriter, r *http.Request) { api.WriteError(w, err) return } - api.ProtoJSONStatus(w, nu, http.StatusOK) + api.ProtoJSON(w, nu) } diff --git a/authority/admin/db/nosql/nosql.go b/authority/admin/db/nosql/nosql.go index 2ce4297c..18599b02 100644 --- a/authority/admin/db/nosql/nosql.go +++ b/authority/admin/db/nosql/nosql.go @@ -15,7 +15,7 @@ var ( provisionersTable = []byte("provisioners") ) -// DB is a struct that implements the AcmeDB interface. +// DB is a struct that implements the AdminDB interface. type DB struct { db nosqlDB.DB authorityID string @@ -54,7 +54,7 @@ func (db *DB) save(ctx context.Context, id string, nu interface{}, old interface } else { oldB, err = json.Marshal(old) if err != nil { - return errors.Wrapf(err, "error marshaling acme type: %s, value: %v", typ, old) + return errors.Wrapf(err, "error marshaling admin type: %s, value: %v", typ, old) } } diff --git a/authority/admin/errors.go b/authority/admin/errors.go index 81ae6d6f..607093b0 100644 --- a/authority/admin/errors.go +++ b/authority/admin/errors.go @@ -12,7 +12,7 @@ import ( "github.com/smallstep/certificates/logging" ) -// ProblemType is the type of the ACME problem. +// ProblemType is the type of the Admin problem. type ProblemType int const ( @@ -33,7 +33,7 @@ const ( ErrorServerInternalType ) -// String returns the string representation of the acme problem type, +// String returns the string representation of the admin problem type, // fulfilling the Stringer interface. func (ap ProblemType) String() string { switch ap { @@ -73,17 +73,17 @@ var ( ErrorNotFoundType: { typ: ErrorNotFoundType.String(), details: "resource not found", - status: 400, + status: http.StatusNotFound, }, ErrorAuthorityMismatchType: { typ: ErrorAuthorityMismatchType.String(), details: "resource not owned by authority", - status: 401, + status: http.StatusUnauthorized, }, ErrorDeletedType: { typ: ErrorDeletedType.String(), details: "resource is deleted", - status: http.StatusUnauthorized, + status: http.StatusNotFound, }, ErrorNotImplementedType: { typ: ErrorNotImplementedType.String(), @@ -104,7 +104,7 @@ var ( } ) -// Error represents an ACME +// Error represents an Admin type Error struct { Type string `json:"type"` Detail string `json:"detail"` diff --git a/authority/authority.go b/authority/authority.go index dbe803ca..0f171fa7 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -508,6 +508,12 @@ func (a *Authority) GetAdminDatabase() admin.DB { return a.adminDB } +// IsAdminAPIEnabled returns a boolean indicating whether the Admin API has +// been enabled. +func (a *Authority) IsAdminAPIEnabled() bool { + return a.config.AuthorityConfig.EnableAdmin +} + // Shutdown safely shuts down any clients, databases, etc. held by the Authority. func (a *Authority) Shutdown() error { if err := a.keyManager.Close(); err != nil { diff --git a/ca/adminClient.go b/ca/adminClient.go index da2ff566..2f3d4b5d 100644 --- a/ca/adminClient.go +++ b/ca/adminClient.go @@ -132,7 +132,7 @@ retry: return nil, readAdminError(resp.Body) } var adm = new(linkedca.Admin) - if err := readJSON(resp.Body, adm); err != nil { + if err := readProtoJSON(resp.Body, adm); err != nil { return nil, errors.Wrapf(err, "error reading %s", u) } return adm, nil @@ -270,7 +270,7 @@ retry: return nil, readAdminError(resp.Body) } var adm = new(linkedca.Admin) - if err := readJSON(resp.Body, adm); err != nil { + if err := readProtoJSON(resp.Body, adm); err != nil { return nil, errors.Wrapf(err, "error reading %s", u) } return adm, nil @@ -334,7 +334,7 @@ retry: return nil, readAdminError(resp.Body) } var adm = new(linkedca.Admin) - if err := readJSON(resp.Body, adm); err != nil { + if err := readProtoJSON(resp.Body, adm); err != nil { return nil, errors.Wrapf(err, "error reading %s", u) } return adm, nil