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

eth: Add authenticated geth rpc capabilities. #1963

Merged
merged 1 commit into from
Dec 29, 2022

Conversation

JoeGruffins
Copy link
Member

@JoeGruffins JoeGruffins commented Nov 17, 2022

eth: Add authenticated geth rpc capabilities.

Add ability for client and server to connect over authenticated
websocket to a geth full node.

depends on #1733
closes #1956

@JoeGruffins JoeGruffins force-pushed the simnetethok branch 6 times, most recently from 4290121 to 6849f41 Compare November 25, 2022 07:10
Copy link
Member Author

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

I hope the typescript code doesn't hurt your eyes.

@@ -73,7 +74,7 @@ const (
walletTypeGeth = "geth"
walletTypeRPC = "rpc"

providersKey = "providers"
providersKey = "providersv1"
Copy link
Member Author

Choose a reason for hiding this comment

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

In this pr changing the type of value for the providers so currently saved settings will not parse correctly. Since eth is not being used by anyone yet probably this easy change is fine I think, but this would be a problem in production code.

Comment on lines -14 to -17
//
// TODO: Running these tests many times on simnet eventually results in all
// transactions returning "unexpected error for test ok: exceeds block gas
// limit". Find out why that is.
Copy link
Member Author

Choose a reason for hiding this comment

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

Have not seen this for the longest and never on testnet. Ok to remove I'm sure.

Comment on lines 24 to 30
<div data-tmpl="repeatableInputOutter" class="w-100 repeatable">
<div data-tmpl="repeatableInput" class="w-100 repeatable">
<label class="form-label ps-1 mb-1 small"> <span class="ico-info"></span></label>
<div class="d-flex align-items-stretch justify-content-center">
<input type="text" class="form-control select flex-grow-1">
<div class="ico-plus fs14 p-1 ms-2 mt-1 pointer hoverbg" data-tmpl="add"></div>
</div>
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the best answer I could come up with for the generic repeatables to have multiple fields. Probably others have a better solution, but this seems to work ok. The idea is that we can add new repeatables easier if we have the outter container, and we can keep them divided if there were more repeatable type options. Right now we only have the eth providers.

Copy link
Member

Choose a reason for hiding this comment

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

I may have a plan for this, but I'll follow up. Not really fleshed out yet.

Comment on lines -47 to -48
export DELTA_HTTP_PORT="38556"
export DELTA_WS_PORT="38557"
Copy link
Member Author

Choose a reason for hiding this comment

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

Delta is a light wallet, so I think we should not use them for testing at the moment. This was probably done on Delta because the wallet will allow the open http/ws ports because an account is not unlocked. The full nodes are unlocked for mining meaning they panic if you try to set these ports in the config. However, we can use the authed port even if the geth node has an unlocked account.

github.com/edsrzf/mmap-go v1.0.0 // indirect
github.com/ethereum/go-ethereum v1.10.25 // indirect
github.com/edsrzf/mmap-go v1.1.0 // indirect
github.com/ethereum/go-ethereum v1.10.24-0.20220902154041-90711efb0ab6 // indirect
Copy link
Member Author

Choose a reason for hiding this comment

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

v1.10.26 is actually latest, but it does not include the commit we need to use the webserver jwt auth. We need ethereum/go-ethereum@90711ef

I'm not sure why the go.mod wants v1.10.24, maybe it's the first common ancestor?

Copy link
Member

@chappjc chappjc Dec 13, 2022

Choose a reason for hiding this comment

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

Looks like ethereum/go-ethereum@90711ef is not on the release/1.10 branch. The PR has the 1.11 milestone, so yeah, they branched the 1.10 maintenance branch after 1.10.24.

This could potentially be problematic if 1.11 is not very soon. We can't release anything that requires a dev branch. They look close, but it's unclear.

I'm a little surprised to see that we needed a new go-ethereum feature to use JTW auth since geth has supported JWT for a long time on the http interface (haven't they? or did the need just arise with authrpc.*?). Did they just now get to adding support to the client?

Copy link
Member Author

Choose a reason for hiding this comment

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

Did they just now get to adding support to the client?

Yes.

I gather the endpoint was created for connecting the beacon chain prism thing, but it connects over http, which is much simpler because you only need to send the auth token once in the initial header. With websocket you need to send it again on disconnects and there is no way to set that and use the provided client.

We could just use http for now and it would work with master.

@JoeGruffins JoeGruffins marked this pull request as ready for review November 25, 2022 07:48
@chappjc chappjc added the ETH label Nov 30, 2022
Copy link
Contributor

@martonp martonp left a comment

Choose a reason for hiding this comment

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

Looks good, but haven't reviewed in detail.

One comment regarding the UI: I think the extra JWT field should be be optional because it will generally be unused. Maybe have a little button next to the url input to add a JWT. If someone has multiple RPCs and each of the JWT field is empty, it'll be slightly distracting. Not a big deal though.

client/webserver/site/src/js/forms.ts Outdated Show resolved Hide resolved
client/asset/eth/eth.go Outdated Show resolved Hide resolved
client/webserver/site/src/js/forms.ts Outdated Show resolved Hide resolved
@JoeGruffins JoeGruffins force-pushed the simnetethok branch 2 times, most recently from bf485b4 to 0c9788b Compare December 1, 2022 06:49
@JoeGruffins
Copy link
Member Author

Marton fixes https://github.com/decred/dcrdex/compare/bf485b46e638bdbaddd31c2f161acf97ab255802..0c9788b330bb1b77c7acfff0c443e71ee657fd14

Maybe have a little button next to the url input to add a JWT.

hmmm

@JoeGruffins
Copy link
Member Author

Fix tooltips https://github.com/decred/dcrdex/compare/0c9788b330bb1b77c7acfff0c443e71ee657fd14..d2a1986f3c734f9180dd813423f7df13c371bc41

For reference here is what the repeatable options currently look like:
image

go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
github.com/edsrzf/mmap-go v1.0.0 // indirect
github.com/ethereum/go-ethereum v1.10.25 // indirect
github.com/edsrzf/mmap-go v1.1.0 // indirect
github.com/ethereum/go-ethereum v1.10.24-0.20220902154041-90711efb0ab6 // indirect
Copy link
Member

@chappjc chappjc Dec 13, 2022

Choose a reason for hiding this comment

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

Looks like ethereum/go-ethereum@90711ef is not on the release/1.10 branch. The PR has the 1.11 milestone, so yeah, they branched the 1.10 maintenance branch after 1.10.24.

This could potentially be problematic if 1.11 is not very soon. We can't release anything that requires a dev branch. They look close, but it's unclear.

I'm a little surprised to see that we needed a new go-ethereum feature to use JTW auth since geth has supported JWT for a long time on the http interface (haven't they? or did the need just arise with authrpc.*?). Did they just now get to adding support to the client?


cfg := new(config)

// NOTE: ipc only is deprecated. Remove this at some point.
Copy link
Member

Choose a reason for hiding this comment

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

Deprecated by geth? Or we are declaring this deprecated with dcrdex?

Copy link
Member Author

Choose a reason for hiding this comment

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

Us. After these changes we can connect over websocket or ipc.

Copy link
Member

Choose a reason for hiding this comment

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

After these changes we can connect over websocket or ipc.

Do we really want to remove support for IPC in the future though? I don't see a reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

no, I just want to remove setting the ipc in the markets.json, and have it point to a config file like other coins, for one because noone is really using this yet and for another maybe we need more start up options in the future.

server/asset/eth/config.go Outdated Show resolved Hide resolved
server/asset/eth/config.go Show resolved Hide resolved
Comment on lines 81 to +82
AuthPort = ${AUTHRPC_PORT}
JWTSecret = "${NODE_DIR}/jwt.hex"
Copy link
Member

@chappjc chappjc Dec 13, 2022

Choose a reason for hiding this comment

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

Now I'm confused about the API services geth offers. There's:

  • HTTPHost / HTTPPort
  • WSHost / WSPort
  • AuthAddr / AuthPort

The first two seem to be only "public" RPCs, by default, although there's a WSExposeAll to override that for ws (WSExposeAll exposes all API modules via the WebSocket RPC interface rather than just the public ones).

The "auth" ones specify the host:port "on which authenticated APIs are provided" but does that mean only authenticated APIs, and thus "public" ones aren't available on this port? I'm confused if "authenticated APIs" are defined as distinct from "public" RPCs (with no intersection), or do authenticated APIs include all the RPCs..

I would expect server should only require the "public API modules", so would server be able to work with geth started with just --http or --ws (not --authrpc?)

What modules are considered public and what are considered authenticated? Referring to:

// HTTPModules is a list of API modules to expose via the HTTP RPC interface.
// If the module list is empty, all RPC API endpoints designated public will be
// exposed.
HTTPModules []string
// WSExposeAll exposes all API modules via the WebSocket RPC interface rather
// than just the public ones.
//
// *WARNING* Only set this if the node is running in a trusted network, exposing
// private APIs to untrusted users is a major security risk.
WSExposeAll bool `toml:",omitempty"`

Where's the list of public vs non-public/private APIs? And which ones are served on AuthPort?

Copy link
Member Author

Choose a reason for hiding this comment

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

geth documentation is not great

I do know that if you try to start a node with an account unlocked and http on, it panics. Will look into it, but we do not want to set either --http or --ws.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know about the past, but currently it seems there is only one authenticated api, and that's the one between geth and the beacon chain.

Relevant places in geth:
The api type:
https://github.com/ethereum/go-ethereum/blob/f51f6edb40f714107cfba90d201cba0a97939dd3/rpc/types.go#L31-L38

You can also see the Public field is deprecated.

And the only time Authenticated is set:
https://github.com/ethereum/go-ethereum/blob/f51f6edb40f714107cfba90d201cba0a97939dd3/eth/catalyst/api.go#L40-L51

Copy link
Member

@chappjc chappjc Dec 14, 2022

Choose a reason for hiding this comment

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

Thanks for posting those links. This is coming into focus now.

Concepts:

  • API namespaces (net, eth, debug, personal, engine, etc)
  • API interfaces (--http, --ws, --authrpc) e.g. ipc/hostport
  • public vs private namespaces
  • authenticated vs unauthenticated interfaces

The namespaces have been classified as either public (e.g. net) vs private (personal, debug, etc.). That concept has been recently removed and the Public field in the exported types deprecated as you have pointed out. ethereum/go-ethereum#25053 ethereum/go-ethereum#25059

The authenticated API appears to have been added because of the engine API used for beacon chain communications, which according to the spec "MUST" be on an authenticated interface that is also separate from the other API interfaces. https://github.com/ethereum/execution-apis/blob/main/src/engine/authentication.md

dex/networks/eth/common.go Outdated Show resolved Hide resolved
client/asset/eth/multirpc.go Outdated Show resolved Hide resolved
@JoeGruffins JoeGruffins force-pushed the simnetethok branch 2 times, most recently from 1037f29 to 69be7a4 Compare December 15, 2022 06:39
@JoeGruffins
Copy link
Member Author

Attempt to make the config change clearer. Add context to dial. Update go.mod and use the geth jwt creation helper https://github.com/decred/dcrdex/compare/1037f299910880d40ff5452414dfafb89c1e166d..69be7a4d36477d63a4135edff0839c692b3b0d23

@buck54321
Copy link
Member

buck54321 commented Dec 17, 2022

One comment regarding the UI: I think the extra JWT field should be be optional because it will generally be unused. Maybe have a little button next to the url input to add a JWT. If someone has multiple RPCs and each of the JWT field is empty, it'll be slightly distracting. Not a big deal though.

I agree with this, but it's definitely going to be a challenge. The ConfigOption system is just not built for this. One thing I was looking at some kind of "subform" like buck54321/dcrdex@6ec23d6...buck54321:dcrdex:repeatable-subforms. Then we can use the ShowByDefault field. It would take some work to get it right in forms.ts though, and I think it's worth its own effort.

@chappjc chappjc self-requested a review December 19, 2022 22:28
@chappjc chappjc added this to the 0.6 milestone Dec 27, 2022
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

Tested dex-test (testnet server) with:

  • IPC path in markets.json (legacy / no changes)
  • IPC path in a new "/opt/geth/dex-settings.conf" file
  • addr=ws://host:port (authrpc) path and jwt= set in that new file

All worked as expected, and dex-test is currently running with geth node RPC via ws authrpc interface. However, for my first shot at JWT, I set jwt=/opt/geth/jwt.hex since I had that already in a file because geth was running with --authrpc.jwtsecret /opt/geth/jwt.hex. This was the error:

2022-12-27 22:04:39.755 [INF] DEX: Starting asset backend "eth"...
failed to start asset "eth": connect failure: unable to create auth function: encoding/hex: invalid byte: U+002F '/'

I realized quickly that I needed to put the hex chars in the conf file instead, but the error was a little confusing because it wasn't prefixed with something like "invalid jwt hex bytes: %w". I figure it should have the ability to read the jwt from an existing jwt.hex file as well, but this is fine.

Also regarding the server, it hurts to suggest it, but for future work we should facilitate use of third-party http RPC providers. An operator may decide to pay for a premium account with something like Ankr, and if that's what it takes to get adoption for ETH by operators, so be it.

@JoeGruffins
Copy link
Member Author

Just rebased.

Add ability for client and server to connect over authenticated
websocket to a geth full node.
@JoeGruffins
Copy link
Member Author

@chappjc chappjc merged commit 75e8aac into decred:master Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

client/eth: Add jwt secret config option.
4 participants