Page MenuHomeVyOS Platform

Disallow duplicate pubkey on peers of a wireguard interface
Backport candidate, Requires assessmentPublicBUG

Description

It is possible to use a specific public key on multiple peers on the same interface:

set interfaces wireguard wg100 peer main  pubkey 'QApDr57t1SSvzg6Gn5KGwSlVXqxp7wHmgKlBc6ww0mg='
set interfaces wireguard wg100 peer ghost pubkey 'QApDr57t1SSvzg6Gn5KGwSlVXqxp7wHmgKlBc6ww0mg='

If you do so, this will disrupt the interface status and statistics reporting as well as potentially prevent one or more of the connected peers from communicating with the server (based on what is covered in "allowed ips" definition of peers), so lets report the statistics:

show interfaces wireguard wg100

the main and ghost peers may show similar statistics including for example "transfer" and "status" while it is inaccurate (outputs are prepared for illustration only, not actual output):

peer: main
  public key: QApDr57t1SSvzg6Gn5KGwSlVXqxp7wHmgKlBc6ww0mg=
  latest handshake: 0:33:12
  status: active
  endpoint: 4.3.2.1:54321
  allowed ips: 192.168.1.5/32
  transfer: 1 MB received, 14 MB sent


peer: ghost
  public key: QApDr57t1SSvzg6Gn5KGwSlVXqxp7wHmgKlBc6ww0mg=
  latest handshake: 9:26:24
  status: active
  endpoint: 1.2.4.4:12345
  allowed ips: 192.168.1.6/32
  transfer: 1 MB received, 14 MB sent

So it appears that some functions (including processing of access lists and reporting statistics) are based on looking up the pubkeys as the primary key. So it is desired that the commit would prevent or warn existence of duplicate pubkeys on the peers of each wireguard interface.

NOTE: The reported behavior has been observed in since v1.3

Details

Difficulty level
Unknown (require assessment)
Version
VyOS 1.4-rolling-202103251004
Why the issue appeared?
Will be filled on close
Is it a breaking change?
Unspecified (possibly destroys the router)
Issue type
Bug (incorrect behavior)

Event Timeline

Alfa80 created this object in space S1 VyOS Public.
Viacheslav changed the subtype of this task from "Task" to "Bug".Oct 27 2022, 10:32 AM
sdev changed the task status from Open to In progress.Oct 27 2022, 10:54 PM
sdev claimed this task.
sdev added a subscriber: sdev.
sdev changed the task status from In progress to Backport candidate.Nov 1 2022, 9:18 AM
sdev moved this task from Need Triage to Finished on the VyOS 1.4 Sagitta board.

This breaks a perfectly valid use case which I utilize regularly: using IPv4 + IPv6 peers with the same public key. Why would I want to create multiple keys for the exact same devices going over IPv4 and IPv6? If you want to include a warning, fine, but don't limit functionality based on someone's interpretation of how something will be used. I understand where this came from, but any time you limit functionality, you limit your users. As Donald Knuth once said:

Unix was not designed to stop you from doing stupid things, because that would also stop you from doing clever things.

(yes, I'm aware it's linux, but still)

@trae32566 My apologies for the inconveniences. You are right. The criteria for triggering this action shall be narrowed down further.
It would be necessary to issue the warning if and only if such colliding peers also specify the exact same remote endpoint addresses (with empty endpoints also being accounted as to be the same).
In other words, we need to identify incoming peers and apply the rule only to them, not the outgoing ones which already have specific remote endpoint addresses statically defined.

NOTE: See this slack thread for further discussions made about the issue