enforce biscuit expiry and revocation in node relay - #334
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the authentication handshake logic in SamNode by extracting trusted public key retrieval into a helper method, utilizing identity.VerifyBiscuit for verification, and adding a check to reject revoked peers. Feedback highlights a critical issue with key rotation in performRouterAuthHandshake where only the first key is used to instantiate the authorizer, and notes that n.BiscuitTimeout is currently uninitialized, causing it to fall back to the default timeout.
performMutualAuth verified the remote biscuit against all trusted control plane keys but then built the role authorizer from trustedKeys[0]. A biscuit signed by a rotated key passed verification and then failed authorization, breaking router-to-router auth for the whole rotation window. Build the authorizer from the key that actually verified, via identity.VerifyBiscuitAndGetKey. Same bug class as the node-side fix in #334.
|
please rebase and squash, |
11cbd79 to
7814c96
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors biscuit verification in internal/node/node.go and its tests, introducing a helper method getTrustedPublicKeys to safely retrieve trusted public keys and utilizing the new identity.VerifyBiscuitAndGetKey function in performRouterAuthHandshake. The reviewer feedback suggests extending this optimization to HandleAuthHandshake and its corresponding test in node_test.go by using identity.VerifyBiscuitAndGetKey there as well, which would eliminate redundant loops over all trusted public keys when determining token expiration.
| } | ||
|
|
||
| b, expiry, err := n.verifyBiscuit(exchange.Biscuit, remotePeer) | ||
| b, err := identity.VerifyBiscuit(exchange.Biscuit, remotePeer, n.getTrustedPublicKeys(), n.BiscuitTimeout) |
There was a problem hiding this comment.
We can use identity.VerifyBiscuitAndGetKey instead of identity.VerifyBiscuit to retrieve the specific public key that successfully verified the biscuit. This allows us to avoid looping over all trusted public keys again when we determine the token's expiration later in this function. Note that this pattern is already correctly used in performRouterAuthHandshake.
| b, err := identity.VerifyBiscuit(exchange.Biscuit, remotePeer, n.getTrustedPublicKeys(), n.BiscuitTimeout) | |
| b, verifyingKey, err := identity.VerifyBiscuitAndGetKey(exchange.Biscuit, remotePeer, n.getTrustedPublicKeys(), n.BiscuitTimeout) |
7814c96 to
08ced9d
Compare
|
@aojea all sorted n squashed |
|
Needs rebase |
08ced9d to
23b76ff
Compare
2 participants
Resolves #333