-
Notifications
You must be signed in to change notification settings - Fork 21
Drop jsonwebtoken dependency #94
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
base: main
Are you sure you want to change the base?
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
|
Fixes #92 |
7ff7d0a to
bdd21e1
Compare
Note you'll have to put that in the OP otherwise it doesn't link the issue. |
rust/auth-impls/Cargo.toml
Outdated
| base64 = { version = "0.22.1", optional = true, default-features = false, features = ["std"] } | ||
| bitcoin_hashes = { version = "0.19", optional = true, default-features = false } | ||
| hex-conservative = { version = "1.0", optional = true, default-features = false } | ||
| rsa = { version = "0.9.10", optional = true, default-features = false, features = ["sha2"] } |
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.
We already have openssl in our tree - I do wonder if we could use openssl::rsa (https://docs.rs/openssl/latest/openssl/rsa/index.html) rather than this additional dependency?
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.
Thank you yes great idea
tnull
left a comment
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.
Thanks for the update, some minor comments, otherwise looks good.
However, I think vss-server in general gets into the realm where it could use some some fuzzing or proptest coverage to ensure that our implementations behave as expected, are safe, never panic, etc.
|
|
||
| const BEARER_PREFIX: &str = "Bearer "; | ||
|
|
||
| fn parse_public_key_pem(pem: &str) -> Result<PKey<Public>, String> { |
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.
Should we just return () or, if you prefer, a proper enum error type here? Using a string introduces some unnecessary allocations, and they can't easily be handled. Same goes for new then ofc.
| serde_json = { version = "1.0.149", optional = true, default-features = false, features = ["std"] } | ||
|
|
||
| [dev-dependencies] | ||
| jsonwebtoken = { version = "9.3.0", default-features = false, features = ["use_pem"] } |
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.
If we already keep it as a dev dependency, can we add some parity/backwards compat tests that ensure our new custom implementation behaves the same way as this did?
Fixes #92.