Drop jsonwebtoken dependency by tankyleo · Pull Request #94 · lightningdevkit/vss-server

tankyleo · 2026-02-09T06:56:32Z

Fixes #92.

ldk-reviews-bot · 2026-02-09T06:56:35Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

tankyleo · 2026-02-09T06:58:48Z

Fixes #92

tnull · 2026-02-09T08:14:30Z

Fixes #92

Note you'll have to put that in the OP otherwise it doesn't link the issue.

tnull · 2026-02-09T08:27:42Z

+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"] }


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?

Thank you yes great idea

tnull

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.

tnull · 2026-02-10T09:49:51Z


 const BEARER_PREFIX: &str = "Bearer ";

+fn parse_public_key_pem(pem: &str) -> Result<PKey<Public>, String> {


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.

tnull · 2026-02-10T09:53:54Z

+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"] }


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?

tnull · 2026-02-23T09:07:59Z

@tankyleo Gentle ping here.

tankyleo · 2026-05-02T00:06:46Z

Let me know what you think tnull, that last commit expands the scope of this PR a little where I add optional validation of exp claims, thought we could get this done along the way

ldk-reviews-bot · 2026-05-04T00:06:53Z

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tnull

Generally LGTM, but added some comments that Codex had for me. Seems they might be worth addressing even if not major?

Let me know what you think tnull, that last commit expands the scope of this PR a little where I add optional validation of exp claims, thought we could get this done along the way

Sounds good, though, AFAIU Codex thinks the validation didn't use to be optional before?

tnull · 2026-05-04T09:07:12Z

+				return Err(VssError::AuthError(String::from("The token is expired")));
+			}
+		}
+


Codex:

High - rust/auth-impls/src/jwt.rs:103-118: aud is ignored. The old validator rejected tokens with an audience when no expected audience was configured; this now accepts tokens minted for another audience/service if signed by the same issuer.

tnull · 2026-05-04T09:07:41Z

+			.map_err(|_| VssError::AuthError(String::from("Claims json decode failed")))?;
+
+		// Check if the token is expired
+		if let Some(exp) = claims.exp {


Codex:

High - rust/auth-impls/src/jwt.rs:39-40, rust/auth-impls/src/jwt.rs:111-116: JWTs without exp are now accepted. main used jsonwebtoken::Validation::new(Algorithm::RS256), which requires exp by default, so this creates non-expiring bearer tokens unless that policy change is
intentional.

tnull · 2026-05-04T09:09:02Z


 const BEARER_PREFIX: &str = "Bearer ";

+fn parse_public_key_pem(pem: &str) -> Result<PKey<Public>, ()> {


Codex:

Medium - rust/auth-impls/src/jwt.rs:45-54, rust/auth-impls/src/jwt.rs:94-98: RS256 is not tied to an RSA key. PKey::public_key_from_der accepts generic public keys, and OpenSSL verifies using the configured key type, so non-RSA keys are not rejected at setup. █

Medium - rust/auth-impls/src/jwt.rs:45-54: PEM parsing regressed to exact -----BEGIN PUBLIC KEY----- SPKI input. jsonwebtoken::from_rsa_pem also accepted common PKCS#1 -----BEGIN RSA PUBLIC KEY----- inputs, so some existing configs may fail to start.

tankyleo · 2026-05-05T01:38:14Z

Thanks addressed review, also passed the code through codex :)

tankyleo · 2026-05-05T01:40:56Z

AFAIU Codex thinks the validation didn't use to be optional before?

Yes Codex is right, I misunderstood things given our old Claims struct only had a single sub member.

tnull

Feel free to squash.

tnull · 2026-05-05T09:01:44Z

 	/// Refer: https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.2
 	sub: String,
+	/// Expiration Time
+	exp: u64,


Codex:

- rust/auth-impls/src/jwt.rs:40: exp is now deserialized as u64 in the application claims. The old production path decoded into Claims { sub } and let jsonwebtoken validate exp separately, which accepted JWT NumericDate values encoded as JSON floats. Tokens with valid fractional exp values will now fail with Claims json decode failed, so this is a compatibility regression.

Maybe that's fine?

Yes I gave it some thought, would prefer to only add this if there is user demand.

This removes `time` from the dependency tree because the crate shipped a security fix behind an MSRV bump. Fixes lightningdevkit#92. We make a best effort attempt to keep the same behavior as when we validated JWTs using `jsonwebtoken`. This includes rejecting any JWTs that have the `aud` field set. We also keep `jsonwebtoken` as a dev-dependency and add parity tests. Unlike `jsonwebtoken`, we now fail to decode JWTs that contain `exp` fields with fractional parts.

tankyleo · 2026-05-05T16:12:25Z

Squashed with no further changes

tankyleo requested a review from tnull February 9, 2026 06:56

tankyleo self-assigned this Feb 9, 2026

tankyleo linked an issue Feb 9, 2026 that may be closed by this pull request

Revert time pin and/or drop jsonwebtoken dependency ASAP #92

Closed

tankyleo force-pushed the 2026-02-drop-jsonwebtoken branch from 7ff7d0a to bdd21e1 Compare February 9, 2026 07:07

tankyleo mentioned this pull request Feb 9, 2026

Downgrade time dependency to 0.3.5 #93

Closed

tnull reviewed Feb 9, 2026

View reviewed changes

tankyleo requested a review from tnull February 10, 2026 07:34

tnull reviewed Feb 10, 2026

View reviewed changes

tankyleo requested a review from tnull May 2, 2026 00:05

tnull reviewed May 4, 2026

View reviewed changes

tankyleo requested a review from tnull May 5, 2026 01:38

tnull reviewed May 5, 2026

View reviewed changes

tankyleo force-pushed the 2026-02-drop-jsonwebtoken branch from 4422d39 to 9ec254d Compare May 5, 2026 16:05

tankyleo requested a review from tnull May 5, 2026 16:12

tnull approved these changes May 5, 2026

View reviewed changes

tnull merged commit f446355 into lightningdevkit:main May 5, 2026
6 checks passed


		const BEARER_PREFIX: &str = "Bearer ";

		fn parse_public_key_pem(pem: &str) -> Result<PKey<Public>, String> {

Conversation

tankyleo commented Feb 9, 2026 • edited by tnull Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tankyleo commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull commented Feb 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull commented Feb 23, 2026

Uh oh!

tankyleo commented May 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented May 4, 2026

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo commented May 5, 2026

Uh oh!

tankyleo commented May 5, 2026

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo commented May 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tankyleo commented Feb 9, 2026 •

edited by tnull

Loading

ldk-reviews-bot commented Feb 9, 2026 •

edited

Loading

tankyleo commented Feb 9, 2026 •

edited

Loading

tankyleo commented May 2, 2026 •

edited

Loading