Autopudate#162
Conversation
There was a problem hiding this comment.
Code Review
This pull request removes unused imports and the fallback re-enrollment logic from the middleware, and updates the verification cache key to include the protocol and target to prevent authorization cache bypasses. It also adds a unit test to verify this fix. The review feedback suggests using a null byte (\x00) instead of a colon (:) as a delimiter in the cache key to prevent potential collision attacks, and closing the pipe writer in the test goroutine to prevent potential test hangs.
| // Check verification cache | ||
| tokenHash := sha256.Sum256(authFrame.Biscuit) | ||
| hashStr := hex.EncodeToString(tokenHash[:]) + ":" + remotePeer.String() | ||
| hashStr := hex.EncodeToString(tokenHash[:]) + ":" + remotePeer.String() + ":" + reqCtx.Protocol + ":" + reqCtx.Target |
There was a problem hiding this comment.
Using a colon (:) as a delimiter for concatenating multiple fields in the cache key can lead to potential cache key collisions if any of the fields (such as reqCtx.Protocol or reqCtx.Target) contain colons. Since reqCtx.Target is derived from the user-controlled authFrame.TargetService, a malicious client could craft a target service name containing colons to collide with another cached authorization entry.
To prevent delimiter collision attacks, use a null byte (\x00) as the separator, as it cannot legitimately appear in standard protocol names or target services.
| hashStr := hex.EncodeToString(tokenHash[:]) + ":" + remotePeer.String() + ":" + reqCtx.Protocol + ":" + reqCtx.Target | |
| hashStr := hex.EncodeToString(tokenHash[:]) + "\x00" + remotePeer.String() + "\x00" + reqCtx.Protocol + "\x00" + reqCtx.Target |
| go func() { | ||
| handler := node.WithBiscuitAuth(func(s network.Stream, reqCtx RequestContext) { | ||
| done <- true | ||
| }) | ||
| handler(serverStream) | ||
| close(done) | ||
| }() |
There was a problem hiding this comment.
If the WithBiscuitAuth handler returns early or fails before writing a response to the stream, the test will hang indefinitely at reader.ReadMsg() because the write end of the pipe (pw2) is never closed.
To prevent test hangs and ensure fast failures in CI/CD, close pw2 when the goroutine executing the handler exits. This can be done cleanly using defer pw2.Close() at the start of the goroutine.
| go func() { | |
| handler := node.WithBiscuitAuth(func(s network.Stream, reqCtx RequestContext) { | |
| done <- true | |
| }) | |
| handler(serverStream) | |
| close(done) | |
| }() | |
| go func() { | |
| defer pw2.Close() | |
| handler := node.WithBiscuitAuth(func(s network.Stream, reqCtx RequestContext) { | |
| done <- true | |
| }) | |
| handler(serverStream) | |
| close(done) | |
| }() |
No description provided.