-
Notifications
You must be signed in to change notification settings - Fork 266
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
Conversation
Hello Sergey. I've applied your code suggestions. Thanks for your time in the meanwhile. |
Ok things are cleaned up now. Testing + tests for it are still missing. |
I updated the description with mentioning we won't implement dual signing. |
Will something like this in
Or should we use instead of |
Another open question. |
I'm thinking maybe it's better to do some checks at runtime? Smth like The problem with I'll try to look in more details later today or tomorrow |
Ok. A runtime check for high enough public_key vsn sounds like a good consideration. |
Seems
|
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? |
I think smth similar to this one is the easiest you can do: 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. |
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. |
There was a problem hiding this 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
Co-authored-by: Sergey Prokhorov <seriy.pr@gmail.com>
Thanks a lot Sergey. Thanks for mentoring me during my first time working with Erlang. This PR is now ready to merge from my side. Tests should pass & my manual test also worked out. |
There was a problem hiding this 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 =)
And merged. Thank you! |
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