Add preliminary EdDSA validation support

vslavik / winsparkle

App update framework for Windows, inspired by Sparkle for macOS

http://winsparkle.org

Other

1.31k stars 267 forks source link

Add preliminary EdDSA validation support #239

Closed ntadej closed 1 year ago

ntadej commented 2 years ago

Add preliminary EdDSA validation support (partially closes #187 as signing is still missing). I tested with valid and invalid signature and it seems to work. Note that more testing is probably needed.

Some comments:

Uses https://github.com/orlp/ed25519.git like the primary Sparkle project, causes minimal size increase of the final dll.
Public key verfication is not there yet, I guess it is enough to just do the Base64 decoding + size check?
Not sure I updated bakefile/vcxproj correctly. I get some errors, but it everything looks fine. The only thing is that the format changed a bit (I guess I'm using a different version of bakefile as the current files were generated with).
Do we need that complicated file reading? I admit I took the first snippet from StackOverflow on how to read full files on windows.
RC integration not tested (I'm not primarily a Windows developer, I'm just trying to port one of my apps to Windows).

Tagging @kornelski and @vslavik.

purejava commented 1 year ago

@ntadej, @Youw, do you see any chance to proceed with this PR? Thanks.

purejava commented 1 year ago

The reason why I am asking is, that I have created Java bindings for WinSparkle and we want to use these to update Cryptomator on Windows, that securely stores vaults in cloud services.

ntadej commented 1 year ago

I am planning to, but Windows is currently at the bottom of priority list for my project.

geraldcombs commented 1 year ago

I think I've fixed the issues identified by @Youw in https://github.com/geraldcombs/winsparkle/tree/eddsa-updates. The only outstanding issue is that SignatureVerifier::VerifyEdDSAPubKey is still a placeholder. I think the proper fix would be to add a corresponding function to the ed25519 public API.

@ntadej if my changes look OK to you, feel free to grab them and add them to this PR (and assign yourself authorship if you'd like). Otherwise I'd be happy to open a separate PR.

vslavik commented 1 year ago

@geraldcombs Thanks!

The only outstanding issue is that SignatureVerifier::VerifyEdDSAPubKey is still a placeholder. I think the proper fix would be to add a corresponding function to the ed25519 public API.

Let's make this simpler: VerifyEdDSAPubKey has little usefulness and the verification is done at signature checking time anyway, so we can either just remove this part or do what was initially suggested:

Public key verfication is not there yet, I guess it is enough to just do the Base64 decoding + size check?

vslavik commented 1 year ago

Otherwise I'd be happy to open a separate PR.

Oh, sorry, forgot to say: please do.

ntadej commented 1 year ago

Yes, sorry for lack of activity here. I also propose you open a separate PR so it will be easier to iterate for you.

geraldcombs commented 1 year ago

Oh, sorry, forgot to say: please do.

Done in #254.

vslavik commented 1 year ago

→ #254