consistent auth token for validator apis #13747

james-prysm · 2024-03-14T22:15:33Z

What type of PR is this?

Other

What does this PR do? Why is it needed?

This PR gives the capabilities to set an auth token via flag, it also removes the jwt component and simply uses the value as an opaque token.

a documentation page should be updated following this change https://docs.prylabs.network/docs/execution-node/authentication

Which issues(s) does this PR fix?

related to ethereum/keymanager-APIs#74

Other notes for review

james-prysm · 2024-03-15T22:18:46Z

validator/rpc/intercepter.go

-	_, err := jwt.Parse(token, s.validateJWT)
-	if err != nil {
-		return status.Errorf(codes.Unauthenticated, "Could not parse JWT token: %v", err)
+	if token != s.authToken || s.authToken == "" {


should empty string be allowed?

validator/rpc/auth_token.go

nalepae · 2024-03-18T10:17:14Z

When clicking on the link, got:

[2024-03-18 11:15:44]  INFO rpc: Once your validator process is running, navigate to the link below to authenticate with the Prysm web interface
[2024-03-18 11:15:44]  INFO rpc: http://127.0.0.1:7500/initialize?token=<xxx>

bazel run //cmd/validator -- --holesky --wallet-password-file /Users/manu/Downloads/password.txt --suggested-fee-recipient <yyy> --enable-beacon-rest-api --rpc

nalepae · 2024-03-18T09:38:39Z

cmd/validator/flags/flags.go

+
+	// AuthTokenPathFlag defines the path to the auth token used to secure the validator api.
+	AuthTokenPathFlag = &cli.StringFlag{
+		Name:  "token",


token name could make the user believe he would write the token itself after --token, while actually --token waits for a path to a file containing a token.

lets change the name

nalepae · 2024-03-18T09:51:49Z

cmd/validator/web/web.go

@@ -43,7 +45,12 @@ var Commands = &cli.Command{
 				gatewayHost := cliCtx.String(flags.GRPCGatewayHost.Name)
 				gatewayPort := cliCtx.Int(flags.GRPCGatewayPort.Name)
 				validatorWebAddr := fmt.Sprintf("%s:%d", gatewayHost, gatewayPort)
-				if err := rpc.CreateAuthToken(walletDirPath, validatorWebAddr); err != nil {
+				authTokenPath := filepath.Join(walletDirPath, api.AuthTokenFileName)


--help does not show any description:

➜ prysm git:(auth-token) ✗ bazel run //cmd/validator -- --holesky web INFO: Analyzed target //cmd/validator:validator (0 packages loaded, 0 targets configured). INFO: From GoLink cmd/validator/validator_/validator: ld: warning: ignoring duplicate libraries: '-lc++', '-lm' INFO: Found 1 target... Target //cmd/validator:validator up-to-date: bazel-bin/cmd/validator/validator_/validator INFO: Elapsed time: 2.016s, Critical Path: 1.74s INFO: 3 processes: 1 internal, 2 darwin-sandbox. INFO: Build completed successfully, 3 total actions INFO: Running command line: bazel-bin/cmd/validator/validator_/validator --holesky web NAME: validator web - Defines commands for interacting with the Prysm web interface. USAGE: validator web command [command options] [arguments...] COMMANDS: generate-auth-token help, h Shows a list of commands or help for one command

nalepae · 2024-03-18T09:54:20Z

cmd/validator/web/web.go

@@ -43,7 +45,12 @@ var Commands = &cli.Command{
 				gatewayHost := cliCtx.String(flags.GRPCGatewayHost.Name)
 				gatewayPort := cliCtx.Int(flags.GRPCGatewayPort.Name)
 				validatorWebAddr := fmt.Sprintf("%s:%d", gatewayHost, gatewayPort)
-				if err := rpc.CreateAuthToken(walletDirPath, validatorWebAddr); err != nil {
+				authTokenPath := filepath.Join(walletDirPath, api.AuthTokenFileName)
+				tempAuthTokenPath := cliCtx.String(flags.AuthTokenPathFlag.Name)


AuthTokenPathFlag is used here but not listed in Flags.

AcceptTosFlag is listed in Flags but not used here.

not sure what this means, i think accepttosflag is at the edge where the command is launched

nalepae · 2024-03-18T09:57:41Z

validator/node/node.go

@@ -639,6 +639,15 @@ func (c *ValidatorClient) registerRPCService(router *mux.Router) error {
 	walletDir := c.cliCtx.String(flags.WalletDirFlag.Name)
 	grpcHeaders := c.cliCtx.String(flags.GrpcHeadersFlag.Name)
 	clientCert := c.cliCtx.String(flags.CertFlag.Name)
+
+	authTokenPath := c.cliCtx.String(flags.AuthTokenPathFlag.Name)


Maybe it would be worth to add some comments here to explain how c.cliCtx.String(flags.AuthTokenPathFlag.Name) could be "" while flags.AuthTokenPathFlag.Name is not "".

validator/rpc/auth_token.go

nalepae · 2024-03-18T10:02:59Z

validator/rpc/auth_token.go

+		jwtKeyHex := strings.TrimSpace(lines[0])
+		secret, err = hex.DecodeString(jwtKeyHex)
+		if err != nil {
+			return


No test covers this case.

nalepae · 2024-03-18T10:04:46Z

validator/rpc/intercepter.go

-	_, err := jwt.Parse(token, s.validateJWT)
-	if err != nil {
-		return status.Errorf(codes.Unauthenticated, "Could not parse JWT token: %v", err)
+	if token != s.authToken || s.authToken == "" {


nalepae · 2024-03-18T10:09:50Z

validator/rpc/server.go

+	}
+
+	if server.authTokenPath != "" {
+		log.Infof("at server start %s", server.authTokenPath)


[2024-03-18 11:07:50] INFO rpc: at server start /Users/manu/Library/Eth2Validators/prysm-wallet-v2/auth-token

This log does not start with a capital letter and is not very clear to me.

this should be removed*

rkapka · 2024-03-29T04:17:21Z

api/jwt.go

+	n, err := randGen.Read(secret)
+	if err != nil {
+		return "", err
+	} else if n <= 0 {


Do we even need this check? According to the doc it's the length of the slice and we already know it's 32. If you insist on having it, then it should be n != 32.

func (r *Rand) Read(p []byte) (n int, err error) Read generates len(p) random bytes and writes them into p. It always returns len(p) and a nil error.

this was something existing i just left it as is

validator/rpc/auth_token.go

cmd/validator/flags/flags.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

rauljordan

The changes make sense to me, but the only concern is just trying it out at runtime to be safe

rauljordan · 2024-04-17T16:12:04Z

cmd/validator/flags/flags.go

+		Name:    "keymanager-token-file",
+		Usage:   "Path to auth token file used for validator apis.",
+		Value:   filepath.Join(filepath.Join(DefaultValidatorDir(), WalletDefaultDirName), api.AuthTokenFileName),
+		Aliases: []string{"validator-api-bearer-file"},


we should probably include the term jwt here, for when people grep through help text

nvm...seems like the old thing is called jwt

rauljordan · 2024-04-17T16:13:10Z

validator/rpc/auth_token.go

@@ -106,16 +92,20 @@ func (s *Server) refreshAuthTokenFromFileChanges(ctx context.Context, authTokenP
 	}
 	for {
 		select {
-		case <-watcher.Events:
+		case event := <-watcher.Events:
+			if event.Op.String() == "REMOVE" {


should we trim string or any other safe things here to make sure we don't miss this request?

yeah I can add something or see if there is a safer way to do this

out on vacation, raul reviewed and oked it, other items will be addressed in a followup pr

james-prysm · 2024-04-18T16:26:46Z

merging in with smaller items taken care of in a separate PR. tested locally via webui

@terencechain

* GET * POST * Revert "Auxiliary commit to revert individual files from 615feb1" This reverts commit 55cf071c684019f3d6124179154c10b2277fda49. * comment fix * deepsource config values block protos get_committee_indices proto and ssz attestation interface Revert "Auxiliary commit to revert individual files from deadb21" This reverts commit 32ad5009537bc5ec0e6caf9f52143d380d00be85. todos get_attesting_indices Revert "Auxiliary commit to revert individual files from dd27897" This reverts commit f39644ed3cb6f3964fc6c86fdf4bd5de2a9668c8. beacon spec changes Fix pending attestation. Build ok Don't return error that can be internally handled (#13887) Co-authored-by: Kasey Kirkham <kasey@users.noreply.github.com> consistent auth token for validator apis (#13747) * wip * fixing tests * adding more tests especially to handle legacy * fixing linting * fixing deepsource issues and flags * fixing some deepsource issues,pathing issues, and logs * some review items * adding additional review feedback * updating to follow updates from ethereum/keymanager-APIs#74 * adjusting functions to match changes in keymanagers PR * Update validator/rpc/auth_token.go Co-authored-by: Radosław Kapka <rkapka@wp.pl> * Update validator/rpc/auth_token.go Co-authored-by: Radosław Kapka <rkapka@wp.pl> * Update validator/rpc/auth_token.go Co-authored-by: Radosław Kapka <rkapka@wp.pl> * review feedback --------- Co-authored-by: Radosław Kapka <rkapka@wp.pl> Use correct port for health check in Beacon API e2e evaluator (#13892) Change example.org DNS record (#13904) DNS Changed for this record causing tests to fail Co-authored-by: Radosław Kapka <rkapka@wp.pl> Electra beacon config (#13907) * Update spectests to v1.5.0 * Add electra config * Fix tests in beacon-chain/rpc/eth/config * gofmt Simplify prune invalid by reusing existing fork choice store call (#13878) Do not remove blobs DB in slasher. (#13881) Refactor beacon-chain/core/helpers tests to be black box (#13906) spectests: fail hard on missing test folders (#13913) Revert "zig: Update zig to recent main branch commit (#13142)" (#13908) This reverts commit b24b60d. Remove EnableEIP4881 flag (#13826) * Remove EnableEIP4881 flag * Gaz * Fix missing error handler * Remove old tree and fix tests * Gaz * Fix build import * Replace depositcache * Add pendingDeposit tests * Nishant's fix * Fix unsafe uint64 to int * Fix other unsafe uint64 to int * Remove: RemovePendingDeposit * Deprecate and remove DisableEIP4881 flag * Check: index not greater than deposit count * Move index check Protobufs for Electra devnet-0 (#13905) * block protos * proto and ssz * stubs * Enable Electra spec test * Pull in EIP-7251 protobuf changes From: #13903 * All EIP7549 containers are passing * All EIP7251 containers passing * including changes from eip7002 * Everything passing except for beacon state hash tree root * fixing eletra state to use electra payload * Fix minimal test. Skip beacon state test * Perston's feedback --------- Co-authored-by: rkapka <radoslaw.kapka@gmail.com> Co-authored-by: james-prysm <james@prysmaticlabs.com> use [32]byte keys in the filesystem cache (#13885) Co-authored-by: Kasey Kirkham <kasey@users.noreply.github.com> Electra: full beacon config (#13918) * Electra: full beacon config Fix TestGetSpec * Fix beacon config spec compliance test so that it properly loads the config from spec tests. Tests failing for now. * fix tests and comply with spec presets beacon-chain/cache: Convert tests to cache_test blackbox testing (#13920) * beacon-chain/cache: convert to blackbox tests (package cache_test) * Move balanceCacheKey to its own file to satisify go fuzz build Electra: Minor proto changes, cloner additions (#13923) * Electra: more proto changes * Roundtrip test for cloners Electra: HTR util for DepositReceipt and ExecutionLayerWithdrawalRequest (#13924) * Electra: HTR utils for DepositReceipts and ExecutionLayerWithdrawalRequests * Tests for HTR utils Electra: beacon-chain/core/helpers (#13921) * Electra helpers * Electra helper tests and other fixes * @terencechain feedback Electra: add electra version Electra: consensus types gocognit exclusion @potuz's suggestion build fix interfaces for indexed att and slashing indexed att usage BuildSignedBeaconBlockFromExecutionPayload slashing usage grpc stubs remove unused methods Electra attestation interfaces

* wip * fixing tests * adding more tests especially to handle legacy * fixing linting * fixing deepsource issues and flags * fixing some deepsource issues,pathing issues, and logs * some review items * adding additional review feedback * updating to follow updates from ethereum/keymanager-APIs#74 * adjusting functions to match changes in keymanagers PR * Update validator/rpc/auth_token.go Co-authored-by: Radosław Kapka <rkapka@wp.pl> * Update validator/rpc/auth_token.go Co-authored-by: Radosław Kapka <rkapka@wp.pl> * Update validator/rpc/auth_token.go Co-authored-by: Radosław Kapka <rkapka@wp.pl> * review feedback --------- Co-authored-by: Radosław Kapka <rkapka@wp.pl>

james-prysm added 2 commits March 14, 2024 13:37

wip

b5dfd5e

fixing tests

d575b2f

james-prysm added Validator Client UX cosmetic / user experience related API Api related tasks labels Mar 14, 2024

james-prysm and others added 5 commits March 15, 2024 13:49

adding more tests especially to handle legacy

ad72aa4

fixing linting

cb156d7

Merge branch 'develop' into auth-token

3629ebe

fixing deepsource issues and flags

82c0b33

fixing some deepsource issues,pathing issues, and logs

7435093

james-prysm added the Blocked Blocked by research or external factors label Mar 15, 2024

james-prysm commented Mar 15, 2024

View reviewed changes

validator/rpc/auth_token.go Outdated Show resolved Hide resolved

james-prysm commented Mar 15, 2024

View reviewed changes

validator/rpc/auth_token.go Outdated Show resolved Hide resolved

james-prysm marked this pull request as ready for review March 15, 2024 22:22

james-prysm requested a review from a team as a code owner March 15, 2024 22:22

james-prysm requested review from potuz, rkapka, nisdas and nalepae and removed request for potuz March 15, 2024 22:22

nalepae previously requested changes Mar 18, 2024

View reviewed changes

james-prysm and others added 7 commits March 18, 2024 10:40

Merge branch 'develop' into auth-token

352812c

Merge branch 'develop' into auth-token

76920bd

some review items

4f7ad89

Merge branch 'develop' into auth-token

377f53b

Merge branch 'develop' into auth-token

722c096

adding additional review feedback

4a0fc69

Merge branch 'develop' into auth-token

6d20c9b

james-prysm and others added 4 commits March 28, 2024 10:33

Merge branch 'develop' into auth-token

a2c9e52

updating to follow updates from ethereum/keymanager-APIs#74

2b9eda6

adjusting functions to match changes in keymanagers PR

1173ad8

Merge branch 'develop' into auth-token

415f6ad

rkapka reviewed Mar 29, 2024

View reviewed changes

nalepae reviewed Mar 29, 2024

View reviewed changes

cmd/validator/flags/flags.go Outdated Show resolved Hide resolved

james-prysm and others added 4 commits March 29, 2024 09:04

Update validator/rpc/auth_token.go

00a2ca6

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

Update validator/rpc/auth_token.go

6a01eb4

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

Update validator/rpc/auth_token.go

9b2c1e3

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

Merge branch 'develop' into auth-token

ad42e4b

james-prysm mentioned this pull request Mar 29, 2024

add jwt generation for validator client prysmaticlabs/documentation#933

Open

james-prysm and others added 3 commits March 29, 2024 10:56

review feedback

25741af

Merge branch 'develop' into auth-token

e96ca2c

Merge branch 'develop' into auth-token

2af2f05

rauljordan approved these changes Apr 17, 2024

View reviewed changes

Merge branch 'develop' into auth-token

2aea1c9

james-prysm removed the Blocked Blocked by research or external factors label Apr 18, 2024

james-prysm added this pull request to the merge queue Apr 18, 2024

Merged via the queue into develop with commit feb16ae Apr 18, 2024
16 of 17 checks passed

james-prysm deleted the auth-token branch April 18, 2024 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

consistent auth token for validator apis #13747

consistent auth token for validator apis #13747

james-prysm commented Mar 14, 2024 •

edited

Loading

james-prysm Mar 15, 2024

nalepae Mar 18, 2024

nalepae commented Mar 18, 2024 •

edited

Loading

nalepae Mar 18, 2024

james-prysm Mar 20, 2024

nalepae Mar 18, 2024

nalepae Mar 18, 2024

james-prysm Mar 21, 2024

nalepae Mar 18, 2024

nalepae Mar 18, 2024

nalepae Mar 18, 2024

nalepae Mar 18, 2024

james-prysm Mar 21, 2024

rkapka Mar 29, 2024

james-prysm Mar 29, 2024

rauljordan left a comment

rauljordan Apr 17, 2024

rauljordan Apr 17, 2024

rauljordan Apr 17, 2024

james-prysm Apr 17, 2024

james-prysm commented Apr 18, 2024

consistent auth token for validator apis #13747

consistent auth token for validator apis #13747

Conversation

james-prysm commented Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalepae commented Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rauljordan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-prysm commented Apr 18, 2024

james-prysm commented Mar 14, 2024 •

edited

Loading

nalepae commented Mar 18, 2024 •

edited

Loading