Update Conjure API registration to use APIRegistrarBidirectional

psiphon/common/refraction/refraction.go

@@ -1,3 +1,4 @@
+//go:build PSIPHON_ENABLE_REFRACTION_NETWORKING
 // +build PSIPHON_ENABLE_REFRACTION_NETWORKING
 
 /*
@@ -325,7 +326,7 @@ func dial(
 				return nil, errors.TraceNew("missing APIRegistrarHTTPClient")
 			}
 
-			refractionDialer.DarkDecoyRegistrar = &refraction_networking_client.APIRegistrar{
+			refractionDialer.DarkDecoyRegistrar = &refraction_networking_client.APIRegistrarBidirectional{
 				Endpoint:        conjureConfig.APIRegistrarURL,
 				ConnectionDelay: conjureConfig.APIRegistrarDelay,
 				MaxRetries:      0,

psiphon/meekConn.go

@@ -774,10 +774,10 @@ func (meek *MeekConn) GetMetrics() common.LogFields {
 // plaintext in the meek traffic. The caller is responsible for securing and
 // obfuscating the request body.
 //
-// ObfuscatedRoundTrip is not safe for concurrent use, and Close must not be
-// called concurrently. The caller must ensure only one ObfuscatedRoundTrip
-// call is active at once and that it completes or is cancelled before calling
-// Close.
+// ObfuscatedRoundTrip is not safe for concurrent use. The caller must ensure
+// only one ObfuscatedRoundTrip call is active at once. If Close is called
+// before or concurrent with ObfuscatedRoundTrip, or before the response body
+// is read, idle connections may be left open.
 func (meek *MeekConn) ObfuscatedRoundTrip(
 	requestCtx context.Context, endPoint string, requestBody []byte) ([]byte, error) {
 
@@ -801,10 +801,6 @@ func (meek *MeekConn) ObfuscatedRoundTrip(
 	// - multiple, concurrent ObfuscatedRoundTrip calls are unsafe due to the
 	//   setDialerRequestContext calls in newRequest.
 	//
-	// - concurrent Close and ObfuscatedRoundTrip calls are unsafe as Close does
-	//   not synchronize with ObfuscatedRoundTrip before calling
-	//   meek.transport.CloseIdleConnections(), so resources could be left open.
-	//
 	// At this time, ObfuscatedRoundTrip is used for tactics in Controller and
 	// the concurrency constraints are satisfied.
 
@@ -839,9 +835,10 @@ func (meek *MeekConn) ObfuscatedRoundTrip(
 // used when TLS and server certificate verification are configured. RoundTrip
 // does not implement any security or obfuscation at the HTTP layer.
 //
-// RoundTrip is not safe for concurrent use, and Close must not be called
-// concurrently. The caller must ensure only one RoundTrip call is active at
-// once and that it completes or is cancelled before calling Close.
+// RoundTrip is not safe for concurrent use. The caller must ensure only one
+// RoundTrip call is active at once. If Close is called before or concurrent
+// with RoundTrip, or before the response body is read, idle connections may
+// be left open.
 func (meek *MeekConn) RoundTrip(request *http.Request) (*http.Response, error) {
 
 	if meek.mode != MeekModePlaintextRoundTrip {

psiphon/tunnel.go

@@ -860,21 +860,36 @@ func dialTunnel(
 			// Performing the full DialMeek/RoundTrip operation here allows us to call
 			// MeekConn.Close and ensure all resources are immediately cleaned up.
 			roundTrip := func(request *http.Request) (*http.Response, error) {
+
 				conn, err := DialMeek(
 					ctx, dialParams.GetMeekConfig(), dialParams.GetDialConfig())
 				if err != nil {
 					return nil, errors.Trace(err)
 				}
 				defer conn.Close()
+
 				response, err := conn.RoundTrip(request)
 				if err != nil {
 					return nil, errors.Trace(err)
 				}
-				// Currently, gotapdance does not read the response body. When that
-				// changes, we will need to ensure MeekConn.Close does not make the
-				// response body unavailable, perhaps by reading into a buffer and
-				// replacing reponse.Body. For now, we can immediately close it.
-				response.Body.Close()
+
+				// Read the response into a buffer and close the response
+				// body, ensuring that MeekConn.Close closes all idle connections.
+				//
+				// Alternatively, we could Clone the request to set
+				// http.Request.Close and avoid keeping any idle connection
+				// open after the response body is read by gotapdance. Since
+				// the response body is small and since gotapdance does not
+				// stream the response body, we're taking this approach which
+				// ensures cleanup.
+
+				body, err := ioutil.ReadAll(response.Body)
+				_ = response.Body.Close()
+				if err != nil {
+					return nil, errors.Trace(err)
+				}
+				response.Body = io.NopCloser(bytes.NewReader(body))
+
 				return response, nil
 			}