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

POC: add ed25519 support based on libsodium (PC) or salty (solo). #478

Merged
merged 8 commits into from
Jan 28, 2021

Conversation

enrikb
Copy link
Contributor

@enrikb enrikb commented Oct 16, 2020

For now:

  • libsodium(-dev) is expected to be preinstalled on build system for PC
    build

  • salty needs to be prebuilt and installed into targets/stm32l432/salty/
    manually

They key type is stored in the 'masked metadata' area of the key ID.

The auth buffers have been enlarged by 32 bytes to be able to append the client hash before ED25519 signature generation instead of creating a full message copy.

I have tested this successfully using fido2-tools and ssh-keygen/ssh on both PC and Solo Hacker.

All feedback much appreciated!

@nickray
Copy link
Member

nickray commented Oct 16, 2020

Very awesome that you are working on this! I'm not too qualified to review C code myself unfortunately, I thought the trickiest bit would be handling storage / distinguishing p256 and ed255 keys?

FYI, salty has a small bug which causes it to generate incorrect signatures ~1% of the time. Not security critical (some operations will just have to be repeated) but a bit annoying. Branch replace-scalar-implementation fixes this by re-using a scalar implementation from dalek-cryptography; will see to make a proper release for this, but you can already use it. I'd generally be very interested in your experience using salty from C, feel free to open issues there, or hit me up on Keybase or Matrix/IRC.

@enrikb
Copy link
Contributor Author

enrikb commented Oct 17, 2020

Hi @nickray,

Very awesome that you are working on this! I'm not too qualified to review C code myself unfortunately, I thought the trickiest bit would be handling storage / distinguishing p256 and ed255 keys?

Currently, I'm kind of working around the issue by storing the algorithm in the masked metadata portion of the credential id very much like the EXT_CRED_PROTECT flags. Not sure yet if this is the best idea, but definitely good enough for testing. Maybe @conorpp can give some advice?

FYI, salty has a small bug which causes it to generate incorrect signatures ~1% of the time.

Thanks for the heads up. I read about the issue and decided to go ahead anyway, as it is not API related or the like.

Branch replace-scalar-implementation fixes this by re-using a scalar implementation from dalek-cryptography; will see to make a proper release for this, but you can already use it.

I will check as soon as I manage to integrate the salty build. Already very close ...

I'd generally be very interested in your experience using salty from C, feel free to open issues there, or hit me up on Keybase or Matrix/IRC.

Let's see, maybe I should give IRC a try after so many years of abstinence ;-)

This is needed to build upcoming support for ED25519 in the 'PC'
version. Without, the CI build will failed as libsodium has not yet been
integrated into the build system.
This will be used for upcoming support of ED25519 in the 'Solo' version.
Not sure if this is the way to do it...

Make salty build.
For now:

- libsodium(-dev) is expected to be preinstalled on build system for PC
build
@enrikb
Copy link
Contributor Author

enrikb commented Oct 17, 2020

Finally, Travis CI passed.

This is a kind of promise that the underlying buffers have the correct
size. We know what we are doing. Hopefully ;-)
@asnelt
Copy link
Contributor

asnelt commented Nov 14, 2020

Tested with ssh-keygen/ssh on Somu Hacker and working well.

@enrikb
Copy link
Contributor Author

enrikb commented Nov 19, 2020

Tested with ssh-keygen/ssh on Somu Hacker and working well.

Thanks for testing and the feedback!

@stintel
Copy link

stintel commented Jan 19, 2021

If this is merged, will this introduce ed25519 support in the next release, also on v1 keys? If so, what is needed to get this done ASAP?

@conorpp
Copy link
Member

conorpp commented Jan 28, 2021

@enrikb Thank you for this contribution! And sorry for taking so long to get to it. Working on merging & testing now. Expect a new secure release soon :)

@conorpp
Copy link
Member

conorpp commented Jan 28, 2021

@asnelt Thank you for testing it out!

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.

5 participants