feat(transport): implement TLS record fragmentation #114

Lanius-collaris · 2023-10-22T04:54:29Z

No description provided.

google-cla · 2023-10-22T04:54:33Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

fortuna

Thanks for the contribution!

fortuna · 2023-10-23T18:15:39Z

transport/tls-record-frag/writer.go

+
+func (w *tlsRecordFragWriter) Write(data []byte) (written int, err error) {
+	length := len(data)
+	if length > 5+maxRecordLength {


This is not the correct behavior, even if the connections is guaranteed TLS. A given write may have a single record, a partial record or multiple records. We need to fix this.

remove the check

read the record length from the data

fix the split calculation

We should assume the first write has the record length.

fortuna · 2023-10-23T18:27:18Z

transport/tls-record-frag/stream_dialer.go

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

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.

transport/tls-record-frag/writer.go

transport/tls-record-frag/stream_dialer.go

jyyi1

Thank a lot for the contribution! Could you please sign the Google CLA to make the CI job pass:

📝 If you are not currently covered under a CLA, please visit https://cla.developers.google.com/. Once you've signed, follow the "New Contributors" link at the bottom of this page to update this check.

jyyi1 · 2023-10-24T15:38:51Z

transport/tls-record-frag/stream_dialer.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package tlsrecordfrag


Suggested change

package tlsrecordfrag

package tlsfrag

jyyi1 · 2023-10-24T18:14:52Z

transport/tls-record-frag/stream_dialer.go

+var _ transport.StreamDialer = (*tlsRecordFragDialer)(nil)
+
+// NewStreamDialer creates a [transport.StreamDialer] that splits the Client Hello Message
+func NewStreamDialer(dialer transport.StreamDialer, prefixBytes int32) (transport.StreamDialer, error) {


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) {

That's too long. Perhaps NewFixedPrefixStreamDialer

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

jyyi1 · 2023-10-24T18:16:32Z

transport/tls-record-frag/stream_dialer.go

+// NewStreamDialer creates a [transport.StreamDialer] that splits the Client Hello Message
+func NewStreamDialer(dialer transport.StreamDialer, prefixBytes int32) (transport.StreamDialer, error) {
+	if dialer == nil {
+		return nil, errors.New("argument dialer must not be nil")


Suggested change

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

return nil, errors.New("an inner dialer is required")

The original code is consistent with the split Dialer:

outline-sdk/transport/split/stream_dialer.go

Line 35 in 375a66d

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.

jyyi1 · 2023-10-24T18:18:27Z

transport/tls-record-frag/stream_dialer.go

+	if dialer == nil {
+		return nil, errors.New("argument dialer must not be nil")
+	}
+	return &tlsRecordFragDialer{dialer: dialer, splitPoint: prefixBytes}, nil


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

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).

jyyi1 · 2023-10-24T18:20:14Z

transport/tls-record-frag/writer.go

+
+const maxRecordLength = 16384 //For the fragments, not for the reassembled record
+
+func NewWriter(writer io.Writer, prefixBytes int32) *tlsRecordFragWriter {


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

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

transport/tls-record-frag/writer.go

jyyi1 · 2023-10-24T18:37:53Z

transport/tls-record-frag/stream_dialer.go

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


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

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

fortuna · 2023-10-24T16:21:27Z

transport/tls-record-frag/stream_dialer.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package tlsrecordfrag


Please make the directory match the package name.

tlssplit or tlsfrag?

transport/tls-record-frag/stream_dialer.go

transport/tls-record-frag/writer.go

fortuna · 2023-10-24T21:08:22Z

transport/tls-record-frag/writer.go

+
+const maxRecordLength = 16384 //For the fragments, not for the reassembled record
+
+func NewWriter(writer io.Writer, prefixBytes int32) *tlsRecordFragWriter {


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

transport/tls-record-frag/writer.go

fortuna · 2023-10-24T21:18:12Z

transport/tls-record-frag/stream_dialer.go

+// NewStreamDialer creates a [transport.StreamDialer] that splits the Client Hello Message
+func NewStreamDialer(dialer transport.StreamDialer, prefixBytes int32) (transport.StreamDialer, error) {
+	if dialer == nil {
+		return nil, errors.New("argument dialer must not be nil")


The original code is consistent with the split Dialer:

outline-sdk/transport/split/stream_dialer.go

Line 35 in 375a66d

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.

fortuna · 2023-10-24T21:20:14Z

transport/tls-record-frag/stream_dialer.go

+
+var _ transport.StreamDialer = (*tlsRecordFragDialer)(nil)
+
+// NewStreamDialer creates a [transport.StreamDialer] that splits the Client Hello Message


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.

fortuna · 2023-10-24T21:21:59Z

transport/tls-record-frag/stream_dialer.go

+var _ transport.StreamDialer = (*tlsRecordFragDialer)(nil)
+
+// NewStreamDialer creates a [transport.StreamDialer] that splits the Client Hello Message
+func NewStreamDialer(dialer transport.StreamDialer, prefixBytes int32) (transport.StreamDialer, error) {


That's too long. Perhaps NewFixedPrefixStreamDialer

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

fortuna · 2023-10-24T21:23:00Z

transport/tls-record-frag/stream_dialer.go

+	if dialer == nil {
+		return nil, errors.New("argument dialer must not be nil")
+	}
+	return &tlsRecordFragDialer{dialer: dialer, splitPoint: prefixBytes}, nil


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).

fortuna · 2023-10-24T21:32:04Z

transport/tls-record-frag/stream_dialer.go

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


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

Lanius-collaris · 2023-10-25T09:20:16Z

Thank a lot for the contribution! Could you please sign the Google CLA to make the CI job pass:

📝 If you are not currently covered under a CLA, please visit https://cla.developers.google.com/. Once you've signed, follow the "New Contributors" link at the bottom of this page to update this check.

@jyyi1 How can I get a non-Chinese phone number to register Google account?

fortuna · 2023-10-25T15:40:34Z

@Lanius-collaris Ugh, I see the issue. The CLA system requires Google account, which in turn seems to require a phone number.

They require Google accounts because they are used to link individual and corporate CLAs to covered contributors and provide authentication for CLA management.

Have you tried the "use existing email" option when creating the account?

Google needs a backup contact information, that's why you need a phone number. But if you provide some email, then you may not need a phone.

Lanius-collaris · 2023-10-25T16:19:09Z

Have you tried the "use existing email" option when creating the account?

@fortuna Yes, I have tried. But I got unknown error.

transport/tls-record-frag/writer.go

tlsRecordFragWriter: check if input is a handshake record return the number of input bytes consumed add an RFC link for reference tlsRecordFragDialer: rename field

Lanius-collaris · 2023-10-26T10:43:39Z

@fortuna I tried again today, after I entered the verification code from mail, Google asked for a number.

feat: implement TLS record fragmentation

751117a

fortuna self-assigned this Oct 23, 2023

fortuna requested changes Oct 23, 2023

View reviewed changes

fortuna requested a review from jyyi1 October 23, 2023 20:30

Lanius-collaris added 2 commits October 24, 2023 10:24

Several fix for tlsRecordFragWriter

4aada28

fix tlsRecordFragWriter.Write()

12e8acf

Lanius-collaris requested a review from fortuna October 24, 2023 06:18

jyyi1 reviewed Oct 24, 2023

View reviewed changes

jyyi1 changed the title ~~feat: implement TLS record fragmentation~~ feat(transport): implement TLS record fragmentation Oct 24, 2023

fortuna requested changes Oct 24, 2023

View reviewed changes

fortuna requested changes Oct 25, 2023

View reviewed changes

transport/tls-record-frag/writer.go Outdated Show resolved Hide resolved

transport/tls-record-frag/writer.go Show resolved Hide resolved

transport/tls-record-frag/writer.go Outdated Show resolved Hide resolved

Several fixes in the tlsrecordfrag package

12e1c28

tlsRecordFragWriter: check if input is a handshake record return the number of input bytes consumed add an RFC link for reference tlsRecordFragDialer: rename field

Lanius-collaris marked this pull request as ready for review October 25, 2023 17:56

Lanius-collaris closed this Nov 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(transport): implement TLS record fragmentation #114

feat(transport): implement TLS record fragmentation #114

Lanius-collaris commented Oct 22, 2023

google-cla bot commented Oct 22, 2023

fortuna left a comment

fortuna Oct 23, 2023

fortuna Oct 23, 2023

jyyi1 Oct 23, 2023

jyyi1 left a comment

jyyi1 Oct 24, 2023

jyyi1 Oct 24, 2023

fortuna Oct 24, 2023

jyyi1 Oct 24, 2023

fortuna Oct 24, 2023

jyyi1 Oct 24, 2023

fortuna Oct 24, 2023

jyyi1 Oct 24, 2023

fortuna Oct 24, 2023

jyyi1 Oct 24, 2023

fortuna Oct 24, 2023

fortuna Oct 24, 2023

Lanius-collaris Oct 25, 2023

fortuna Oct 24, 2023

fortuna Oct 24, 2023

fortuna Oct 24, 2023

fortuna Oct 24, 2023

fortuna Oct 24, 2023

fortuna Oct 24, 2023

Lanius-collaris commented Oct 25, 2023

fortuna commented Oct 25, 2023

Lanius-collaris commented Oct 25, 2023 •

edited

Loading

Lanius-collaris commented Oct 26, 2023

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

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


		const maxRecordLength = 16384 //For the fragments, not for the reassembled record

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


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

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

feat(transport): implement TLS record fragmentation #114

feat(transport): implement TLS record fragmentation #114

Conversation

Lanius-collaris commented Oct 22, 2023

google-cla bot commented Oct 22, 2023

fortuna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyyi1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lanius-collaris commented Oct 25, 2023

fortuna commented Oct 25, 2023

Lanius-collaris commented Oct 25, 2023 • edited Loading

Lanius-collaris commented Oct 26, 2023

Lanius-collaris commented Oct 25, 2023 •

edited

Loading