search

gravitational / teleport

The easiest, and most secure way to access and protect all of your infrastructure.
https://goteleport.com
GNU Affero General Public License v3.0
17.2k stars 1.73k forks source link

no issuer in SP xml results in `invalid memory address or nil pointer dereference` #32804

Open programmerq opened 10 months ago

programmerq commented 10 months ago

Expected behavior:

Gracefully handle unexpected XML errors in AuthnRequest document.

Similar errors that are caught will result in a Bad Request response, combined with a log message about what went wrong.

Current behavior:

When onboarding an external application to work with Teleport's SAML idp feature, I encountered an ERR_EMPTY_RESPONSE error on the POST to the /enterprise/saml-idp/sso endpoint.

The Teleport proxy that handled the request logged this message when the error occurred:

teleport-proxy-5b77f77695-wnx8v teleport 2023-09-28T16:28:11Z ERRO [PROXY:SER] "proxy2023/09/28 16:28:11 http: panic serving 10.42.2.0:29576: runtime error: invalid memory address or nil pointer dereference\ngoroutine 873416 [running]:\nnet/http.(*conn).serve.func1()\n\tnet/http/server.go:1854 +0xbf\npanic({0x765ada0, 0xe167da0})\n\truntime/panic.go:890 +0x263\ngithub.com/crewjam/saml.(*IdpAuthnRequest).Validate(0xc00152b6c0)\n\tgithub.com/crewjam/saml@v0.4.14-0.20230420111643-34930b26d33b/identity_provider.go:444 +0x3a6\ngithub.com/crewjam/saml.(*IdentityProvider).ServeSSO(0xc001508500, {0x9bf6a20, 0xc0018f4240}, 0x0?)\n\tgithub.com/crewjam/saml@v0.4.14-0.20230420111643-34930b26d33b/identity_provider.go:234 +0x11f\ngithub.com/gravitational/teleport/e/lib/idp/saml.(*Service).handleSSO(0xc0015f5c00, {0x9bf6a20, 0xc0018f4240}, 0xc001d6c400, {0xf8?, 0x8362460?, 0x9b47601?})\n\tgithub.com/gravitational/teleport/e/lib/idp/saml/handler.go:144 +0x265\ngithub.com/gravitational/teleport/e/lib/idp/saml.(*Service).withAuthCtx.func1({0x9bf6a20, 0xc0018f4240}, 0xc001d6c200, {0x0, 0x0, 0x0})\n\tgithub.com/gravitational/teleport/e/lib/idp/saml/handler.go:89 +0x1e9\ngithub.com/julienschmidt/httprouter.(*Router).ServeHTTP(0xc001a62f00, {0x9bf6a20, 0xc0018f4240}, 0xc001d6c200)\n\tgithub.com/julienschmidt/httprouter@v1.3.0/router.go:399 +0x85c\ngithub.com/gravitational/teleport/e/lib/idp/saml.(*Service).ServeHTTP(0x0?, {0x9bf6a20?, 0xc0018f4240?}, 0xc001d6c200?)\n\tgithub.com/gravitational/teleport/e/lib/idp/saml/handler.go:53 +0xfd\ngithub.com/gravitational/teleport/e/lib/web.(*Plugin).withSAMLAuth.func1({0x9bf6a20, 0xc0018f4240}, 0xc001d6c100, {0x4?, 0xc002dcdae9?, 0x7a3d620?})\n\tgithub.com/gravitational/teleport/e/lib/web/plugin.go:313 +0x3bd\ngithub.com/gravitational/teleport/lib/httplib.MakeHandlerWithErrorWriter.func1({0x9bf6a20, 0xc0018f4240}, 0xc002dcdad0?, {0xc001531400, 0x1, 0x1})\n\tgithub.com/gravitational/teleport/lib/httplib/httplib.go:117 +0x9e\ngithub.com/julienschmidt/httprouter.(*Router).ServeHTTP(0xc00295d018, {0x9bf6a20, 0xc0018f4240}, 0xc001d6c100)\n\tgithub.com/julienschmidt/httprouter@v1.3.0/router.go:399 +0x85c\ngithub.com/gravitational/teleport/lib/web.(*APIHandler).ServeHTTP(0xc001964ee0, {0x9bf6a20, 0xc0018f4240}, 0xc001d6c100)\n\tgithub.com/gravitational/teleport/lib/web/apiserver.go:302 +0x10b\ngithub.com/gravitational/oxy/ratelimit.(*TokenLimiter).ServeHTTP(0xc002540b80, {0x9bf6a20, 0xc0018f4240}, 0xc001d6c100)\n\tgithub.com/gravitational/oxy@v0.0.0-20221029012416-9fbf4c444680/ratelimit/tokenlimiter.go:118 +0x1ce\ngithub.com/gravitational/oxy/connlimit.(*ConnLimiter).ServeHTTP(0xc0026df080, {0x9bf6a20, 0xc0018f4240}, 0xc003d66e10?)\n\tgithub.com/gravitational/oxy@v0.0.0-20221029012416-9fbf4c444680/connlimit/connlimit.go:75 +0x31d\ngithub.com/gravitational/teleport/lib/httplib.MakeTracingHandler.func1({0x9bf6a20, 0xc0018f4240}, 0xc001d6c100)\n\tgithub.com/gravitational/teleport/lib/httplib/httplib.go:94 +0x1ad\nnet/http.HandlerFunc.ServeHTTP(0x9bf7728?, {0x9bf6a20?, 0xc0018f4240?}, 0x9b42858?)\n\tnet/http/server.go:2122 +0x2f\ngo.opentelemetry.io/contrib/instrumentation/net/http/otelhttp.(*Handler).ServeHTTP(0xc00281c9c0, {0x9bf4440?, 0xc001d68620}, 0xc001d6c000)\n\tgo.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.40.0/handler.go:213 +0x1231\nnet/http.serverHandler.ServeHTTP({0x9bc83d8?}, {0x9bf4440, 0xc001d68620}, 0xc001d6c000)\n\tnet/http/server.go:2936 +0x316\nnet/http.(*conn).serve(0xc002023830, {0x9bf7728, 0xc003d66c30})\n\tnet/http/server.go:1995 +0x612\ncreated by net/http.(*Server).Serve\n\tnet/http/server.go:3089 +0x5ed\n" pid:6.1 utils/cli.go:417
Click for unescaped stacktrace ``` proxy2023/09/28 16:28:11 http: panic serving 10.42.2.0:29576: runtime error: invalid memory address or nil pointer dereference goroutine 873416 [running]: net/http.(*conn).serve.func1() net/http/server.go:1854 +0xbf panic({0x765ada0, 0xe167da0}) runtime/panic.go:890 +0x263 github.com/crewjam/saml.(*IdpAuthnRequest).Validate(0xc00152b6c0) github.com/crewjam/saml@v0.4.14-0.20230420111643-34930b26d33b/identity_provider.go:444 +0x3a6 github.com/crewjam/saml.(*IdentityProvider).ServeSSO(0xc001508500, {0x9bf6a20, 0xc0018f4240}, 0x0?) github.com/crewjam/saml@v0.4.14-0.20230420111643-34930b26d33b/identity_provider.go:234 +0x11f github.com/gravitational/teleport/e/lib/idp/saml.(*Service).handleSSO(0xc0015f5c00, {0x9bf6a20, 0xc0018f4240}, 0xc001d6c400, {0xf8?, 0x8362460?, 0x9b47601?}) github.com/gravitational/teleport/e/lib/idp/saml/handler.go:144 +0x265 github.com/gravitational/teleport/e/lib/idp/saml.(*Service).withAuthCtx.func1({0x9bf6a20, 0xc0018f4240}, 0xc001d6c200, {0x0, 0x0, 0x0}) github.com/gravitational/teleport/e/lib/idp/saml/handler.go:89 +0x1e9 github.com/julienschmidt/httprouter.(*Router).ServeHTTP(0xc001a62f00, {0x9bf6a20, 0xc0018f4240}, 0xc001d6c200) github.com/julienschmidt/httprouter@v1.3.0/router.go:399 +0x85c github.com/gravitational/teleport/e/lib/idp/saml.(*Service).ServeHTTP(0x0?, {0x9bf6a20?, 0xc0018f4240?}, 0xc001d6c200?) github.com/gravitational/teleport/e/lib/idp/saml/handler.go:53 +0xfd github.com/gravitational/teleport/e/lib/web.(*Plugin).withSAMLAuth.func1({0x9bf6a20, 0xc0018f4240}, 0xc001d6c100, {0x4?, 0xc002dcdae9?, 0x7a3d620?}) github.com/gravitational/teleport/e/lib/web/plugin.go:313 +0x3bd github.com/gravitational/teleport/lib/httplib.MakeHandlerWithErrorWriter.func1({0x9bf6a20, 0xc0018f4240}, 0xc002dcdad0?, {0xc001531400, 0x1, 0x1}) github.com/gravitational/teleport/lib/httplib/httplib.go:117 +0x9e github.com/julienschmidt/httprouter.(*Router).ServeHTTP(0xc00295d018, {0x9bf6a20, 0xc0018f4240}, 0xc001d6c100) github.com/julienschmidt/httprouter@v1.3.0/router.go:399 +0x85c github.com/gravitational/teleport/lib/web.(*APIHandler).ServeHTTP(0xc001964ee0, {0x9bf6a20, 0xc0018f4240}, 0xc001d6c100) github.com/gravitational/teleport/lib/web/apiserver.go:302 +0x10b github.com/gravitational/oxy/ratelimit.(*TokenLimiter).ServeHTTP(0xc002540b80, {0x9bf6a20, 0xc0018f4240}, 0xc001d6c100) github.com/gravitational/oxy@v0.0.0-20221029012416-9fbf4c444680/ratelimit/tokenlimiter.go:118 +0x1ce github.com/gravitational/oxy/connlimit.(*ConnLimiter).ServeHTTP(0xc0026df080, {0x9bf6a20, 0xc0018f4240}, 0xc003d66e10?) github.com/gravitational/oxy@v0.0.0-20221029012416-9fbf4c444680/connlimit/connlimit.go:75 +0x31d github.com/gravitational/teleport/lib/httplib.MakeTracingHandler.func1({0x9bf6a20, 0xc0018f4240}, 0xc001d6c100) github.com/gravitational/teleport/lib/httplib/httplib.go:94 +0x1ad net/http.HandlerFunc.ServeHTTP(0x9bf7728?, {0x9bf6a20?, 0xc0018f4240?}, 0x9b42858?) net/http/server.go:2122 +0x2f go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp.(*Handler).ServeHTTP(0xc00281c9c0, {0x9bf4440?, 0xc001d68620}, 0xc001d6c000) go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.40.0/handler.go:213 +0x1231 net/http.serverHandler.ServeHTTP({0x9bc83d8?}, {0x9bf4440, 0xc001d68620}, 0xc001d6c000) net/http/server.go:2936 +0x316 net/http.(*conn).serve(0xc002023830, {0x9bf7728, 0xc003d66c30}) net/http/server.go:1995 +0x612 created by net/http.(*Server).Serve net/http/server.go:3089 +0x5ed ```

This happened every time I tried to log in.

It turns out that the SP had not been configured to include the service provider Issuer in the AuthnRequest XML that was sent in the POST to the teleport endpoint.

<AuthnRequest xmlns="urn:oasis:names:tc:SAML:2.0:protocol" ID="pcnh6xvvhf7uqy5inxdcrxb4v" Version="2.0" IssueInstant="2023-09-28T16:28:11Z" Destination="https://teleport.example.com/enterprise/saml-idp/sso" ProtocolBinding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" AssertionConsumerServiceURL="https://argo.example.internal/api/dex/callback">
    <NameIDPolicy xmlns="urn:oasis:names:tc:SAML:2.0:protocol" AllowCreate="true" Format="urn:oasis:names:tc:SAML:2.0:nameid-format:persistent"></NameIDPolicy>
</AuthnRequest>

In the crewjam/saml/identity_provider.go file mentioned in the stacktrace, it appears to expect that field to be present. I was able to adjust the SP configuration to include that in its XML AuthnRequest document, and clear the error.

I am still reporting this because invalid memory address or nil pointer dereference errors should be handled.

Bug details:

The app in question was ArgoCD. They use the dex project to implement their SAML SP support. The dex docs say "Optional: Manually specify dex's Issuer value." about the entityIssuer field that I needed to add to get the XML to include the Issuer tag that was expected.

dex config file for connector that caused the issue ``` connectors: - type: saml id: teleport name: Teleport config: ssoURL: https://teleport.example.com/enterprise/saml-idp/sso # You need `caData` _OR_ `ca`, but not both. #caData: # You need `caData` _OR_ `ca`, but not both. # Path to mount the secret to the dex container ca: /path/to/ca.pem redirectURI: https://argo.example.internal/api/dex/callback usernameAttr: urn:oid:0.9.2342.19200300.100.1.1 emailAttr: urn:oid:0.9.2342.19200300.100.1.1 groupsAttr: urn:oid:1.3.6.1.4.1.5923.1.1.1.1 nameIDPolicyFormat: transient ```
dex config with the issuer correctly set ``` connectors: - type: saml id: teleport name: Teleport config: ssoURL: https://teleport.example.com/enterprise/saml-idp/sso # You need `caData` _OR_ `ca`, but not both. #caData: # You need `caData` _OR_ `ca`, but not both. # Path to mount the secret to the dex container ca: /path/to/ca.pem redirectURI: https://argo.example.internal/api/dex/callback entityIssuer: https://argo.example.internal/api/dex/callback usernameAttr: urn:oid:0.9.2342.19200300.100.1.1 emailAttr: urn:oid:0.9.2342.19200300.100.1.1 groupsAttr: urn:oid:1.3.6.1.4.1.5923.1.1.1.1 nameIDPolicyFormat: transient ```
mdwn commented 10 months ago

@programmerq Does this crash the proxy, or does the proxy persist after hitting this error?

programmerq commented 10 months ago

@programmerq Does this crash the proxy, or does the proxy persist after hitting this error?

The proxy doesn't crash.

zmb3 commented 8 months ago

@mdwn what ever happened with this one? Do we still have a known panic that needs fixing?

mdwn commented 8 months ago

This needs to be fixed upstream. I started a PR for it and got sidetracked by access list stuff. Given that this doesn't actually crash the parent process, I figured this wasn't as scary as it sounds, so it kind of fell by the wayside for now. I can reprioritize if need be.

oliverfrye commented 8 months ago

I think I just hit this exact issue when setting up my Teleport Enterprise Cloud with ArgoCD. It took me an hour or two to work out what I'd done wrong since I didn't have the benefit of being able to see the logs. I didn't even get a 500 error back from Teleport - the connection is just closed immediately, which is really hard to debug.