validate uuid requests (#98)

* validate uuid requests

Signed-off-by: Mike Mason <mimason@equinix.com>

* golanglint-ci action skip-pkg-cache

Seeing issues with running golanglint-ci action, one solution was to
disable package cache.

https://github.com/metal-toolbox/hollow-metadataservice/actions/runs/4683823976/jobs/8299350650?pr=98
golangci/golangci-lint-action#23 (comment)

Signed-off-by: Mike Mason <mimason@equinix.com>

---------

Signed-off-by: Mike Mason <mimason@equinix.com>

.github/workflows/lint-test.yml

@@ -35,6 +35,7 @@ jobs:
         with:
           version: v1.51.1
           args: --timeout=5m
+          skip-pkg-cache: true
 
       - name: Run go tests for generated models code
         run: go test -race ./internal/models

go.mod

@@ -10,6 +10,7 @@ require (
 	github.com/gin-contrib/zap v0.1.0
 	github.com/gin-gonic/gin v1.9.0
 	github.com/go-playground/validator/v10 v10.12.0
+	github.com/google/uuid v1.3.0
 	github.com/jmoiron/sqlx v1.3.5
 	github.com/lib/pq v1.10.7
 	github.com/mitchellh/go-homedir v1.1.0

go.sum

@@ -642,8 +642,6 @@ go.etcd.io/etcd/api/v3 v3.5.4/go.mod h1:5GB2vv4A4AOn3yk7MftYGHkUfGtDHnEraIjym4dY
 go.etcd.io/etcd/client/pkg/v3 v3.5.4/go.mod h1:IJHfcCEKxYu1Os13ZdwCwIUTUVGYTSAM3YSwc9/Ac1g=
 go.etcd.io/etcd/client/v2 v2.305.4/go.mod h1:Ud+VUwIi9/uQHOMA+4ekToJ12lTxlv0zB/+DHwTGEbU=
 go.etcd.io/etcd/client/v3 v3.5.4/go.mod h1:ZaRkVgBZC+L+dLCjTcF1hRXpgZXQPOvnA/Ak/gq3kiY=
-go.hollow.sh/toolbox v0.4.1 h1:4ANXNRybTyLd0T/3+ePXp9PAnr+j7HXQDTeiEAaup3k=
-go.hollow.sh/toolbox v0.4.1/go.mod h1:kroiA62wlwnxLixA+aWV9Oc26piRaAYMn+uqHYRRfdc=
 go.hollow.sh/toolbox v0.5.1 h1:x/tRdpgUFckT/pw6FySxUpSFxhw+Sc66zxbwlra+br4=
 go.hollow.sh/toolbox v0.5.1/go.mod h1:nfUNHobFfItossU5I0m7h2dw+0nY7rQt3DSUm2wGI5Q=
 go.infratographer.com/x v0.0.3 h1:7txN1iBq1Ts4XF6Ea/9/LWK2vpTeXKnh+DjdJ1OLHVQ=

pkg/api/v1/router.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/gin-gonic/gin"
 	"github.com/go-playground/validator/v10"
+	"github.com/google/uuid"
 	"github.com/jmoiron/sqlx"
 	"go.uber.org/zap"
 
@@ -59,6 +60,12 @@ var (
 	// - the item wasn't found in the DB
 	// - the item wasn't found in the upstream lookup service
 	errNotFound = errors.New("not found")
+
+	// ErrUUIDNotFound is returned when an expected uuid is not provided.
+	ErrUUIDNotFound = errors.New("uuid not found")
+
+	// ErrInvalidUUID is returned when an invalid uuid is provided.
+	ErrInvalidUUID = errors.New("invalid uuid")
 )
 
 // Router provides a router for the v1 API
@@ -263,3 +270,18 @@ func setupValidator() {
 		return name
 	})
 }
+
+// getUUIDParam parses and validates a UUID from the request params if the param is found
+func getUUIDParam(c *gin.Context, name string) (string, error) {
+	id, ok := c.Params.Get(name)
+
+	if !ok || id == "" {
+		return "", ErrUUIDNotFound
+	}
+
+	if _, err := uuid.Parse(id); err != nil {
+		return "", ErrInvalidUUID
+	}
+
+	return id, nil
+}

pkg/api/v1/router_instance_metadata.go

@@ -87,10 +87,11 @@ func (r *Router) instanceMetadataGet(c *gin.Context) {
 // which instances the metadata service already knows about, and which
 // instances may still need their metadata pushed to the service.
 func (r *Router) instanceMetadataGetInternal(c *gin.Context) {
-	instanceID, ok := c.Params.Get("instance-id")
+	instanceID, err := getUUIDParam(c, "instance-id")
+
+	if err != nil {
+		invalidUUIDResponse(c, err)
 
-	if !ok || instanceID == "" {
-		notFoundResponse(c)
 		return
 	}
 
@@ -140,10 +141,11 @@ func (r *Router) instanceUserdataGet(c *gin.Context) {
 // which instances the userdata service already knows about, and which
 // instances may still need their userdata pushed to the service.
 func (r *Router) instanceUserdataGetInternal(c *gin.Context) {
-	instanceID, ok := c.Params.Get("instance-id")
+	instanceID, err := getUUIDParam(c, "instance-id")
+
+	if err != nil {
+		invalidUUIDResponse(c, err)
 
-	if !ok || instanceID == "" {
-		notFoundResponse(c)
 		return
 	}
 
@@ -240,10 +242,11 @@ func (r *Router) instanceMetadataDelete(c *gin.Context) {
 	// When deleting metadata for an instance, we need to check if there is
 	// userdata stored for the instance. If there is not, we should go ahead and
 	// also delete the associated instance_ip_addresses rows.
-	instanceID, ok := c.Params.Get("instance-id")
+	instanceID, err := getUUIDParam(c, "instance-id")
+
+	if err != nil {
+		invalidUUIDResponse(c, err)
 
-	if !ok || instanceID == "" {
-		notFoundResponse(c)
 		return
 	}
 
@@ -261,10 +264,11 @@ func (r *Router) instanceUserdataDelete(c *gin.Context) {
 	// When deleting userdata for an instance, we need to check if there is
 	// metadata stored for the instance. If there is not, we should go ahead and
 	// also delete the associated instance_ip_addresses rows.
-	instanceID, ok := c.Params.Get("instance-id")
+	instanceID, err := getUUIDParam(c, "instance-id")
+
+	if err != nil {
+		invalidUUIDResponse(c, err)
 
-	if !ok || instanceID == "" {
-		notFoundResponse(c)
 		return
 	}
 

pkg/api/v1/router_instance_metadata_test.go

@@ -622,6 +622,12 @@ func TestGetMetadataInternal(t *testing.T) {
 			http.StatusNotFound,
 			"",
 		},
+		{
+			"invalid ID",
+			"bad-id",
+			http.StatusBadRequest,
+			"",
+		},
 		{
 			"Instance A",
 			dbtools.FixtureInstanceA.InstanceID,

pkg/api/v1/router_responses.go

@@ -46,6 +46,20 @@ func badRequestResponse(c *gin.Context, message string, err error) {
 	c.AbortWithStatusJSON(http.StatusBadRequest, &ErrorResponse{Message: message, Errors: errMsgs})
 }
 
+func invalidUUIDResponse(c *gin.Context, err error) {
+	if err != nil {
+		switch {
+		case errors.Is(err, ErrUUIDNotFound):
+			notFoundResponse(c)
+		case errors.Is(err, ErrInvalidUUID):
+			c.Error(err) //nolint:errcheck // no need to check this error.
+			c.AbortWithStatusJSON(http.StatusBadRequest, &ErrorResponse{Message: err.Error(), Errors: []string{err.Error()}})
+		}
+	}
+
+	c.AbortWithStatusJSON(http.StatusInternalServerError, &ErrorResponse{Errors: []string{"internal server error"}})
+}
+
 func getErrorMessagesFromError(err error) []string {
 	if err == nil {
 		return []string{}