enbility / ship-go / 33387789344

93%

Build Type

push

github

Committed by web-flow

Commit Message

fix(ship): resolve double connections by SKI and recency (TC_SHIP_CONN_001) (#106)

# fix(ship): resolve double connections by SKI and recency
(TC_SHIP_CONN_001)

Fixes #96.

## The problem

SHIP 12.2.2 says the node with the bigger 160-bit SKI keeps the **most
recent**
connection to a peer and closes all others. `keepThisConnection`
implemented something
else — keep whichever connection the higher-SKI node *initiated* — and
said so in its own
comment:

```go
// This is hard to implement without any flaws. Therefore I chose a
// different approach: The connection initiated by the higher SKI will be kept
```

That rule is symmetric, so it always converged on one connection, but it
is not the
spec's rule, and TC_SHIP_CONN_001 fails on it. With the DUT holding the
larger SKI the
test tool opens connection B as soon as it sees the TCP stream of the
DUT's connection A;
the DUT is required to keep B and close A, and instead keeps A and tears
down B after B
has completed a handshake.

Getting the criterion right is necessary but not sufficient. The old
check ran after the
TLS handshake *and* the websocket upgrade, while the test expects A
closed "within 30s
(latest before the TLS handshake of connection B completes)" — and at
that point A is not
even a connection yet, it is an in-flight `dialer.Dial` with no handle
to cancel.

## What changed

**The criterion.** `keepThisConnection` is replaced by
`resolveDoubleConnection`. The
direction a connection was opened in is no longer an input — that
parameter *was* the bug.

**The decision moved into the TLS handshake.** `verifyPeerCertificate`
now performs the
arbiter check, which is what SHIP 12.2.2 asks for: *"The SHIP node with
the greater SKI
SHOULD check for double connections directly during the TLS handshake."*
It never rejects
the incoming connection; it only retires the older one.

**The outgoing dial became cancellable.** `connectionsInitiating`
carries the raw socket
(`dialState`), so that check can abo... (continued)

Coverage Stats

154 of 169 new or added lines in 7 files covered. (91.12%)

5 existing lines in 4 files now uncovered.

6022 of 6471 relevant lines covered (93.06%)

15184.64 hits per line