feat: add ES512 algorithm support using P-521 elliptic curve#478
feat: add ES512 algorithm support using P-521 elliptic curve#478arsenin-kitsoft wants to merge 1 commit intoKeats:masterfrom
Conversation
arsenin-kitsoft
changed the title
arsenin-kitsoft
changed the title
|
I didn't see that PR. Can you rebase? |
arsenin-kitsoft
force-pushed
the
master
branch
from
1fce73b to
fd96c1c
Compare
|
Couldn't rebase, but reapplied changes manually instead. Passes tests with green. |
|
@Keats can you please take another look at this PR? |
|
Ci is failing |
|
@Keats Fixed CI issues that I could replicate locally. Please verify. |
| p384 = { version = "0.13.0", optional = true, features = ["ecdsa"] } | ||
| p256 = { version = "0.13.2", optional = true, features = ["ecdsa", "pkcs8"] } | ||
| p384 = { version = "0.13.0", optional = true, features = ["ecdsa", "pkcs8"] } | ||
| p521 = { version = "0.13.0", optional = true, features = ["ecdsa", "pkcs8"] } |
There was a problem hiding this comment.
There is a 0.14-rc8 published recently on crates.io, 0.13 is 2 years old. I think the new version might make the rust-crypto code easier?
There was a problem hiding this comment.
0.14 is in heavy development for the last half year, based on rc history. Are you sure you want an rc dep in your crate?
There was a problem hiding this comment.
p256 and p384 are the same story. I've tried to update them all to latest rc and tests were not failing, but I don't think this should be in scope for this particular PR.
There was a problem hiding this comment.
@Keats plz confirm if you want to update all p* crates (same maintainer) to rc version or we could skip it for now.
There was a problem hiding this comment.
the main question was whether the 0.14 would remove some of the custom code needed in this PR. I haven't really followed when it would release, we shouldn't depend on a rc though
|
@Keats tests are failing on rustc 1.86.0, which is quite old. I've ran them on locally on v1.93.1 and they're green. Should we update the CI? |
|
Just checked - they're also green at 1.88.0 |
|
let's go with 1.88 |
|
@Keats bumped pinned version 1.86.0 -> 1.88.0, let's proceed with the CI. |
|
@arsenin-kitsoft the root cause of CI previously failing was that I'll need a bit of time to fully familiarize myself with the rest of the code base, but after that I'll hopefully find some time to review this and other PRs. In the meantime could you rebase such that you a) drop the commit updating ci and b) don't have merge commits in your history? I think these could also all be squashed, especially the fixes, but you can leave that for later as well |
|
@arckoor removed the commit bumping rustc version pin; got rid of the merge commit; squashed Note that the pinned rustc is currently 1.86.0, not 1.85.0 (see https://github.com/Keats/jsonwebtoken/blob/master/.github/workflows/ci.yml#L44). |
4 participants
Suggested changes for !250