[management] Setup key improvements (#2775)

management/server/account.go

@@ -153,6 +153,7 @@ type AccountManager interface {
 	FindExistingPostureCheck(accountID string, checks *posture.ChecksDefinition) (*posture.Checks, error)
 	GetAccountIDForPeerKey(ctx context.Context, peerKey string) (string, error)
 	GetAccountSettings(ctx context.Context, accountID string, userID string) (*Settings, error)
+	DeleteSetupKey(ctx context.Context, accountID, userID, keyID string) error
 }
 
 type DefaultAccountManager struct {

management/server/account_test.go

@@ -1010,7 +1010,6 @@ func TestAccountManager_AddPeer(t *testing.T) {
 		return
 	}
 	expectedPeerKey := key.PublicKey().String()
-	expectedSetupKey := setupKey.Key
 
 	peer, _, _, err := manager.AddPeer(context.Background(), setupKey.Key, "", &nbpeer.Peer{
 		Key:  expectedPeerKey,
@@ -1035,10 +1034,6 @@ func TestAccountManager_AddPeer(t *testing.T) {
 		t.Errorf("expecting just added peer's IP %s to be in a network range %s", peer.IP.String(), account.Network.Net.String())
 	}
 
-	if peer.SetupKey != expectedSetupKey {
-		t.Errorf("expecting just added peer to have SetupKey = %s, got %s", expectedSetupKey, peer.SetupKey)
-	}
-
 	if account.Network.CurrentSerial() != 1 {
 		t.Errorf("expecting Network Serial=%d to be incremented by 1 and be equal to %d when adding new peer to account", serial, account.Network.CurrentSerial())
 	}
@@ -2367,15 +2362,13 @@ func TestAccount_GetNextPeerExpiration(t *testing.T) {
 						LoginExpired: false,
 					},
 					LoginExpirationEnabled: true,
-					SetupKey:               "key",
 				},
 				"peer-2": {
 					Status: &nbpeer.PeerStatus{
 						Connected:    true,
 						LoginExpired: false,
 					},
 					LoginExpirationEnabled: true,
-					SetupKey:               "key",
 				},
 			},
 			expiration:             time.Second,
@@ -2529,15 +2522,13 @@ func TestAccount_GetNextInactivePeerExpiration(t *testing.T) {
 						LoginExpired: false,
 					},
 					InactivityExpirationEnabled: true,
-					SetupKey:                    "key",
 				},
 				"peer-2": {
 					Status: &nbpeer.PeerStatus{
 						Connected:    true,
 						LoginExpired: false,
 					},
 					InactivityExpirationEnabled: true,
-					SetupKey:                    "key",
 				},
 			},
 			expiration:             time.Second,

management/server/activity/codes.go

@@ -146,6 +146,8 @@ const (
 	AccountPeerInactivityExpirationEnabled         Activity = 65
 	AccountPeerInactivityExpirationDisabled        Activity = 66
 	AccountPeerInactivityExpirationDurationUpdated Activity = 67
+
+	SetupKeyDeleted Activity = 68
 )
 
 var activityMap = map[Activity]Code{
@@ -219,6 +221,7 @@ var activityMap = map[Activity]Code{
 	AccountPeerInactivityExpirationEnabled:         {"Account peer inactivity expiration enabled", "account.peer.inactivity.expiration.enable"},
 	AccountPeerInactivityExpirationDisabled:        {"Account peer inactivity expiration disabled", "account.peer.inactivity.expiration.disable"},
 	AccountPeerInactivityExpirationDurationUpdated: {"Account peer inactivity expiration duration updated", "account.peer.inactivity.expiration.update"},
+	SetupKeyDeleted: {"Setup key deleted", "setupkey.delete"},
 }
 
 // StringCode returns a string code of the activity

management/server/http/api/openapi.yml

@@ -530,10 +530,9 @@ components:
           type: string
           example: reusable
         expires_in:
-          description: Expiration time in seconds
+          description: Expiration time in seconds, 0 will mean the key never expires
           type: integer
-          minimum: 86400
-          maximum: 31536000
+          minimum: 0
           example: 86400
         revoked:
           description: Setup key revocation status
@@ -2018,6 +2017,32 @@ paths:
           "$ref": "#/components/responses/forbidden"
         '500':
           "$ref": "#/components/responses/internal_error"
+    delete:
+      summary: Delete a Setup Key
+      description: Delete a Setup Key
+      tags: [ Setup Keys ]
+      security:
+        - BearerAuth: [ ]
+        - TokenAuth: [ ]
+      parameters:
+        - in: path
+          name: keyId
+          required: true
+          schema:
+            type: string
+          description: The unique identifier of a setup key
+      responses:
+        '200':
+          description: Delete status code
+          content: { }
+        '400':
+          "$ref": "#/components/responses/bad_request"
+        '401':
+          "$ref": "#/components/responses/requires_authentication"
+        '403':
+          "$ref": "#/components/responses/forbidden"
+        '500':
+          "$ref": "#/components/responses/internal_error"
   /api/groups:
     get:
       summary: List all Groups

management/server/http/handler.go

@@ -141,6 +141,7 @@ func (apiHandler *apiHandler) addSetupKeysEndpoint() {
 	apiHandler.Router.HandleFunc("/setup-keys", keysHandler.CreateSetupKey).Methods("POST", "OPTIONS")
 	apiHandler.Router.HandleFunc("/setup-keys/{keyId}", keysHandler.GetSetupKey).Methods("GET", "OPTIONS")
 	apiHandler.Router.HandleFunc("/setup-keys/{keyId}", keysHandler.UpdateSetupKey).Methods("PUT", "OPTIONS")
+	apiHandler.Router.HandleFunc("/setup-keys/{keyId}", keysHandler.DeleteSetupKey).Methods("DELETE", "OPTIONS")
 }
 
 func (apiHandler *apiHandler) addPoliciesEndpoint() {

management/server/http/peers_handler_test.go

@@ -13,12 +13,13 @@ import (
 	"time"
 
 	"github.com/gorilla/mux"
+	"golang.org/x/exp/maps"
+
 	"github.com/netbirdio/netbird/management/server"
 	nbgroup "github.com/netbirdio/netbird/management/server/group"
 	"github.com/netbirdio/netbird/management/server/http/api"
 	"github.com/netbirdio/netbird/management/server/jwtclaims"
 	nbpeer "github.com/netbirdio/netbird/management/server/peer"
-	"golang.org/x/exp/maps"
 
 	"github.com/stretchr/testify/assert"
 
@@ -168,7 +169,6 @@ func TestGetPeers(t *testing.T) {
 	peer := &nbpeer.Peer{
 		ID:                     testPeerID,
 		Key:                    "key",
-		SetupKey:               "setupkey",
 		IP:                     net.ParseIP("100.64.0.1"),
 		Status:                 &nbpeer.PeerStatus{Connected: true},
 		Name:                   "PeerName",

management/server/http/setupkeys_handler.go

@@ -61,10 +61,8 @@ func (h *SetupKeysHandler) CreateSetupKey(w http.ResponseWriter, r *http.Request
 
 	expiresIn := time.Duration(req.ExpiresIn) * time.Second
 
-	day := time.Hour * 24
-	year := day * 365
-	if expiresIn < day || expiresIn > year {
-		util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "expiresIn should be between 1 day and 365 days"), w)
+	if expiresIn < 0 {
+		util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "expiresIn can not be in the past"), w)
 		return
 	}
 
@@ -76,14 +74,19 @@ func (h *SetupKeysHandler) CreateSetupKey(w http.ResponseWriter, r *http.Request
 	if req.Ephemeral != nil {
 		ephemeral = *req.Ephemeral
 	}
+
 	setupKey, err := h.accountManager.CreateSetupKey(r.Context(), accountID, req.Name, server.SetupKeyType(req.Type), expiresIn,
 		req.AutoGroups, req.UsageLimit, userID, ephemeral)
 	if err != nil {
 		util.WriteError(r.Context(), err, w)
 		return
 	}
 
-	writeSuccess(r.Context(), w, setupKey)
+	apiSetupKeys := toResponseBody(setupKey)
+	// for the creation we need to send the plain key
+	apiSetupKeys.Key = setupKey.Key
+
+	util.WriteJSONObject(r.Context(), w, apiSetupKeys)
 }
 
 // GetSetupKey is a GET request to get a SetupKey by ID
@@ -98,7 +101,7 @@ func (h *SetupKeysHandler) GetSetupKey(w http.ResponseWriter, r *http.Request) {
 	vars := mux.Vars(r)
 	keyID := vars["keyId"]
 	if len(keyID) == 0 {
-		util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "invalid key ID"), w)
+		util.WriteError(r.Context(), status.NewInvalidKeyIDError(), w)
 		return
 	}
 
@@ -123,7 +126,7 @@ func (h *SetupKeysHandler) UpdateSetupKey(w http.ResponseWriter, r *http.Request
 	vars := mux.Vars(r)
 	keyID := vars["keyId"]
 	if len(keyID) == 0 {
-		util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "invalid key ID"), w)
+		util.WriteError(r.Context(), status.NewInvalidKeyIDError(), w)
 		return
 	}
 
@@ -181,6 +184,30 @@ func (h *SetupKeysHandler) GetAllSetupKeys(w http.ResponseWriter, r *http.Reques
 	util.WriteJSONObject(r.Context(), w, apiSetupKeys)
 }
 
+func (h *SetupKeysHandler) DeleteSetupKey(w http.ResponseWriter, r *http.Request) {
+	claims := h.claimsExtractor.FromRequestContext(r)
+	accountID, userID, err := h.accountManager.GetAccountIDFromToken(r.Context(), claims)
+	if err != nil {
+		util.WriteError(r.Context(), err, w)
+		return
+	}
+
+	vars := mux.Vars(r)
+	keyID := vars["keyId"]
+	if len(keyID) == 0 {
+		util.WriteError(r.Context(), status.NewInvalidKeyIDError(), w)
+		return
+	}
+
+	err = h.accountManager.DeleteSetupKey(r.Context(), accountID, userID, keyID)
+	if err != nil {
+		util.WriteError(r.Context(), err, w)
+		return
+	}
+
+	util.WriteJSONObject(r.Context(), w, emptyObject{})
+}
+
 func writeSuccess(ctx context.Context, w http.ResponseWriter, key *server.SetupKey) {
 	w.Header().Set("Content-Type", "application/json")
 	w.WriteHeader(200)
@@ -206,7 +233,7 @@ func toResponseBody(key *server.SetupKey) *api.SetupKey {
 
 	return &api.SetupKey{
 		Id:         key.Id,
-		Key:        key.Key,
+		Key:        key.KeySecret,
 		Name:       key.Name,
 		Expires:    key.ExpiresAt,
 		Type:       string(key.Type),

management/server/http/setupkeys_handler_test.go

@@ -67,6 +67,13 @@ func initSetupKeysTestMetaData(defaultKey *server.SetupKey, newKey *server.Setup
 			ListSetupKeysFunc: func(_ context.Context, accountID, userID string) ([]*server.SetupKey, error) {
 				return []*server.SetupKey{defaultKey}, nil
 			},
+
+			DeleteSetupKeyFunc: func(_ context.Context, accountID, userID, keyID string) error {
+				if keyID == defaultKey.Id {
+					return nil
+				}
+				return status.Errorf(status.NotFound, "key %s not found", keyID)
+			},
 		},
 		claimsExtractor: jwtclaims.NewClaimsExtractor(
 			jwtclaims.WithFromRequestContext(func(r *http.Request) jwtclaims.AuthorizationClaims {
@@ -81,18 +88,21 @@ func initSetupKeysTestMetaData(defaultKey *server.SetupKey, newKey *server.Setup
 }
 
 func TestSetupKeysHandlers(t *testing.T) {
-	defaultSetupKey := server.GenerateDefaultSetupKey()
+	defaultSetupKey, _ := server.GenerateDefaultSetupKey()
 	defaultSetupKey.Id = existingSetupKeyID
 
 	adminUser := server.NewAdminUser("test_user")
 
-	newSetupKey := server.GenerateSetupKey(newSetupKeyName, server.SetupKeyReusable, 0, []string{"group-1"},
+	newSetupKey, plainKey := server.GenerateSetupKey(newSetupKeyName, server.SetupKeyReusable, 0, []string{"group-1"},
 		server.SetupKeyUnlimitedUsage, true)
+	newSetupKey.Key = plainKey
 	updatedDefaultSetupKey := defaultSetupKey.Copy()
 	updatedDefaultSetupKey.AutoGroups = []string{"group-1"}
 	updatedDefaultSetupKey.Name = updatedSetupKeyName
 	updatedDefaultSetupKey.Revoked = true
 
+	expectedNewKey := toResponseBody(newSetupKey)
+	expectedNewKey.Key = plainKey
 	tt := []struct {
 		name              string
 		requestType       string
@@ -134,7 +144,7 @@ func TestSetupKeysHandlers(t *testing.T) {
 				[]byte(fmt.Sprintf("{\"name\":\"%s\",\"type\":\"%s\",\"expires_in\":86400, \"ephemeral\":true}", newSetupKey.Name, newSetupKey.Type))),
 			expectedStatus:   http.StatusOK,
 			expectedBody:     true,
-			expectedSetupKey: toResponseBody(newSetupKey),
+			expectedSetupKey: expectedNewKey,
 		},
 		{
 			name:        "Update Setup Key",
@@ -150,6 +160,14 @@ func TestSetupKeysHandlers(t *testing.T) {
 			expectedBody:     true,
 			expectedSetupKey: toResponseBody(updatedDefaultSetupKey),
 		},
+		{
+			name:           "Delete Setup Key",
+			requestType:    http.MethodDelete,
+			requestPath:    "/api/setup-keys/" + defaultSetupKey.Id,
+			requestBody:    bytes.NewBuffer([]byte("")),
+			expectedStatus: http.StatusOK,
+			expectedBody:   false,
+		},
 	}
 
 	handler := initSetupKeysTestMetaData(defaultSetupKey, newSetupKey, updatedDefaultSetupKey, adminUser)
@@ -164,6 +182,7 @@ func TestSetupKeysHandlers(t *testing.T) {
 			router.HandleFunc("/api/setup-keys", handler.CreateSetupKey).Methods("POST", "OPTIONS")
 			router.HandleFunc("/api/setup-keys/{keyId}", handler.GetSetupKey).Methods("GET", "OPTIONS")
 			router.HandleFunc("/api/setup-keys/{keyId}", handler.UpdateSetupKey).Methods("PUT", "OPTIONS")
+			router.HandleFunc("/api/setup-keys/{keyId}", handler.DeleteSetupKey).Methods("DELETE", "OPTIONS")
 			router.ServeHTTP(recorder, req)
 
 			res := recorder.Result()

management/server/metrics/selfhosted.go

@@ -267,7 +267,7 @@ func (w *Worker) generateProperties(ctx context.Context) properties {
 				peersSSHEnabled++
 			}
 
-			if peer.SetupKey == "" {
+			if peer.UserID != "" {
 				userPeers++
 			}