gen-smtp / gen_smtp

The extensible Erlang SMTP client and server library.
Other
683 stars 266 forks source link

Add DKIM Ed25519 capability #276

Closed cw789 closed 3 years ago

cw789 commented 3 years ago

This pull request depends on https://github.com/erlang/otp/pull/5157. And so DKIM with Ed25519 signing will only be available with OTP 24.1+.

We only will support this or that (RSA / Ed25519) for now. Dual signing with RSA & Ed25519 won't be part of this PR.

Close #275

cw789 commented 3 years ago

Hello Sergey. I've applied your code suggestions. Thanks for your time in the meanwhile. I've tried some things again for the problem above, but it still seems to be too deep buried for my Erlang or crypto experience.

cw789 commented 3 years ago

Ok things are cleaned up now. Testing + tests for it are still missing. I don't know how to make this with the -ifdef(OTP_RELEASE). thing. I would keep this open as draft until OTP 24.1 is out - if ok?

cw789 commented 3 years ago

I updated the description with mentioning we won't implement dual signing.

cw789 commented 3 years ago

Will something like this in mimemail.erl work?

%% TODO: Remove me once we require Erlang/OTP 24+
-ifdef(OTP_RELEASE).
  -if(?OTP_RELEASE >= 24).
    -define(DKIM_ED25519, true).
  -endif.
-endif.

-ifdef(DKIM_ED25519).
dkim_get_algorithm_digest(Algorithm) ->
    case Algorithm of
        'rsa-sha256' -> sha256;
        'ed25519-sha256' -> none
    end.
-else.
dkim_get_algorithm_digest(Algorithm) ->
    case Algorithm of
        'rsa-sha256' -> sha256;
        'ed25519-sha256' -> throw("DKIM Ed25519 requires Erlang/OTP 24.1+").
    end.
-endif.

Or should we use instead of -if(?OTP_RELEASE >= 24) something likely as -if(version(erlang) >= [24, 1, whatever].

cw789 commented 3 years ago

Another open question. I would like to duplicate dkim_sign_rsa_test_() and make an adapted dkim_sign_ed25519_test_. How could I make sure this test only runs on Erlang/OTP 24.1+?

seriyps commented 3 years ago

I'm thinking maybe it's better to do some checks at runtime? Smth like lists:member(ed25519, crypto:supports(curves))? But then we might need to also implement the fix backport as I suggested in https://github.com/gen-smtp/gen_smtp/pull/276#discussion_r701441329 Or we may also check the version of the public_key OTP application application:get_key(public_key, vsn). to see if it includes the bugfix https://github.com/erlang/otp/pull/5157

The problem with -if(?OTP_RELEASE >= 24). is that there is no way to check if you are running 24.0 or 24.1.

I'll try to look in more details later today or tomorrow

cw789 commented 3 years ago

Ok. A runtime check for high enough public_key vsn sounds like a good consideration.

seriyps commented 3 years ago

Seems public_key-1.11.2 is the one where the fix is released (actually today): https://erlang.org/download/OTP-24.1.README

--- Improvements and New Features ---

OTP-17609 Application(s): public_key Related Id(s): GH-5156, GH-5157

           When decoding an 'ECPrivateKey' unwrap the private key.
           For more precise information see RFC 8410, section 7.
cw789 commented 3 years ago

It seems that comparing SemVer versions isn't something done out of the box in Erlang. Is it worth to implement a version_compare function or is there any simpler possibility I oversee?

seriyps commented 3 years ago

I think smth similar to this one is the easiest you can do:

https://github.com/gen-smtp/gen_smtp/pull/171/files#diff-870779fb647905a4c2f76a5679211924ddf4ced941daa9594f1b1064a8dceff2R145-R153

So

{ok, VerString} = application:get_key(public_key, vsn),
Ver = lists:map(fun erlang:list_to_integer/1,
                    string:tokens(VerString, ".")),
    if Ver < [1, 11, 2] ->
            ...not supported...;
       true ->
            ...supported...
    end.
cw789 commented 3 years ago

Thanks Sergey. Half of what I already had - but I missed the part of converting the strings to integer.

The test now will still fail. I've not jet managed to setup the rebar3 test thing properly on my machine.

cw789 commented 3 years ago

Thanks a lot Sergey. Thanks for mentoring me during my first time working with Erlang. Hope it wasn't too cumbersome, as I've jumped into the cold water without understanding the Erlang syntax beforehand. I've found out that a lot of the cool things in Elixir were already cool things in Erlang before - very well thought.

This PR is now ready to merge from my side. Tests should pass & my manual test also worked out.

mworrell commented 3 years ago

And merged. Thank you!