From c6d7694dcc462f943878a042d584efed1fe59eec Mon Sep 17 00:00:00 2001 From: Lisa Kim Date: Thu, 2 Jan 2025 10:17:13 -0800 Subject: [PATCH] Lax regex and remove prefix versioning on new web clients --- constants.go | 3 --- lib/auth/trustedcluster.go | 2 +- lib/client/https_client.go | 3 +-- lib/client/weblogin_test.go | 2 +- lib/httplib/httplib.go | 16 ++++++++-------- lib/web/apiserver.go | 24 ++++++++++++++++-------- lib/web/apiserver_test.go | 17 ++++++++++++++++- 7 files changed, 43 insertions(+), 24 deletions(-) diff --git a/constants.go b/constants.go index 39754ec6ca149..79f97ae24bfaf 100644 --- a/constants.go +++ b/constants.go @@ -25,9 +25,6 @@ import ( "github.com/gravitational/trace" ) -// WebAPIVersionOne is a current webapi version -const WebAPIVersionOne = "v1" - const ( // SSHAuthSock is the environment variable pointing to the // Unix socket the SSH agent is running on. diff --git a/lib/auth/trustedcluster.go b/lib/auth/trustedcluster.go index 5ae0ad522e331..869dad81f12ec 100644 --- a/lib/auth/trustedcluster.go +++ b/lib/auth/trustedcluster.go @@ -673,7 +673,7 @@ func (a *Server) sendValidateRequestToProxy(host string, validateRequest *authcl opts = append(opts, roundtrip.HTTPClient(insecureWebClient)) } - clt, err := roundtrip.NewClient(proxyAddr.String(), teleport.WebAPIVersionOne, opts...) + clt, err := roundtrip.NewClient(proxyAddr.String(), "", opts...) if err != nil { return nil, trace.Wrap(err) } diff --git a/lib/client/https_client.go b/lib/client/https_client.go index e707dfea9c0f6..11e263a6d519e 100644 --- a/lib/client/https_client.go +++ b/lib/client/https_client.go @@ -28,7 +28,6 @@ import ( "github.com/gravitational/trace" "golang.org/x/net/http/httpproxy" - "github.com/gravitational/teleport" tracehttp "github.com/gravitational/teleport/api/observability/tracing/http" apiutils "github.com/gravitational/teleport/api/utils" "github.com/gravitational/teleport/lib/httplib" @@ -62,7 +61,7 @@ func httpTransport(insecure bool, pool *x509.CertPool) *http.Transport { func NewWebClient(url string, opts ...roundtrip.ClientParam) (*WebClient, error) { opts = append(opts, roundtrip.SanitizerEnabled(true)) - clt, err := roundtrip.NewClient(url, teleport.WebAPIVersionOne, opts...) + clt, err := roundtrip.NewClient(url, "", opts...) if err != nil { return nil, trace.Wrap(err) } diff --git a/lib/client/weblogin_test.go b/lib/client/weblogin_test.go index cca05b892fe2b..1008308411d50 100644 --- a/lib/client/weblogin_test.go +++ b/lib/client/weblogin_test.go @@ -74,7 +74,7 @@ func TestHostCredentialsHttpFallback(t *testing.T) { // Start an http server (not https) so that the request only succeeds // if the fallback occurs. var handler http.HandlerFunc = func(w http.ResponseWriter, r *http.Request) { - if r.RequestURI != "/v1/webapi/host/credentials" { + if r.RequestURI != "/webapi/host/credentials" { w.WriteHeader(http.StatusNotFound) return } diff --git a/lib/httplib/httplib.go b/lib/httplib/httplib.go index a0439b4e5dcd2..8b241584817da 100644 --- a/lib/httplib/httplib.go +++ b/lib/httplib/httplib.go @@ -223,24 +223,24 @@ type Version struct { func ReplyRouteNotFoundJSONWithVersionField(w http.ResponseWriter, versionStr string) { SetDefaultSecurityHeaders(w.Header()) + errObj := &trace.TraceErr{ + Err: trace.NotFound("path not found"), + } + ver, err := semver.NewVersion(versionStr) - verObj := Version{} if err == nil { - verObj = Version{ + verObj := Version{ Major: ver.Major, Minor: ver.Minor, Patch: ver.Patch, String: versionStr, PreRelease: string(ver.PreRelease), } + fields := make(map[string]interface{}) + fields["proxyVersion"] = verObj + errObj.Fields = fields } - fields := make(map[string]interface{}) - fields["proxyVersion"] = verObj - errObj := &trace.TraceErr{ - Err: trace.NotFound("path not found"), - Fields: fields, - } roundtrip.ReplyJSON(w, http.StatusNotFound, errObj) } diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index dae12e3c42b0c..71bb76af586c9 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -109,8 +109,8 @@ import ( "github.com/gravitational/teleport/lib/web/ui" ) -// apiPrefixRegex matches pathname starting with /vN/webapi or /vN/enterprise -var apiPrefixRegex = regexp.MustCompile(`^/v(\d+)/(webapi|enterprise)`) +// apiPrefixRegex matches pathnames starting with /v/ +var apiPrefixRegex = regexp.MustCompile(`^/v(\d+)/(.+)`) const ( // SSOLoginFailureMessage is a generic error message to avoid disclosing sensitive SSO failure messages. @@ -616,13 +616,21 @@ func NewHandler(cfg Config, opts ...HandlerOption) (*APIHandler, error) { notFoundRoutingHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // Request is going to the API? - // If no routes were matched, it could be because certain paths don't expect any version prefixes. - // Only prefix "v1" will be stripped to preserve the legacy behavior before introducing v1 + N. - if matches := apiPrefixRegex.FindStringSubmatch(r.URL.Path); matches != nil && len(matches) > 1 { + // If no routes were matched, it could be because it's a path with `v1` prefix + // (eg: the Teleport web app will call "most" endpoints with v1 prefixed). + // + // `v1` paths are not defined with `v1` prefix. If the path turns out to be prefixed + // with `v1`, it will be stripped and served again. Historically, that's how it started + // and should be kept that way to prevent breakage. + // + // v2+ prefixes will be expected by both caller and definition and will not be stripped. + if matches := apiPrefixRegex.FindStringSubmatch(r.URL.Path); matches != nil && len(matches) == 3 { + postVersionPrefixPath := fmt.Sprintf("/%s", matches[2]) versionNum := matches[1] - if versionNum == "1" { - // Try path matching again with the stripped prefix. - http.StripPrefix("/"+teleport.WebAPIVersionOne, h).ServeHTTP(w, r) + + // Regex check the rest of path to ensure we aren't allowing paths like /v1/v2/webapi + if matches := apiPrefixRegex.FindStringSubmatch(postVersionPrefixPath); matches == nil && versionNum == "1" { + http.StripPrefix("/v1", h).ServeHTTP(w, r) return } httplib.ReplyRouteNotFoundJSONWithVersionField(w, teleport.Version) diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index 375440b8797c3..e1f1e87e4fcfe 100644 --- a/lib/web/apiserver_test.go +++ b/lib/web/apiserver_test.go @@ -3484,6 +3484,21 @@ func TestEndpointNotFoundHandling(t *testing.T) { endpoint: "v1/something/else", shouldErr: true, }, + { + name: "invalid triple version prefixes", + endpoint: "v1/v1/v1/webapi/token", + shouldErr: true, + }, + { + name: "invalid just prefix", + endpoint: "v1/", + shouldErr: true, + }, + { + name: "invalid prefix", + endpoint: "v1s/webapi/token", + shouldErr: true, + }, } for _, tc := range tt { @@ -5110,7 +5125,7 @@ func TestDeleteMFA(t *testing.T) { jar, err := cookiejar.New(nil) require.NoError(t, err) opts := []roundtrip.ClientParam{roundtrip.BearerAuth(pack.session.Token), roundtrip.CookieJar(jar), roundtrip.HTTPClient(client.NewInsecureWebClient())} - rclt, err := roundtrip.NewClient(proxy.webURL.String(), teleport.WebAPIVersionOne, opts...) + rclt, err := roundtrip.NewClient(proxy.webURL.String(), "", opts...) require.NoError(t, err) clt := client.WebClient{Client: rclt} jar.SetCookies(&proxy.webURL, pack.cookies)