ethereumjs / ethereumjs-monorepo

Monorepo for the Ethereum VM TypeScript Implementation
2.57k stars 747 forks source link

Overly strict JWT decoding? #3508

Closed Tumas closed 1 month ago

Tumas commented 1 month ago

Hi,

I've noticed that ethereumjs expects base64url encoded jwt tokens with padding and it fails if padding is not present.

https://github.com/ethereumjs/ethereumjs-monorepo/blob/80fcde6449b491772bb915a5341fce5f816c6345/packages/client/src/ext/jwt-simple.ts#L124 https://github.com/paulmillr/scure-base/blob/a131a619960dd000ad7d4f24a5a64d02ecf655d9/index.ts#L367 https://github.com/paulmillr/scure-base/blob/a131a619960dd000ad7d4f24a5a64d02ecf655d9/index.ts#L132

Surprisingly, creating JWT payload with the only required claims field ( 'iat' + timestamp ) fits the padding scheme, so no padding is required and it works. However, the issue arises when more optional claim fields are added (exp, nbf, id, clv).

To complicate things even more, when considering interop with consensus layer clients, most Rust libraries used by consensus clients use base64url encoding without padding:

https://docs.rs/jsonwebtoken/9.3.0/src/jsonwebtoken/serialization.rs.html#7 used by Lighthouse https://github.com/jedisct1/rust-jwt-simple/blob/2f951bb7a8623e374b17ea76f855986c283e1459/src/token.rs#L111 used by Grandine

As far as I understand, it is because padding is not mandatory.

Is there a particular reason why padding is strictly required on ethereumjs side?

g11tech commented 1 month ago

can you give an example jwt simple token for us to debug,fix/test against?

Tumas commented 1 month ago

Sure,

eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpYXQiOjE3MjExNDQzMjksImlkIjoiaGVsbG8ifQ.ShrhYX0U8FUC3KtuctIrYpibgAT7Grzm0Ni1QqnjReo

you can see Payload and Header in debugger: https://jwt.io/#debugger-io?token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpYXQiOjE3MjExNDQzMjksImlkIjoiaGVsbG8ifQ.ShrhYX0U8FUC3KtuctIrYpibgAT7Grzm0Ni1QqnjReo

jochem-brouwer commented 1 month ago

I am assuming you are using the JWT secret in the client by using the --jwtsecret option? (If not, where are you using it?).

You are trying to use this JWT secret for both our client, and a CL - right?

jochem-brouwer commented 1 month ago

Are you using a "simple" 32-byte hex encoded JWT secret? What CL are you testing against? And how did you stumble upon this problem? :smile:

So many questions :sweat_smile:

holgerd77 commented 1 month ago

So but just to drop here: we "internalized" (in the sense of: take the code over in the code base with a license on top to avoid drawing in the dependencies) the JWT library we are using recently and there was some discussion around padding taking place along: https://github.com/ethereumjs/ethereumjs-monorepo/pull/3458#issuecomment-2178650968

Do not have the full overview about the details but it might very well be that there changed something along (padding now applied while it was not before or so).

Tumas commented 1 month ago

Are you using a "simple" 32-byte hex encoded JWT secret? What CL are you testing against? And how did you stumble upon this problem? 😄

So many questions 😅

I'm testing interoperability between EthereumJS and Grandine CL client on Kurtosis. Though this issue can be observed with other CL clients as well if specific CLI params are given. JWT secret is managed by Kurtosis and yes, it is simple 32-byte hex encoded secret. The same secret is passed to both EL & CL.

Nevermind the secret though. JWT token is split into 3 parts separated by '.' character and first two parts do not depend on the secret. They are just base64url encoded JSON (with or without padding). EthereumJS raises error when trying to decode second part of the JWT token due to the lack of padding (if some extra optional claims are present) and request fails with authentication error.

holgerd77 commented 1 month ago

Hi there, thanks for the detailed explanation, could you nevertheless just paste the concrete token you are using here and which breaks on EthereumJS? That would help a lot for testing! 🙏 (or am I still missing the point?)

Tumas commented 1 month ago

Sure,

eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpYXQiOjE3MjExNDQzMjksImlkIjoiaGVsbG8ifQ.ShrhYX0U8FUC3KtuctIrYpibgAT7Grzm0Ni1QqnjReo

you can see Payload and Header in debugger: https://jwt.io/#debugger-io?token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpYXQiOjE3MjExNDQzMjksImlkIjoiaGVsbG8ifQ.ShrhYX0U8FUC3KtuctIrYpibgAT7Grzm0Ni1QqnjReo

Sure. Here it is. It only additionally contains an optional "id" claim set to 'hello'. "id" and "clv" claims are permitted by the spec:

https://github.com/ethereum/execution-apis/blob/main/src/engine/authentication.md#jwt-claims

jochem-brouwer commented 1 month ago

Hey there, if I setup a Grandine <-> EthereumJS connection I get 401 errors. How do I make Grandine spit out a more verbose log of that 401 message? I am trying to see anything in our verbose logs but I cannot yet find any errors there :thinking:

I am assuming this 401 error on communication is related to this issue? You can also join our discord, if you want :smile: https://discord.gg/TNwARpR

jochem-brouwer commented 1 month ago

image

Never mind, I can reproduce!! Thanks a lot, we will start to figure this out!

jochem-brouwer commented 1 month ago

Thanks so much for this report @Tumas, we have fixed it in https://github.com/ethereumjs/ethereumjs-monorepo/pull/3511, also tested locally, Grandine <-> EthJS communications now work!

Tumas commented 1 month ago

Thanks for the fix & quick reaction! :clap: :fire: :fire: