feat(transport): general TLS ClientHello fragmentation StreamDialer

[jyyi1]

We introduce a new StreamDialer designed for TLS ClientHello fragmentation, as discussed in Circumventing the GFW with TLS Record Fragmentation. This general-purpose dialer allows for implementing custom splitting logic, enabling diverse fragmentation strategies.

Here is a screenshot that demonstrates the impact of using this StreamDialer to always split at the first 5 bytes of the Client Hello packet:

[jyyi1]

This is to resolve the CodeQL error: https://github.com/Jigsaw-Code/outline-sdk/pull/133/checks?check_run_id=18912663629

(Although the standard library AppendUint16 is using the same code to append the data)

[fortuna]

Why aren't you returning on error?

[jyyi1]

Because according to the doc https://pkg.go.dev/io#Reader:

Callers should always process the n > 0 bytes returned before considering the error err.

In our case, "processing" means determine whether we have read a full header or a full record. After that, we will jump out of the loop when err != nil (see the condition in for loop).

[fortuna]

I find the padding and the conflation of the state before and after the header a bit hard to understand.

Consider separating the states and getting rid of the padding variable. For example, this can handle the state before we have the header more clearly:

for {
  // Still waiting to finish the header
  if len(b.data) < recordHeaderLen {
    // Makes it clear we are reading up to recordHeaderLen. No padding
    m, err := r.Read(b.data[len(b.data):recordHeaderLen])
    b.data = b.data[:len(b.data)+m]
    n += int64(m)
    if err != nil {
      return n, err  // could return unexpected EOF if EOF.
    }
    if len(b.data) == recordHeaderLen {
     hdr, err := validateTLSRecord(b.data)
     if err != nil {
       b.validationStatus = err
       return err       
     }
     buf := make([]byte, 0, recordHeaderLen+hdr.PayloadLen())
     b.data = append(buf, b.data...)
    }
    continue
  }
  // State after we have the header
  if !b.validationStatus {
    return 0, b.validationStatus
  }
  ...
}

[jyyi1]

Refactored it into 3 stages: appending header; validation; appending payload.

[fortuna]

Why don't you write to the base writer here?

[jyyi1]

A single Write might fail, so you still need to track the remaining bytes somewhere in w, and you need to Write them to w.base again in the following w.Writes or w.ReadFroms issued by the caller.

Since the Write to w.base logic will spread across different functions (i.e. w.Write, w.ReadFrom, after split, after invalid w.helloBuf without split), I think it makes more sense to separate the "flush bytes to base" logic from the split, and have a dedicated helper doing that.

[fortuna]

Why don't you just write to the base writer here?

[jyyi1]

Same as above. ☝️