Add option to verify JWT tokens in the HTTP Authorization header by tankyleo · Pull Request #72 · lightningdevkit/vss-server

tankyleo · 2025-12-06T02:05:17Z

No description provided.

ldk-reviews-bot · 2025-12-06T02:05:20Z

👋 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.

ldk-reviews-bot · 2025-12-08T02:05:51Z

🔔 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

Seems relatively straightforward - but for my understanding: is the plan to also add the issuance part in this PR?

tnull · 2025-12-08T08:41:01Z

 pub(crate) struct Config {
 	pub(crate) server_config: ServerConfig,
-	pub(crate) postgresql_config: Option<PostgreSQLConfig>,
+	pub(crate) postgresql_config: PostgreSQLConfig,


Why is this mandatory now? How will this work with the InMemoryStore or any other impl we'll add?

We currently don't support any alternative backends, and expect if this is None. I would prefer failing earlier, with the Config member type to represent our current status (ie not Option<PostgreSQLConfig> but PostgreSQLConfig).
The error message if the postgresql_config table is not present is this one:
Failed to load configuration: TOML parse error at line 1, column 1 | 1 | [server_config] | ^ missing field `postgresql_config`

That being said, I'm thinking we keep this Option to open up the InMemoryStore use case later.

That being said, I'm thinking we keep this Option to open up the InMemoryStore use case later.
Yeah, I think it would be preferable if we already know that we'll have different backends.

tankyleo · 2025-12-08T16:45:21Z

Seems relatively straightforward - but for my understanding: is the plan to also add the issuance part in this PR?

Do you think issuance is in-scope for VSS-server ? My immediate thinking is VSS-server should only be verifying, and not touch the private key. Issuance should be handled by something separate with different security measures that does have this privileged access to the private key.

See vss channel, it seems the immediate request from users is just the ability to set the public key for verifying JWT tokens.

ldk-reviews-bot · 2025-12-10T16:51:56Z

🔔 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 · 2025-12-11T13:47:58Z

Do you think issuance is in-scope for VSS-server ? My immediate thinking is VSS-server should only be verifying, and not touch the private key. Issuance should be handled by something separate with different security measures that does have this privileged access to the private key.
See vss channel, it seems the immediate request from users is just the ability to set the public key for verifying JWT tokens.

Discussed this elsewhere: it would be good to add issuance to the vss-server in order to provide users with a ready-to-go solution. Though, that doesn't need to happen as part of this PR ofc.

tnull

Generally looks good, some minor comments.

Do we see a chance of adding a test for the authorizer? Maybe with some fixtures?

tnull · 2025-12-11T13:49:52Z

 pub(crate) struct ServerConfig {
 	pub(crate) host: String,
 	pub(crate) port: u16,
+	pub(crate) rsa_pub_file_path: Option<String>,


Shouldn't that rather be part of a new optional JwtAuthConfig section?

Please make it also possible to use an env var, since it's easier to use that on a k8s environment

See below @frnandu we now ask for the full PEM string, not the file that contains the string, either as an env var, or in the config.

tnull · 2025-12-11T13:54:14Z

 use serde::{Deserialize, Serialize};
 use std::collections::HashMap;

+pub use jsonwebtoken::DecodingKey;


nit: Hmm, leaking this auth-depdenent type to main.rs is a bit strange. Wouldn't it rather make sense to have JWTAuthorizer::new be fallible and handle the teh DecodingKey::from_rsa_pem step?

tnull · 2025-12-11T13:55:16Z

+mod util;
+mod vss_service;
+
+use util::config::{Config, ServerConfig};


nit: This likely should import be added in the above group.

tnull · 2025-12-11T14:01:19Z

 std::process::exit(1);
 },
 };
+	let addr: SocketAddr = match format!("{}:{}", host, port).parse() {


There is a lot going on here. Can we declutter this? Btw, since we're already here, might make sense to switch this to the From<(I, u16)> for SocketAddr implementation rather than allocating a string and then parsing it again.

this is personal preference, but in the follow-up PR I consolidate the host and port setting into a single address setting, string type, ie "127.0.0.1:54234". We would then allocate the string upfront. WDYT ? Overall looking to reduce the number of lines in the config files, and the number of different settings. The address the server binds to is a single setting in my head :)

Yeah probably makes sense to make it one.

tnull

LGTM, please squash.

The RSA public key against which the JWT tokens are verified can be set either via a configuration file setting, or an environment variable, with the latter having the higher priority.

tankyleo · 2025-12-16T16:36:37Z

Squashed with no further changes.

tankyleo · 2025-12-06T02:05:17Z

No description provided.

ldk-reviews-bot · 2025-12-06T02:05:20Z

👋 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.

ldk-reviews-bot · 2025-12-08T02:05:51Z

🔔 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

Seems relatively straightforward - but for my understanding: is the plan to also add the issuance part in this PR?

tnull · 2025-12-08T08:41:01Z

 pub(crate) struct Config {
 	pub(crate) server_config: ServerConfig,
-	pub(crate) postgresql_config: Option<PostgreSQLConfig>,
+	pub(crate) postgresql_config: PostgreSQLConfig,


Why is this mandatory now? How will this work with the InMemoryStore or any other impl we'll add?

We currently don't support any alternative backends, and expect if this is None. I would prefer failing earlier, with the Config member type to represent our current status (ie not Option<PostgreSQLConfig> but PostgreSQLConfig).
The error message if the postgresql_config table is not present is this one:
Failed to load configuration: TOML parse error at line 1, column 1 | 1 | [server_config] | ^ missing field `postgresql_config`

That being said, I'm thinking we keep this Option to open up the InMemoryStore use case later.

That being said, I'm thinking we keep this Option to open up the InMemoryStore use case later.
Yeah, I think it would be preferable if we already know that we'll have different backends.

tankyleo · 2025-12-08T16:45:21Z

Seems relatively straightforward - but for my understanding: is the plan to also add the issuance part in this PR?

Do you think issuance is in-scope for VSS-server ? My immediate thinking is VSS-server should only be verifying, and not touch the private key. Issuance should be handled by something separate with different security measures that does have this privileged access to the private key.

See vss channel, it seems the immediate request from users is just the ability to set the public key for verifying JWT tokens.

ldk-reviews-bot · 2025-12-10T16:51:56Z

🔔 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 · 2025-12-11T13:47:58Z

Do you think issuance is in-scope for VSS-server ? My immediate thinking is VSS-server should only be verifying, and not touch the private key. Issuance should be handled by something separate with different security measures that does have this privileged access to the private key.
See vss channel, it seems the immediate request from users is just the ability to set the public key for verifying JWT tokens.

Discussed this elsewhere: it would be good to add issuance to the vss-server in order to provide users with a ready-to-go solution. Though, that doesn't need to happen as part of this PR ofc.

tnull

Generally looks good, some minor comments.

Do we see a chance of adding a test for the authorizer? Maybe with some fixtures?

tnull · 2025-12-11T13:49:52Z

 pub(crate) struct ServerConfig {
 	pub(crate) host: String,
 	pub(crate) port: u16,
+	pub(crate) rsa_pub_file_path: Option<String>,


Shouldn't that rather be part of a new optional JwtAuthConfig section?

Please make it also possible to use an env var, since it's easier to use that on a k8s environment

See below @frnandu we now ask for the full PEM string, not the file that contains the string, either as an env var, or in the config.

tnull · 2025-12-11T13:54:14Z

 use serde::{Deserialize, Serialize};
 use std::collections::HashMap;

+pub use jsonwebtoken::DecodingKey;


nit: Hmm, leaking this auth-depdenent type to main.rs is a bit strange. Wouldn't it rather make sense to have JWTAuthorizer::new be fallible and handle the teh DecodingKey::from_rsa_pem step?

tnull · 2025-12-11T13:55:16Z

+mod util;
+mod vss_service;
+
+use util::config::{Config, ServerConfig};


nit: This likely should import be added in the above group.

tnull · 2025-12-11T14:01:19Z

 std::process::exit(1);
 },
 };
+	let addr: SocketAddr = match format!("{}:{}", host, port).parse() {


There is a lot going on here. Can we declutter this? Btw, since we're already here, might make sense to switch this to the From<(I, u16)> for SocketAddr implementation rather than allocating a string and then parsing it again.

this is personal preference, but in the follow-up PR I consolidate the host and port setting into a single address setting, string type, ie "127.0.0.1:54234". We would then allocate the string upfront. WDYT ? Overall looking to reduce the number of lines in the config files, and the number of different settings. The address the server binds to is a single setting in my head :)

Yeah probably makes sense to make it one.

tnull

LGTM, please squash.

The RSA public key against which the JWT tokens are verified can be set either via a configuration file setting, or an environment variable, with the latter having the higher priority.

tankyleo · 2025-12-16T16:36:37Z

Squashed with no further changes.

tankyleo · 2025-12-06T02:05:17Z

No description provided.

ldk-reviews-bot · 2025-12-06T02:05:20Z

👋 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.

ldk-reviews-bot · 2025-12-08T02:05:51Z

🔔 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

Seems relatively straightforward - but for my understanding: is the plan to also add the issuance part in this PR?

tnull · 2025-12-08T08:41:01Z

 pub(crate) struct Config {
 	pub(crate) server_config: ServerConfig,
-	pub(crate) postgresql_config: Option<PostgreSQLConfig>,
+	pub(crate) postgresql_config: PostgreSQLConfig,


Why is this mandatory now? How will this work with the InMemoryStore or any other impl we'll add?

We currently don't support any alternative backends, and expect if this is None. I would prefer failing earlier, with the Config member type to represent our current status (ie not Option<PostgreSQLConfig> but PostgreSQLConfig).
The error message if the postgresql_config table is not present is this one:
Failed to load configuration: TOML parse error at line 1, column 1 | 1 | [server_config] | ^ missing field `postgresql_config`

That being said, I'm thinking we keep this Option to open up the InMemoryStore use case later.

That being said, I'm thinking we keep this Option to open up the InMemoryStore use case later.
Yeah, I think it would be preferable if we already know that we'll have different backends.

tankyleo · 2025-12-08T16:45:21Z

Seems relatively straightforward - but for my understanding: is the plan to also add the issuance part in this PR?

Do you think issuance is in-scope for VSS-server ? My immediate thinking is VSS-server should only be verifying, and not touch the private key. Issuance should be handled by something separate with different security measures that does have this privileged access to the private key.

See vss channel, it seems the immediate request from users is just the ability to set the public key for verifying JWT tokens.

ldk-reviews-bot · 2025-12-10T16:51:56Z

🔔 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 · 2025-12-11T13:47:58Z

Do you think issuance is in-scope for VSS-server ? My immediate thinking is VSS-server should only be verifying, and not touch the private key. Issuance should be handled by something separate with different security measures that does have this privileged access to the private key.
See vss channel, it seems the immediate request from users is just the ability to set the public key for verifying JWT tokens.

Discussed this elsewhere: it would be good to add issuance to the vss-server in order to provide users with a ready-to-go solution. Though, that doesn't need to happen as part of this PR ofc.

tnull

Generally looks good, some minor comments.

Do we see a chance of adding a test for the authorizer? Maybe with some fixtures?

tnull · 2025-12-11T13:49:52Z

 pub(crate) struct ServerConfig {
 	pub(crate) host: String,
 	pub(crate) port: u16,
+	pub(crate) rsa_pub_file_path: Option<String>,


Shouldn't that rather be part of a new optional JwtAuthConfig section?

Please make it also possible to use an env var, since it's easier to use that on a k8s environment

See below @frnandu we now ask for the full PEM string, not the file that contains the string, either as an env var, or in the config.

tnull · 2025-12-11T13:54:14Z

 use serde::{Deserialize, Serialize};
 use std::collections::HashMap;

+pub use jsonwebtoken::DecodingKey;


nit: Hmm, leaking this auth-depdenent type to main.rs is a bit strange. Wouldn't it rather make sense to have JWTAuthorizer::new be fallible and handle the teh DecodingKey::from_rsa_pem step?

tnull · 2025-12-11T13:55:16Z

+mod util;
+mod vss_service;
+
+use util::config::{Config, ServerConfig};


nit: This likely should import be added in the above group.

tnull · 2025-12-11T14:01:19Z

 std::process::exit(1);
 },
 };
+	let addr: SocketAddr = match format!("{}:{}", host, port).parse() {


There is a lot going on here. Can we declutter this? Btw, since we're already here, might make sense to switch this to the From<(I, u16)> for SocketAddr implementation rather than allocating a string and then parsing it again.

this is personal preference, but in the follow-up PR I consolidate the host and port setting into a single address setting, string type, ie "127.0.0.1:54234". We would then allocate the string upfront. WDYT ? Overall looking to reduce the number of lines in the config files, and the number of different settings. The address the server binds to is a single setting in my head :)

Yeah probably makes sense to make it one.

tnull

LGTM, please squash.

The RSA public key against which the JWT tokens are verified can be set either via a configuration file setting, or an environment variable, with the latter having the higher priority.

tankyleo · 2025-12-16T16:36:37Z

Squashed with no further changes.

tankyleo · 2025-12-06T02:05:17Z

No description provided.

ldk-reviews-bot · 2025-12-06T02:05:20Z

👋 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.

ldk-reviews-bot · 2025-12-08T02:05:51Z

🔔 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

Seems relatively straightforward - but for my understanding: is the plan to also add the issuance part in this PR?

tnull · 2025-12-08T08:41:01Z

 pub(crate) struct Config {
 	pub(crate) server_config: ServerConfig,
-	pub(crate) postgresql_config: Option<PostgreSQLConfig>,
+	pub(crate) postgresql_config: PostgreSQLConfig,


Why is this mandatory now? How will this work with the InMemoryStore or any other impl we'll add?

We currently don't support any alternative backends, and expect if this is None. I would prefer failing earlier, with the Config member type to represent our current status (ie not Option<PostgreSQLConfig> but PostgreSQLConfig).
The error message if the postgresql_config table is not present is this one:
Failed to load configuration: TOML parse error at line 1, column 1 | 1 | [server_config] | ^ missing field `postgresql_config`

That being said, I'm thinking we keep this Option to open up the InMemoryStore use case later.

That being said, I'm thinking we keep this Option to open up the InMemoryStore use case later.
Yeah, I think it would be preferable if we already know that we'll have different backends.

tankyleo · 2025-12-08T16:45:21Z

Seems relatively straightforward - but for my understanding: is the plan to also add the issuance part in this PR?

Do you think issuance is in-scope for VSS-server ? My immediate thinking is VSS-server should only be verifying, and not touch the private key. Issuance should be handled by something separate with different security measures that does have this privileged access to the private key.

See vss channel, it seems the immediate request from users is just the ability to set the public key for verifying JWT tokens.

ldk-reviews-bot · 2025-12-10T16:51:56Z

🔔 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 · 2025-12-11T13:47:58Z

Do you think issuance is in-scope for VSS-server ? My immediate thinking is VSS-server should only be verifying, and not touch the private key. Issuance should be handled by something separate with different security measures that does have this privileged access to the private key.
See vss channel, it seems the immediate request from users is just the ability to set the public key for verifying JWT tokens.

Discussed this elsewhere: it would be good to add issuance to the vss-server in order to provide users with a ready-to-go solution. Though, that doesn't need to happen as part of this PR ofc.

tnull

Generally looks good, some minor comments.

Do we see a chance of adding a test for the authorizer? Maybe with some fixtures?

tnull · 2025-12-11T13:49:52Z

 pub(crate) struct ServerConfig {
 	pub(crate) host: String,
 	pub(crate) port: u16,
+	pub(crate) rsa_pub_file_path: Option<String>,


Shouldn't that rather be part of a new optional JwtAuthConfig section?

Please make it also possible to use an env var, since it's easier to use that on a k8s environment

See below @frnandu we now ask for the full PEM string, not the file that contains the string, either as an env var, or in the config.

tnull · 2025-12-11T13:54:14Z

 use serde::{Deserialize, Serialize};
 use std::collections::HashMap;

+pub use jsonwebtoken::DecodingKey;


nit: Hmm, leaking this auth-depdenent type to main.rs is a bit strange. Wouldn't it rather make sense to have JWTAuthorizer::new be fallible and handle the teh DecodingKey::from_rsa_pem step?

tnull · 2025-12-11T13:55:16Z

+mod util;
+mod vss_service;
+
+use util::config::{Config, ServerConfig};


nit: This likely should import be added in the above group.

tnull · 2025-12-11T14:01:19Z

 std::process::exit(1);
 },
 };
+	let addr: SocketAddr = match format!("{}:{}", host, port).parse() {


There is a lot going on here. Can we declutter this? Btw, since we're already here, might make sense to switch this to the From<(I, u16)> for SocketAddr implementation rather than allocating a string and then parsing it again.

this is personal preference, but in the follow-up PR I consolidate the host and port setting into a single address setting, string type, ie "127.0.0.1:54234". We would then allocate the string upfront. WDYT ? Overall looking to reduce the number of lines in the config files, and the number of different settings. The address the server binds to is a single setting in my head :)

Yeah probably makes sense to make it one.

tnull

LGTM, please squash.

The RSA public key against which the JWT tokens are verified can be set either via a configuration file setting, or an environment variable, with the latter having the higher priority.

tankyleo · 2025-12-16T16:36:37Z

Squashed with no further changes.

tankyleo · 2025-12-06T02:05:17Z

No description provided.

ldk-reviews-bot · 2025-12-06T02:05:20Z

👋 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.

ldk-reviews-bot · 2025-12-08T02:05:51Z

🔔 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

Seems relatively straightforward - but for my understanding: is the plan to also add the issuance part in this PR?

tnull · 2025-12-08T08:41:01Z

 pub(crate) struct Config {
 	pub(crate) server_config: ServerConfig,
-	pub(crate) postgresql_config: Option<PostgreSQLConfig>,
+	pub(crate) postgresql_config: PostgreSQLConfig,


Why is this mandatory now? How will this work with the InMemoryStore or any other impl we'll add?

We currently don't support any alternative backends, and expect if this is None. I would prefer failing earlier, with the Config member type to represent our current status (ie not Option<PostgreSQLConfig> but PostgreSQLConfig).
The error message if the postgresql_config table is not present is this one:
Failed to load configuration: TOML parse error at line 1, column 1 | 1 | [server_config] | ^ missing field `postgresql_config`

That being said, I'm thinking we keep this Option to open up the InMemoryStore use case later.

That being said, I'm thinking we keep this Option to open up the InMemoryStore use case later.
Yeah, I think it would be preferable if we already know that we'll have different backends.

tankyleo · 2025-12-08T16:45:21Z

Seems relatively straightforward - but for my understanding: is the plan to also add the issuance part in this PR?

Do you think issuance is in-scope for VSS-server ? My immediate thinking is VSS-server should only be verifying, and not touch the private key. Issuance should be handled by something separate with different security measures that does have this privileged access to the private key.

See vss channel, it seems the immediate request from users is just the ability to set the public key for verifying JWT tokens.

ldk-reviews-bot · 2025-12-10T16:51:56Z

🔔 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 · 2025-12-11T13:47:58Z

Do you think issuance is in-scope for VSS-server ? My immediate thinking is VSS-server should only be verifying, and not touch the private key. Issuance should be handled by something separate with different security measures that does have this privileged access to the private key.
See vss channel, it seems the immediate request from users is just the ability to set the public key for verifying JWT tokens.

Discussed this elsewhere: it would be good to add issuance to the vss-server in order to provide users with a ready-to-go solution. Though, that doesn't need to happen as part of this PR ofc.

tnull

Generally looks good, some minor comments.

Do we see a chance of adding a test for the authorizer? Maybe with some fixtures?

tnull · 2025-12-11T13:49:52Z

 pub(crate) struct ServerConfig {
 	pub(crate) host: String,
 	pub(crate) port: u16,
+	pub(crate) rsa_pub_file_path: Option<String>,


Shouldn't that rather be part of a new optional JwtAuthConfig section?

Please make it also possible to use an env var, since it's easier to use that on a k8s environment

See below @frnandu we now ask for the full PEM string, not the file that contains the string, either as an env var, or in the config.

tnull · 2025-12-11T13:54:14Z

 use serde::{Deserialize, Serialize};
 use std::collections::HashMap;

+pub use jsonwebtoken::DecodingKey;


nit: Hmm, leaking this auth-depdenent type to main.rs is a bit strange. Wouldn't it rather make sense to have JWTAuthorizer::new be fallible and handle the teh DecodingKey::from_rsa_pem step?

tnull · 2025-12-11T13:55:16Z

+mod util;
+mod vss_service;
+
+use util::config::{Config, ServerConfig};


nit: This likely should import be added in the above group.

tnull · 2025-12-11T14:01:19Z

 std::process::exit(1);
 },
 };
+	let addr: SocketAddr = match format!("{}:{}", host, port).parse() {


There is a lot going on here. Can we declutter this? Btw, since we're already here, might make sense to switch this to the From<(I, u16)> for SocketAddr implementation rather than allocating a string and then parsing it again.

this is personal preference, but in the follow-up PR I consolidate the host and port setting into a single address setting, string type, ie "127.0.0.1:54234". We would then allocate the string upfront. WDYT ? Overall looking to reduce the number of lines in the config files, and the number of different settings. The address the server binds to is a single setting in my head :)

Yeah probably makes sense to make it one.

tnull

LGTM, please squash.

The RSA public key against which the JWT tokens are verified can be set either via a configuration file setting, or an environment variable, with the latter having the higher priority.

tankyleo · 2025-12-16T16:36:37Z

Squashed with no further changes.

tankyleo · 2025-12-06T02:05:17Z

No description provided.

ldk-reviews-bot · 2025-12-06T02:05:20Z

👋 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.

ldk-reviews-bot · 2025-12-08T02:05:51Z

🔔 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

Seems relatively straightforward - but for my understanding: is the plan to also add the issuance part in this PR?

tnull · 2025-12-08T08:41:01Z

 pub(crate) struct Config {
 	pub(crate) server_config: ServerConfig,
-	pub(crate) postgresql_config: Option<PostgreSQLConfig>,
+	pub(crate) postgresql_config: PostgreSQLConfig,


Why is this mandatory now? How will this work with the InMemoryStore or any other impl we'll add?

We currently don't support any alternative backends, and expect if this is None. I would prefer failing earlier, with the Config member type to represent our current status (ie not Option<PostgreSQLConfig> but PostgreSQLConfig).
The error message if the postgresql_config table is not present is this one:
Failed to load configuration: TOML parse error at line 1, column 1 | 1 | [server_config] | ^ missing field `postgresql_config`

That being said, I'm thinking we keep this Option to open up the InMemoryStore use case later.

That being said, I'm thinking we keep this Option to open up the InMemoryStore use case later.
Yeah, I think it would be preferable if we already know that we'll have different backends.

tankyleo · 2025-12-08T16:45:21Z

Seems relatively straightforward - but for my understanding: is the plan to also add the issuance part in this PR?

Do you think issuance is in-scope for VSS-server ? My immediate thinking is VSS-server should only be verifying, and not touch the private key. Issuance should be handled by something separate with different security measures that does have this privileged access to the private key.

See vss channel, it seems the immediate request from users is just the ability to set the public key for verifying JWT tokens.

ldk-reviews-bot · 2025-12-10T16:51:56Z

🔔 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 · 2025-12-11T13:47:58Z

Do you think issuance is in-scope for VSS-server ? My immediate thinking is VSS-server should only be verifying, and not touch the private key. Issuance should be handled by something separate with different security measures that does have this privileged access to the private key.
See vss channel, it seems the immediate request from users is just the ability to set the public key for verifying JWT tokens.

Discussed this elsewhere: it would be good to add issuance to the vss-server in order to provide users with a ready-to-go solution. Though, that doesn't need to happen as part of this PR ofc.

tnull

Generally looks good, some minor comments.

Do we see a chance of adding a test for the authorizer? Maybe with some fixtures?

tnull · 2025-12-11T13:49:52Z

 pub(crate) struct ServerConfig {
 	pub(crate) host: String,
 	pub(crate) port: u16,
+	pub(crate) rsa_pub_file_path: Option<String>,


Shouldn't that rather be part of a new optional JwtAuthConfig section?

Please make it also possible to use an env var, since it's easier to use that on a k8s environment

See below @frnandu we now ask for the full PEM string, not the file that contains the string, either as an env var, or in the config.

tnull · 2025-12-11T13:54:14Z

 use serde::{Deserialize, Serialize};
 use std::collections::HashMap;

+pub use jsonwebtoken::DecodingKey;


nit: Hmm, leaking this auth-depdenent type to main.rs is a bit strange. Wouldn't it rather make sense to have JWTAuthorizer::new be fallible and handle the teh DecodingKey::from_rsa_pem step?

tnull · 2025-12-11T13:55:16Z

+mod util;
+mod vss_service;
+
+use util::config::{Config, ServerConfig};


nit: This likely should import be added in the above group.

tnull · 2025-12-11T14:01:19Z

 std::process::exit(1);
 },
 };
+	let addr: SocketAddr = match format!("{}:{}", host, port).parse() {


There is a lot going on here. Can we declutter this? Btw, since we're already here, might make sense to switch this to the From<(I, u16)> for SocketAddr implementation rather than allocating a string and then parsing it again.

this is personal preference, but in the follow-up PR I consolidate the host and port setting into a single address setting, string type, ie "127.0.0.1:54234". We would then allocate the string upfront. WDYT ? Overall looking to reduce the number of lines in the config files, and the number of different settings. The address the server binds to is a single setting in my head :)

Yeah probably makes sense to make it one.

tnull

LGTM, please squash.

The RSA public key against which the JWT tokens are verified can be set either via a configuration file setting, or an environment variable, with the latter having the higher priority.

tankyleo · 2025-12-16T16:36:37Z

Squashed with no further changes.

tankyleo · 2025-12-06T02:05:17Z

No description provided.

ldk-reviews-bot · 2025-12-06T02:05:20Z

👋 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.

ldk-reviews-bot · 2025-12-08T02:05:51Z

🔔 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

Seems relatively straightforward - but for my understanding: is the plan to also add the issuance part in this PR?

tnull · 2025-12-08T08:41:01Z

 pub(crate) struct Config {
 	pub(crate) server_config: ServerConfig,
-	pub(crate) postgresql_config: Option<PostgreSQLConfig>,
+	pub(crate) postgresql_config: PostgreSQLConfig,


Why is this mandatory now? How will this work with the InMemoryStore or any other impl we'll add?

We currently don't support any alternative backends, and expect if this is None. I would prefer failing earlier, with the Config member type to represent our current status (ie not Option<PostgreSQLConfig> but PostgreSQLConfig).
The error message if the postgresql_config table is not present is this one:
Failed to load configuration: TOML parse error at line 1, column 1 | 1 | [server_config] | ^ missing field `postgresql_config`

That being said, I'm thinking we keep this Option to open up the InMemoryStore use case later.

That being said, I'm thinking we keep this Option to open up the InMemoryStore use case later.
Yeah, I think it would be preferable if we already know that we'll have different backends.

tankyleo · 2025-12-08T16:45:21Z

Seems relatively straightforward - but for my understanding: is the plan to also add the issuance part in this PR?

Do you think issuance is in-scope for VSS-server ? My immediate thinking is VSS-server should only be verifying, and not touch the private key. Issuance should be handled by something separate with different security measures that does have this privileged access to the private key.

See vss channel, it seems the immediate request from users is just the ability to set the public key for verifying JWT tokens.

ldk-reviews-bot · 2025-12-10T16:51:56Z

🔔 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 · 2025-12-11T13:47:58Z

Do you think issuance is in-scope for VSS-server ? My immediate thinking is VSS-server should only be verifying, and not touch the private key. Issuance should be handled by something separate with different security measures that does have this privileged access to the private key.
See vss channel, it seems the immediate request from users is just the ability to set the public key for verifying JWT tokens.

Discussed this elsewhere: it would be good to add issuance to the vss-server in order to provide users with a ready-to-go solution. Though, that doesn't need to happen as part of this PR ofc.

tnull

Generally looks good, some minor comments.

Do we see a chance of adding a test for the authorizer? Maybe with some fixtures?

tnull · 2025-12-11T13:49:52Z

 pub(crate) struct ServerConfig {
 	pub(crate) host: String,
 	pub(crate) port: u16,
+	pub(crate) rsa_pub_file_path: Option<String>,


Shouldn't that rather be part of a new optional JwtAuthConfig section?

Please make it also possible to use an env var, since it's easier to use that on a k8s environment

See below @frnandu we now ask for the full PEM string, not the file that contains the string, either as an env var, or in the config.

tnull · 2025-12-11T13:54:14Z

 use serde::{Deserialize, Serialize};
 use std::collections::HashMap;

+pub use jsonwebtoken::DecodingKey;


nit: Hmm, leaking this auth-depdenent type to main.rs is a bit strange. Wouldn't it rather make sense to have JWTAuthorizer::new be fallible and handle the teh DecodingKey::from_rsa_pem step?

tnull · 2025-12-11T13:55:16Z

+mod util;
+mod vss_service;
+
+use util::config::{Config, ServerConfig};


nit: This likely should import be added in the above group.

tnull · 2025-12-11T14:01:19Z

 std::process::exit(1);
 },
 };
+	let addr: SocketAddr = match format!("{}:{}", host, port).parse() {


There is a lot going on here. Can we declutter this? Btw, since we're already here, might make sense to switch this to the From<(I, u16)> for SocketAddr implementation rather than allocating a string and then parsing it again.

this is personal preference, but in the follow-up PR I consolidate the host and port setting into a single address setting, string type, ie "127.0.0.1:54234". We would then allocate the string upfront. WDYT ? Overall looking to reduce the number of lines in the config files, and the number of different settings. The address the server binds to is a single setting in my head :)

Yeah probably makes sense to make it one.

tnull

LGTM, please squash.

The RSA public key against which the JWT tokens are verified can be set either via a configuration file setting, or an environment variable, with the latter having the higher priority.

tankyleo · 2025-12-16T16:36:37Z

Squashed with no further changes.

tankyleo · 2025-12-06T02:05:17Z

No description provided.

ldk-reviews-bot · 2025-12-06T02:05:20Z

👋 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.

ldk-reviews-bot · 2025-12-08T02:05:51Z

🔔 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

Seems relatively straightforward - but for my understanding: is the plan to also add the issuance part in this PR?

tnull · 2025-12-08T08:41:01Z

 pub(crate) struct Config {
 	pub(crate) server_config: ServerConfig,
-	pub(crate) postgresql_config: Option<PostgreSQLConfig>,
+	pub(crate) postgresql_config: PostgreSQLConfig,


Why is this mandatory now? How will this work with the InMemoryStore or any other impl we'll add?

We currently don't support any alternative backends, and expect if this is None. I would prefer failing earlier, with the Config member type to represent our current status (ie not Option<PostgreSQLConfig> but PostgreSQLConfig).
The error message if the postgresql_config table is not present is this one:
Failed to load configuration: TOML parse error at line 1, column 1 | 1 | [server_config] | ^ missing field `postgresql_config`

That being said, I'm thinking we keep this Option to open up the InMemoryStore use case later.

That being said, I'm thinking we keep this Option to open up the InMemoryStore use case later.
Yeah, I think it would be preferable if we already know that we'll have different backends.

tankyleo · 2025-12-08T16:45:21Z

Seems relatively straightforward - but for my understanding: is the plan to also add the issuance part in this PR?

Do you think issuance is in-scope for VSS-server ? My immediate thinking is VSS-server should only be verifying, and not touch the private key. Issuance should be handled by something separate with different security measures that does have this privileged access to the private key.

See vss channel, it seems the immediate request from users is just the ability to set the public key for verifying JWT tokens.

ldk-reviews-bot · 2025-12-10T16:51:56Z

🔔 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 · 2025-12-11T13:47:58Z

Do you think issuance is in-scope for VSS-server ? My immediate thinking is VSS-server should only be verifying, and not touch the private key. Issuance should be handled by something separate with different security measures that does have this privileged access to the private key.
See vss channel, it seems the immediate request from users is just the ability to set the public key for verifying JWT tokens.

Discussed this elsewhere: it would be good to add issuance to the vss-server in order to provide users with a ready-to-go solution. Though, that doesn't need to happen as part of this PR ofc.

tnull

Generally looks good, some minor comments.

Do we see a chance of adding a test for the authorizer? Maybe with some fixtures?

tnull · 2025-12-11T13:49:52Z

 pub(crate) struct ServerConfig {
 	pub(crate) host: String,
 	pub(crate) port: u16,
+	pub(crate) rsa_pub_file_path: Option<String>,


Shouldn't that rather be part of a new optional JwtAuthConfig section?

Please make it also possible to use an env var, since it's easier to use that on a k8s environment

See below @frnandu we now ask for the full PEM string, not the file that contains the string, either as an env var, or in the config.

tnull · 2025-12-11T13:54:14Z

 use serde::{Deserialize, Serialize};
 use std::collections::HashMap;

+pub use jsonwebtoken::DecodingKey;


nit: Hmm, leaking this auth-depdenent type to main.rs is a bit strange. Wouldn't it rather make sense to have JWTAuthorizer::new be fallible and handle the teh DecodingKey::from_rsa_pem step?

tnull · 2025-12-11T13:55:16Z

+mod util;
+mod vss_service;
+
+use util::config::{Config, ServerConfig};


nit: This likely should import be added in the above group.

tnull · 2025-12-11T14:01:19Z

 std::process::exit(1);
 },
 };
+	let addr: SocketAddr = match format!("{}:{}", host, port).parse() {


There is a lot going on here. Can we declutter this? Btw, since we're already here, might make sense to switch this to the From<(I, u16)> for SocketAddr implementation rather than allocating a string and then parsing it again.

this is personal preference, but in the follow-up PR I consolidate the host and port setting into a single address setting, string type, ie "127.0.0.1:54234". We would then allocate the string upfront. WDYT ? Overall looking to reduce the number of lines in the config files, and the number of different settings. The address the server binds to is a single setting in my head :)

Yeah probably makes sense to make it one.

tnull

LGTM, please squash.

The RSA public key against which the JWT tokens are verified can be set either via a configuration file setting, or an environment variable, with the latter having the higher priority.

tankyleo · 2025-12-16T16:36:37Z

Squashed with no further changes.

tankyleo requested a review from tnull December 6, 2025 02:05

tankyleo force-pushed the rust-jwt-auth branch from 96d6359 to d127bd1Compare December 6, 2025 23:00

tankyleo mentioned this pull request Dec 6, 2025
Add default database settting, and env var overrides #73
Merged

tnull reviewed Dec 8, 2025
View reviewed changes

tankyleo requested a review from tnull December 8, 2025 16:51

tnull reviewed Dec 11, 2025
View reviewed changes

tankyleo self-assigned this Dec 11, 2025

tankyleo added this to Weekly Goals Dec 11, 2025

tankyleo moved this to Goal: Merge in Weekly Goals Dec 11, 2025

tankyleo force-pushed the rust-jwt-auth branch 3 times, most recently from 7d4b8bd to 5c555c0Compare December 15, 2025 23:54

tankyleo requested a review from tnull December 16, 2025 00:02

tnull reviewed Dec 16, 2025
View reviewed changes

Add option to verify JWT tokens in the HTTP Authorization header
fed1cfe
The RSA public key against which the JWT tokens are verified can be set either via a configuration file setting, or an environment variable, with the latter having the higher priority.

tankyleo force-pushed the rust-jwt-auth branch from 5c555c0 to fed1cfeCompare December 16, 2025 16:35

tankyleo requested a review from tnull December 16, 2025 16:36

tnull approved these changes Dec 17, 2025
View reviewed changes

tnull merged commit 6a2278a into lightningdevkit:mainDec 17, 2025
2 checks passed

github-project-automationBot moved this from Goal: Merge to Done in Weekly Goals Dec 17, 2025

tankyleo deleted the rust-jwt-auth branch December 17, 2025 17:14

enigbe mentioned this pull request Feb 6, 2026
Add matrix testing for VSS authorizer variants lightningdevkit/ldk-node#786
Closed