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

Wrong alg used in header for Ed25519 keys #334

Open
zblach opened this issue Sep 20, 2019 · 14 comments
Open

Wrong alg used in header for Ed25519 keys #334

zblach opened this issue Sep 20, 2019 · 14 comments

Comments

@zblach
Copy link

zblach commented Sep 20, 2019

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

anakinj commented Dec 3, 2020

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:

  • Not to change the generated alg header (The decoding should not care about the given header anyway)
  • Start supporting the correct algorithm (EdDSA) in the decode method

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

@mattgibson
Copy link

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

anakinj commented Feb 26, 2021

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

anakinj commented Feb 26, 2021

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

@mattgibson
Copy link

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

@mattgibson
Copy link

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

anakinj commented Feb 26, 2021

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

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

@anakinj
Copy link
Member

anakinj commented Feb 26, 2021

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

renato-zannon commented Mar 16, 2021

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

anakinj commented Feb 2, 2023

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

wparad commented May 1, 2024

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

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"])

@anakinj
Copy link
Member

anakinj commented Sep 15, 2024

There has been some work done to extract the algorithms that require external dependencies to separate gems. EdDSA being the first one that got that treatment. The goal is to remove the algorithm without native support from this gem in the future.

Check out https://rubygems.org/gems/jwt-eddsa for eddsa support with a alg header according to the rfc.

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

No branches or pull requests

7 participants