On 2023/08/13 08:46, Harald Dunkel wrote: > Hi Stuart, > > On 2023-08-11 11:39:24, Stuart Henderson wrote: > > On 2023/08/11 08:47, Harald Dunkel wrote: > > > > > > For forensic measures in case of an incident it is crucial to > > > have the peers public key. This string is constant over time > > > (unless it is not rotated for security). The first 16 or 10 > > > chars should do., e.g. > > > > > > % grep 3QUz9EgDY4 /var/log/messages > > > : > > > Aug 9 15:22:02 mygate /bsd: wg0: Sending handshake initiation to peer 17 > > > (3QUz9EgDY4) > > > Aug 9 15:22:07 mygate /bsd: wg0: Handshake for peer 17 (3QUz9EgDY4) did > > > not complete after 5 seconds, retrying (try 19) > > > > Is that just meant as an example, or do you have a diff? If you have a > > diff, please send it, because from a quick read it seems doing that is > > non-trivial (logging the peer description would be simpler, but whether > > it's pubkey or descr, I'm pretty sure it requires taking a lock to > > access this information, and that makes it a fairly complex change to > > review). > > > > This was just a mock, of course. I don't want to request complex changes, > just something better suitable than "peer 17".
While it may _seem_ simple, the peer number is easily available at the various points debug logging is done, but information like key (or description) is not and must be looked up. The code doing lookup needs to ensure that it isn't being changed concurrently (wg packet processing for one peer is often likely to run on one cpu while things change on another - for example if a peer is removed from configuration). Maybe it's possible to copy the pubkey (or a truncation thereof) somewhere more accessible (I guess it might be ok to access from the softc, I don't think the key can change once a peer is created), but there may be a good reason why that wasn't already done. (this is beyond what I'm comfortable with in C). > I have seen the public key > as some kind of not modifiable identifier for a VPN connection. Adding > the description to the log file wouldn't help, since there is just a > single description for a wireguard connection with multiple peers (if > I understood ifconfig(8) correctly). I mean the per-peer description not the interface description. (It's easy to add a naive implementation to log that, but such a naive implementation will break in some situations, perhaps badly - crashes or worse). The log messages you're talking about are meant for debug not normal operation, and there are a lot of them. If locks _are_ needed then reasoning about lock behaviour for each of them would make it complex to implement and review changes for them - and of course adding more locks will make things slower. IIUC the whole protocol is designed so that e.g. peer public key lookups like this don't need to be done very often, and that locks can be kept to a minimum. So no surprise it's using a simple approach to logging. > I am talking about the setup for a road-warrior VPN gateway. The road- > warriors are supposed to have their own IP address, but they use the > same subnet and routing. (offtopic but i really hate that "road warrior" term. yeuch yeuch yeuch!). > > It would be much easier to log the public key and peer number when the > > peer is created, but then you'll need to keep more logs. > > > > Some peers are connected 24/7 for weeks. disk is cheap :)
