Skip to content

[client] Shorten peer public key in connection logs #4283

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[client] Shorten peer public key in connection logs #4283

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f396277
Shorten peer public key in connection logs
pappz Aug 4, 2025
ffeacd5
Fix key shortening function to handle short public keys
pappz Aug 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion client/internal/peer/conn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ func NewConn(config ConnConfig, services ServiceDependencies) (*Conn, error) {
return nil, fmt.Errorf("allowed IPs is empty")
}

connLog := log.WithField("peer", config.Key)
connLog := log.WithField("peer", shortenKey(config.Key))

var conn = &Conn{
Log: connLog,
Expand Down
5 changes: 5 additions & 0 deletions client/internal/peer/key.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package peer

Copy link
Preview

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

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

The function lacks documentation explaining its purpose, parameters, and behavior. Add a comment describing that it returns the first 7 characters of a public key for logging purposes.

Suggested change
// shortenKey returns the first 7 characters of a public key for logging purposes.
// Parameters:
// - pubKey: the public key string to be shortened.
// Returns:
// - A string containing the first 7 characters of the provided public key.

Copilot uses AI. Check for mistakes.

func shortenKey(pubKey string) string {
Copy link
Preview

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

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

The function doesn't handle cases where the input string is shorter than 7 characters, which will cause a runtime panic. Add a length check: if len(pubKey) < 7 { return pubKey } before the slice operation.

Suggested change
func shortenKey(pubKey string) string {
func shortenKey(pubKey string) string {
if len(pubKey) < 7 {
return pubKey
}

Copilot uses AI. Check for mistakes.

Copy link
Contributor

@nazarewk nazarewk Aug 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I would like to make it wrapped (' / ") or prefixed (eg: pk:{pubKey} or pk({pubKey})), so it's possible to identify when processing logs.

As an example I currently have a general Base64 regex of length 44 that occasionally even matches file paths with specific lengths due to large scope of Base64 character set:

peersRe = regexp.MustCompile(`[a-zA-Z0-9+/=]{44}`)

This is not much of an issue for 44 characters that consist a WireGuard Public Key, but will very likely return too much false positives for just a 7 character key.

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 peerid: prefix is not good enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

for peer: Key it is ok-ish, but I won't be able to locate it in automation if somebody forgot about it and used it somewhere else later on

Copy link
Contributor

@nazarewk nazarewk Aug 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

let's at least make it clear in the function's documentation that this is meant to be wrapped if it were to be used anywhere else

Copy link
Contributor

@nazarewk nazarewk Aug 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

How about we shorten it the way I do my markdown notes? A few leading characters, some form of ellipsis and a few trailing characters:

2025-03-24T09:07:31+01:00 INFO [peer: IoCN.../HA=] client/internal/peer/handshaker.go:91: received connection confirmation, running version 0.39.1 and with remote WireGuard listen port 51820
2025-07-29T16:00:36-04:00 INFO [peer: a64b...jGM=] client/internal/peer/conn.go:509: start to communicate with peer via relay

would be both readable, easy to identify explicitly and search for

PS: I just put triple dots there, but we could use UTF ellipsis https://www.compart.com/en/unicode/U+2026

return pubKey[:7]
Copy link
Preview

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

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

The magic number 7 is hardcoded without explanation. Consider adding a named constant like const keyPrefixLength = 7 to make the intent clear and allow for easier maintenance.

Suggested change
func shortenKey(pubKey string) string {
return pubKey[:7]
const keyPrefixLength = 7
func shortenKey(pubKey string) string {
return pubKey[:keyPrefixLength]

Copilot uses AI. Check for mistakes.

}
Loading