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.

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

Conversation

pappz
Copy link
Contributor

@pappz pappz commented Aug 4, 2025

Describe your changes

Shorter pub keys in logs:

2025-08-04T22:26:06Z INFO 285 [peer: 5ylZjz6] client/internal/peer/conn.go:478: created new wgProxy for relay connection: 127.0.0.1:1
2025-08-04T22:26:06Z INFO 64 [peer: 5ylZjz6] client/internal/peer/wg_watcher.go:88: WireGuard watcher started
2025-08-04T22:26:07Z INFO 285 [peer: 5ylZjz6] client/internal/peer/conn.go:509: start to communicate with peer via relay
2025-08-04T22:26:07Z INFO 453 [peer: 5ylZjz6] client/internal/peer/conn.go:349: set ICE to active connection
2025-08-04T22:26:07Z INFO 64 [peer: 5ylZjz6] client/internal/peer/wg_watcher.go:112: WireGuard watcher stopped
2025-08-04T22:26:09Z INFO 294 [peer: 5ylZjz6] client/internal/peer/guard/guard.go:74: start reconnect loop...

Issue ticket number and link

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)
  • Extended the README / documentation, if necessary

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

@Copilot Copilot AI review requested due to automatic review settings August 4, 2025 22:29
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a utility function to shorten peer public keys in connection logs for improved readability. The change aims to make log outputs more concise while maintaining sufficient information for debugging purposes.

  • Adds a new shortenKey function that truncates public keys to the first 7 characters
  • Updates connection logging to use the shortened key format instead of the full public key

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
client/internal/peer/key.go Introduces the shortenKey helper function
client/internal/peer/conn.go Updates connection logging to use shortened peer keys

@@ -0,0 +1,5 @@
package peer

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.

Comment on lines 3 to 4
func shortenKey(pubKey string) string {
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.

@@ -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.

@pappz pappz changed the title Shorten peer public key in connection logs [client] Shorten peer public key in connection logs Aug 4, 2025
Copy link

sonarqubecloud bot commented Aug 4, 2025

@@ -0,0 +1,8 @@
package peer

func shortenKey(pubKey string) string {
Copy link
Contributor

@nazarewk nazarewk Aug 5, 2025

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

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

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

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.

3 participants