Skip to content

Autopudate#162

Open
aojea wants to merge 2 commits into
google:mainfrom
aojea:autopudate
Open

Autopudate#162
aojea wants to merge 2 commits into
google:mainfrom
aojea:autopudate

Conversation

@aojea

@aojea aojea commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

security-medium medium

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.

Suggested change
hashStr := hex.EncodeToString(tokenHash[:]) + ":" + remotePeer.String() + ":" + reqCtx.Protocol + ":" + reqCtx.Target
hashStr := hex.EncodeToString(tokenHash[:]) + "\x00" + remotePeer.String() + "\x00" + reqCtx.Protocol + "\x00" + reqCtx.Target

Comment on lines +556 to +562
go func() {
handler := node.WithBiscuitAuth(func(s network.Stream, reqCtx RequestContext) {
done <- true
})
handler(serverStream)
close(done)
}()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

Suggested change
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)
}()

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.

1 participant