From 6bb1a34a3746d54981119d679a3ce87549351c35 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 18 Jan 2024 09:44:05 -0600 Subject: [PATCH] fix: allow ports in wildcard url configuration (#11657) * fix: allow ports in wildcard url configuration This just forwards the port to the ui that generates urls. Our existing parsing + regex already supported ports for subdomain app requests. --- coderd/agentapi/manifest.go | 23 +++--- coderd/agentapi/manifest_internal_test.go | 94 ++++++++++++++++++++++ coderd/coderd.go | 2 +- coderd/workspaceapps.go | 8 +- coderd/workspaceapps/apptest/apptest.go | 32 ++++++++ coderd/workspaceapps/apptest/setup.go | 3 +- coderd/workspaceapps/appurl/appurl.go | 45 ++++++++++- coderd/workspaceapps/appurl/appurl_test.go | 12 +-- coderd/workspaceproxies.go | 3 +- coderd/workspaceproxies_test.go | 3 +- enterprise/coderd/workspaceproxy_test.go | 4 +- enterprise/wsproxy/wsproxy_test.go | 5 +- 12 files changed, 203 insertions(+), 31 deletions(-) create mode 100644 coderd/agentapi/manifest_internal_test.go diff --git a/coderd/agentapi/manifest.go b/coderd/agentapi/manifest.go index 9091cb992a..2d81aef775 100644 --- a/coderd/agentapi/manifest.go +++ b/coderd/agentapi/manifest.go @@ -3,7 +3,6 @@ package agentapi import ( "context" "database/sql" - "fmt" "net/url" "strings" "sync/atomic" @@ -108,19 +107,14 @@ func (a *ManifestAPI) GetManifest(ctx context.Context, _ *agentproto.GetManifest return nil, xerrors.Errorf("fetching workspace agent data: %w", err) } - appHost := appurl.ApplicationURL{ + appSlug := appurl.ApplicationURL{ AppSlugOrPort: "{{port}}", AgentName: workspaceAgent.Name, WorkspaceName: workspace.Name, Username: owner.Username, } - vscodeProxyURI := a.AccessURL.Scheme + "://" + strings.ReplaceAll(a.AppHostname, "*", appHost.String()) - if a.AppHostname == "" { - vscodeProxyURI += a.AccessURL.Hostname() - } - if a.AccessURL.Port() != "" { - vscodeProxyURI += fmt.Sprintf(":%s", a.AccessURL.Port()) - } + + vscodeProxyURI := vscodeProxyURI(appSlug, a.AccessURL, a.AppHostname) var gitAuthConfigs uint32 for _, cfg := range a.ExternalAuthConfigs { @@ -155,6 +149,17 @@ func (a *ManifestAPI) GetManifest(ctx context.Context, _ *agentproto.GetManifest }, nil } +func vscodeProxyURI(app appurl.ApplicationURL, accessURL *url.URL, appHost string) string { + // This will handle the ports from the accessURL or appHost. + appHost = appurl.SubdomainAppHost(appHost, accessURL) + // If there is no appHost, then we want to use the access url as the proxy uri. + if appHost == "" { + appHost = accessURL.Host + } + // Return the url with a scheme and any wildcards replaced with the app slug. + return accessURL.Scheme + "://" + strings.ReplaceAll(appHost, "*", app.String()) +} + func dbAgentMetadataToProtoDescription(metadata []database.WorkspaceAgentMetadatum) []*agentproto.WorkspaceAgentMetadata_Description { ret := make([]*agentproto.WorkspaceAgentMetadata_Description, len(metadata)) for i, metadatum := range metadata { diff --git a/coderd/agentapi/manifest_internal_test.go b/coderd/agentapi/manifest_internal_test.go new file mode 100644 index 0000000000..30d144d1e9 --- /dev/null +++ b/coderd/agentapi/manifest_internal_test.go @@ -0,0 +1,94 @@ +package agentapi + +import ( + "fmt" + "net/url" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/workspaceapps/appurl" +) + +func Test_vscodeProxyURI(t *testing.T) { + t.Parallel() + + coderAccessURL, err := url.Parse("https://coder.com") + require.NoError(t, err) + + accessURLWithPort, err := url.Parse("https://coder.com:8080") + require.NoError(t, err) + + basicApp := appurl.ApplicationURL{ + Prefix: "prefix", + AppSlugOrPort: "slug", + AgentName: "agent", + WorkspaceName: "workspace", + Username: "user", + } + + cases := []struct { + Name string + App appurl.ApplicationURL + AccessURL *url.URL + AppHostname string + Expected string + }{ + { + // No hostname proxies through the access url. + Name: "NoHostname", + AccessURL: coderAccessURL, + AppHostname: "", + App: basicApp, + Expected: coderAccessURL.String(), + }, + { + Name: "NoHostnameAccessURLPort", + AccessURL: accessURLWithPort, + AppHostname: "", + App: basicApp, + Expected: accessURLWithPort.String(), + }, + { + Name: "Hostname", + AccessURL: coderAccessURL, + AppHostname: "*.apps.coder.com", + App: basicApp, + Expected: fmt.Sprintf("https://%s.apps.coder.com", basicApp.String()), + }, + { + Name: "HostnameWithAccessURLPort", + AccessURL: accessURLWithPort, + AppHostname: "*.apps.coder.com", + App: basicApp, + Expected: fmt.Sprintf("https://%s.apps.coder.com:%s", basicApp.String(), accessURLWithPort.Port()), + }, + { + Name: "HostnameWithPort", + AccessURL: coderAccessURL, + AppHostname: "*.apps.coder.com:4444", + App: basicApp, + Expected: fmt.Sprintf("https://%s.apps.coder.com:%s", basicApp.String(), "4444"), + }, + { + // Port from hostname takes precedence over access url port. + Name: "HostnameWithPortAccessURLWithPort", + AccessURL: accessURLWithPort, + AppHostname: "*.apps.coder.com:4444", + App: basicApp, + Expected: fmt.Sprintf("https://%s.apps.coder.com:%s", basicApp.String(), "4444"), + }, + } + + for _, c := range cases { + c := c + t.Run(c.Name, func(t *testing.T) { + t.Parallel() + + require.NotNilf(t, c.AccessURL, "AccessURL is required") + + output := vscodeProxyURI(c.App, c.AccessURL, c.AppHostname) + require.Equal(t, c.Expected, output) + }) + } +} diff --git a/coderd/coderd.go b/coderd/coderd.go index 05291ec4bc..e3c935971f 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -93,7 +93,7 @@ type Options struct { // AppHostname should be the wildcard hostname to use for workspace // applications INCLUDING the asterisk, (optional) suffix and leading dot. // It will use the same scheme and port number as the access URL. - // E.g. "*.apps.coder.com" or "*-apps.coder.com". + // E.g. "*.apps.coder.com" or "*-apps.coder.com" or "*.apps.coder.com:8080". AppHostname string // AppHostnameRegex contains the regex version of options.AppHostname as // generated by appurl.CompileHostnamePattern(). It MUST be set if diff --git a/coderd/workspaceapps.go b/coderd/workspaceapps.go index 8cfa287009..b519bc2a29 100644 --- a/coderd/workspaceapps.go +++ b/coderd/workspaceapps.go @@ -3,7 +3,6 @@ package coderd import ( "context" "database/sql" - "fmt" "net/http" "net/url" "strings" @@ -32,13 +31,8 @@ import ( // @Router /applications/host [get] // @Deprecated use api/v2/regions and see the primary proxy. func (api *API) appHost(rw http.ResponseWriter, r *http.Request) { - host := api.AppHostname - if host != "" && api.AccessURL.Port() != "" { - host += fmt.Sprintf(":%s", api.AccessURL.Port()) - } - httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.AppHostResponse{ - Host: host, + Host: appurl.SubdomainAppHost(api.AppHostname, api.AccessURL), }) } diff --git a/coderd/workspaceapps/apptest/apptest.go b/coderd/workspaceapps/apptest/apptest.go index a38189a1ff..2c4963060b 100644 --- a/coderd/workspaceapps/apptest/apptest.go +++ b/coderd/workspaceapps/apptest/apptest.go @@ -963,6 +963,38 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.Equal(t, http.StatusOK, resp.StatusCode) }) + t.Run("WildcardPortOK", func(t *testing.T) { + t.Parallel() + + // Manually specifying a port should override the access url port on + // the app host. + appDetails := setupProxyTest(t, &DeploymentOptions{ + // Just throw both the wsproxy and primary to same url. + AppHost: "*.test.coder.com:4444", + PrimaryAppHost: "*.test.coder.com:4444", + }) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + u := appDetails.SubdomainAppURL(appDetails.Apps.Owner) + t.Logf("url: %s", u) + require.Equal(t, "4444", u.Port(), "port should be 4444") + + // Assert the api response the UI uses has the port. + apphost, err := appDetails.SDKClient.AppHost(ctx) + require.NoError(t, err) + require.Equal(t, "*.test.coder.com:4444", apphost.Host, "apphost has port") + + resp, err := requestWithRetries(ctx, t, appDetails.AppClient(t), http.MethodGet, u.String(), nil) + require.NoError(t, err) + defer resp.Body.Close() + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, proxyTestAppBody, string(body)) + require.Equal(t, http.StatusOK, resp.StatusCode) + }) + t.Run("SuffixWildcardNotMatch", func(t *testing.T) { t.Parallel() diff --git a/coderd/workspaceapps/apptest/setup.go b/coderd/workspaceapps/apptest/setup.go index 92d3d23c8a..99d91c2e20 100644 --- a/coderd/workspaceapps/apptest/setup.go +++ b/coderd/workspaceapps/apptest/setup.go @@ -47,6 +47,7 @@ const ( // DeploymentOptions are the options for creating a *Deployment with a // DeploymentFactory. type DeploymentOptions struct { + PrimaryAppHost string AppHost string DisablePathApps bool DisableSubdomainApps bool @@ -407,7 +408,7 @@ func createWorkspaceWithApps(t *testing.T, client *codersdk.Client, orgID uuid.U Username: me.Username, } proxyURL := "http://" + appHost.String() + strings.ReplaceAll(primaryAppHost.Host, "*", "") - require.Equal(t, proxyURL, manifest.VSCodePortProxyURI) + require.Equal(t, manifest.VSCodePortProxyURI, proxyURL) } agentCloser := agent.New(agent.Options{ Client: agentClient, diff --git a/coderd/workspaceapps/appurl/appurl.go b/coderd/workspaceapps/appurl/appurl.go index f3e6878c03..4daa05a7e3 100644 --- a/coderd/workspaceapps/appurl/appurl.go +++ b/coderd/workspaceapps/appurl/appurl.go @@ -3,6 +3,7 @@ package appurl import ( "fmt" "net" + "net/url" "regexp" "strings" @@ -20,6 +21,36 @@ var ( validHostnameLabelRegex = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`) ) +// SubdomainAppHost returns the URL of the apphost for subdomain based apps. +// It will omit the scheme. +// +// Arguments: +// apphost: Expected to contain a wildcard, example: "*.coder.com" +// accessURL: The access url for the deployment. +// +// Returns: +// 'apphost:port' +// +// For backwards compatibility and for "accessurl=localhost:0" purposes, we need +// to use the port from the accessurl if the apphost doesn't have a port. +// If the user specifies a port in the apphost, we will use that port instead. +func SubdomainAppHost(apphost string, accessURL *url.URL) string { + if apphost == "" { + return "" + } + + if apphost != "" && accessURL.Port() != "" { + // This should always parse if we prepend a scheme. We should add + // the access url port if the apphost doesn't have a port specified. + appHostU, err := url.Parse(fmt.Sprintf("https://%s", apphost)) + if err != nil || (err == nil && appHostU.Port() == "") { + apphost += fmt.Sprintf(":%s", accessURL.Port()) + } + } + + return apphost +} + // ApplicationURL is a parsed application URL hostname. type ApplicationURL struct { Prefix string @@ -140,9 +171,7 @@ func CompileHostnamePattern(pattern string) (*regexp.Regexp, error) { if strings.Contains(pattern, "http:") || strings.Contains(pattern, "https:") { return nil, xerrors.Errorf("hostname pattern must not contain a scheme: %q", pattern) } - if strings.Contains(pattern, ":") { - return nil, xerrors.Errorf("hostname pattern must not contain a port: %q", pattern) - } + if strings.HasPrefix(pattern, ".") || strings.HasSuffix(pattern, ".") { return nil, xerrors.Errorf("hostname pattern must not start or end with a period: %q", pattern) } @@ -155,6 +184,16 @@ func CompileHostnamePattern(pattern string) (*regexp.Regexp, error) { if !strings.HasPrefix(pattern, "*") { return nil, xerrors.Errorf("hostname pattern must only contain an asterisk at the beginning: %q", pattern) } + + // If there is a hostname:port, we only care about the hostname. For hostname + // pattern reasons, we do not actually care what port the client is requesting. + // Any port provided here is used for generating urls for the ui, not for + // validation. + hostname, _, err := net.SplitHostPort(pattern) + if err == nil { + pattern = hostname + } + for i, label := range strings.Split(pattern, ".") { if i == 0 { // We have to allow the asterisk to be a valid hostname label, so diff --git a/coderd/workspaceapps/appurl/appurl_test.go b/coderd/workspaceapps/appurl/appurl_test.go index 617d9380e5..98a34c6003 100644 --- a/coderd/workspaceapps/appurl/appurl_test.go +++ b/coderd/workspaceapps/appurl/appurl_test.go @@ -193,11 +193,6 @@ func TestCompileHostnamePattern(t *testing.T) { pattern: "https://*.hi.com", errorContains: "must not contain a scheme", }, - { - name: "Invalid_ContainsPort", - pattern: "*.hi.com:8080", - errorContains: "must not contain a port", - }, { name: "Invalid_StartPeriod", pattern: ".hi.com", @@ -249,6 +244,13 @@ func TestCompileHostnamePattern(t *testing.T) { errorContains: "contains invalid label", }, + { + name: "Valid_ContainsPort", + pattern: "*.hi.com:8080", + // Although a port is provided, the regex already matches any port. + // So it is ignored for validation purposes. + expectedRegex: `([^.]+)\.hi\.com`, + }, { name: "Valid_Simple", pattern: "*.hi", diff --git a/coderd/workspaceproxies.go b/coderd/workspaceproxies.go index fca0968195..b8572cafc7 100644 --- a/coderd/workspaceproxies.go +++ b/coderd/workspaceproxies.go @@ -11,6 +11,7 @@ import ( "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/httpapi" + "github.com/coder/coder/v2/coderd/workspaceapps/appurl" "github.com/coder/coder/v2/codersdk" ) @@ -43,7 +44,7 @@ func (api *API) PrimaryRegion(ctx context.Context) (codersdk.Region, error) { IconURL: proxy.IconUrl, Healthy: true, PathAppURL: api.AccessURL.String(), - WildcardHostname: api.AppHostname, + WildcardHostname: appurl.SubdomainAppHost(api.AppHostname, api.AccessURL), }, nil } diff --git a/coderd/workspaceproxies_test.go b/coderd/workspaceproxies_test.go index 60718f8a22..86518dd7e4 100644 --- a/coderd/workspaceproxies_test.go +++ b/coderd/workspaceproxies_test.go @@ -1,6 +1,7 @@ package coderd_test import ( + "fmt" "testing" "github.com/google/uuid" @@ -44,7 +45,7 @@ func TestRegions(t *testing.T) { require.NotEmpty(t, regions[0].IconURL) require.True(t, regions[0].Healthy) require.Equal(t, client.URL.String(), regions[0].PathAppURL) - require.Equal(t, appHostname, regions[0].WildcardHostname) + require.Equal(t, fmt.Sprintf("%s:%s", appHostname, client.URL.Port()), regions[0].WildcardHostname) // Ensure the primary region ID is constant. regions2, err := client.Regions(ctx) diff --git a/enterprise/coderd/workspaceproxy_test.go b/enterprise/coderd/workspaceproxy_test.go index d7bc53992e..17e17240dc 100644 --- a/enterprise/coderd/workspaceproxy_test.go +++ b/enterprise/coderd/workspaceproxy_test.go @@ -62,7 +62,7 @@ func TestRegions(t *testing.T) { require.NotEmpty(t, regions[0].IconURL) require.True(t, regions[0].Healthy) require.Equal(t, client.URL.String(), regions[0].PathAppURL) - require.Equal(t, appHostname, regions[0].WildcardHostname) + require.Equal(t, fmt.Sprintf("%s:%s", appHostname, client.URL.Port()), regions[0].WildcardHostname) // Ensure the primary region ID is constant. regions2, err := client.Regions(ctx) @@ -149,7 +149,7 @@ func TestRegions(t *testing.T) { require.NotEmpty(t, regions[0].IconURL) require.True(t, regions[0].Healthy) require.Equal(t, client.URL.String(), regions[0].PathAppURL) - require.Equal(t, appHostname, regions[0].WildcardHostname) + require.Equal(t, fmt.Sprintf("%s:%s", appHostname, client.URL.Port()), regions[0].WildcardHostname) // Ensure non-zero fields of the default proxy require.NotZero(t, primary.Name) diff --git a/enterprise/wsproxy/wsproxy_test.go b/enterprise/wsproxy/wsproxy_test.go index 46f00d9752..0d440165df 100644 --- a/enterprise/wsproxy/wsproxy_test.go +++ b/enterprise/wsproxy/wsproxy_test.go @@ -449,10 +449,13 @@ func TestWorkspaceProxyWorkspaceApps(t *testing.T) { <-proxyStatsCollectorFlushDone } + if opts.PrimaryAppHost == "" { + opts.PrimaryAppHost = "*.primary.test.coder.com" + } client, closer, api, user := coderdenttest.NewWithAPI(t, &coderdenttest.Options{ Options: &coderdtest.Options{ DeploymentValues: deploymentValues, - AppHostname: "*.primary.test.coder.com", + AppHostname: opts.PrimaryAppHost, IncludeProvisionerDaemon: true, RealIPConfig: &httpmw.RealIPConfig{ TrustedOrigins: []*net.IPNet{{