chore: support signed token query param for web terminal

coder · deansheather · Apr 20, 2023 · Apr 18, 2023 · Apr 18, 2023 · Apr 20, 2023
commit aa200ff1d86af67fcc08c0832e07bd85963857a2
diff --git a/coderd/workspaceapps/apptest/apptest.go b/coderd/workspaceapps/apptest/apptest.go
@@ -22,6 +22,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/xerrors"
+	"nhooyr.io/websocket"
 
 	"github.com/coder/coder/coderd/coderdtest"
 	"github.com/coder/coder/coderd/rbac"
@@ -46,43 +47,9 @@ func Run(t *testing.T, factory DeploymentFactory) {
 			t.Skip("ConPTY appears to be inconsistent on Windows.")
 		}
 
-		appDetails := setupProxyTest(t, nil)
-
-		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
-		defer cancel()
-
-		// Run the test against the path app hostname since that's where the
-		// reconnecting-pty proxy server we want to test is mounted.
-		client := appDetails.AppClient(t)
-		conn, err := client.WorkspaceAgentReconnectingPTY(ctx, appDetails.Agent.ID, uuid.New(), 80, 80, "/bin/bash")
-		require.NoError(t, err)
-		defer conn.Close()
-
-		// First attempt to resize the TTY.
-		// The websocket will close if it fails!
-		data, err := json.Marshal(codersdk.ReconnectingPTYRequest{
-			Height: 250,
-			Width:  250,
-		})
-		require.NoError(t, err)
-		_, err = conn.Write(data)
-		require.NoError(t, err)
-		bufRead := bufio.NewReader(conn)
-
-		// Brief pause to reduce the likelihood that we send keystrokes while
-		// the shell is simultaneously sending a prompt.
-		time.Sleep(100 * time.Millisecond)
-
-		data, err = json.Marshal(codersdk.ReconnectingPTYRequest{
-			Data: "echo test\r\n",
-		})
-		require.NoError(t, err)
-		_, err = conn.Write(data)
-		require.NoError(t, err)
-
-		expectLine := func(matcher func(string) bool) {
+		expectLine := func(r *bufio.Reader, matcher func(string) bool) {
 			for {
-				line, err := bufRead.ReadString('\n')
+				line, err := r.ReadString('\n')
 				require.NoError(t, err)
 				if matcher(line) {
 					break
@@ -96,8 +63,88 @@ func Run(t *testing.T, factory DeploymentFactory) {
 			return strings.Contains(line, "test") && !strings.Contains(line, "echo")
 		}
 
-		expectLine(matchEchoCommand)
-		expectLine(matchEchoOutput)
+		t.Run("OK", func(t *testing.T) {
+			appDetails := setupProxyTest(t, nil)
+
+			ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
+			defer cancel()
+
+			// Run the test against the path app hostname since that's where the
+			// reconnecting-pty proxy server we want to test is mounted.
+			client := appDetails.AppClient(t)
+			conn, err := client.WorkspaceAgentReconnectingPTY(ctx, appDetails.Agent.ID, uuid.New(), 80, 80, "/bin/bash")
+			require.NoError(t, err)
+			defer conn.Close()
+
+			// First attempt to resize the TTY.
+			// The websocket will close if it fails!
+			data, err := json.Marshal(codersdk.ReconnectingPTYRequest{
+				Height: 250,
+				Width:  250,
+			})
+			require.NoError(t, err)
+			_, err = conn.Write(data)
+			require.NoError(t, err)
+			bufRead := bufio.NewReader(conn)
+
+			// Brief pause to reduce the likelihood that we send keystrokes while
+			// the shell is simultaneously sending a prompt.
+			time.Sleep(100 * time.Millisecond)
+
+			data, err = json.Marshal(codersdk.ReconnectingPTYRequest{
+				Data: "echo test\r\n",
+			})
+			require.NoError(t, err)
+			_, err = conn.Write(data)
+			require.NoError(t, err)
+
+			expectLine(bufRead, matchEchoCommand)
+			expectLine(bufRead, matchEchoOutput)
+		})
+
+		t.Run("SignedTokenQueryParameter", func(t *testing.T) {
+			t.Parallel()
+
+			appDetails := setupProxyTest(t, nil)
+			if appDetails.AppHostIsPrimary {
+				t.Skip("Tickets are not used for terminal requests on the primary.")
+			}
+
+			u := *appDetails.PathAppBaseURL
+			u.Path = fmt.Sprintf("/api/v2/workspaceagents/%s/pty", appDetails.Agent.ID.String())
+
+			ctx := testutil.Context(t, testutil.WaitLong)
+			issueRes, err := appDetails.SDKClient.IssueReconnectingPTYSignedToken(ctx, codersdk.IssueReconnectingPTYSignedTokenRequest{
+				URL:     u.String(),
+				AgentID: appDetails.Agent.ID,
+			})
+			require.NoError(t, err)
+
+			// Try to connect to the endpoint with the signed token and no other
+			// authentication.
+			q := u.Query()
+			q.Set("reconnect", uuid.NewString())
+			q.Set("height", strconv.Itoa(24))
+			q.Set("width", strconv.Itoa(80))
+			q.Set("command", `/bin/sh -c "echo test"`)
+			q.Set(codersdk.SignedAppTokenQueryParameter, issueRes.SignedToken)
+			u.RawQuery = q.Encode()
+
+			//nolint:bodyclose
+			wsConn, res, err := websocket.Dial(ctx, u.String(), nil)
+			if !assert.NoError(t, err) {
+				dump, err := httputil.DumpResponse(res, true)
+				if err == nil {
+					t.Log(string(dump))
+				}
+				return
+			}
+			defer wsConn.Close(websocket.StatusNormalClosure, "")
+			conn := websocket.NetConn(ctx, wsConn, websocket.MessageBinary)
+			bufRead := bufio.NewReader(conn)
+
+			expectLine(bufRead, matchEchoOutput)
+		})
 	})
 
 	t.Run("WorkspaceAppsProxyPath", func(t *testing.T) {

diff --git a/coderd/workspaceapps/proxy_test.go b/coderd/workspaceapps/proxy_test.go
@@ -1,8 +1,3 @@
 package workspaceapps_test
 
-// NOTE: for now, app proxying tests are still in their old locations, pending
-// being moved to their own package.
-//
-// See:
-// - coderd/workspaceapps_test.go
-// - coderd/workspaceagents_test.go (for PTY)
+// App tests can be found in the apptest package.
diff --git a/coderd/workspaceapps/token.go b/coderd/workspaceapps/token.go
@@ -220,13 +220,30 @@ func (k SecurityKey) DecryptAPIKey(encryptedAPIKey string) (string, error) {
 	return payload.APIKey, nil
 }
 
+// FromRequest returns the signed token from the request, if it exists and is
+// valid. The caller must check that the token matches the request.
 func FromRequest(r *http.Request, key SecurityKey) (*SignedToken, bool) {
-	// Get the existing token from the request.
-	tokenCookie, err := r.Cookie(codersdk.DevURLSignedAppTokenCookie)
-	if err == nil {
-		token, err := key.VerifySignedToken(tokenCookie.Value)
+	// Get the token string from the request. We usually use a cookie for this,
+	// but for web terminal we also support a query parameter to support
+	// cross-domain terminal access.
+	tokenStr := ""
+	tokenCookie, cookieErr := r.Cookie(codersdk.DevURLSignedAppTokenCookie)
+	if cookieErr == nil {
+		tokenStr = tokenCookie.Value
+	} else {
+		tokenStr = r.URL.Query().Get(codersdk.SignedAppTokenQueryParameter)
+	}
+
+	if tokenStr != "" {
+		token, err := key.VerifySignedToken(tokenStr)
 		if err == nil {
 			req := token.Request.Normalize()
+			if cookieErr != nil && req.AccessMethod != AccessMethodTerminal {
+				// The request must be a terminal request if we're using a
+				// query parameter.
+				return nil, false
+			}
+
 			err := req.Validate()
 			if err == nil {
 				// The request has a valid signed app token, which is a valid

diff --git a/codersdk/client.go b/codersdk/client.go
@@ -46,6 +46,15 @@ const (
 	// token.
 	//nolint:gosec
 	DevURLSignedAppTokenCookie = "coder_devurl_signed_app_token"
+	// SignedAppTokenQueryParameter is the name of the query parameter that
+	// stores a temporary JWT that can be used to authenticate instead of the
+	// session token. This is only acceptable on reconnecting-pty requests, not
+	// apps.
+	//
+	// It has a random suffix to avoid conflict with user query parameters on
+	// apps.
+	//nolint:gosec
+	SignedAppTokenQueryParameter = "coder_signed_app_token_23db1dde"
 
 	// BypassRatelimitHeader is the custom header to use to bypass ratelimits.
 	// Only owners can bypass rate limits. This is typically used for scale testing.

diff --git a/docs/api/applications enterprise.md b/docs/api/applications enterprise.md
@@ -0,0 +1 @@
+# Applications Enterprise
diff --git a/enterprise/coderd/workspaceproxy_test.go b/enterprise/coderd/workspaceproxy_test.go
@@ -359,6 +359,7 @@ func TestReconnectingPTYSignedToken(t *testing.T) {
 		require.NoError(t, err)
 		require.NotEmpty(t, res.SignedToken)
 
-		// Verify the token is valid for connecting to the terminal.
+		// The token is validated in the apptest suite, so we don't need to
+		// validate it here.
 	})
 }