(v6) Fix app access websockets support #6028

r0mant · 2021-03-17T00:05:45Z

What

This pull request fixes websockets proxying through the application access.

I used the Node-RED application to test this according to the @webvictim's example in the ticket linked above. With these changes the websockets connection gets proxied correctly and the output in the inspector is consistent with the "working" screenshot from that ticket.

How

The meat of the fix is in lib/utils/fakeconn.go. Websockets forwarding works by hijacking the raw connection from ResponseWriter and passing all traffic back/forth between the client/server:

https://github.com/gravitational/oxy/blob/1.0.4/forward/fwd.go#L270

underlyingConn, _, err := hijacker.Hijack()

During investigation I found out that Hijack call is where it would hang indefinitely.

After further investigation, I found that before returning raw connection to the user during hijack, Go runtime tries to cancel all pending reads on the connection:

https://github.com/golang/go/blob/go1.15.10/src/net/http/server.go#L315

c.r.abortPendingRead()

It does that by setting read deadline far in the past and waiting for pending reads to fail:

https://github.com/golang/go/blob/go1.15.10/src/net/http/server.go#L736

cr.conn.rwc.SetReadDeadline(aLongTimeAgo)

The problem is that all connections coming out of reverse tunnel are wrapped in utils.ChConn:

https://github.com/gravitational/teleport/blob/master/lib/reversetunnel/transport.go#L301

p.server.HandleConnection(utils.NewChConn(p.sconn, p.channel))

which doesn't implement deadlines:

https://github.com/gravitational/teleport/blob/master/lib/utils/fakeconn.go#L192-L196

// SetReadDeadline sets a connection read deadline
// ignored for the channel connection
func (c *ChConn) SetReadDeadline(t time.Time) error {
	return nil
}

Thus, this PR introduces a CancelableChConn that extends ChConn with read deadlines support, based on the net.Pipe.

Note, I only used CancelableChConn for App Access servers to limit the scope and potential impact of the change. We could just update regular ChConn so it applies to all reverse tunnel connections but I opted to be on a safe side since this functionality is only required for websockets support through AAP.

There also were a couple of tweaks required here and there to have oxy/forward properly forward websockets.

a-palchikov · 2021-03-17T13:37:51Z

lib/utils/fakeconn.go

+	//
+	// This goroutine stops when either the SSH channel closes or this
+	// connection is closed e.g. by a http.Server (see Close below).
+	go io.Copy(writer, ch)


nit: not sure if this goroutine races with writer.Close but I'd ensure that it exits before CancelableChConn.Close exits at least. On the bonus side, I'd ensure that it exits before the writer is closed,

Yeah, I'm not sure there's necessarily a race - if any part of the connection (writer or ChConn) closes, the copy will just stop which isn't much different from just closing the connection during a read.

My primary concern with this was to avoid a goroutine leak which I think can't happen if any part of the connection closes (which it does). We could add a channel signaling that the copy has exited to be a bit paranoid though.

a-palchikov · 2021-03-17T13:38:31Z

lib/utils/fakeconn.go

+
+// SetDeadline sets the channel connection read/write deadlines.
+func (c *CancelableChConn) SetDeadline(t time.Time) error {
+	return c.reader.SetDeadline(t)


Which makes me wonder whether the original ChConn should return errors instead of nil for the APIs it does not implement - WDYT? Would be at least interesting to see whether it breaks anywhere in tests.

Yeah, I'm pretty sure things will break in mysterious ways :) A lot of Go runtime uses deadlines and will trip if we start returning errors. I'm actually more surprised that things seems to work ok without deadlines implemented on ChConn (well, besides this websockets issue).

lib/utils/fakeconn.go

lib/utils/fakeconn_test.go

lib/web/app/transport.go

russjones · 2021-03-18T01:27:49Z

lib/reversetunnel/transport.go

+			p.log.Debugf("Handing off connection to a local %q service.", dreq.ConnType)
+			switch dreq.ConnType {
+			case types.AppTunnel:
+				// Use channel connection wrapper that supports deadlines when
+				// proxying applications. This is required for websockets to
+				// work over Application Access.
+				p.server.HandleConnection(utils.NewCancelableChConn(p.sconn, p.channel))
+			default:
+				p.server.HandleConnection(utils.NewChConn(p.sconn, p.channel))
+			}


Any reason why you separated these two and created CancelableChConn instead of just adding deadline support to ChConn?

@russjones No good reason other than out of abundance of caution. I can just update ChConn if you feel strongly about it, but I just felt that this functionality is only required for app access and wanted to avoid the risk of potentially introducing some side-effects to other types of services like ssh (and esp. since it's going to a minor release).

Makes sense for 6.1. What do you think updating ChConn on master to support deadlines but keeping them separate on branch/v6? master is still 6 months away from becoming 7.0 so we would get a fair amount of testing in during that time plus you would get any additional testing for Application Access on 6.1.

@russjones Yeah, that was one of my alternative ideas as well. We can do that. I'll rebase this PR off of branch/v6 and open the one to master with just updated ChConn.

Alright, looks good to me, merge it in after rebase off of branch/v6.

russjones · 2021-03-18T01:28:58Z

lib/srv/app/transport.go

+// Rewrite rewrites the websockets request.
+func (r *websocketsRewriter) Rewrite(req *http.Request) {
+	// Update host to the target app's host to make sure it's forwarded correctly.
+	req.URL.Host = r.uri.Host
+}


This doesn't go through the round tripper?

No, websockets use their own forwarder logic and a separate rewriter as well:

https://github.com/gravitational/oxy/blob/master/forward/fwd.go#L124-L128

r0mant added backport-required application-access labels Mar 17, 2021

r0mant requested review from a-palchikov, awly, russjones and fspmarshall March 17, 2021 00:05

r0mant requested review from alex-kovoy and klizhentas as code owners March 17, 2021 00:05

r0mant self-assigned this Mar 17, 2021

r0mant added this to the 6.1 milestone Mar 17, 2021

a-palchikov approved these changes Mar 17, 2021

View reviewed changes

russjones reviewed Mar 18, 2021

View reviewed changes

r0mant force-pushed the roman/ws branch from 5456959 to 5d65dfb Compare March 18, 2021 22:37

russjones approved these changes Mar 18, 2021

View reviewed changes

r0mant changed the base branch from master to branch/v6 March 18, 2021 23:40

r0mant changed the title ~~Fix app access websockets support~~ (v6) Fix app access websockets support Mar 18, 2021

r0mant force-pushed the roman/ws branch 2 times, most recently from 9ede6c1 to 9a00854 Compare March 19, 2021 15:28

Fix app access websockets support

13f4b02

r0mant force-pushed the roman/ws branch from 9a00854 to 13f4b02 Compare March 19, 2021 20:17

r0mant mentioned this pull request Mar 19, 2021

(7.0) Fix app access websockets support #6072

Merged

Merge branch 'branch/v6' into roman/ws

f240877

r0mant merged commit 7061b52 into branch/v6 Mar 19, 2021

r0mant deleted the roman/ws branch March 19, 2021 22:41

webvictim mentioned this pull request May 5, 2021

ami auto branch 1620236164 #6745

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(v6) Fix app access websockets support #6028

(v6) Fix app access websockets support #6028

r0mant commented Mar 17, 2021

a-palchikov Mar 17, 2021

r0mant Mar 17, 2021

a-palchikov Mar 17, 2021

r0mant Mar 17, 2021

russjones Mar 18, 2021

r0mant Mar 18, 2021

russjones Mar 18, 2021 •

edited

Loading

r0mant Mar 18, 2021

russjones Mar 18, 2021

russjones Mar 18, 2021

r0mant Mar 18, 2021

(v6) Fix app access websockets support #6028

(v6) Fix app access websockets support #6028

Conversation

r0mant commented Mar 17, 2021

What

How

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

russjones Mar 18, 2021 • edited Loading

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

russjones Mar 18, 2021 •

edited

Loading