jwt / ruby-jwt

A ruby implementation of the RFC 7519 OAuth JSON Web Token (JWT) standard.
https://jwt.github.io/ruby-jwt/
MIT License
3.58k stars 373 forks source link

Wrong `alg` used in header for Ed25519 keys #334

Open zblach opened 4 years ago

zblach commented 4 years ago

The example tokens in this library use "alg": "ED25519", but the related spec seems to suggest that it be "EdDSA" instead.

https://tools.ietf.org/html/rfc8037#appendix-A.4

anakinj commented 3 years ago

I think you are correct. Now I wonder how we could make this right. The feature has been out in the wild for a pretty long time already and encoded tokens get this incorrect alg header.

I would suggest:

Maybe start printing some warning of the usage of the algorithm ED25519 so we could remove that at some point...

mattgibson commented 3 years ago

Not to change the generated alg header (The decoding should not care about the given header anyway)

I just ran into this when trying to use a Ruby-generated token in an Elixir app. The Elixir library is using the alg header to choose the decryption algorithm and is case-sensitive, so ED25519 causes a signature error as it can't work out how to handle it, but Ed25519 works.

anakinj commented 3 years ago

Just out of curiosity, what Elixir library is this?

My biggest concern is that if it is just changed to something else old ruby-jwt<-> new ruby-jwt compatibility will at least break.

anakinj commented 3 years ago

If you are in control of the token generation you could change the constant controlling this locally: JWT::Algos::Eddsa::SUPPORTED = ["Ed25519"]

mattgibson commented 3 years ago

I was using Joken, but the underlying low level library it uses is JOSE

mattgibson commented 3 years ago

If you are in control of the token generation you could change the constant controlling this locally: JWT::Algos::Eddsa::SUPPORTED = ["Ed25519"]

Yes, I found that and it worked when generating the tokens. I think for now I'm going to fork the gem and ensure it will decrypt both Ed25519 and ED25519 tokens. Thanks for the pointer!

anakinj commented 3 years ago

But apparently this Joken component is expecting the EdDSA algorithms to have the exact curve used in the alg field. So changing the alg to EdDSA will not make this gem compatible either.

mattgibson commented 3 years ago

Ah, that's a good point. Possibly Joken is not implementing the RFC correctly either.

anakinj commented 3 years ago

Btw. I think the easiest workaround is just to monkeypatch the JWT::Algos::Eddsa::SUPPORTED constant. No need for forks.

require 'jwt'
require 'rbnacl'

JWT::Algos::Eddsa::SUPPORTED = ['Ed25519']

signing_key = ::RbNaCl::Signatures::Ed25519::SigningKey.new('a'*32)
token = JWT.encode({con: 'tent'}, signing_key.verify_key, 'Ed25519')
renato-zannon commented 3 years ago

I was looking into actually generating the tokens with the correct "alg" value (in my case, because I wanted them to be verifiable by ruby-jose), and had to modify a bit more than just the constant.

Specifically, the conditionals on eddsa.rb were comparing alg to the key primitive ("EdDSA" vs :ed25519) and raising IncorrectAlgorithm as a result.

I've made a quick monkeypatch on my side to override this and allow EdDSA anyway. Is this something that would be sensible to add to the project? If so, I would be happy to open a PR.

EDIT: nevermind, I was mistaken. By changing ED25519 to Ed25519, as was suggested above, ruby-jose is also able to verify the tokens

anakinj commented 1 year ago

I would suggest introducing a breaking change and change the header handling to be inline what the RFC and world is expecting for the version 3.0.0

wparad commented 4 months ago

Wow, still broken in 2024. I don't know if this is the optimal patch, but it for sure is easy:

module JWTExtensions
  def encode_header
    # https://github.com/jwt/ruby-jwt/blob/main/lib/jwt/encode.rb#L17
    @headers["alg"] = @headers["alg"].downcase == "ed25519" ? "EdDSA" : @headers["alg"]
    super
  end
end

module JWT
  class Encode
    prepend JWTExtensions
  end
end
SebastianPoell commented 2 weeks ago

Instead of redefining the constant (which triggers a Ruby warning), a way of patching this without warning would be to remove the constant and set it again:

JWT::JWA::Eddsa.send(:remove_const, "SUPPORTED")
JWT::JWA::Eddsa.const_set("SUPPORTED", ["Ed25519"])