Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(transport): implement TLS record fragmentation #114

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions transport/tls-record-frag/stream_dialer.go
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call the package tlssplit, so it can be more easily used. Or perhaps tlsfrag is better.
@jyyi1 thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like tlsfrag. And for the stream dialer's name, I prefer tlsClientHelloFragStreamDialer to indicate that it will only be applied to client hello packets.

Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Copyright 2023 Jigsaw Operations LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package tlsrecordfrag
jyyi1 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
package tlsrecordfrag
package tlsfrag

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the directory match the package name.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tlssplit or tlsfrag?


import (
"context"
"errors"

"github.com/Jigsaw-Code/outline-sdk/transport"
)

type tlsRecordFragDialer struct {
dialer transport.StreamDialer
splitPoint int32
}

var _ transport.StreamDialer = (*tlsRecordFragDialer)(nil)

// NewStreamDialer creates a [transport.StreamDialer] that splits the Client Hello Message
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to clarify the behavior.

Suggested change
// NewStreamDialer creates a [transport.StreamDialer] that splits the Client Hello Message
// NewStreamDialer creates a [transport.StreamDialer] that splits the first TLS record if it's a handshake message. If the record has length N, the new records will have length prefixBytes and N - prefixBytes, plus their headers.

@jyyi1 let's make sure we agree on this behavior.

func NewStreamDialer(dialer transport.StreamDialer, prefixBytes int32) (transport.StreamDialer, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to put the "fixed split point" into the function name. Because we might need to introduce other kinds of fragmentation dialers in the future, like DomainNameSplitDialer, or MultipleSegmentsSplitDialer.

Suggested change
func NewStreamDialer(dialer transport.StreamDialer, prefixBytes int32) (transport.StreamDialer, error) {
func NewStreamDialerWithFixedSplitPoint(dialer transport.StreamDialer, prefixBytes int32) (transport.StreamDialer, error) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's too long. Perhaps NewFixedPrefixStreamDialer

But I doubt we will need anything more complex anytime soon, so NewStreamDialer may be ok.

if dialer == nil {
return nil, errors.New("argument dialer must not be nil")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return nil, errors.New("argument dialer must not be nil")
return nil, errors.New("an inner dialer is required")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original code is consistent with the split Dialer:

return nil, errors.New("argument dialer must not be nil")

I think we can keep it as is for now, and change them all later.

}
return &tlsRecordFragDialer{dialer: dialer, splitPoint: prefixBytes}, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check prefixBytes as well?

Suggested change
return &tlsRecordFragDialer{dialer: dialer, splitPoint: prefixBytes}, nil
if prefixBytes <= 0 {
return nil, errors.New("prefixBytes must be positive")
}
return &tlsRecordFragDialer{dialer: dialer, splitPoint: prefixBytes}, nil

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code is consistent with split, and that's helpful for the user.
Also, returning an error makes the use harder. We can just assume <=0 means disabled (perhaps document that).

Lanius-collaris marked this conversation as resolved.
Show resolved Hide resolved
}

// Dial implements [transport.StreamDialer].Dial.
func (d *tlsRecordFragDialer) Dial(ctx context.Context, remoteAddr string) (transport.StreamConn, error) {
innerConn, err := d.dialer.Dial(ctx, remoteAddr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to inspect remoteAddr, and make sure that we are only splitting TLS traffic (not regular TCP traffic). Here are some common ports serving TLS traffic (feel free to add more):

  • 443, HTTPS
  • 990, FTPS
  • 563, NNTPS
  • 636, LDAPS
  • 993, IMAPS
  • 995, POP3S

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. We should be able to restrict to specific ports.
But I would like the user to be able to specify that list.

@jyyi1 what are your thoughts on the questions below?

How to specify the ports?
Perhaps we pass a function that takes a host and port, and outputs a binary decision.
It could be passed in the New function with variadic options, like we do here:
https://github.com/Jigsaw-Code/outline-sdk/blob/375a66df04ef48659c73f0a7ed1156b04b2008a4/network/packet_listener_proxy.go#L58C68-L58C75

Or as a setter.

What's the default behavior?
I think the default should be either all or nothing. But perhaps 443, since that's the most common scenario.

Or perhaps we can force the user to make a decision by making the function mandatory in the constructor. That's probably my favorite. We can provide constant functions for "all" or "443 only". Also a EnableForPorts(portList) that returns a function that enables for that list. That way someone can do:

dialer, err := tlsfrag.NewStreamDialer(inner, 10, tlsfrag.EnablePorts(int[]{443}))

or

dialer, err := tlsfrag.NewStreamDialer(inner, 10, tlsfrag.Enable443)

Reference
Please link to https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xhtml?search=TLS

if err != nil {
return nil, err
}
return transport.WrapConn(innerConn, innerConn, NewWriter(innerConn, d.splitPoint)), nil
}
132 changes: 132 additions & 0 deletions transport/tls-record-frag/writer.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
// Copyright 2023 The Outline Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package tlsrecordfrag

import (
"encoding/binary"
"io"
)

type tlsRecordFragWriter struct {
writer io.Writer
prefixBytes int32
}

const maxRecordLength = 16384 //For the fragments, not for the reassembled record
Lanius-collaris marked this conversation as resolved.
Show resolved Hide resolved

func NewWriter(writer io.Writer, prefixBytes int32) *tlsRecordFragWriter {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether we want to expose this, maybe make it private? newWriter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should provide the building blocks, so users can create their own dialers.
Tying to a dialer is a big drawback, that's why we expose the writers for split and Shadowsocks.
With the writer, you can easily write a Dialer, but not the other way around

return &tlsRecordFragWriter{writer, prefixBytes}
}

func (w *tlsRecordFragWriter) dontFrag(first []byte, source io.Reader) (written int64, err error) {
tmp, err := w.writer.Write(first)
written = int64(tmp)
w.prefixBytes = 0
if err != nil {
return written, err
}
n, err := io.Copy(w.writer, source)
written += n
return written, err
}

func (w *tlsRecordFragWriter) ReadFrom(source io.Reader) (written int64, err error) {
if 0 < w.prefixBytes {
var recordHeader [5]byte
_, err := io.ReadFull(source, recordHeader[:])
if err != nil {
return 0, err
}
recordLength := int32(binary.BigEndian.Uint16(recordHeader[3:]))
if w.prefixBytes >= recordLength {
Lanius-collaris marked this conversation as resolved.
Show resolved Hide resolved
return w.dontFrag(recordHeader[:], source)
}
if recordLength > maxRecordLength {
return w.dontFrag(recordHeader[:], source)
}
// Allocate buffer that fits the entire record after the split (2*header + payload).
buf := make([]byte, recordLength+10)
Lanius-collaris marked this conversation as resolved.
Show resolved Hide resolved
header := recordHeader[:3]

copy(buf, header)
binary.BigEndian.PutUint16(buf[3:], uint16(w.prefixBytes))
n2, err := io.ReadFull(source, buf[5:5+w.prefixBytes])
if err != nil {
w.prefixBytes = 0
return 0, err
}

copy(buf[5+n2:], header)
binary.BigEndian.PutUint16(buf[5+n2+3:], uint16(recordLength-w.prefixBytes))
_, err = io.ReadFull(source, buf[10+w.prefixBytes:])
if err != nil {
w.prefixBytes = 0
return 0, err
}

tmp, err := w.writer.Write(buf)
Lanius-collaris marked this conversation as resolved.
Show resolved Hide resolved
w.prefixBytes = 0
written = int64(tmp)
if err != nil {
return written, err
}
}
n, err := io.Copy(w.writer, source)
written += n
return written, err
}

func (w *tlsRecordFragWriter) Write(data []byte) (written int, err error) {
if 0 < w.prefixBytes {
Lanius-collaris marked this conversation as resolved.
Show resolved Hide resolved
length := int32(len(data))
if w.prefixBytes+5 >= length {
w.prefixBytes = 0
return w.writer.Write(data)
}

recordLength := int32(binary.BigEndian.Uint16(data[3:]))
remainder := data[5:]
remainderLength := length - 5
hasPartialRecord := recordLength > remainderLength
hasMultipleRecords := recordLength < remainderLength
isRecordOverflow := recordLength > maxRecordLength

if hasPartialRecord || w.prefixBytes == recordLength || isRecordOverflow {
w.prefixBytes = 0
return w.writer.Write(data)
}
//Need fragmentation, allocate data + header*1
buf := make([]byte, length+5)
header := data[:3]
record1 := remainder[:w.prefixBytes]
record2 := remainder[w.prefixBytes:recordLength]

copy(buf, header)
binary.BigEndian.PutUint16(buf[3:], uint16(w.prefixBytes))
copy(buf[5:], record1)

copy(buf[5+w.prefixBytes:], header)
binary.BigEndian.PutUint16(buf[5+3+w.prefixBytes:], uint16(len(record2)))
copy(buf[5+5+w.prefixBytes:], record2)

if hasMultipleRecords {
copy(buf[5+5+recordLength:], remainder[recordLength:])
}

w.prefixBytes = 0
return w.writer.Write(buf)
Lanius-collaris marked this conversation as resolved.
Show resolved Hide resolved
}
return w.writer.Write(data)
}
87 changes: 87 additions & 0 deletions transport/tls-record-frag/writer_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
// Copyright 2023 Jigsaw Operations LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package tlsrecordfrag

import (
"bytes"
"io"
"testing"

"encoding/binary"
"github.com/stretchr/testify/require"
)

type collectWrites struct {
writes [][]byte
}

var _ io.Writer = (*collectWrites)(nil)

var header = []byte{0x16, 0x03, 0x01}

func (w *collectWrites) Write(data []byte) (int, error) {
dataCopy := make([]byte, len(data))
copy(dataCopy, data)
w.writes = append(w.writes, dataCopy)
return len(data), nil
}

func TestWrite(t *testing.T) {
data := []byte{0x16, 0x03, 0x01, 0, 10, 0x01, 0, 0, 6, 0x03, 0x03, 1, 2, 3, 4}
var innerWriter collectWrites
trfWriter := NewWriter(&innerWriter, 1)
n, err := trfWriter.Write(data)
require.NoError(t, err)
require.Equal(t, n, len(data)+5)
require.Equal(t, [][]byte{[]byte{0x16, 0x03, 0x01, 0, 1, 0x1, 0x16, 0x03, 0x01, 0, 9, 0, 0, 6, 0x03, 0x03, 1, 2, 3, 4}}, innerWriter.writes)
}

func TestReadFrom(t *testing.T) {
data := []byte{0x16, 0x03, 0x01, 0, 10, 0x01, 0, 0, 6, 0x03, 0x03, 1, 2, 3, 4, 0xff}
var innerWriter collectWrites
trfWriter := NewWriter(&innerWriter, 2)
n, err := trfWriter.ReadFrom(bytes.NewReader(data))
require.NoError(t, err)
require.Equal(t, n, int64(len(data))+5)
require.Equal(t, [][]byte{[]byte{0x16, 0x03, 0x01, 0, 2, 0x1, 0, 0x16, 0x03, 0x01, 0, 8, 0, 6, 0x03, 0x03, 1, 2, 3, 4}, []byte{0xff}}, innerWriter.writes)
}

func TestWrite_MultipleRecords(t *testing.T) {
var innerWriter collectWrites
trfWriter := NewWriter(&innerWriter, 3)
data := make([]byte, 15)
copy(data, header)
binary.BigEndian.PutUint16(data[3:], 4)
copy(data[5:], []byte{4, 3, 2, 1})
copy(data[9:], header)
binary.BigEndian.PutUint16(data[12:], 1)
data[14] = 0x7f

_, err := trfWriter.Write(data)
require.NoError(t, err)

result := make([]byte, 20)
copy(result, header)
binary.BigEndian.PutUint16(result[3:], 3)
copy(result[5:], []byte{4, 3, 2})

copy(result[8:], header)
binary.BigEndian.PutUint16(result[11:], 1)
result[13] = 1

copy(result[14:], []byte{0x16, 0x03, 0x01, 0, 0x01, 0x7f})

require.Equal(t, [][]byte{result}, innerWriter.writes)
}