From 30960c8325f566b48a5172822cdbb8809dac019b Mon Sep 17 00:00:00 2001 From: Przemko Robakowski Date: Wed, 9 Mar 2022 22:34:47 +0100 Subject: [PATCH 1/5] Allow MFA devices with `/` in names to be deleted --- lib/web/apiserver.go | 2 +- lib/web/apiserver_test.go | 47 +++++++++++++++++++++++++++++++++++++++ lib/web/mfa.go | 2 +- 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index c57ea509fe5a0..6f80a345a56a1 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -383,7 +383,7 @@ func NewHandler(cfg Config, opts ...HandlerOption) (*APIHandler, error) { h.POST("/webapi/mfa/login/begin", h.withLimiter(challengeLimiter, h.mfaLoginBegin)) h.POST("/webapi/mfa/login/finish", httplib.MakeHandler(h.mfaLoginFinish)) h.POST("/webapi/mfa/login/finishsession", httplib.MakeHandler(h.mfaLoginFinishSession)) - h.DELETE("/webapi/mfa/token/:token/devices/:devicename", httplib.MakeHandler(h.deleteMFADeviceWithTokenHandle)) + h.DELETE("/webapi/mfa/token/:token/devices/*devicename", httplib.MakeHandler(h.deleteMFADeviceWithTokenHandle)) h.GET("/webapi/mfa/token/:token/devices", httplib.MakeHandler(h.getMFADevicesWithTokenHandle)) h.POST("/webapi/mfa/token/:token/authenticatechallenge", httplib.MakeHandler(h.createAuthenticateChallengeWithTokenHandle)) h.POST("/webapi/mfa/token/:token/registerchallenge", httplib.MakeHandler(h.createRegisterChallengeWithTokenHandle)) diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index acaa4e7d8107d..2deca812238d8 100644 --- a/lib/web/apiserver_test.go +++ b/lib/web/apiserver_test.go @@ -2223,6 +2223,53 @@ func TestAddMFADevice(t *testing.T) { } } +func TestDeleteMFA(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newWebPack(t, 1) + proxy := env.proxies[0] + pack := proxy.authPack(t, "foo@example.com") + + //setting up client manually because we need sanitizer off + 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.WebAPIVersion, opts...) + require.NoError(t, err) + clt := client.WebClient{Client: rclt} + jar.SetCookies(&proxy.webURL, pack.cookies) + + totpCode, err := totp.GenerateCode(pack.otpSecret, env.clock.Now().Add(30*time.Second)) + require.NoError(t, err) + + // Obtain a privilege token. + endpoint := pack.clt.Endpoint("webapi", "users", "privilege", "token") + re, err := pack.clt.PostJSON(ctx, endpoint, &privilegeTokenRequest{ + SecondFactorToken: totpCode, + }) + + require.NoError(t, err) + var privilegeToken string + require.NoError(t, json.Unmarshal(re.Bytes(), &privilegeToken)) + + names := []string{"x", "??", "%123/", "///", "my/device", "?/%&*1"} + for _, devName := range names { + devName := devName + t.Run(devName, func(t *testing.T) { + t.Parallel() + otpSecret := base32.StdEncoding.EncodeToString([]byte(devName)) + dev, err := services.NewTOTPDevice(devName, otpSecret, env.clock.Now()) + require.NoError(t, err) + err = env.server.Auth().UpsertMFADevice(ctx, pack.user, dev) + require.NoError(t, err) + + enc := url.PathEscape(devName) + _, err = clt.Delete(ctx, pack.clt.Endpoint("webapi", "mfa", "token", privilegeToken, "devices", enc)) + require.NoError(t, err) + }) + } +} + func TestGetMFADevicesWithAuth(t *testing.T) { t.Parallel() env := newWebPack(t, 1) diff --git a/lib/web/mfa.go b/lib/web/mfa.go index 8813bd5ee432d..1f332829aa881 100644 --- a/lib/web/mfa.go +++ b/lib/web/mfa.go @@ -60,7 +60,7 @@ func (h *Handler) getMFADevicesHandle(w http.ResponseWriter, r *http.Request, p func (h *Handler) deleteMFADeviceWithTokenHandle(w http.ResponseWriter, r *http.Request, p httprouter.Params) (interface{}, error) { if err := h.GetProxyClient().DeleteMFADeviceSync(r.Context(), &proto.DeleteMFADeviceSyncRequest{ TokenID: p.ByName("token"), - DeviceName: p.ByName("devicename"), + DeviceName: p.ByName("devicename")[1:], }); err != nil { return nil, trace.Wrap(err) } From 2a11d7ce292041c2948e3d2d82f2440b869cc8f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Skrz=C4=99tnicki?= Date: Fri, 11 Mar 2022 17:12:31 +0100 Subject: [PATCH 2/5] Use forked httprouter with RawPath fix. --- go.mod | 1 + go.sum | 5 ++--- lib/web/apiserver.go | 5 ++++- lib/web/mfa.go | 2 +- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index 59a76b1bf3cae..8d89361088dea 100644 --- a/go.mod +++ b/go.mod @@ -266,6 +266,7 @@ replace ( github.com/go-redis/redis/v8 => github.com/gravitational/redis/v8 v8.11.5-0.20220211010318-7af711b76a91 github.com/gogo/protobuf => github.com/gravitational/protobuf v1.3.2-0.20201123192827-2b9fcfaffcbf github.com/gravitational/teleport/api => ./api + github.com/julienschmidt/httprouter => github.com/halorium/httprouter v0.0.0-20180326174710-cfd8d8d26fd7 github.com/siddontang/go-mysql v1.1.0 => github.com/gravitational/go-mysql v1.1.1-teleport.2 github.com/sirupsen/logrus => github.com/gravitational/logrus v1.4.4-0.20210817004754-047e20245621 github.com/vulcand/predicate => github.com/gravitational/predicate v1.2.1 diff --git a/go.sum b/go.sum index 468cf93640ef3..54cbf4872ed5b 100644 --- a/go.sum +++ b/go.sum @@ -487,6 +487,8 @@ github.com/grpc-ecosystem/go-grpc-middleware v1.0.0/go.mod h1:FiyG127CGDf3tlThmg github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0/go.mod h1:8NvIoxWQoOIhqOTXgfV/d3M/q6VIi02HzZEHgUlZvzk= github.com/grpc-ecosystem/grpc-gateway v1.9.0/go.mod h1:vNeuVxBJEsws4ogUvrchl83t/GYV9WGTSLVdBhOQFDY= github.com/grpc-ecosystem/grpc-gateway v1.16.0/go.mod h1:BDjrQk3hbvj6Nolgz8mAMFbcEtjT1g+wF4CSlocrBnw= +github.com/halorium/httprouter v0.0.0-20180326174710-cfd8d8d26fd7 h1:Dtbw2sVYJ8Aapws/qJCQK7PXDxlKQ8D5gJZ+/tV2zjc= +github.com/halorium/httprouter v0.0.0-20180326174710-cfd8d8d26fd7/go.mod h1:cTM3lbd4sLEbfLy0zvhhUg3m2dT7y+1bLSdarSMhLz4= github.com/hashicorp/consul/api v1.1.0/go.mod h1:VmuI/Lkw1nC05EYQWNKwWGbkg+FbDBtguAZLlVdkD9Q= github.com/hashicorp/consul/sdk v0.1.1/go.mod h1:VKf9jXwCTEY1QZP2MOLRhb5i/I/ssyNV1vwHyQBF0x8= github.com/hashicorp/errwrap v1.0.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4= @@ -590,9 +592,6 @@ github.com/jstemmer/go-junit-report v0.0.0-20190106144839-af01ea7f8024/go.mod h1 github.com/jstemmer/go-junit-report v0.9.1/go.mod h1:Brl9GWCQeLvo8nXZwPNNblvFj/XSXhF0NWZEnDohbsk= github.com/jtolds/gls v4.20.0+incompatible h1:xdiiI2gbIgH/gLH7ADydsJ1uDOEzR8yvV7C0MuV77Wo= github.com/jtolds/gls v4.20.0+incompatible/go.mod h1:QJZ7F/aHp+rZTRtaJ1ow/lLfFfVYBRgL+9YlvaHOwJU= -github.com/julienschmidt/httprouter v1.2.0/go.mod h1:SYymIcj16QtmaHHD7aYtjjsJG7VTCxuUUipMqKk8s4w= -github.com/julienschmidt/httprouter v1.3.0 h1:U0609e9tgbseu3rBINet9P48AI/D3oJs4dN7jwJOQ1U= -github.com/julienschmidt/httprouter v1.3.0/go.mod h1:JR6WtHb+2LUe8TCKY3cZOxFyyO8IZAc4RVcycCCAKdM= github.com/kardianos/osext v0.0.0-20190222173326-2bc1f35cddc0 h1:iQTw/8FWTuc7uiaSepXwyf3o52HaUYcV+Tu66S3F5GA= github.com/kardianos/osext v0.0.0-20190222173326-2bc1f35cddc0/go.mod h1:1NbS8ALrpOvjt0rHPNLyCIeMtbizbir8U//inJ+zuB8= github.com/karrick/godirwalk v1.8.0/go.mod h1:H5KPZjojv4lE+QYImBI8xVtrBRgYrIVsaRPx4tDPEn4= diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index 6f80a345a56a1..cebf2fea0b38a 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -230,6 +230,9 @@ func NewHandler(cfg Config, opts ...HandlerOption) (*APIHandler, error) { ClusterFeatures: cfg.ClusterFeatures, } + // for properly handling url-encoded parameter values. + h.UseRawPath = true + for _, o := range opts { if err := o(h); err != nil { return nil, trace.Wrap(err) @@ -383,7 +386,7 @@ func NewHandler(cfg Config, opts ...HandlerOption) (*APIHandler, error) { h.POST("/webapi/mfa/login/begin", h.withLimiter(challengeLimiter, h.mfaLoginBegin)) h.POST("/webapi/mfa/login/finish", httplib.MakeHandler(h.mfaLoginFinish)) h.POST("/webapi/mfa/login/finishsession", httplib.MakeHandler(h.mfaLoginFinishSession)) - h.DELETE("/webapi/mfa/token/:token/devices/*devicename", httplib.MakeHandler(h.deleteMFADeviceWithTokenHandle)) + h.DELETE("/webapi/mfa/token/:token/devices/:devicename", httplib.MakeHandler(h.deleteMFADeviceWithTokenHandle)) h.GET("/webapi/mfa/token/:token/devices", httplib.MakeHandler(h.getMFADevicesWithTokenHandle)) h.POST("/webapi/mfa/token/:token/authenticatechallenge", httplib.MakeHandler(h.createAuthenticateChallengeWithTokenHandle)) h.POST("/webapi/mfa/token/:token/registerchallenge", httplib.MakeHandler(h.createRegisterChallengeWithTokenHandle)) diff --git a/lib/web/mfa.go b/lib/web/mfa.go index 1f332829aa881..8813bd5ee432d 100644 --- a/lib/web/mfa.go +++ b/lib/web/mfa.go @@ -60,7 +60,7 @@ func (h *Handler) getMFADevicesHandle(w http.ResponseWriter, r *http.Request, p func (h *Handler) deleteMFADeviceWithTokenHandle(w http.ResponseWriter, r *http.Request, p httprouter.Params) (interface{}, error) { if err := h.GetProxyClient().DeleteMFADeviceSync(r.Context(), &proto.DeleteMFADeviceSyncRequest{ TokenID: p.ByName("token"), - DeviceName: p.ByName("devicename")[1:], + DeviceName: p.ByName("devicename"), }); err != nil { return nil, trace.Wrap(err) } From da39a1b6a61cac3bc723e7538c0f037ac71a0047 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Skrz=C4=99tnicki?= Date: Fri, 8 Apr 2022 09:56:34 +0200 Subject: [PATCH 3/5] Target fork with fix: gravitational/httprouter --- go.mod | 4 ++-- go.sum | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index ef75edeb63680..7bf399552c71d 100644 --- a/go.mod +++ b/go.mod @@ -89,6 +89,7 @@ require ( github.com/sirupsen/logrus v1.8.1 github.com/stretchr/testify v1.7.0 github.com/tstranex/u2f v0.0.0-20160508205855-eb799ce68da4 + github.com/ucarion/urlpath v0.0.0-20200424170820-7ccc79b76bbb github.com/vulcand/predicate v1.2.0 go.etcd.io/etcd/api/v3 v3.5.1 go.etcd.io/etcd/client/v3 v3.5.1 @@ -231,7 +232,6 @@ require ( github.com/spf13/cobra v1.2.1 // indirect github.com/spf13/pflag v1.0.5 // indirect github.com/thales-e-security/pool v0.0.2 // indirect - github.com/ucarion/urlpath v0.0.0-20200424170820-7ccc79b76bbb // indirect github.com/x448/float16 v0.8.4 // indirect github.com/xdg-go/pbkdf2 v1.0.0 // indirect github.com/xdg-go/scram v1.0.2 // indirect @@ -272,7 +272,7 @@ replace ( github.com/go-redis/redis/v8 => github.com/gravitational/redis/v8 v8.11.5-0.20220211010318-7af711b76a91 github.com/gogo/protobuf => github.com/gravitational/protobuf v1.3.2-0.20201123192827-2b9fcfaffcbf github.com/gravitational/teleport/api => ./api - github.com/julienschmidt/httprouter => github.com/halorium/httprouter v0.0.0-20180326174710-cfd8d8d26fd7 + github.com/julienschmidt/httprouter => github.com/gravitational/httprouter v1.3.1-0.20220408074523-c876c5e705a5 github.com/keys-pub/go-libfido2 => github.com/gravitational/go-libfido2 v1.5.3-0.20220330170708-36815bbb94b7 github.com/russellhaering/gosaml2 => github.com/gravitational/gosaml2 v0.0.0-20220318224559-f06932032ae2 github.com/siddontang/go-mysql v1.1.0 => github.com/gravitational/go-mysql v1.1.1-teleport.2 diff --git a/go.sum b/go.sum index 7051ae8e9d84a..d10c1a976e92a 100644 --- a/go.sum +++ b/go.sum @@ -504,6 +504,8 @@ github.com/gravitational/go-oidc v0.0.5 h1:kxsCknoOZ+KqIAoYLLdHuQcvcc+SrQlnT7xxI github.com/gravitational/go-oidc v0.0.5/go.mod h1:SevmOUNdOB0aD9BAIgjptZ6oHkKxMZZgA70nwPfgU/w= github.com/gravitational/gosaml2 v0.0.0-20220318224559-f06932032ae2 h1:8z1D1fehTDV20wkiGX+JTnlevvEUeVEh4LCygvOrFBs= github.com/gravitational/gosaml2 v0.0.0-20220318224559-f06932032ae2/go.mod h1:PiLt5KX4EMjlMIq3WLRR/xb5yqhiwtQhGr8wmU0b08M= +github.com/gravitational/httprouter v1.3.1-0.20220408074523-c876c5e705a5 h1:qg8FcGwRACSHortU1UxCSo9nF0t34rPWjk9Nef3j2Ic= +github.com/gravitational/httprouter v1.3.1-0.20220408074523-c876c5e705a5/go.mod h1:JR6WtHb+2LUe8TCKY3cZOxFyyO8IZAc4RVcycCCAKdM= github.com/gravitational/kingpin v2.1.11-0.20190130013101-742f2714c145+incompatible h1:CfyZl3nyo9K5lLqOmqvl9/IElY1UCnOWKZiQxJ8HKdA= github.com/gravitational/kingpin v2.1.11-0.20190130013101-742f2714c145+incompatible/go.mod h1:LWxG30M3FcrjhOn3T4zz7JmBoQJ45MWZmOXgy9Ganoc= github.com/gravitational/license v0.0.0-20210218173955-6d8fb49b117a h1:PN5vAN1ZA0zqdpM6wNdx6+bkdlQ5fImd75oaIHSbOhY= @@ -541,8 +543,6 @@ github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0/go.mod h1:8NvIoxWQoOIhqOTXgf github.com/grpc-ecosystem/grpc-gateway v1.9.0/go.mod h1:vNeuVxBJEsws4ogUvrchl83t/GYV9WGTSLVdBhOQFDY= github.com/grpc-ecosystem/grpc-gateway v1.9.5/go.mod h1:vNeuVxBJEsws4ogUvrchl83t/GYV9WGTSLVdBhOQFDY= github.com/grpc-ecosystem/grpc-gateway v1.16.0/go.mod h1:BDjrQk3hbvj6Nolgz8mAMFbcEtjT1g+wF4CSlocrBnw= -github.com/halorium/httprouter v0.0.0-20180326174710-cfd8d8d26fd7 h1:Dtbw2sVYJ8Aapws/qJCQK7PXDxlKQ8D5gJZ+/tV2zjc= -github.com/halorium/httprouter v0.0.0-20180326174710-cfd8d8d26fd7/go.mod h1:cTM3lbd4sLEbfLy0zvhhUg3m2dT7y+1bLSdarSMhLz4= github.com/hashicorp/consul/api v1.1.0/go.mod h1:VmuI/Lkw1nC05EYQWNKwWGbkg+FbDBtguAZLlVdkD9Q= github.com/hashicorp/consul/api v1.3.0/go.mod h1:MmDNSzIMUjNpY/mQ398R4bk2FnqQLoPndWW5VkKPlCE= github.com/hashicorp/consul/sdk v0.1.1/go.mod h1:VKf9jXwCTEY1QZP2MOLRhb5i/I/ssyNV1vwHyQBF0x8= From cbf4ffd88fbea9dac5a43aed4064b7250d5ef33c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Skrz=C4=99tnicki?= Date: Fri, 8 Apr 2022 10:15:01 +0200 Subject: [PATCH 4/5] Enable UseRawPath everywhere. --- lib/auth/apiserver.go | 1 + lib/httplib/httplib_test.go | 1 + lib/kube/proxy/forwarder.go | 2 ++ lib/web/app/handler.go | 1 + 4 files changed, 5 insertions(+) diff --git a/lib/auth/apiserver.go b/lib/auth/apiserver.go index d9ee0df2029ad..1ad9787335915 100644 --- a/lib/auth/apiserver.go +++ b/lib/auth/apiserver.go @@ -88,6 +88,7 @@ func NewAPIServer(config *APIConfig) (http.Handler, error) { Clock: clockwork.NewRealClock(), } srv.Router = *httprouter.New() + srv.Router.UseRawPath = true // Kubernetes extensions srv.POST("/:version/kube/csr", srv.withAuth(srv.processKubeCSR)) diff --git a/lib/httplib/httplib_test.go b/lib/httplib/httplib_test.go index 9b7ff1e7c70e8..ce69f6d91a71c 100644 --- a/lib/httplib/httplib_test.go +++ b/lib/httplib/httplib_test.go @@ -62,6 +62,7 @@ type testHandler struct { func newTestHandler() *testHandler { h := &testHandler{} h.Router = *httprouter.New() + h.Router.UseRawPath = true h.POST("/v1/sessions/:id/stream", MakeHandler(h.postSessionChunkOriginal)) h.POST("/v1/namespaces/:namespace/sessions/:id/stream", MakeHandler(h.postSessionChunkNamespace)) return h diff --git a/lib/kube/proxy/forwarder.go b/lib/kube/proxy/forwarder.go index 60d313f9406ea..f61f72bb71a7d 100644 --- a/lib/kube/proxy/forwarder.go +++ b/lib/kube/proxy/forwarder.go @@ -245,6 +245,8 @@ func NewForwarder(cfg ForwarderConfig) (*Forwarder, error) { }, } + fwd.router.UseRawPath = true + fwd.router.POST("/api/:ver/namespaces/:podNamespace/pods/:podName/exec", fwd.withAuth(fwd.exec)) fwd.router.GET("/api/:ver/namespaces/:podNamespace/pods/:podName/exec", fwd.withAuth(fwd.exec)) diff --git a/lib/web/app/handler.go b/lib/web/app/handler.go index a641d858dbff5..3208247959985 100644 --- a/lib/web/app/handler.go +++ b/lib/web/app/handler.go @@ -122,6 +122,7 @@ func NewHandler(ctx context.Context, c *HandlerConfig) (*Handler, error) { // Create the application routes. h.router = httprouter.New() + h.router.UseRawPath = true h.router.GET("/x-teleport-auth", makeRouterHandler(h.handleFragment)) h.router.POST("/x-teleport-auth", makeRouterHandler(h.handleFragment)) h.router.GET("/teleport-logout", h.withRouterAuth(h.handleLogout)) From 8edf5c8a45f9ed89ed4b3c303b97d876eaf245be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Skrz=C4=99tnicki?= Date: Tue, 12 Apr 2022 10:09:02 +0200 Subject: [PATCH 5/5] Formatting change. --- lib/web/apiserver_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index 58ab3fbcdbc50..b0f3c5447e25c 100644 --- a/lib/web/apiserver_test.go +++ b/lib/web/apiserver_test.go @@ -2310,8 +2310,8 @@ func TestDeleteMFA(t *testing.T) { re, err := pack.clt.PostJSON(ctx, endpoint, &privilegeTokenRequest{ SecondFactorToken: totpCode, }) - require.NoError(t, err) + var privilegeToken string require.NoError(t, json.Unmarshal(re.Bytes(), &privilegeToken))