From 54656e172fe23124bf2285939d9eda000e6e920f Mon Sep 17 00:00:00 2001 From: james-prysm <90280386+james-prysm@users.noreply.github.com> Date: Tue, 29 Oct 2024 09:37:14 -0500 Subject: [PATCH] keymanager API: bug fixes and inconsistencies (#14586) * fixing error handling and returning empty requests with the wrong wallet type * changelog * adding some unit tests * Update validator/rpc/handlers_keymanager.go Co-authored-by: Sammy Rosso <15244892+saolyn@users.noreply.github.com> * Update CHANGELOG.md Co-authored-by: Sammy Rosso <15244892+saolyn@users.noreply.github.com> * Update validator/rpc/handlers_keymanager.go Co-authored-by: Sammy Rosso <15244892+saolyn@users.noreply.github.com> * Update validator/rpc/intercepter.go Co-authored-by: Sammy Rosso <15244892+saolyn@users.noreply.github.com> * Update validator/rpc/intercepter_test.go Co-authored-by: Sammy Rosso <15244892+saolyn@users.noreply.github.com> * Update validator/rpc/intercepter_test.go Co-authored-by: Sammy Rosso <15244892+saolyn@users.noreply.github.com> * lint --------- Co-authored-by: Sammy Rosso <15244892+saolyn@users.noreply.github.com> --- CHANGELOG.md | 2 ++ validator/rpc/BUILD.bazel | 1 + validator/rpc/handlers_keymanager.go | 12 ++++++++++-- validator/rpc/handlers_keymanager_test.go | 20 ++++++++++++++++++++ validator/rpc/intercepter.go | 7 ++++--- validator/rpc/intercepter_test.go | 11 +++++++++++ 6 files changed, 48 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1f6279196..22acddd53e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,8 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve cache impossible. This has been fixed with updates to rules_oci and bazel-lib. - Fixed an issue where the length check between block body KZG commitments and the existing cache from the database was incompatible. - Fix `--backfill-oldest-slot` handling - this flag was totally broken, the code would always backfill to the default slot [pr](https://github.com/prysmaticlabs/prysm/pull/14584) +- Fix keymanager API should return corrected error format for malformed tokens +- Fix keymanager API so that get keys returns an empty response instead of a 500 error when using an unsupported keystore. ### Security diff --git a/validator/rpc/BUILD.bazel b/validator/rpc/BUILD.bazel index 43c039ec91..7eafb93d23 100644 --- a/validator/rpc/BUILD.bazel +++ b/validator/rpc/BUILD.bazel @@ -117,6 +117,7 @@ go_test( "//encoding/bytesutil:go_default_library", "//io/file:go_default_library", "//io/logs/mock:go_default_library", + "//network/httputil:go_default_library", "//proto/prysm/v1alpha1:go_default_library", "//testing/assert:go_default_library", "//testing/require:go_default_library", diff --git a/validator/rpc/handlers_keymanager.go b/validator/rpc/handlers_keymanager.go index 39b8fd2962..9a178a7ccf 100644 --- a/validator/rpc/handlers_keymanager.go +++ b/validator/rpc/handlers_keymanager.go @@ -50,7 +50,11 @@ func (s *Server) ListKeystores(w http.ResponseWriter, r *http.Request) { return } if s.wallet.KeymanagerKind() != keymanager.Derived && s.wallet.KeymanagerKind() != keymanager.Local { - httputil.HandleError(w, errors.Wrap(err, "Prysm validator keys are not stored locally with this keymanager type").Error(), http.StatusInternalServerError) + log.Debugf("List keystores keymanager api expected wallet type %s but got %s", s.wallet.KeymanagerKind().String(), keymanager.Local.String()) + response := &ListKeystoresResponse{ + Data: make([]*Keystore, 0), + } + httputil.WriteJson(w, response) return } pubKeys, err := km.FetchValidatingPublicKeys(ctx) @@ -402,7 +406,11 @@ func (s *Server) ListRemoteKeys(w http.ResponseWriter, r *http.Request) { return } if s.wallet.KeymanagerKind() != keymanager.Web3Signer { - httputil.HandleError(w, "Prysm Wallet is not of type Web3Signer. Please execute validator client with web3signer flags.", http.StatusInternalServerError) + log.Debugf("List remote keys keymanager api expected wallet type %s but got %s", s.wallet.KeymanagerKind().String(), keymanager.Web3Signer.String()) + response := &ListKeystoresResponse{ + Data: make([]*Keystore, 0), + } + httputil.WriteJson(w, response) return } pubKeys, err := km.FetchValidatingPublicKeys(ctx) diff --git a/validator/rpc/handlers_keymanager_test.go b/validator/rpc/handlers_keymanager_test.go index 9257753599..0614db2cc0 100644 --- a/validator/rpc/handlers_keymanager_test.go +++ b/validator/rpc/handlers_keymanager_test.go @@ -119,6 +119,16 @@ func TestServer_ListKeystores(t *testing.T) { ) } }) + t.Run("calling list remote while using a local wallet returns empty", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, fmt.Sprintf("/eth/v1/remotekeys"), nil) + wr := httptest.NewRecorder() + wr.Body = &bytes.Buffer{} + s.ListRemoteKeys(wr, req) + require.Equal(t, http.StatusOK, wr.Code) + resp := &ListRemoteKeysResponse{} + require.NoError(t, json.Unmarshal(wr.Body.Bytes(), resp)) + require.Equal(t, 0, len(resp.Data)) + }) } func TestServer_ImportKeystores(t *testing.T) { @@ -1366,6 +1376,16 @@ func TestServer_ListRemoteKeys(t *testing.T) { require.DeepEqual(t, hexutil.Encode(expectedKeys[i][:]), resp.Data[i].Pubkey) } }) + t.Run("calling list keystores while using a remote wallet returns empty", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, fmt.Sprintf("/eth/v1/keystores"), nil) + wr := httptest.NewRecorder() + wr.Body = &bytes.Buffer{} + s.ListKeystores(wr, req) + require.Equal(t, http.StatusOK, wr.Code) + resp := &ListKeystoresResponse{} + require.NoError(t, json.Unmarshal(wr.Body.Bytes(), resp)) + require.Equal(t, 0, len(resp.Data)) + }) } func TestServer_ImportRemoteKeys(t *testing.T) { diff --git a/validator/rpc/intercepter.go b/validator/rpc/intercepter.go index 1f574a433e..eb42755237 100644 --- a/validator/rpc/intercepter.go +++ b/validator/rpc/intercepter.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/prysmaticlabs/prysm/v5/api" + "github.com/prysmaticlabs/prysm/v5/network/httputil" "github.com/sirupsen/logrus" "google.golang.org/grpc" "google.golang.org/grpc/codes" @@ -41,18 +42,18 @@ func (s *Server) AuthTokenHandler(next http.Handler) http.Handler { // ignore some routes reqToken := r.Header.Get("Authorization") if reqToken == "" { - http.Error(w, "unauthorized: no Authorization header passed. Please use an Authorization header with the jwt created in the prysm wallet", http.StatusUnauthorized) + httputil.HandleError(w, "Unauthorized: no Authorization header passed. Please use an Authorization header with the jwt created in the prysm wallet", http.StatusUnauthorized) return } tokenParts := strings.Split(reqToken, "Bearer ") if len(tokenParts) != 2 { - http.Error(w, "Invalid token format", http.StatusBadRequest) + httputil.HandleError(w, "Invalid token format", http.StatusBadRequest) return } token := tokenParts[1] if strings.TrimSpace(token) != s.authToken || strings.TrimSpace(s.authToken) == "" { - http.Error(w, "Forbidden: token value is invalid", http.StatusForbidden) + httputil.HandleError(w, "Forbidden: token value is invalid", http.StatusForbidden) return } } diff --git a/validator/rpc/intercepter_test.go b/validator/rpc/intercepter_test.go index 94a0461da4..85b7c7a565 100644 --- a/validator/rpc/intercepter_test.go +++ b/validator/rpc/intercepter_test.go @@ -2,11 +2,13 @@ package rpc import ( "context" + "encoding/json" "net/http" "net/http/httptest" "testing" "github.com/prysmaticlabs/prysm/v5/api" + "github.com/prysmaticlabs/prysm/v5/network/httputil" "github.com/prysmaticlabs/prysm/v5/testing/require" "google.golang.org/grpc" "google.golang.org/grpc/metadata" @@ -72,6 +74,9 @@ func TestServer_AuthTokenHandler(t *testing.T) { require.NoError(t, err) testHandler.ServeHTTP(rr, req) require.Equal(t, http.StatusUnauthorized, rr.Code) + errJson := &httputil.DefaultJsonError{} + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), errJson)) + require.StringContains(t, "Unauthorized", errJson.Message) }) t.Run("wrong auth token was sent", func(t *testing.T) { rr := httptest.NewRecorder() @@ -80,6 +85,9 @@ func TestServer_AuthTokenHandler(t *testing.T) { req.Header.Set("Authorization", "Bearer YOUR_JWT_TOKEN") // Replace with a valid JWT token testHandler.ServeHTTP(rr, req) require.Equal(t, http.StatusForbidden, rr.Code) + errJson := &httputil.DefaultJsonError{} + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), errJson)) + require.StringContains(t, "token value is invalid", errJson.Message) }) t.Run("good auth token was sent", func(t *testing.T) { rr := httptest.NewRecorder() @@ -95,6 +103,9 @@ func TestServer_AuthTokenHandler(t *testing.T) { require.NoError(t, err) testHandler.ServeHTTP(rr, req) require.Equal(t, http.StatusUnauthorized, rr.Code) + errJson := &httputil.DefaultJsonError{} + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), errJson)) + require.StringContains(t, "Unauthorized", errJson.Message) }) t.Run("initialize does not need auth", func(t *testing.T) { rr := httptest.NewRecorder()