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

Add DKIM Ed25519 capability #276

Merged
merged 1 commit into from
Sep 25, 2021
Merged

Add DKIM Ed25519 capability #276

merged 1 commit into from
Sep 25, 2021

Conversation

cw789
Copy link
Contributor

@cw789 cw789 commented Sep 2, 2021

This pull request depends on erlang/otp#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

src/mimemail.erl Outdated Show resolved Hide resolved
src/mimemail.erl Outdated Show resolved Hide resolved
src/mimemail.erl Outdated Show resolved Hide resolved
src/mimemail.erl Outdated Show resolved Hide resolved
@cw789
Copy link
Contributor Author

cw789 commented Sep 2, 2021

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.

src/mimemail.erl Outdated Show resolved Hide resolved
@cw789 cw789 marked this pull request as ready for review September 3, 2021 16:44
@cw789
Copy link
Contributor Author

cw789 commented Sep 3, 2021

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 cw789 marked this pull request as draft September 3, 2021 16:45
@cw789
Copy link
Contributor Author

cw789 commented Sep 3, 2021

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

@cw789
Copy link
Contributor Author

cw789 commented Sep 22, 2021

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

cw789 commented Sep 22, 2021

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

seriyps commented Sep 22, 2021

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 #276 (comment)
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 erlang/otp#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
Copy link
Contributor Author

cw789 commented Sep 22, 2021

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

@seriyps
Copy link
Collaborator

seriyps commented Sep 22, 2021

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

cw789 commented Sep 24, 2021

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

seriyps commented Sep 24, 2021

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

cw789 commented Sep 24, 2021

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.

Copy link
Collaborator

@seriyps seriyps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really great! Thanks! I added some less critical comments, up to you if you want to fix them or not

src/mimemail.erl Show resolved Hide resolved
src/mimemail.erl Outdated Show resolved Hide resolved
src/mimemail.erl Outdated Show resolved Hide resolved
README.markdown Show resolved Hide resolved
src/mimemail.erl Outdated Show resolved Hide resolved
Co-authored-by: Sergey Prokhorov <seriy.pr@gmail.com>
@cw789 cw789 marked this pull request as ready for review September 25, 2021 07:22
@cw789
Copy link
Contributor Author

cw789 commented Sep 25, 2021

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.

Copy link
Collaborator

@seriyps seriyps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Thanks for not giving up on it =)

@seriyps seriyps requested a review from mworrell September 25, 2021 16:58
@mworrell mworrell merged commit 5ce941b into gen-smtp:master Sep 25, 2021
@mworrell
Copy link
Collaborator

And merged. Thank you!

@cw789 cw789 deleted the dkim_ed25519 branch September 26, 2021 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DKIM with Ed25519 key
3 participants