Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify authentication and token management #74

Merged
merged 3 commits into from
Mar 29, 2024

Conversation

nflaig
Copy link
Collaborator

@nflaig nflaig commented Mar 8, 2024

Based on the discussion on discord, clarifies authentication (token format) and adds a few more notes on how the token should be managed by the binary (validator client / remote signer).

These are just the basic requirements a client should support, and ideally be backward compatible for all clients and not break existing tooling (e.g dappnode). The goal is to make it clearer for consumers on what to expect and improve client interoperability.

Additional protections can be implemented based on client specific requirements such as application layer HTTPS or a more secure approach to store the token on the file system.

Related

Comment on lines 16 to 19
- Log the token file path to stdout when running the binary with the key manager API enabled
- Read the token from a file available to the binary, the path to the token file should be configurable
- If the token file does not exist or is empty, generate a new token and write it to the file
- The token should remain the same across multiple restarts of the binary
Copy link
Collaborator

@dapplion dapplion Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would omit the stdout part, focus on the file parameter and follow jwt-secret spec:

Suggested change
- Log the token file path to stdout when running the binary with the key manager API enabled
- Read the token from a file available to the binary, the path to the token file should be configurable
- If the token file does not exist or is empty, generate a new token and write it to the file
- The token should remain the same across multiple restarts of the binary
The keymanager binary SHOULD accept a configuration parameter: `keymanager-secret`, which designates a file containing the hex-encoded token of at least 256 bits. If such a parameter is not given, the client SHOULD generate such a token, valid for the duration of the execution. If such a parameter is given, but the file or token cannot be read, the client SHOULD treat this as an error: either abort the startup, or show error and continue without exposing the keymanager routes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keymanager-secret

What about keymanager-token? this would make it clearer that it's the actual token and not a signing key

If such a parameter is not given, the client SHOULD generate such a token, valid for the duration of the execution

Does this mean if the token is not explicitly set by the user it should be regenerated on every restart? or what is meant by "duration of the execution"?

I think we want to make sure that the token remains the same across multiple process restarts, unless a user explicitly regenerates it by deleting the token file.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a better name? one of the regrets was to call this the keymanager apis in the first place... maybe it can be simplified to just token or api-token?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it can be simplified to just token or api-token?

Not many clients follow the flag name in the engine auth spec either, I don't mind using just token as the flag name here and each client can use their preferred name in the end.

I don't think there is much value in standardizing the name as there are already plenty of flags that are different across clients, so you will have to look at the CLI reference docs either way. The more important part is to have consistent behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against changing our api flags but do we really need to be so prescriptive about the exact flag to use?

The key points are

  • ssl by default, especially non localhost interactions
  • must be authenticated due to the nature of the interactions by default
  • any supplied token should be able to be set by command line argument (reference to existing file).
    • reporting the location that the key is read from would help users and should be considered.
  • some kind of ability to disable ssl purely for devops and testing would be ideal.

For teku at least, our file is just loaded and used as the token value, we've no extra data in that file we're referencing... one of the reasons I think i'd prefer not being prescriptive on the actual flag name is that its possible people have conflicting standards and it might be difficult to find something that satisfies existing standards (as well as potential breaking changes for people needing to rename flags).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rolfyone we can just have a recommendation of MUST have a flag, and SHOULD be named api-token

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or token-file realized it might be confusing that it's a path to a file and not providing the token itself

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or token-file realized it might be confusing that it's a path to a file and not providing the token itself

depends on if we wanna use similar naming convention as jwt-secret, generally speaking token-file is definitely less ambiguous

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--keymanager-token-file is already implemented by nimbus, and is a very descriptive flag imo.

Copy link
Collaborator

@rolfyone rolfyone Mar 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

teku's is --validator-api-bearer-file if that works? could potentially alias it if it makes life easier in a future release...

@james-prysm
Copy link
Collaborator

should we add a new endpoint that shows the token location like what lighthouse did? https://lighthouse-book.sigmaprime.io/api-vc-endpoints.html#get-lighthouseauth

@nflaig
Copy link
Collaborator Author

nflaig commented Mar 12, 2024

should we add a new endpoint that shows the token location like what lighthouse did? https://lighthouse-book.sigmaprime.io/api-vc-endpoints.html#get-lighthouseauth

It was brought up in #9 as well but what's the expected use case for this?

@james-prysm
Copy link
Collaborator

should we add a new endpoint that shows the token location like what lighthouse did? https://lighthouse-book.sigmaprime.io/api-vc-endpoints.html#get-lighthouseauth

It was brought up in #9 as well but what's the expected use case for this?

we just print it in the logs, if it's auto generated there's usually a default path, it might be useful for someone to want to get that value if they lose it. that's mostly it i think?

@nflaig
Copy link
Collaborator Author

nflaig commented Mar 28, 2024

we just print it in the logs, if it's auto generated there's usually a default path, it might be useful for someone to want to get that value if they lose it. that's mostly it i think?

Having a hard time seeing anyone using this, especially since all clients should have a configuration parameter now to set the file path. Could add this later if there is demand for it.

james-prysm added a commit to prysmaticlabs/prysm that referenced this pull request Mar 28, 2024
@james-prysm james-prysm merged commit f99e415 into ethereum:master Mar 29, 2024
2 checks passed
github-merge-queue bot pushed a commit to prysmaticlabs/prysm that referenced this pull request Apr 18, 2024
* 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 <[email protected]>

* Update validator/rpc/auth_token.go

Co-authored-by: Radosław Kapka <[email protected]>

* Update validator/rpc/auth_token.go

Co-authored-by: Radosław Kapka <[email protected]>

* review feedback

---------

Co-authored-by: Radosław Kapka <[email protected]>
rkapka added a commit to prysmaticlabs/prysm that referenced this pull request Apr 30, 2024
* 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 <[email protected]>

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 <[email protected]>

* Update validator/rpc/auth_token.go

Co-authored-by: Radosław Kapka <[email protected]>

* Update validator/rpc/auth_token.go

Co-authored-by: Radosław Kapka <[email protected]>

* review feedback

---------

Co-authored-by: Radosław Kapka <[email protected]>

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 <[email protected]>

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 <[email protected]>
Co-authored-by: james-prysm <[email protected]>

use [32]byte keys in the filesystem cache (#13885)

Co-authored-by: Kasey Kirkham <[email protected]>

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
rkapka added a commit to prysmaticlabs/prysm that referenced this pull request Apr 30, 2024
* 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 <[email protected]>

* Update validator/rpc/auth_token.go

Co-authored-by: Radosław Kapka <[email protected]>

* Update validator/rpc/auth_token.go

Co-authored-by: Radosław Kapka <[email protected]>

* review feedback

---------

Co-authored-by: Radosław Kapka <[email protected]>
@nflaig nflaig mentioned this pull request May 9, 2024
nisdas pushed a commit to prysmaticlabs/prysm that referenced this pull request Jul 4, 2024
* 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 <[email protected]>

* Update validator/rpc/auth_token.go

Co-authored-by: Radosław Kapka <[email protected]>

* Update validator/rpc/auth_token.go

Co-authored-by: Radosław Kapka <[email protected]>

* review feedback

---------

Co-authored-by: Radosław Kapka <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants