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

Overly strict JWT decoding? #3508

Closed
Tumas opened this issue Jul 16, 2024 · 12 comments · Fixed by #3511
Closed

Overly strict JWT decoding? #3508

Tumas opened this issue Jul 16, 2024 · 12 comments · Fixed by #3511

Comments

@Tumas
Copy link

Tumas commented Jul 16, 2024

Hi,

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

const payload = JSON.parse(bytesToUtf8(base64url.decode(payloadSeg)))

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?

@Tumas Tumas changed the title Overly strict JWT token decoding? Overly strict JWT decoding? Jul 16, 2024
@g11tech
Copy link
Contributor

g11tech commented Jul 16, 2024

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

@Tumas
Copy link
Author

Tumas commented Jul 16, 2024

Sure,

eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpYXQiOjE3MjExNDQzMjksImlkIjoiaGVsbG8ifQ.ShrhYX0U8FUC3KtuctIrYpibgAT7Grzm0Ni1QqnjReo

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

@jochem-brouwer
Copy link
Member

jochem-brouwer commented Jul 16, 2024

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
Copy link
Member

jochem-brouwer commented Jul 16, 2024

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 😅

@holgerd77
Copy link
Member

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:
#3458 (comment)

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
Copy link
Author

Tumas commented Jul 17, 2024

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
Copy link
Member

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
Copy link
Author

Tumas commented Jul 17, 2024

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
Copy link
Member

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 🤔

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

@jochem-brouwer
Copy link
Member

image

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

@jochem-brouwer
Copy link
Member

jochem-brouwer commented Jul 17, 2024

Thanks so much for this report @Tumas, we have fixed it in #3511, also tested locally, Grandine <-> EthJS communications now work!

@Tumas
Copy link
Author

Tumas commented Jul 17, 2024

Thanks for the fix & quick reaction! 👏 🔥 🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants