Implement DELETE /user/pubkey/:pubKey

api/handlers.go

@@ -2,6 +2,7 @@ package api
 
 import (
 	"context"
+	"crypto/subtle"
 	"encoding/json"
 	"io"
 	"io/ioutil"
@@ -656,6 +657,43 @@ func (api *API) userPUT(u *database.User, w http.ResponseWriter, req *http.Reque
 	api.loginUser(w, u, true)
 }
 
+// userPubKeyDELETE removes a given pubkey from the list of pubkeys associated
+// with this user. It does not require a challenge-response because the used
+// does not need to prove the key is theirs.
+func (api *API) userPubKeyDELETE(u *database.User, w http.ResponseWriter, req *http.Request, ps httprouter.Params) {
+	ctx := req.Context()
+	var pk database.PubKey
+	err := pk.LoadString(ps.ByName("pubKey"))
+	if err != nil {
+		api.WriteError(w, err, http.StatusBadRequest)
+		return
+	}
+	if !u.HasKey(pk) {
+		// This pubkey does not belong to this user.
+		api.WriteError(w, errors.New("the given pubkey is not associated with this user"), http.StatusBadRequest)
+		return
+	}
+	// Find the position of the pubkey in the list.
+	keyIdx := -1
+	for i, k := range u.PubKeys {
+		if subtle.ConstantTimeCompare(pk[:], k[:]) == 1 {
+			keyIdx = i
+			break
+		}
+	}
+	if keyIdx == -1 {
+		build.Critical("Reaching this should be impossible. It would indicate a concurrent change of the user struct.")
+	}
+	// Remove the pubkey.
+	u.PubKeys = append(u.PubKeys[:keyIdx], u.PubKeys[keyIdx+1:]...)
+	err = api.staticDB.UserSave(ctx, u)
+	if err != nil {
+		api.WriteError(w, err, http.StatusInternalServerError)
+		return
+	}
+	api.WriteSuccess(w)
+}
+
 // userPubKeyRegisterGET generates an update challenge for the caller.
 func (api *API) userPubKeyRegisterGET(u *database.User, w http.ResponseWriter, req *http.Request, _ httprouter.Params) {
 	ctx := req.Context()

api/routes.go

@@ -16,7 +16,7 @@ import (
 var (
 	// APIKeyHeader holds the name of the header we use for API keys. This
 	// header name matches the established standard used by Swagger and others.
-	APIKeyHeader = "Skynet-API-Key"
+	APIKeyHeader = "Skynet-API-Key" // #nosec
 	// ErrNoAPIKey is an error returned when we expect an API key but we don't
 	// find one.
 	ErrNoAPIKey = errors.New("no api key found")
@@ -55,6 +55,7 @@ func (api *API) buildHTTPRoutes() {
 	api.staticRouter.DELETE("/user", api.withAuth(api.userDELETE))
 	api.staticRouter.GET("/user/limits", api.noAuth(api.userLimitsGET))
 	api.staticRouter.GET("/user/stats", api.withAuth(api.userStatsGET))
+	api.staticRouter.DELETE("/user/pubkey/:pubKey", api.WithDBSession(api.withAuth(api.userPubKeyDELETE)))
 	api.staticRouter.GET("/user/pubkey/register", api.WithDBSession(api.withAuth(api.userPubKeyRegisterGET)))
 	api.staticRouter.POST("/user/pubkey/register", api.WithDBSession(api.withAuth(api.userPubKeyRegisterPOST)))
 	api.staticRouter.GET("/user/uploads", api.withAuth(api.userUploadsGET))

main.go

@@ -53,12 +53,12 @@ const (
 	envServerDomain = "SERVER_DOMAIN"
 	// envStripeAPIKey hold the name of the environment variable for Stripe's
 	// API key. It's only required when integrating with Stripe.
-	envStripeAPIKey = "STRIPE_API_KEY"
+	envStripeAPIKey = "STRIPE_API_KEY" // #nosec
 	// envMaxNumAPIKeysPerUser hold the name of the environment variable which
 	// sets the limit for number of API keys a single user can create. If a user
 	// reaches that limit they can always delete some API keys in order to make
 	// space for new ones.
-	envMaxNumAPIKeysPerUser = "ACCOUNTS_MAX_NUM_API_KEYS_PER_USER"
+	envMaxNumAPIKeysPerUser = "ACCOUNTS_MAX_NUM_API_KEYS_PER_USER" // #nosec
 )
 
 type (

test/api/challenge_test.go

@@ -309,3 +309,93 @@ func testUserAddPubKey(t *testing.T, at *test.AccountsTester) {
 		t.Fatalf("Expected pubKey '%s', got '%s',", hex.EncodeToString(pk[:]), hex.EncodeToString(u3.PubKeys[0]))
 	}
 }
+
+// testUserDeletePubKey ensures that users can delete pubkeys from their
+// accounts.
+func testUserDeletePubKey(t *testing.T, at *test.AccountsTester) {
+	name := test.DBNameForTest(t.Name())
+	u, c, err := test.CreateUserAndLogin(at, name)
+	if err != nil {
+		t.Fatal("Failed to create a user and log in:", err)
+	}
+	defer func() {
+		if err = u.Delete(at.Ctx); err != nil {
+			t.Error(errors.AddContext(err, "failed to delete user in defer"))
+		}
+	}()
+	at.SetCookie(c)
+	defer at.ClearCredentials()
+
+	sk, pk := crypto.GenerateKeyPair()
+	var ch database.Challenge
+
+	// Request a new challenge.
+	queryParams := url.Values{}
+	queryParams.Set("pubKey", hex.EncodeToString(pk[:]))
+	r, b, err := at.Get("/user/pubkey/register", queryParams)
+	err = json.Unmarshal(b, &ch)
+	if err != nil || r.StatusCode != http.StatusOK {
+		t.Fatal("Failed to get a challenge:", err, r.Status, string(b))
+	}
+	chBytes, err := hex.DecodeString(ch.Challenge)
+	if err != nil {
+		t.Fatal("Invalid challenge:", err)
+	}
+	// Solve the challenge.
+	response := append(chBytes, append([]byte(database.ChallengeTypeUpdate), []byte(database.PortalName)...)...)
+	bodyParams := url.Values{}
+	bodyParams.Set("response", hex.EncodeToString(response))
+	bodyParams.Set("signature", hex.EncodeToString(ed25519.Sign(sk[:], response)))
+	r, b, err = at.Post("/user/pubkey/register", nil, bodyParams)
+	if err != nil {
+		t.Fatalf("Failed to confirm the update. Status %d, body '%s', error '%s'", r.StatusCode, string(b), err)
+	}
+	// Make sure the user's pubKey is properly set.
+	u1, err := at.DB.UserBySub(at.Ctx, u.Sub)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(u1.PubKeys) != 1 {
+		t.Fatal("Expected one pubkey assigned, got none.")
+	}
+	if subtle.ConstantTimeCompare(u1.PubKeys[0], pk[:]) != 1 {
+		t.Fatalf("Expected pubKey '%s', got '%s',", hex.EncodeToString(pk[:]), hex.EncodeToString(u1.PubKeys[0]))
+	}
+
+	// Call DELETE without a cookie.
+	at.ClearCredentials()
+	r, b, err = at.Delete("/user/pubkey/"+hex.EncodeToString(pk[:]), nil)
+	if err == nil || r.StatusCode != http.StatusUnauthorized {
+		t.Fatalf("Expected to fail with 401. Status %d, body '%s', error '%s'", r.StatusCode, string(b), err)
+	}
+	at.SetCookie(c)
+	// Call DELETE with an invalid key.
+	r, b, err = at.Delete("/user/pubkey/INVALID_KEY", nil)
+	if err == nil || r.StatusCode != http.StatusBadRequest {
+		t.Fatalf("Expected to fail with 400. Status %d, body '%s', error '%s'", r.StatusCode, string(b), err)
+	}
+	_, pk1 := crypto.GenerateKeyPair()
+	// Call DELETE with a key that doesn't belong to this user.
+	r, b, err = at.Delete("/user/pubkey/"+hex.EncodeToString(pk1[:]), nil)
+	if err == nil || r.StatusCode != http.StatusBadRequest {
+		t.Fatalf("Expected to fail with 400. Status %d, body '%s', error '%s'", r.StatusCode, string(b), err)
+	}
+	// Call DELETE with correct parameters.
+	r, b, err = at.Delete("/user/pubkey/"+hex.EncodeToString(pk[:]), nil)
+	if err != nil || r.StatusCode != http.StatusNoContent {
+		t.Fatalf("Expected to succeed. Status %d, body '%s', error '%s'", r.StatusCode, string(b), err)
+	}
+	// Verify that the key was deleted.
+	u2, err := at.DB.UserBySub(at.Ctx, u.Sub)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(u2.PubKeys) > 0 {
+		t.Fatal("Expected no public keys, got", len(u2.PubKeys))
+	}
+	// Call DELETE with the already deleted key.
+	r, b, err = at.Delete("/user/pubkey/"+hex.EncodeToString(pk[:]), nil)
+	if err == nil || r.StatusCode != http.StatusBadRequest {
+		t.Fatalf("Expected to fail with 400. Status %d, body '%s', error '%s'", r.StatusCode, string(b), err)
+	}
+}

test/api/handlers_test.go

@@ -59,6 +59,7 @@ func TestHandlers(t *testing.T) {
 		{name: "LoginLogout", test: testHandlerLoginPOST},
 		{name: "UserEdit", test: testUserPUT},
 		{name: "UserAddPubKey", test: testUserAddPubKey},
+		{name: "DeletePubKey", test: testUserDeletePubKey},
 		{name: "UserDelete", test: testUserDELETE},
 		{name: "UserLimits", test: testUserLimits},
 		{name: "UserDeleteUploads", test: testUserUploadsDELETE},