wgcfg: new package #11
Conversation
All atomic access must be aligned to 64 bits, even on 32-bit platforms. Go promises that the start of allocated structs is aligned to 64 bits. So, place the atomically-accessed things first in the struct so that they benefit from that alignment. As a side bonus, it cleanly separates fields that are accessed by atomic ops, and those that should be accessed under mu. Also adds a test that will fail consistently on 32-bit platforms if the struct ever changes again to violate the rules. This is likely not needed because unaligned access crashes reliably, but this will reliably fail even if tests accidentally pass due to lucky alignment. Signed-Off-By: David Anderson <[email protected]>
Signed-off-by: Avery Pennarun <[email protected]>
The sticky socket code stays in the device package for now, as it reaches deeply into the peer list. This is the first step in an effort to split some code out of the very busy device package. Signed-off-by: David Crawshaw <[email protected]>
Signed-off-by: Brad Fitzpatrick <[email protected]>
Signed-off-by: Brad Fitzpatrick <[email protected]>
Based on types and config parser from wireguard-windows. Signed-off-by: David Crawshaw <[email protected]>
|
@danderson this is I think entirely self-contained so shouldn't need much review. |
Signed-off-by: Tyler Kropp <[email protected]>
Signed-off-by: Brad Fitzpatrick <[email protected]>
Signed-off-by: Brad Fitzpatrick <[email protected]>
| return key, nil | ||
| } | ||
|
|
||
| // SymmetricKey is a chacha20poly1305 key. |
There was a problem hiding this comment.
Note that this is not the case with wireguard's pre-shared keys, which are 32byte values that are hashed into noise's chainkey.
There was a problem hiding this comment.
Pushed a light comment fix: d127a16
| func (ip IP) Is6() bool { return !ip.Is4() } | ||
|
|
||
| // Is4 reports whether ip is an IPv4 address. | ||
| func (ip IP) Is4() bool { |
There was a problem hiding this comment.
Typically throughout WireGuard, we've used [4]byte for v4 and [16]byte for v6, considering the Go standard library's choice of v6-mapped-v4 to be a mistake. Especially since somebody might legitimately put a ::ffff:1.2.3.4 address through WireGuard and expect it to be v6 and not punned into v4.
| // it's not IPv4. | ||
| func (ip IP) To4() []byte { | ||
| if ip.Is4() { | ||
| return ip.Addr[12:16] |
| // IP is an IPv4 or an IPv6 address. | ||
| // | ||
| // Internally the address is always represented in its IPv6 form. | ||
| // IPv4 addresses use the IPv4-in-IPv6 syntax. |
| } | ||
|
|
||
| func (ip IP) MarshalText() ([]byte, error) { | ||
| return []byte(ip.String()), nil |
There was a problem hiding this comment.
Not super familiar here -- what's with the function name and signature? Can't we just have String() like usual? Is this for some weird reflection serialization?
| } | ||
|
|
||
| func IPv4(b0, b1, b2, b3 byte) (ip IP) { | ||
| ip.Addr[10], ip.Addr[11] = 0xff, 0xff // IPv4-in-IPv6 prefix |
| c := int8(r.Mask) | ||
| i := 0 | ||
| if r.IP.Is4() { | ||
| i = 12 |
There was a problem hiding this comment.
If you handle v4 properly, this becomes a single register-sized and/mask.
| return &net.IPNet{IP: r.IP.IP(), Mask: net.CIDRMask(int(r.Mask), bits)} | ||
| } | ||
|
|
||
| func (r CIDR) Contains(ip IP) bool { |
There was a problem hiding this comment.
Is this actually used anywhere in WireGuard?
Do you plan on using it inside of wireguard-windows or something?
| } | ||
|
|
||
| func (a *Key) LessThan(b *Key) bool { | ||
| for i := range a { |
There was a problem hiding this comment.
What is the purpose of this? Seems dangerous, given it's not constant time.
Key lookup structures should use hashing rather than binary searches.
| return subtle.ConstantTimeCompare(zeros[:], k[:]) == 1 | ||
| } | ||
|
|
||
| func (k *Key) MarshalJSON() ([]byte, error) { |
There was a problem hiding this comment.
Why json here? wireguard-go doesn't use json.
| "CON", "PRN", "AUX", "NUL", | ||
| "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9", | ||
| "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9", | ||
| } |
There was a problem hiding this comment.
Looks like you took this out of wireguard-windows, but does it really make sense here in the generic wireguard-go package?
| } | ||
|
|
||
| func FromWgQuick(s string, name string) (*Config, error) { | ||
| if !TunnelNameIsValid(name) { |
There was a problem hiding this comment.
wireguard-go doesn't parse wg-quick for anything. What's this for?
|
|
||
| // TODO(apenwarr): This is incompatibe with current Device.IpcSetOperation. | ||
| // It duplicates all the parser stuff in there, but is missing some | ||
| // keywords. Nothing useful seems to need it anymore. |
There was a problem hiding this comment.
If it's broken and nothing uses it, why is this included? Seems like the TODO can be addressed immediately.
Config semantics are important and this code seems pretty half-baked (there's a "TODO: this is broken, remove me" still in there). Reopening this. Maintaining a code base does not mean committing stuff in a "throw it over the fence" manner. It means having a real attention to detail and care, which takes focus before committing. I didn't want to have to comment on any of this and let you handle everything, but the quality here is questionable. Please fix this stuff up. |
zx2c4-bot
force-pushed
the
master
branch
3 times, most recently
from
6b5293b
to
6005c57
Compare
zx2c4-bot
force-pushed
the
master
branch
6 times, most recently
from
58beb0f
to
54dbe24
Compare
zx2c4-bot
force-pushed
the
master
branch
4 times, most recently
from
1ae3898
to
4e9e5da
Compare
zx2c4-bot
force-pushed
the
master
branch
4 times, most recently
from
eba36c5
to
ffb742d
Compare
zx2c4-bot
force-pushed
the
master
branch
5 times, most recently
from
89a9432
to
b9669b7
Compare
Update script & readme
No description provided.