Opened 5 months ago

Closed 3 weeks ago

#31028 closed defect (fixed)

Migrate away from the custom websocket library

Reported by: arlolra Owned by: arlolra
Priority: Medium Milestone:
Component: Circumvention/Snowflake Version:
Severity: Normal Keywords:
Cc: arlolra, cohosh, phw, dcf Actual Points:
Parent ID: Points:
Reviewer: cohosh Sponsor:

Description

Child Tickets

TicketStatusOwnerSummaryComponent
#28726closedarlolraLoosen restrictions on message sizes in WebSocket serverCircumvention/Snowflake
#29125closedarlolraMake websocket server tolerant of HTTP/2Circumvention/Snowflake

Change History (8)

comment:1 Changed 7 weeks ago by arlolra

Owner: set to arlolra
Status: newassigned

comment:2 Changed 7 weeks ago by arlolra

Starting something for this in https://github.com/keroserene/snowflake/commits/trac31028 but haven't tested it yet.

comment:3 Changed 6 weeks ago by arlolra

Status: assignedneeds_review

Starting something for this in ​https://github.com/keroserene/snowflake/commits/trac31028 but haven't tested it yet.

Ok, tested with a local bridge and it seems to work.

comment:4 Changed 5 weeks ago by cohosh

Reviewer: cohosh

comment:5 Changed 5 weeks ago by cohosh

It's looking good. I left two comments on the commit.

Should we move to the Gorilla websocket library for the proxy-go instances as well? The GoDoc for the x/net websocket package suggests Gorilla since it is more actively maintained.

comment:6 Changed 5 weeks ago by cohosh

Status: needs_reviewneeds_information

comment:7 Changed 4 weeks ago by dcf

Status: needs_informationmerge_ready

I had a look at this and my observations are the same as cohosh's in comment:5. Sending a CloseMessage before closing seems prudent as that's what the old code did.

Should we move to the Gorilla websocket library for the proxy-go instances as well?

Sure, IMO it's not as important because proxy-go only connects to a known server (not receiving connections from anywhere as the server does), and only needs to be compatible with the server's WebSocket implementation (unlike the server which has to be compatible with many browsers). It's a good idea anyway. I'd say let's get this ticket closed out first.

comment:8 in reply to:  7 Changed 3 weeks ago by arlolra

Resolution: fixed
Status: merge_readyclosed

Replying to dcf:

I had a look at this and my observations are the same as cohosh's in comment:5. Sending a CloseMessage before closing seems prudent as that's what the old code did.

Right, sorry for the omission. I merged a patch for that in,
https://gitweb.torproject.org/pluggable-transports/snowflake.git/commit/?id=abefae158716b9f56692ea16336c1f8185eda27e

and the bulk of this in,
https://gitweb.torproject.org/pluggable-transports/snowflake.git/commit/?id=c417fd5599c5d39951c606856c69a9f05941afd3

Should we move to the Gorilla websocket library for the proxy-go instances as well?

Sure, IMO it's not as important because proxy-go only connects to a known server (not receiving connections from anywhere as the server does), and only needs to be compatible with the server's WebSocket implementation (unlike the server which has to be compatible with many browsers). It's a good idea anyway. I'd say let's get this ticket closed out first.

I opened #32465 for that.

Note: See TracTickets for help on using tickets.