feat(gateway): add JWT authentication #6535

f3r10 · 2024-12-10T19:12:19Z

Motivation

Add support for JWT gateway authentication

This PR adds one additional endpoint:

/auth/session if the password is correct, this endpoint returns a JWT token that must be used in the other endpoints that require authentication.

How to test it using the CLI:

gateway-cli --address=http://127.0.0.1:16265/v1 --rpcpassword=theresnosecondbest get-session-jwt-auth

gateway-cli --address=http://127.0.0.1:16265/v1 --rpcjwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpZCI6IjAzY2I1MTBjOWJiMzA5ODNkOTYzODk3ZWUyNjlkM2FiZmY3Nzg0YTVjMmVkYWM3YjQzMzg1NjIyMmMzZGIxYWQ1ZSIsImV4cCI6MTczMzg1ODUxN30.4FNxyn6Y-M_vfPRstxgrv4weK9MMRv31Inijw6JJo80 get-balances

elsirion

Thx for the PR, logic looks good, just code organization could be improved. Generally I'd recommend to think about how arguments to functions can be reduced, that way you typically get a good feeling for where a function should live (e.g. verification belongs into AuthManager because it has access to the secret and that's all you need to verify).

elsirion · 2024-12-13T11:18:27Z

gateway/cli/src/main.rs

+            cli.rpcpassword.clone(),
+            cli.rpcjwt.clone(),


~~Now needing both is unfortunate, ideally the password would just be an implementation detail of some function/struct providing fetching the session token. What's the plan to evolve this?~~

I see, both are optional.

elsirion · 2024-12-13T11:21:23Z

gateway/ln-gateway/src/lib.rs

+        let gateway_id = Self::load_or_create_gateway_id(&gateway_db).await;
+
+        // for JWT it is necessary to create an encoding secret that will be used each
+        // time a new JWT token is generated. The encoding secret ensures token's
+        // integrity and authenticity, so it is necessary to use a secure random
+        // number generator to generate strong keys.
+        let encoding_secret: [u8; 16] = rand::thread_rng().gen();


The secret being genearted on the fly means a GW restart invalidates old tokens I assume? Not a problem imo, just want to point it out so everyone is aware.

Yes, you are right. Where should I put a note about that? Would it be okay as an additional comment, or should I create an additional document for that?

elsirion · 2024-12-13T11:24:29Z

gateway/ln-gateway/src/rpc/rpc_client.rs

 }

 impl GatewayRpcClient {
-    pub fn new(versioned_api: SafeUrl, password: Option<String>) -> Self {
+    pub fn new(versioned_api: SafeUrl, password: Option<String>, jwt_code: Option<String>) -> Self {


What are the semantics of providing a password, a JWT or both? Please document. E.g. does providing the password lead to automatic requesting of a JWT whenever the previous expires?

Providing both a password and a JWT does not make sense.
Providing an initial password has the purpose of generating a JWT that has to be used to request private endpoints.

Maybe instead of providing optional parameters (JWT and password) would be better to provide an enum for each case, WDYT?

elsirion · 2024-12-13T11:30:24Z

gateway/ln-gateway/src/rpc/rpc_server.rs

+        }
+        return Ok(next.run(request).await);
+    }
+    //TODO remove this fallback when all the callers support JWT auth


Should create an issue once merged

yes, @elsirion it would be better to create additional issues in this repository, and also I think it would be necessary to create an issue in the ui repository.

elsirion · 2024-12-13T11:31:36Z

gateway/ln-gateway/src/rpc/rpc_server.rs

+    let auth_manager = gateway.auth_manager.lock().await;
+    if let Ok(decoded_token_data) = jsonwebtoken::decode::<crate::auth_manager::Session>(
+        &token,
+        &jsonwebtoken::DecodingKey::from_secret(auth_manager.encoding_secret.as_ref()),
+        &jsonwebtoken::Validation::default(),
+    ) {
+        let now = fedimint_core::time::now()
+            .duration_since(UNIX_EPOCH)
+            .expect("Time went backwards")
+            .as_secs();
+        if now > decoded_token_data.claims.exp {
+            return Err(StatusCode::UNAUTHORIZED);
+        }


This could be a separate function for checking the JWT, would make the code cleaner.

EDIT: should live inside AuthManager after reading the full diff

elsirion · 2024-12-13T11:38:49Z

gateway/ln-gateway/src/lib.rs

@@ -197,6 +199,9 @@ pub struct Gateway {
    /// The gateway's federation manager.
    federation_manager: Arc<RwLock<FederationManager>>,

+    /// The gateway's authentication manager
+    auth_manager: Arc<Mutex<AuthManager>>,


Why does the auth manager need to be mutated at all? Can we remove the Acr<Mutex<_>>?

elsirion · 2024-12-13T11:40:11Z

gateway/ln-gateway/src/rpc/rpc_server.rs

+        let session = auth_manager
+            .generate_session()
+            .map_err(|_| AdminGatewayError::Unauthorized)?;
+        let token = session
+            .encode_jwt(&auth_manager.encoding_secret)
+            .map_err(|_| AdminGatewayError::Unauthorized)?;


Should be encapsulated as one function in the AuthManager. There's no reason to generate a session without signing it imo. Just call the fn something that makes it clear that the session tokens are signed.

elsirion · 2024-12-13T11:41:55Z

gateway/ln-gateway/src/auth_manager.rs

+pub struct Session {
+    /// the unique identifier of the session.
+    pub id: PublicKey,
+    /// the expire time of the session
+    pub exp: u64,
+}


Member fields can be private afaik. Only the auth manager should access them to either create a session or check it.

elsirion · 2024-12-13T11:42:11Z

gateway/ln-gateway/src/auth_manager.rs

+    pub fn new(id: PublicKey, expiry: u64) -> Self {
+        let now = fedimint_core::time::now()
+            .duration_since(UNIX_EPOCH)
+            .expect("Time went backwards")
+            .as_secs();
+        Self {
+            id,
+            exp: now + expiry,
+        }
+    }


This could become private too

elsirion · 2024-12-13T11:42:48Z

gateway/ln-gateway/src/auth_manager.rs

+    pub fn encode_jwt(self, encoding_secret: &[u8; 16]) -> anyhow::Result<String> {
+        let claim = self;
+        encode(
+            &Header::default(),
+            &claim,
+            &EncodingKey::from_secret(encoding_secret),
+        )
+        .map_err(|_| anyhow::anyhow!("Unable to generate jwt token session"))
+    }


Can be moved to AuthManager and used inside generate_and_sign_session

f3r10 requested review from a team as code owners December 10, 2024 19:12

f3r10 mentioned this pull request Dec 10, 2024

feat(gateway): Add admin session authentication manager #6147

Closed

elsirion reviewed Dec 13, 2024

View reviewed changes

f3r10 added 2 commits December 18, 2024 14:25

feat(gateway): add JWT authentication

17d2928

chore(gateway): code organization tweaks

34a4808

f3r10 force-pushed the add_jwt_auth_method branch from acdfd59 to 34a4808 Compare December 18, 2024 19:27

f3r10 requested a review from elsirion December 18, 2024 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gateway): add JWT authentication #6535

feat(gateway): add JWT authentication #6535

f3r10 commented Dec 10, 2024

elsirion left a comment

elsirion Dec 13, 2024

elsirion Dec 13, 2024

f3r10 Dec 18, 2024

elsirion Dec 13, 2024

f3r10 Dec 18, 2024

elsirion Dec 13, 2024

f3r10 Dec 18, 2024

elsirion Dec 13, 2024

elsirion Dec 13, 2024

elsirion Dec 13, 2024

elsirion Dec 13, 2024

elsirion Dec 13, 2024

elsirion Dec 13, 2024

feat(gateway): add JWT authentication #6535

Are you sure you want to change the base?

feat(gateway): add JWT authentication #6535

Conversation

f3r10 commented Dec 10, 2024

Motivation

This PR adds one additional endpoint:

How to test it using the CLI:

elsirion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment