apple / swift-crypto

Open-source implementation of a substantial portion of the API of Apple CryptoKit suitable for use on Linux platforms.
https://apple.github.io/swift-crypto
Apache License 2.0
1.47k stars 165 forks source link

Update wycheproof testvectors to v1 #154

Closed Sajjon closed 1 year ago

Sajjon commented 1 year ago

Update some Wycheproof testvectors (v0) to new testvectors_v1, which in general include more tests per file.

Checklist

If you've made changes to gyb files

Motivation:

We ought to use as many and as up to date test vectors as possible. Wycheproofs v0 were created 4 years ago and were due some upgrade.

Modification & Result

(I took the liberty to merge these to sections together)

More tests are now run 🎉 below is a table over all changes and result.

Testvector Previous name V0 number of tests V1 number of tests Source code unchanged? Test code unchanged?
x25519_test unchanged 87 518 ✅ ✅
ed25519_test eddsa_test 111 150 ✅ JSON: changed single key
aes_gcm_test unchanged 217 313 ✅ ✅
chacha20_poly1305_test unchanged 151 325 ✅ ✅
ecdh_secp256r1_ecpoint_test unchanged 99 355 ✅ ✅
ecdh_secp256r1_test unchanged 340 612 ✅ Logic: fatalError -> failable init

I can upgrade all...

... but I wanted to hear first that you think it a good idea...

If so, should I do it in a single PR? I.e. add more commits to this PR?

Sajjon commented 1 year ago

@swift-server-bot test this please

Sajjon commented 1 year ago

Ah maybe I lack access to trigger swift-server-bot to run tests... one can always try 😅

Lukasa commented 1 year ago

Good news though, I can tackle them!

Lukasa commented 1 year ago

@swift-server-bot test this please

Sajjon commented 1 year ago

@Lukasa would you be interested in me continuing with the rest of the vectors? I mean to say, if you don't think it a good idea, best I don't waste the time 😅.

If you think it it a good idea, do you prefer all upgraded in the same PR? Or split across many?

Let me know :)

Lukasa commented 1 year ago

Hey @Sajjon, sorry for letting this sit here. I haven't lost track of it, I promise. I think in general this is something we want to accept, but I think splitting across PRs will make life easier.

Sajjon commented 1 year ago

Great, I will split into multiple ones and referenced them here and then close this one. Might be some Days before I can do that though.

Talk soon!

Sajjon commented 1 year ago

@Lukasa I've split this PR into six new ones, I will continue with the rest later next week. Should we have an umbrella issue for all PRs? Since they are strongly related. I can create that, and then we can close this PR.

Let me know how you wanna proceed! :)

Lukasa commented 1 year ago

The first 6 are merged, thanks!

Sajjon commented 1 year ago

Great, I can maybe do some more later this week.

Sajjon commented 1 year ago

Closed in favour of umbrella issue: https://github.com/apple/swift-crypto/issues/165