go-ethereum

go-ethereum copied to clipboard

This PR:

• Implements testing of the RPC authentication conditions as specified by the Engine API authentication spec
• Adds getters to the node to retrieve the WS and HTTP auth endpoints (useful for testing when the port is assigned dynamically, mirrors the existing getters for non-auth endpoints)
• Updates an old comment that still referred to JWT secrets being passed by direct cli arg instead of cli arg to file.
• Implements rpc.DialWithAuth, rpc.DialHTTPWithAuth, rpc.DialWebsocketWithAuth that take a rpc.HeaderAuthProvider and implemented with rpc.JWTAuthProvider (along with simple test-only implementations). Thanks to @trianglesphere for implementing this (Optimism uses the Engine API and authentication too, and we want to contribute things back upstream :) )
  • I have thought about using an "option" pattern too (rpc.DialContext(ctx, endpoint, options... Option) where one Option can add authentication. But the authentication is specific to http and websockets, so the option pattern would be weird with IPC etc. dials. And adding optional args to the existing DialContext might break some users that rely on the function type (regular callers wouldn't be affected). So I didn't use this pattern.
• Tests the authentication:
  • That the engine and eth modules are available
  • That basic auth works
  • That other secrets don't work
  • That the "none" algorithm trick doesn't work
  • That the issued-at time is verified with the spec 60 second margin
  • That repeated http calls are also authenticated
  • With all of the above, that the dial-with-auth of the rpc client works

One thing that I'd call out is the ability to potentially add the id/clv claims (these are optional in the spec and TBH I don't know if they are used, but they are there). The id strings would need to be passed into the NewAuthProvider fn (or in a variant of it).

It's also probably fine to skip this for now until those fields are more commonly used.

I think I addressed all open review, let me know if there are other things to test or change.

Note: the time related tests are flaky on a slower machine, since they rely on the existing auth code, which uses time.Now. Maybe we can refactor the existing auth code to use a clock that can be mocked, so the test does not rely on time anymore?

Note: the time related tests are flaky on a slower machine, since they rely on the existing auth code, which uses time.Now. Maybe we can refactor the existing auth code to use a clock that can be mocked, so the test does not rely on time anymore?

I recently fixed a similar issue here: https://github.com/ethereum/go-ethereum/pull/25120 -- by changing it so that the token is lazily created just before the request is made. That approach is probably simpler than mocking a clock throughout the pipeline?

The auth header creation is already deferred to just before the client.Do, the timestamp creation is accurate.

The problem with CI turned out to be different: when testing boundaries with millisecond accuracy you need timestamps to be accurate too. But second-precision was being used, even though JWT supports "NumericDate" format for string timestamps that have higher precision. See https://datatracker.ietf.org/doc/html/rfc7519#section-2 and the jwt.NumericDate type.

The precision is now configurable in the client (the server already accepted arbitrary precision), so we can test the boundary accurately. But it's up to the caller to set their own precision here. (not all servers may implement the spec properly).

Also, I fixed an issue where the same auth-headers were being reused for websocket reconnections. Since the auth is time sensitive the auth header needs to be reset.

@holiman this PR is ready for review again.

Hey @protolambda, this is just to let you know I haven't forgotten about this PR. Will get to it eventually!

Tests passed, but then got interrupted after I enthusiastically changed pushed a second commit to use NumericDate with truncated time based on desired precision, instead of changing the global precision var in the jwt package.

In NumericDate.UnmarshalJSON it parses it as float, and converts it into a time in newNumericDateFromSeconds, but then constructs the numeric date with NewNumericDate instead of &NumericDate{Time: ...}. So the server truncates the precision of incoming issued-at claim times, even if the request has a detailed time.

And during NumericDate.MarshalJSON it also applies the truncate, so initializing the NumericDate doesn't work.

There are a bunch of options:

1. Set the global jwt.TimePrecision to a nanosecond, and then the time precision set by the user will always stay untouched on both server/client usage.
2. Set the global jwt.TimePrecision to a millisecond, and don't offer the option to the user to change it. If the issued-at bound is 5 seconds, I think it's reasonable to allow millisecond precision, or the 1/5 of the range is enforced by chance of at what time it rounds down.
3. Keep the second time precision, but only override the jwt.TimePrecision global in the test, so the test of the 5 second boundary is accurate, even though it's not for the user.

Personally I think option 2 is best; almost no code, no surprise to the user about time precision affecting the request validity, and simple (no options you've to be aware of). Will update the code with that now.

Edit: updated code, squashed the commits about different time precision changes. And fixed code linting.

While this PR was stuck the execution API specs changed the expiry time from 5 seconds to 60 seconds. So I'm going to update the tests + revert the seconds->milliseconds precision change.

This is ready now. I have change this to use an 'options' scheme after all, because the added client constructor functions were just a bit too much duplication.

Another change: the client-side JWT authentication provider is now also in package node. I don't want to put it in package rpc because it would add the JWT library dependency to a lot of other packages that don't need it. Also, the tests for this provider were already in package node.

To create a JWT-authenticated client, you do this:

import (
    "github.com/ethereum/go-ethereum/node"
    "github.com/ethereum/go-ethereum/rpc"
)

func main() {
    var secret [32]byte
    ctx := context.Background()
    c, err := rpc.DialOptions(ctx, "http://...", rpc.WithHTTPAuth(node.NewJWTAuth(secret)))
    ...
}