lxd: Improve certificate add token validation

lxd/certificates.go

@@ -233,9 +233,20 @@ func clusterMemberJoinTokenValid(s *state.State, r *http.Request, projectName st
 
 			// Depending on whether it's a local operation or not, expiry will either be a time.Time or a string.
 			if s.ServerName == foundOp.Location {
-				expiry, _ = expiresAt.(time.Time)
+				expiry, ok = expiresAt.(time.Time)
+				if !ok {
+					return nil, fmt.Errorf("Unexpected expiry type in cluster join token operation: %T (%v)", expiresAt, expiresAt)
+				}
 			} else {
-				expiry, _ = time.Parse(time.RFC3339Nano, expiresAt.(string))
+				expiryStr, ok := expiresAt.(string)
+				if !ok {
+					return nil, fmt.Errorf("Unexpected expiry type in cluster join token operation: %T (%v)", expiresAt, expiresAt)
+				}
+
+				expiry, err = time.Parse(time.RFC3339Nano, expiryStr)
+				if err != nil {
+					return nil, fmt.Errorf("Invalid expiry format in cluster join token operation: %w (%q)", err, expiryStr)
+				}
 			}
 
 			// Check if token has expired.
@@ -277,53 +288,77 @@ func certificateTokenValid(s *state.State, r *http.Request, addToken *api.Certif
 		}
 	}
 
-	if foundOp != nil {
-		// Token is single-use, so cancel it now.
-		err = operationCancel(s, r, api.ProjectDefaultName, foundOp)
-		if err != nil {
-			return nil, fmt.Errorf("Failed to cancel operation %q: %w", foundOp.ID, err)
-		}
+	if foundOp == nil {
+		// No operation found.
+		return nil, nil
+	}
 
-		expiresAt, ok := foundOp.Metadata["expiresAt"]
-		if ok {
-			expiry, _ := expiresAt.(time.Time)
+	// Token is single-use, so cancel it now.
+	err = operationCancel(s, r, api.ProjectDefaultName, foundOp)
+	if err != nil {
+		return nil, fmt.Errorf("Failed to cancel operation %q: %w", foundOp.ID, err)
+	}
 
-			// Check if token has expired.
-			if time.Now().After(expiry) {
-				return nil, api.StatusErrorf(http.StatusForbidden, "Token has expired")
+	expiresAt, ok := foundOp.Metadata["expiresAt"]
+	if ok {
+		var expiry time.Time
+
+		// Depending on whether it's a local operation or not, expiry will either be a time.Time or a string.
+		if s.ServerName == foundOp.Location {
+			expiry, ok = expiresAt.(time.Time)
+			if !ok {
+				return nil, fmt.Errorf("Unexpected expiry type in certificate add operation: %T (%v)", expiresAt, expiresAt)
+			}
+		} else {
+			expiryStr, ok := expiresAt.(string)
+			if !ok {
+				return nil, fmt.Errorf("Unexpected expiry type in certificate add operation: %T (%v)", expiresAt, expiresAt)
+			}
+
+			expiry, err = time.Parse(time.RFC3339Nano, expiryStr)
+			if err != nil {
+				return nil, fmt.Errorf("Invalid expiry format in certificate add operation: %w (%q)", err, expiryStr)
 			}
 		}
 
-		// Certificate add tokens must have a request field in the metadata.
-		tokenReqAny, ok := foundOp.Metadata["request"]
-		if !ok {
-			return nil, fmt.Errorf(`Missing "request" key in certificate add operation data`)
+		// Check if token has expired.
+		if time.Now().After(expiry) {
+			return nil, api.StatusErrorf(http.StatusForbidden, "Token has expired")
 		}
+	}
 
-		tokenReq, ok := tokenReqAny.(api.CertificatesPost)
-		if !ok {
-			// If the operation is running on another member, the returned metadata will have been unmarshalled
-			// into a map[string]any. Rather than wrangling type assertions, just marshal and unmarshal the data into
-			// the correct type.
-			buf := bytes.NewBuffer(nil)
-			err := json.NewEncoder(buf).Encode(tokenReqAny)
-			if err != nil {
-				return nil, fmt.Errorf("Bad certificate add operation data: %w", err)
-			}
+	// Certificate add tokens must have a request field in the metadata.
+	tokenReqAny, ok := foundOp.Metadata["request"]
+	if !ok {
+		return nil, fmt.Errorf(`Missing "request" key in certificate add operation data`)
+	}
 
-			err = json.NewDecoder(buf).Decode(&tokenReq)
-			if err != nil {
-				return nil, fmt.Errorf("Bad certificate add operation data: %w", err)
-			}
+	var tokenReq api.CertificatesPost
 
-			foundOp.Metadata["request"] = tokenReq
+	// Depending on whether it's a local operation or not, request field will either be a api.CertificatesPost
+	// or a JSON string of api.CertificatesPost.
+	if s.ServerName == foundOp.Location {
+		tokenReq, ok = tokenReqAny.(api.CertificatesPost)
+		if !ok {
+			return nil, fmt.Errorf("Unexpected request type in certificate add operation: %T", tokenReqAny)
+		}
+	} else {
+		// If the operation is running on another member, the returned metadata will have been unmarshalled
+		// into a map[string]any. Rather than wrangling type assertions, just marshal and unmarshal the
+		// data into the correct type.
+		buf := bytes.NewBuffer(nil)
+		err = json.NewEncoder(buf).Encode(tokenReqAny)
+		if err != nil {
+			return nil, fmt.Errorf("Bad certificate add operation data: %w", err)
 		}
 
-		return &tokenReq, nil
+		err = json.NewDecoder(buf).Decode(&tokenReq)
+		if err != nil {
+			return nil, fmt.Errorf("Bad certificate add operation data: %w", err)
+		}
 	}
 
-	// No operation found.
-	return nil, nil
+	return &tokenReq, nil
 }
 
 // swagger:operation POST /1.0/certificates?public certificates certificates_post_untrusted

test/suites/clustering.sh

@@ -3905,6 +3905,39 @@ test_clustering_trust_add() {
   ns2="${prefix}2"
   spawn_lxd_and_join_cluster "${ns2}" "${bridge}" "${cert}" 2 1 "${LXD_TWO_DIR}" "${LXD_ONE_DIR}"
 
+  # Check using token that is expired
+
+  # Set token expiry to 1 seconds
+  lxc config set core.remote_token_expiry 1S
+
+  # Get a certificate add token from LXD_ONE. The operation will run on LXD_ONE locally.
+  lxd_one_token="$(LXD_DIR="${LXD_ONE_DIR}" lxc config trust add --name foo --quiet)"
+  sleep 2
+
+  # Expect one running token operation.
+  operation_uuid="$(LXD_DIR="${LXD_ONE_DIR}" lxc operation list --format csv | grep -F "TOKEN,Executing operation,RUNNING" | cut -d, -f1 )"
+  LXD_DIR="${LXD_TWO_DIR}" lxc operation list --format csv | grep -qF "${operation_uuid},TOKEN,Executing operation,RUNNING"
+  is_uuid_v4 "${operation_uuid}"
+
+  # Get the address of LXD_TWO.
+  lxd_two_address="https://$(LXD_DIR="${LXD_TWO_DIR}" lxc config get core.https_address)"
+
+  # Test adding the remote using the address of LXD_TWO with the token operation running on LXD_ONE.
+  # LXD_TWO does not have the operation running locally, so it should find the UUID of the operation in the database
+  # and query LXD_ONE for it. LXD_TWO should cancel the operation by sending a DELETE /1.0/operations/{uuid} to LXD_ONE
+  # and needs to parse the metadata of the operation into the correct type to complete the trust process.
+  # The expiry time should be parsed and found to be expired so the add action should fail.
+  ! lxc remote add lxd_two "${lxd_two_address}" --accept-certificate --token "${lxd_one_token}" || false
+
+  # Expect the operation to be cancelled.
+  LXD_DIR="${LXD_ONE_DIR}" lxc operation list --format csv | grep -qF "${operation_uuid},TOKEN,Executing operation,CANCELLED"
+  LXD_DIR="${LXD_TWO_DIR}" lxc operation list --format csv | grep -qF "${operation_uuid},TOKEN,Executing operation,CANCELLED"
+
+  # Set token expiry to 1 hour
+  lxc config set core.remote_token_expiry 1H
+
+  # Check using token that isn't expired
+
   # Get a certificate add token from LXD_ONE. The operation will run on LXD_ONE locally.
   lxd_one_token="$(LXD_DIR="${LXD_ONE_DIR}" lxc config trust add --name foo --quiet)"
 
@@ -3929,6 +3962,9 @@ test_clustering_trust_add() {
   # Clean up
   lxc remote rm lxd_two
 
+  # Unset token expiry
+  lxc config unset core.remote_token_expiry
+
   LXD_DIR="${LXD_TWO_DIR}" lxd shutdown
   LXD_DIR="${LXD_ONE_DIR}" lxd shutdown
   sleep 0.5
@@ -3940,4 +3976,4 @@ test_clustering_trust_add() {
 
   kill_lxd "${LXD_ONE_DIR}"
   kill_lxd "${LXD_TWO_DIR}"
-}
+}

test/suites/remote.sh

@@ -111,8 +111,8 @@ test_remote_url_with_token() {
 
   lxc config trust rm "$(lxc config trust list -f json | jq -r '.[].fingerprint')"
 
-  # Set token expiry to 5 seconds
-  lxc config set core.remote_token_expiry 5S
+  # Set token expiry to 1 seconds
+  lxc config set core.remote_token_expiry 1S
 
   # Generate new token
   token="$(lxc config trust add --name foo | tail -n1)"
@@ -130,7 +130,7 @@ test_remote_url_with_token() {
   token="$(lxc config trust add --name foo | tail -n1)"
 
   # This will cause the token to expire
-  sleep 5
+  sleep 2
 
   # Try adding remote. This should fail.
   ! lxc_remote remote add test "${token}" || false