From c18d863a3e3f2a78ec501870ba75df7414e29c91 Mon Sep 17 00:00:00 2001 From: Arceliar Date: Fri, 26 Jan 2018 17:30:51 -0600 Subject: [PATCH] update comments, mostly TODO/FIXME notes --- src/yggdrasil/address.go | 2 +- src/yggdrasil/admin.go | 5 ++--- src/yggdrasil/core.go | 5 +---- src/yggdrasil/dht.go | 7 +------ src/yggdrasil/ndp.go | 2 +- src/yggdrasil/peer.go | 20 +++++++++++++++++--- src/yggdrasil/router.go | 23 ++++++++++++++--------- src/yggdrasil/search.go | 2 +- src/yggdrasil/session.go | 11 +++++------ src/yggdrasil/switch.go | 7 ++++++- src/yggdrasil/udp.go | 7 ++++--- src/yggdrasil/util.go | 4 ++-- src/yggdrasil/wire.go | 13 +++++++------ 13 files changed, 62 insertions(+), 46 deletions(-) diff --git a/src/yggdrasil/address.go b/src/yggdrasil/address.go index 2a492c7a..b5ac73d2 100644 --- a/src/yggdrasil/address.go +++ b/src/yggdrasil/address.go @@ -44,7 +44,7 @@ func address_addrForNodeID(nid *NodeID) *address { } if !done && bit == 0 { done = true - continue // FIXME this assumes that ones <= 127 + continue // FIXME? this assumes that ones <= 127, probably only worth changing by using a variable length uint64, but that would require changes to the addressing scheme, and I'm not sure ones > 127 is realistic } bits = (bits << 1) | bit nBits++ diff --git a/src/yggdrasil/admin.go b/src/yggdrasil/admin.go index 4b24193d..58840cf6 100644 --- a/src/yggdrasil/admin.go +++ b/src/yggdrasil/admin.go @@ -6,9 +6,8 @@ import "bytes" import "fmt" import "sort" -// TODO: Make all of this JSON +// TODO? Make all of this JSON // TODO: Add authentication -// TODO: Is any of this thread safe? type admin struct { core *Core @@ -138,7 +137,7 @@ func (a *admin) handleRequest(conn net.Conn) { if isIn { continue } - newInfo.name = "missing" + newInfo.name = "?" newInfo.key = key infos[key] = newInfo } diff --git a/src/yggdrasil/core.go b/src/yggdrasil/core.go index 08aa594c..be0c6aec 100644 --- a/src/yggdrasil/core.go +++ b/src/yggdrasil/core.go @@ -6,9 +6,6 @@ import "regexp" type Core struct { // This is the main data structure that holds everything else for a node - // TODO? move keys out of core and into something more appropriate - // e.g. box keys live in sessions - // sig keys live in peers or sigs (or wherever signing/validating logic is) boxPub boxPubKey boxPriv boxPrivKey sigPub sigPubKey @@ -42,7 +39,7 @@ func (c *Core) init(bpub *boxPubKey, // TODO separate init and start functions // Init sets up structs // Start launches goroutines that depend on structs being set up - // This is pretty much required to avoid race conditions + // This is pretty much required to completely avoid race conditions util_initByteStore() c.log = log.New(ioutil.Discard, "", 0) c.boxPub, c.boxPriv = *bpub, *bpriv diff --git a/src/yggdrasil/dht.go b/src/yggdrasil/dht.go index 16e5c892..1b8dcf71 100644 --- a/src/yggdrasil/dht.go +++ b/src/yggdrasil/dht.go @@ -18,10 +18,6 @@ Slight changes *do* make it blackhole hard, bootstrapping isn't an easy problem */ -// TODO handle the case where we try to look ourself up -// Ends up at bucket index NodeIDLen -// That's 1 too many - import "sort" import "time" @@ -34,7 +30,6 @@ const dht_lookup_size = 2 // This should be at least 1, below 2 is const dht_bucket_number = 8 * NodeIDLen // This shouldn't be changed type dhtInfo struct { - // TODO save their nodeID so we don't need to rehash if we need it again nodeID_hidden *NodeID key boxPubKey coords []byte @@ -115,7 +110,7 @@ func (t *dht) handleRes(res *dhtRes) { rinfo := dhtInfo{ key: res.key, coords: res.coords, - send: time.Now(), // Technically wrong but should be OK... FIXME or not + send: time.Now(), // Technically wrong but should be OK... recv: time.Now(), } // If they're already in the table, then keep the correct send time diff --git a/src/yggdrasil/ndp.go b/src/yggdrasil/ndp.go index f6b123a7..c56174b0 100644 --- a/src/yggdrasil/ndp.go +++ b/src/yggdrasil/ndp.go @@ -6,7 +6,7 @@ package yggdrasil import "golang.org/x/net/icmp" import "encoding/binary" -import "unsafe" +import "unsafe" // TODO investigate if this can be done without resorting to unsafe type macAddress [6]byte type ipv6Address [16]byte diff --git a/src/yggdrasil/peer.go b/src/yggdrasil/peer.go index 4c24e233..98f9cfb2 100644 --- a/src/yggdrasil/peer.go +++ b/src/yggdrasil/peer.go @@ -1,11 +1,26 @@ package yggdrasil // TODO cleanup, this file is kind of a mess +// Commented code should be removed +// Live code should be better commented -// FIXME? this part may be at least sligtly vulnerable to replay attacks +// FIXME (!) this part may be at least sligtly vulnerable to replay attacks // The switch message part should catch / drop old tstamps // So the damage is limited // But you could still mess up msgAnc / msgHops and break some things there +// It needs to ignore messages with a lower seq +// Probably best to start setting seq to a timestamp in that case... + +// FIXME (!?) if it takes too long to communicate all the msgHops, then things hit a horizon +// That could happen with a peer over a high-latency link, with many msgHops +// Possible workarounds: +// 1. Pre-emptively send all hops when one is requested, or after any change +// Maybe requires changing how the throttle works and msgHops are saved +// In case some arrive out of order or are dropped +// This is relatively easy to implement, but could be wasteful +// 2. Save your old locator, sigs, etc, so you can respond to older ancs +// And finish requesting an old anc before updating to a new one +// But that may lead to other issues if not done carefully... import "time" import "sync" @@ -166,7 +181,7 @@ func (p *peer) handleTraffic(packet []byte, pTypeLen int) { toPort, newTTL := p.core.switchTable.lookup(coords, ttl) if toPort == p.port { return - } // FIXME? shouldn't happen, does it? would loop + } to := p.core.peers.getPorts()[toPort] if to == nil { return @@ -200,7 +215,6 @@ func (p *peer) sendLinkPacket(packet []byte) { func (p *peer) handleLinkTraffic(bs []byte) { packet := wire_linkProtoTrafficPacket{} - // TODO throttle on returns? if !packet.decode(bs) { return } diff --git a/src/yggdrasil/router.go b/src/yggdrasil/router.go index b96815fe..b5b687f9 100644 --- a/src/yggdrasil/router.go +++ b/src/yggdrasil/router.go @@ -2,7 +2,7 @@ package yggdrasil // This part does most of the work to handle packets to/from yourself // It also manages crypto and dht info -// TODO? move dht stuff into another goroutine? +// TODO clean up old/unused code, maybe improve comments on whatever is left // Send: // Receive a packet from the tun @@ -43,14 +43,19 @@ func (r *router) init(core *Core) { r.addr = *address_addrForNodeID(&r.core.dht.nodeID) in := make(chan []byte, 1024) // TODO something better than this... p := r.core.peers.newPeer(&r.core.boxPub, &r.core.sigPub) //, out, in) - // TODO set in/out functions on the new peer... - p.out = func(packet []byte) { in <- packet } // FIXME in theory it blocks... + p.out = func(packet []byte) { + // This is to make very sure it never blocks + for { + select { + case in <- packet: + return + default: + util_putBytes(<-in) + } + } + } r.in = in - // TODO? make caller responsible for go-ing if it needs to not block - r.out = func(packet []byte) { p.handlePacket(packet, nil) } - // TODO attach these to the tun - // Maybe that's the core's job... - // It creates tun, creates the router, creates channels, sets them? + r.out = func(packet []byte) { p.handlePacket(packet, nil) } // The caller is responsible for go-ing if it needs to not block recv := make(chan []byte, 1024) send := make(chan []byte, 1024) r.recv = recv @@ -154,7 +159,7 @@ func (r *router) sendPacket(bs []byte) { } func (r *router) recvPacket(bs []byte, theirAddr *address, theirSubnet *subnet) { - // TODO? move this into the session? + // Note: called directly by the session worker, not the router goroutine //fmt.Println("Recv packet") if len(bs) < 24 { util_putBytes(bs) diff --git a/src/yggdrasil/search.go b/src/yggdrasil/search.go index cdba41b5..3dcb8d5b 100644 --- a/src/yggdrasil/search.go +++ b/src/yggdrasil/search.go @@ -169,7 +169,7 @@ func (s *searches) handleSearchRes(res *searchRes) { panic("This should never happen") } } - // FIXME replay attacks could mess with coords? + // FIXME (!) replay attacks could mess with coords? Give it a handle (tstamp)? sinfo.coords = res.coords sinfo.packet = info.packet s.core.sessions.ping(sinfo) diff --git a/src/yggdrasil/session.go b/src/yggdrasil/session.go index 913918f1..3fbf4c6a 100644 --- a/src/yggdrasil/session.go +++ b/src/yggdrasil/session.go @@ -29,7 +29,6 @@ type sessionInfo struct { tstamp int64 // tstamp from their last session ping, replay attack mitigation } -// FIXME replay attacks (include nonce or some sequence number) type sessionPing struct { sendPermPub boxPubKey // Sender's permanent key handle handle // Random number to ID session @@ -42,15 +41,15 @@ type sessionPing struct { // Returns true if the session was updated, false otherwise func (s *sessionInfo) update(p *sessionPing) bool { if !(p.tstamp > s.tstamp) { + // To protect against replay attacks return false } if p.sendPermPub != s.theirPermPub { + // Should only happen if two sessions got the same handle + // That shouldn't be allowed anyway, but if it happens then let one time out return false - } // Shouldn't happen + } if p.sendSesPub != s.theirSesPub { - // FIXME need to protect against replay attacks - // Put a sequence number or a timestamp or something in the pings? - // Or just return false, make the session time out? s.theirSesPub = p.sendSesPub s.theirHandle = p.handle s.sharedSesKey = *getSharedKey(&s.mySesPriv, &s.theirSesPub) @@ -144,7 +143,7 @@ func (ss *sessions) createSession(theirPermKey *boxPubKey) *sessionInfo { pub, priv := newBoxKeys() sinfo.mySesPub = *pub sinfo.mySesPriv = *priv - sinfo.myNonce = *newBoxNonce() // TODO make sure nonceIsOK tolerates this + sinfo.myNonce = *newBoxNonce() higher := false for idx := range ss.core.boxPub { if ss.core.boxPub[idx] > sinfo.theirPermPub[idx] { diff --git a/src/yggdrasil/switch.go b/src/yggdrasil/switch.go index 72cc96d2..04b86709 100644 --- a/src/yggdrasil/switch.go +++ b/src/yggdrasil/switch.go @@ -11,6 +11,11 @@ package yggdrasil // TODO? use a pre-computed lookup table (python version had this) // A little annoying to do with constant changes from bandwidth estimates +// FIXME (!) throttle how often root updates are accepted +// If the root starts spaming with new timestamps, it should only affect their neighbors +// The rest of the network should see announcements at a somewhat reasonable rate +// Maybe no faster than 2x the usual update interval + import "time" import "sync" import "sync/atomic" @@ -250,7 +255,7 @@ func (t *switchTable) cleanPeers() { } func (t *switchTable) cleanDropped() { - // TODO only call this after root changes, not periodically + // TODO? only call this after root changes, not periodically for root := range t.drop { if !firstIsBetter(&root, &t.data.locator.root) { delete(t.drop, root) diff --git a/src/yggdrasil/udp.go b/src/yggdrasil/udp.go index e8258e6d..d41593b6 100644 --- a/src/yggdrasil/udp.go +++ b/src/yggdrasil/udp.go @@ -5,10 +5,13 @@ package yggdrasil // It's intended to use UDP, so debugging/optimzing this is a high priority // TODO? use golang.org/x/net/ipv6.PacketConn's ReadBatch and WriteBatch? // To send all chunks of a message / recv all available chunks in one syscall +// That might be faster on supported platforms, but it needs investigation // Chunks are currently murged, but outgoing messages aren't chunked // This is just to support chunking in the future, if it's needed and debugged // Basically, right now we might send UDP packets that are too large +// TODO remove old/unused code and better document live code + import "net" import "time" import "sync" @@ -21,8 +24,6 @@ type udpInterface struct { conns map[connAddr]*connInfo } -//type connAddr string // TODO something more efficient, but still a valid map key - type connAddr struct { ip [16]byte port int @@ -153,7 +154,7 @@ func (iface *udpInterface) handleKeys(msg []byte, addr connAddr) { } iface.mutex.RLock() conn, isIn := iface.conns[addr] - iface.mutex.RUnlock() // TODO? keep the lock longer?... + iface.mutex.RUnlock() if !isIn { udpAddr := addr.toUDPAddr() themNodeID := getNodeID(&ks.box) diff --git a/src/yggdrasil/util.go b/src/yggdrasil/util.go index 46e513d2..f8826491 100644 --- a/src/yggdrasil/util.go +++ b/src/yggdrasil/util.go @@ -42,7 +42,7 @@ func util_unlockthread() { runtime.UnlockOSThread() } -/* +/* Used previously, but removed because casting to an interface{} allocates... var byteStore sync.Pool = sync.Pool{ New: func () interface{} { return []byte(nil) }, } @@ -52,7 +52,7 @@ func util_getBytes() []byte { } func util_putBytes(bs []byte) { - byteStore.Put(bs) // FIXME? The cast to interface{} allocates... + byteStore.Put(bs) // This is the part that allocates } */ diff --git a/src/yggdrasil/wire.go b/src/yggdrasil/wire.go index cd9e059f..7dbd7b8e 100644 --- a/src/yggdrasil/wire.go +++ b/src/yggdrasil/wire.go @@ -3,6 +3,8 @@ package yggdrasil // Wire formatting tools // These are all ugly and probably not very secure +// TODO clean up unused/commented code, and add better comments to whatever is left + // Packet types, as an Encode_uint64 at the start of each packet // TODO? make things still work after reordering (after things stabilize more?) // Type safety would also be nice, `type wire_type uint64`, rewrite as needed? @@ -10,9 +12,9 @@ const ( wire_Traffic = iota // data being routed somewhere, handle for crypto wire_ProtocolTraffic // protocol traffic, pub keys for crypto wire_LinkProtocolTraffic // link proto traffic, pub keys for crypto - wire_SwitchAnnounce // TODO put inside protocol traffic header - wire_SwitchHopRequest // TODO put inside protocol traffic header - wire_SwitchHop // TODO put inside protocol traffic header + wire_SwitchAnnounce // inside protocol traffic header + wire_SwitchHopRequest // inside protocol traffic header + wire_SwitchHop // inside protocol traffic header wire_SessionPing // inside protocol traffic header wire_SessionPong // inside protocol traffic header wire_DHTLookupRequest // inside protocol traffic header @@ -119,7 +121,6 @@ func wire_decode_coords(packet []byte) ([]byte, int) { } //////////////////////////////////////////////////////////////////////////////// -// TODO move this msg stuff somewhere else, use encode() and decode() methods // Announces that we can send parts of a Message with a particular seq type msgAnnounce struct { @@ -295,7 +296,7 @@ func wire_chop_uint64(toUInt64 *uint64, fromSlice *[]byte) bool { // Wire traffic packets type wire_trafficPacket struct { - ttl uint64 // TODO? hide this as a wire format detail, not set by user + ttl uint64 coords []byte handle handle nonce boxNonce @@ -336,7 +337,7 @@ func (p *wire_trafficPacket) decode(bs []byte) bool { } type wire_protoTrafficPacket struct { - ttl uint64 // TODO? hide this as a wire format detail, not set by user + ttl uint64 coords []byte toKey boxPubKey fromKey boxPubKey