Skip to content

JWT authorization header based on LNURL Auth #26

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

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

wvanlint
Copy link
Contributor

@wvanlint wvanlint commented Feb 9, 2024

  • Implements a specific HeaderProvider that will provide a JWT authorization header based on LNURL Auth.
    • LUD-05 will be used for key derivation instead of LUD-13.
    • A token field will be expected in a successful LUD-04 verification response, containing the JWT token.
    • JWT tokens will be cached according to their expiry.

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a really high-level pass, mostly questions about the design choices and introduced dependencies.

Cargo.toml Outdated
async-trait = "0.1.77"
futures = "0.3.30"
url = "2.5.0"
hmac = "0.12.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use bitcoin::hashes crate here rather than the extra hmac/sha2 dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Cargo.lock Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is a library, checking in Cargo.lock should be avoided.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah accidentally added this, added to .gitignore.

Cargo.toml Outdated

[target.'cfg(genproto)'.build-dependencies]
prost-build = { version = "0.11.3" }
reqwest = { version = "0.11.13", features = ["blocking"] }

[dev-dependencies]
axum = { version = "0.7.4", features = ["ws"] }
mockall = "0.12.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we need yet another mocking library? Why can't we use the mockito dependency we already have available?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mockito library is a specific mocking library for HTTP calls. The mockall library allows more general mocking of Rust traits.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this as Websockets were removed in favor of simple HTTP calls by depending on a token field in the LNURL Auth response.

Cargo.toml Outdated
tungstenite = "0.21.0"
async-trait = "0.1.77"
futures = "0.3.30"
url = "2.5.0"
Copy link
Contributor

@tnull tnull Feb 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced that we need to introduce a dependency just to check whether a URL is invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also use this library to handle parsing the domain name, parsing the query parameters, and adding additional sig and key query parameters. We can implement this functionality, but it would be nice to use the library here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have tried to minimize dependencies in this crate,
it might make sense to feature-gate these additional deps to "lnurl-auth" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put the LNURL Auth JWT implementation and relevant dependencies under a feature.

#[async_trait]
pub trait HeaderProvider {
/// Error type when returning headers.
type Error: std::error::Error;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably define a clear error type here rather than using type Error: std::error::Error;.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you have in mind here? I added the associated type with a trait bound because each implementation might run into different kinds of errors. Returning fixed headers is infallible, but LNURL Auth might run into protocol-specific errors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, yeah, I guess. I'd generally still prefer to clearly define the possible error cases as this kind of pattern would often result in using dyn Error which is a mess to reliably/predictably handle by the user (and also comes with a bunch of overhead, such as heap allocations).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplified these into some basic error cases, but let me know if using String or std::io::Error would be better.

Cargo.toml Outdated
@@ -16,11 +16,25 @@ prost = "0.11.6"
reqwest = { version = "0.11.13", features = ["rustls-tls"] }
tokio = { version = "1", default-features = false, features = ["time"] }
rand = "0.8.5"
bitcoin = "0.31.1"
tokio-tungstenite = "0.21.0"
tungstenite = "0.21.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you expand on why this needs to use websockets rather than, e.g., just HTTPS requests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIU, the LNURL Auth spec defines a flow that "side-loads" authentication: a call providing a signature will be made by the client to the server, but the server can only return success/failure: https://github.com/lnurl/luds/blob/luds/04.md#wallet-to-service-interaction-flow. The server is responsible for correlating that call to the original challenge and continuing the user's intent there. In this case, the Websocket connection is used to maintain the context between providing the challenge and issuing the JWT token and allows the server to push the JWT token without the client having to poll.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline. Although leveraging Websockets can support multiple devices e.g. with a QR scanning flow and it is used in Mutiny wallet, it is not relevant to our use case. We confirmed a token field can be added to the LNURL Auth response, and the simplification is definitely worthwhile. Changed this to only use HTTP(S) requests and returning a token field.

Cargo.toml Outdated
@@ -16,11 +16,23 @@ prost = "0.11.6"
reqwest = { version = "0.11.13", features = ["rustls-tls"] }
tokio = { version = "1", default-features = false, features = ["time"] }
rand = "0.8.5"
bitcoin = "0.31.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LDK/LDK Node are currently on bitcoin v0.30.2. While we'll upgrade soon, it might be best to keep it compatible here for the time being.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this to use 0.30.2.

@wvanlint wvanlint force-pushed the lnurl_auth branch 2 times, most recently from 4a26989 to 95cce65 Compare March 19, 2024 18:30
@wvanlint wvanlint marked this pull request as ready for review March 19, 2024 21:26
@wvanlint wvanlint force-pushed the lnurl_auth branch 2 times, most recently from 3b83e76 to 35a9bc9 Compare March 19, 2024 22:03
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for removing the websocket requirement, this scheme is much simpler!

Generally LGTM, just a few questions and comments.


/// Defines a trait around how headers are provided for each VSS request.
#[async_trait]
pub trait HeaderProvider {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually I'm no fan of using variable prefixes. However, in this case we'll want to expose this trait in other interfaces, e.g., in LDK Node (if we ever wanted to expose the trait there), where HeaderProvider would suggest that it's a general interface. Unfortunately, trait aliases are not supported by Rust yet, so we can't expose the trait under a different name downstream. Could we therefore rename it (and the corresponding error type) to VssHeaderProvider?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added prefixes.

pub trait HeaderProvider {
/// Returns the HTTP headers to be used for a VSS request.
/// This method is called on each request, and should likely perform some form of caching.
async fn get_headers(&self) -> Result<HeaderMap, HeaderProviderError>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have this return a core::collections::HashMap rather than the reqwest-specific HeaderMap?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think headers can be a multimap, changed this to Vec<(String, String)>.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I usually would prefer Vec<(String, String)>, too, however, in this case I'm afraid that UniFFI doesn't support tuples (see https://mozilla.github.io/uniffi-rs/udl/builtin_types.html), so using HashMap would be appreciated in this instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

url: String,
headers: HeaderMap,
client: reqwest::Client,
jwt_token: Mutex<Option<String>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we introduce a JwtToken type that holds both the token string and the expiry? This would avoid the possibility that both fields are updated independently. We could also hold an RwLock<JwtToken>, which would avoid us having to lock the Mutex just to check the expiry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

engine: Secp256k1<All>,
parent_key: ExtendedPrivKey,
url: String,
headers: HeaderMap,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this field be called default_headers to disambiguate it from the headers actually sent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

let children: Vec<ChildNumber> = (0..4)
.map(|i| u32::from_be_bytes(result[(i * 4)..((i + 1) * 4)].try_into().unwrap()))
.map(ChildNumber::from)
.collect::<Vec<_>>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to avoid this allocation by using DerivationPath::from_iter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Ok(lnurl.to_string())
}

#[derive(Deserialize)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could always derive Debug, possibly also Clone, as they might come handy eventually.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


fn parse_expiry(jwt_token: &str) -> Result<Option<u64>, HeaderProviderError> {
let parts: Vec<&str> = jwt_token.split('.').collect();
let invalid = || HeaderProviderError::InvalidData(format!("invalid JWT token: {}", jwt_token));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question regarding string sanitization as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to using UntrustedString.

#[derive(Debug)]
pub enum HeaderProviderError {
/// Invalid data was encountered.
InvalidData(String),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make these variants InvalidData { error: String }, etc?

(Unfortunately, the UniFFI bindings generator doesn't support tuple structs generally, so it would help us to expose the trait in LDK Node, if we'd ever want to go this way)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

src/lib.rs Outdated
@@ -25,3 +25,6 @@ pub mod util;

// Encryption-Decryption related crate-only helpers.
pub(crate) mod crypto;

/// A collection of (authentication-related) header providers.
pub mod headers;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, should this module be called auth or header_auth rather than just headers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be other use cases for headers, so I left it open ended. Can remove or reword the reference to authentication though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But for other use cases we might want to introduce another module, no? What would be a non-auth use case that would reuse exactly the types introduced here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the VssHeaderProvider trait and FixedHeaders type are both generic, they can be used to set various HTTP headers, maybe User-Agent for example?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, possibly, although I doubt this would ever happen without any additional refactoring becoming necessary. In any case, the module naming isn't that important, so feel free to leave as is if you prefer.

@wvanlint wvanlint force-pushed the lnurl_auth branch 3 times, most recently from 865e15a to 438a41b Compare March 21, 2024 00:12
@wvanlint wvanlint requested a review from tnull March 21, 2024 00:18
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks for addressing the feedback!

Basically LGTM from my side, mod the outstanding comments/nits (use HashMap in trait, improve UntrustedString, possibly rename module).

Probably @G8XSU wants to have a look also.

SystemTime::now().duration_since(SystemTime::UNIX_EPOCH).unwrap().as_secs() + EXPIRY_BUFFER_SECS
> expiry
})
.unwrap_or(false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be true? If we don't have a token/expiry set, wouldn't it mean we'd consider it expired?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have a token, it's possible it does not have an expiry (not a good idea, but that's up to the JWT signer). However, if the token is not present, we would need to fetch it. Moved the is_expired method to the JWT token to make these cases clearer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, alright.

if !self.is_expired() && !force_refresh {
let jwt_token = self.jwt_token.read().unwrap();
if let Some(jwt_token) = jwt_token.deref() {
return Ok(jwt_token.token_str.clone());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could make this a simple if/else rather than using an early-return pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@wvanlint wvanlint requested a review from tnull March 22, 2024 21:35
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from my side, I think.

@wvanlint
Copy link
Contributor Author

wvanlint commented Apr 2, 2024

@G8XSU Could you write down what you mentioned during the LDK dev sync for anything that needs to be addressed?

pub trait VssHeaderProvider {
/// Returns the HTTP headers to be used for a VSS request.
/// This method is called on each request, and should likely perform some form of caching.
async fn get_headers(&self) -> Result<HashMap<String, String>, VssHeaderProviderError>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo, we should also provide request as input to this trait.
This can enable adding headers for request-signing etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was adding this, but the trait does need to remain object-safe since we use dyn VssHeaderProvider, so we can't use generics here to provide the structured request. The binary serialization of the body can be provided though specifically for request signing. Shall I add that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes i think we can add serialized request, that should be enough for request signing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


/// Errors around providing headers for each VSS request.
#[derive(Debug)]
pub enum VssHeaderProviderError {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was planning to re-use the existing defined VssError here.

1 additional variant needs to be added i.e. VssError::AuthError (let me make this proto change)
apart from that we should re-use InvalidRequestError and InternalError.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! This will be a change in https://github.com/lightningdevkit/vss-server/blob/main/app/src/main/proto/vss.proto right? Would you like to make the change or shall I open a PR?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the AuthError and InternalError now.

impl LnurlAuthJwt {
/// Creates a new JWT provider based on LNURL Auth.
///
/// The LNURL Auth keys are derived based on the wallet seed according to LUD-05.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this mandatory?
Are we good if we derive the key from vss-seed/vss-secret?
This is because i don't expect this headerProvider to have access to wallet seed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think this is useful for portability following the LNURL Auth spec. I believe we can have access similar to here: https://github.com/lightningdevkit/ldk-node/blob/640a1fdb7833ad9c74ede0926f990d85ac1b3bca/src/builder.rs#L342 ?

We discussed this a little bit on Slack (https://cash.slack.com/archives/C03Q9S7K99R/p1707986857336199?thread_ts=1706544261.279069&cid=C03Q9S7K99R) whether to go with LUD-05 or LUD-13 (based on a node signature), but LUD-05 seemed more straight-forward.

Copy link
Collaborator

@G8XSU G8XSU May 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, this auth provider shouldn't have access to wallet seed, we can derive the parent key for auth and provide to auth-provider.

now that parent key can either be derived from wallet seed directly or from vss-secret.

i preferred the later because both data-encryption and request-signing keys would be derived from vss-secret but i see your point about benefit of deriving from wallet seed to follow spec. (but i thought the spec is just "derive a key by some path")

I am ok with both the options here depending on if ldk-node plans on using lnurl auth at other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we provide a helper for the parent key derivation to avoid user error? If so, the exposure might be similar? Overall, I felt like giving the wallet seed directly is less error prone and should be safe if the repo is well-reviewed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't just about this authorizer, imo this pattern of passing around wallet seed to code(potentially implemented by non-ldk-node) should be avoided, even if we maintain this repo directly.

we can just send the hardened derived parent key.

@tnull , any thoughts around seed derivation here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the deviation you mentioned the return of a JWT token from the LNURL Auth server?

Yes. And that generally we're implementing a protocol that is not specified anywhere but in code. This is fine, but it means we're currently rolling something pretty custom and if interoperability with other protocols should be ascertained in the future, I expect to be further changes necessary anyways?

The only reason to use the specification would be to get to a point where Lightning wallets can be portable between implementations with just a wallet seed, and have a well-defined derivation from the wallet seed to the linking key to access the protected data on the VSS server side. That might be out of scope right now though, and things will work either way for a single implementation. It just > might be difficult to migrate existing VSS data to a different linking key, other things like using JWT are easier to > change I think.

Right, but it's ambiguous as it is: I think we expect the seed to be the seed of the onchain wallet, but we also have the seed for the Lightning wallet, which may or may not be derived from the former (usually is, e.g., in LDK Node). Some nodes might not even have an onchain wallet readily available, so they'd need to use something different. All that is to say: if we want to define a standard, we should do exactly that: clearly define which derivation path is expected (e.g., '877' for 'VSS'?).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't expect ldk-node specific storage to be interoperable with other wallets.
This only needs to be interoperable within ldk-node, and for that deriving from any path of master should be fine.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so that we are on same page here,
Seed in this case will be seed_bytes derived from wallet master key and not the actual master seed bytes, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, added a comment there.

Looking at this again, I'm also down to just take in the parent key as an argument instead, and the user can decide how to derive it, following the spec with m/138' from https://github.com/lnurl/luds/blob/luds/05.md or something else.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I think we can just take parent key bytes as input, and recommend m/138' path from master_seed for lud5 compat. (we can do it in follow-up i think.)

url: String,
default_headers: HashMap<String, String>,
client: reqwest::Client,
jwt_token: RwLock<Option<JwtToken>>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be renamed to "cached_jwt_token"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

.derive_priv(engine, &linking_key_path)
.map_err(VssHeaderProviderError::from)?
.to_priv();
let public_key = private_key.public_key(engine);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we rename public_key -> linking_key and private_key ->linking_key_xpriv?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
let _ = URL_SAFE_NO_PAD.decode(parts[0]).map_err(|_| invalid())?;
let bytes = URL_SAFE_NO_PAD.decode(parts[1]).map_err(|_| invalid())?;
let _ = URL_SAFE_NO_PAD.decode(parts[2]).map_err(|_| invalid())?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to verify something here? will we get jwt signature ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, we can do more extensive validation (which might require additional dependencies), but I think it's okay to consider the JWT token as opaque as possible as validation will always occur at the consuming service. We also can't always verify the signature, e.g. the JWT token is signed by a symmetric key known by a joint LNURL Auth / VSS provider.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iiuc, we expect claim.subject==linkingKey, maybe we could just verify that in claim?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In most cases I believe. The JWT signer - VSS combination might choose some user identifier, so I don't think we should enforce that.

Cargo.toml Outdated
tungstenite = "0.21.0"
async-trait = "0.1.77"
futures = "0.3.30"
url = "2.5.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have tried to minimize dependencies in this crate,
it might make sense to feature-gate these additional deps to "lnurl-auth" ?

@wvanlint wvanlint requested a review from G8XSU April 25, 2024 23:06

// Sign k1 parameter with linking private key.
let hashing_private_key = hashing_key(engine, parent_key)?;
let linking_key_path = linking_key_path(&hashing_private_key, domain)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does hosted service provider map/link node's identity with linking_key for authorization purpose ?
will we expose some get method in ldk-node interface?

As backend would be using linking-key as user-identity, we have established authentication, but how does service provide correlate with their actual paid-user etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're currently aiming to keep authentication and authorization separate, using an API key is sufficient for our authorization purposes for now. We discussed this briefly offline here https://square.enterprise.slack.com/archives/C03Q9S7K99R/p1710885872464219?thread_ts=1706544261.279069&channel=C03Q9S7K99R&message_ts=1710885872.464219

We can verify a node id signature in the future or do something more complex like https://github.com/ZmnSCPxj-jr/lsptoken. The latter aims to reduce the conflict of interest when the VSS provider is also the LSP of the user. The LSP token implementation could prove that a VSS request is coming from a LSP user without the LSP being able to determine exactly which counterparty it is.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand for that usecase it is sufficient to have fixed header based api-key authorization,
I wanted to understand if other users might need access to linkingKey for user-identification for authorization purpose,
if yes, then ldk-node will need access to it and somehow might need to expose it in future?

(whether to expose linkingKey or not can be tackled separately and doesn't block this pr)

cc: @tnull

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree we can just leave it for now. If we ever find a use case that would want to reuse the linking key, we can reconsider if it's worth it. Possibly at that point it might even make sense to run a separate auth round, at least if there is no hard requirement to reuse exactly the same linking key.

@wvanlint
Copy link
Contributor Author

@G8XSU Split off the header provider trait in #31 as requested.

// The key of the LNURL key query parameter.
const KEY_QUERY_PARAM: &str = "key";
// The authorization header name.
const AUTHORIZATION: &str = "authorization";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'A' should be capital for 'Authorization' header

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I think headers are case-insensitive though.

impl LnurlAuthJwt {
/// Creates a new JWT provider based on LNURL Auth.
///
/// The LNURL Auth keys are derived based on the wallet seed according to LUD-05.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't just about this authorizer, imo this pattern of passing around wallet seed to code(potentially implemented by non-ldk-node) should be avoided, even if we maintain this repo directly.

we can just send the hardened derived parent key.

@tnull , any thoughts around seed derivation here?

}
let _ = URL_SAFE_NO_PAD.decode(parts[0]).map_err(|_| invalid())?;
let bytes = URL_SAFE_NO_PAD.decode(parts[1]).map_err(|_| invalid())?;
let _ = URL_SAFE_NO_PAD.decode(parts[2]).map_err(|_| invalid())?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iiuc, we expect claim.subject==linkingKey, maybe we could just verify that in claim?


// Sign k1 parameter with linking private key.
let hashing_private_key = hashing_key(engine, parent_key)?;
let linking_key_path = linking_key_path(&hashing_private_key, domain)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand for that usecase it is sufficient to have fixed header based api-key authorization,
I wanted to understand if other users might need access to linkingKey for user-identification for authorization purpose,
if yes, then ldk-node will need access to it and somehow might need to expose it in future?

(whether to expose linkingKey or not can be tackled separately and doesn't block this pr)

cc: @tnull

@wvanlint
Copy link
Contributor Author

Rebased after #31.

@wvanlint wvanlint requested review from G8XSU and tnull July 19, 2024 18:45
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good to me, just a few comments from my side.

Cargo.toml Outdated
[dependencies]
prost = "0.11.6"
reqwest = { version = "0.11.13", default-features = false, features = ["rustls-tls"] }
reqwest = { version = "0.11.13", default-features = false, features = ["rustls-tls", "json"] }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems setting the json feature here might only be necessary when lnurl-auth is enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made this feature conditional.

Cargo.toml Outdated
tokio = { version = "1", default-features = false, features = ["time"] }
rand = "0.8.5"
async-trait = "0.1.77"
bitcoin = { version = "0.30.2", optional = true }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you set default-features = false and enable only what you need for all of these new dependencies?

For bitcoin, we probably also want to enable std at least, and given that we're about to update in LDK, lets go directly with 0.32.2 here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


/// Provides a JWT token based on LNURL Auth.
/// The LNURL and JWT token are exchanged over a Websocket connection.
pub struct LnurlAuthJwt {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is an "LnurlAuthJwt"? Could we choose a name that is a little more self-explanatory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe LnurlAuthToJwtProvider?

pub fn new(
seed: &[u8], url: String, default_headers: HashMap<String, String>,
) -> Result<LnurlAuthJwt, VssHeaderProviderError> {
let engine = Secp256k1::new();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be Secp256k1::signing_only I believe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Cargo.toml Outdated
@@ -11,12 +11,20 @@ categories = ["web-programming::http-client", "cryptography::cryptocurrencies"]

build = "build.rs"

[features]
lnurl-auth = ["dep:bitcoin", "dep:url", "dep:base64", "dep:serde", "dep:serde_json"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a default feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

#[derive(Debug, Clone)]
struct JwtToken {
token_str: String,
expiry: Option<u64>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets call this expiry_secs or store the Duration? Or, given that we assume std anyways, can we make this a SystemTime?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stored as SystemTime.

engine.input(domain_name.as_bytes());
let result = Hmac::<sha256::Hash>::from_engine(engine).to_byte_array();
let children = (0..4)
.map(|i| u32::from_be_bytes(result[(i * 4)..((i + 1) * 4)].try_into().unwrap()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do something that is a bit more obviously safe and add an unwrap safety comment? E.g.:

	// unwrap safety: We take 4-byte chunks, so TryInto for [u8; 4] never fails.
	let children = result
		.chunks_exact(4)
		.take(4)
		.map(|i| u32::from_be_bytes(i.try_into().unwrap()))
		.map(ChildNumber::from);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


#[derive(Deserialize, Debug, Clone)]
struct ExpiryClaim {
exp: Option<u64>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you spell out the name of this variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. This is the deserialization specification though, so added a serde rename field attribute.

@wvanlint wvanlint requested a review from tnull July 29, 2024 22:57
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from my side.

Copy link
Collaborator

@G8XSU G8XSU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, please feel free to squash!

impl LnurlAuthJwt {
/// Creates a new JWT provider based on LNURL Auth.
///
/// The LNURL Auth keys are derived based on the wallet seed according to LUD-05.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so that we are on same page here,
Seed in this case will be seed_bytes derived from wallet master key and not the actual master seed bytes, right?

@wvanlint
Copy link
Contributor Author

wvanlint commented Aug 8, 2024

Squashed.

@G8XSU
Copy link
Collaborator

G8XSU commented Aug 9, 2024

Minor change in constructor to take in parent-key derived from master_seed instead of master_seed to be done in follow-up.

@G8XSU G8XSU merged commit c1f06c7 into lightningdevkit:main Aug 9, 2024
2 checks passed
@wvanlint wvanlint deleted the lnurl_auth branch August 13, 2024 00:00
@G8XSU G8XSU mentioned this pull request Aug 23, 2024
G8XSU added a commit to G8XSU/vss-rust-client that referenced this pull request Aug 23, 2024
Major Changes include:
* Signature change in vss-client constructor. (in lightningdevkit#31 )
* Vss-client can now also return AuthError if AuthException is returned from server. (lightningdevkit#30)
* Adds VssHeaderProvider, can be used for auth and request signing.(lightningdevkit#31)
* Adds LnurlAuthToJwtProvider, provides LnUrl based JWT auth. (lightningdevkit#26)
* Adds KeyObfuscator, to provide client-side key obfuscation. (lightningdevkit#32)
* Package now has enforced MSRV of 1.63.0. (lightningdevkit#19)

This is a minor version bump because there are non-backward compatible changes in vss-client usage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants