From 0d46e050837375f94d65ef3999073d8a6b904ed3 Mon Sep 17 00:00:00 2001 From: Sid Date: Thu, 28 Mar 2024 09:14:59 +0100 Subject: [PATCH] Render user friendly UI for unhandled auth errors (#173959) ## Summary Closes #171040 ## Fixes - Added a specific check for 500 errors when auth fails with OIDC/SAML - Renders unauthenticated page with the redirect set for `/` ## Steps to test There are multiple ways to test the scenarios for rendering the unauthenticated use case. ### Use existing testing configs to run Kibana with different configurations: Changes added to the test suite here: [x-pack/test/security_functional/plugins/test_endpoints/server/init_routes.ts](https://github.com/elastic/kibana/pull/173959/files#diff-d4d10bb4dd30278eac5887d8be2ce2a9638d7741209be2ece7c0600e422175fd) assist in the testing of the scenarios. 1. Login selector is enabled ``` node scripts/functional_tests_server.js --config x-pack/test/security_functional/login_selector.config.ts ``` 2. Login selector is disabled, login page is available, but not default == we ignore the existence of /login UI ``` node scripts/functional_tests_server.js --config x-pack/test/security_api_integration/saml.config.ts ``` 3. Login selector is not enabled, but login page is available and default ``` node scripts/functional_tests_server.js --config x-pack/test/security_api_integration/http_bearer.config.ts ``` Now, when you run Kibana in any of these test configurations you must first navigate directly to http://localhost:5620/login to log in as elastic user and then access the following URLs and see how the code behaves ``` http://localhost:5620/authentication/app/not_auth_flow http://localhost:5620/authentication/app/not_auth_flow?statusCode=400 http://localhost:5620/authentication/app/not_auth_flow?statusCode=401 http://localhost:5620/authentication/app/not_auth_flow?statusCode=500 # Auth flow routes - part of the auth flow http://localhost:5620/authentication/app/auth_flow http://localhost:5620/authentication/app/auth_flow?statusCode=400 http://localhost:5620/authentication/app/auth_flow?statusCode=401 http://localhost:5620/authentication/app/auth_flow?statusCode=500 ``` ### Run kibana as usual for testing To test this by running kibana locally, you'll first need to set up your local instance to run with SAML - Login to https://oktanaut.app.elastic.dev/ - Fill in your local endpoint - Set Stack version 8.0+ and platform to Other Then with the cofigs given to you from there, run ES as follows: ``` yarn es snapshot --license trial \ ... your config ``` and then run KBN as ``` yarn start --no-base-path \ -xpack.security.authc.selector.enabled=false ... rest of the config from Oktanaut goes here ``` This will start up your local stack pointing to SAML. At this point, we need to force an error to test this flow. There are a few ways to do that: #### Option A - Go to the SAML provider in [`/x-pack/plugins/security/server/authentication/providers/saml.ts`](https://github.com/elastic/kibana/blob/main/x-pack/plugins/security/server/authentication/providers/saml.ts) - Replace [`ids: !isIdPInitiatedLogin ? [stateRequestId] : [],` with `ids: []`](https://github.com/elastic/kibana/blob/main/x-pack/plugins/security/server/authentication/providers/saml.ts#L372) - Comment out the line [`...(providerRealm ? { realm: providerRealm } : {})`](https://github.com/elastic/kibana/blob/main/x-pack/plugins/security/server/authentication/providers/saml.ts#L374) #### Option B You could also do the following to reproduce the error on main: - Open kibana in a private browser window. - Then start the login flow which redirects you to okta. - Before entering your password, switch off the ES instance running locally. - Then login which should give you a redirect back to the saml/callback page with an error JSON. In the PR, this should give you the unauthenticated screen #### Option C Force one (or both) of the Kibana (SAML/OIDC) routes to throw a custom error For example, in `x-pack/plugins/security/server/routes/authentication/saml.ts:L64`, add the following code snippet ```ts return response.customError({ statusCode: 500 }); ``` Now running through the SAML login flow on main should show a JSON error string whereas this PR should show an unauthenticated error page. This sends a bad request to the SAML provider which results in the error which should now show you an Unauthenticated page. ## Release Notes: - Renders a user-friendly UI for unhandled login failures. https://github.com/elastic/kibana/issues/171040 --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> --- .../authentication_service.test.ts | 119 +++++++++++++++++- .../authentication/authentication_service.ts | 32 +++-- x-pack/plugins/security/server/index.ts | 2 +- .../tests/http_bearer/access_token.ts | 50 ++++++++ .../tests/saml/saml_login.ts | 56 +++++++++ .../test_endpoints/server/init_routes.ts | 27 ++++ .../login_selector/basic_functionality.ts | 24 ++++ 7 files changed, 299 insertions(+), 11 deletions(-) diff --git a/x-pack/plugins/security/server/authentication/authentication_service.test.ts b/x-pack/plugins/security/server/authentication/authentication_service.test.ts index 0572c6c804d22..721e0a82874f8 100644 --- a/x-pack/plugins/security/server/authentication/authentication_service.test.ts +++ b/x-pack/plugins/security/server/authentication/authentication_service.test.ts @@ -966,7 +966,6 @@ describe('AuthenticationService', () => { const { authenticator, onPreResponseHandler } = getService(); authenticator.getRequestOriginalURL.mockReturnValue('/mock-server-basepath/app/some'); mockCanRedirectRequest.mockReturnValue(true); - await expect( onPreResponseHandler( httpServerMock.createKibanaRequest({ path: '/app/some', query: { param: 'one two' } }), @@ -1056,5 +1055,123 @@ describe('AuthenticationService', () => { }); }); }); + + describe('handles unexpected post-authentication failures', () => { + let mockReturnedValue: { type: any; body: string }; + let mockOnPreResponseToolkit: jest.Mocked; + beforeEach(() => { + mockReturnedValue = { type: 'render' as any, body: 'body' }; + mockOnPreResponseToolkit = httpServiceMock.createOnPreResponseToolkit(); + mockOnPreResponseToolkit.render.mockReturnValue(mockReturnedValue); + }); + + it('when login page is unavailable', async () => { + mockSetupAuthenticationParams.config = createConfig( + ConfigSchema.validate({ + authc: { + providers: { saml: { saml1: { order: 150, realm: 'saml1' } } }, + }, + }), + loggingSystemMock.create().get(), + { isTLSEnabled: false } + ); + const mockRenderUnauthorizedPage = jest + .requireMock('./unauthenticated_page') + .renderUnauthenticatedPage.mockReturnValue('rendered-view'); + + const { authenticator, onPreResponseHandler } = getService(); + authenticator.getRequestOriginalURL.mockReturnValue('/mock-server-basepath/app/some'); + mockCanRedirectRequest.mockReturnValue(true); + + await expect( + onPreResponseHandler( + httpServerMock.createKibanaRequest({ + path: '/app/some', + query: { param: 'one two' }, + routeTags: [ROUTE_TAG_AUTH_FLOW], + }), + { statusCode: 500 }, + mockOnPreResponseToolkit + ) + ).resolves.toBe(mockReturnedValue); + + expect(mockOnPreResponseToolkit.render).toHaveBeenCalledWith({ + body: 'rendered-view', + headers: { + 'Content-Security-Policy': CspConfig.DEFAULT.header, + }, + }); + + expect(mockRenderUnauthorizedPage).toHaveBeenCalledWith({ + basePath: mockSetupAuthenticationParams.http.basePath, + staticAssets: expect.any(Object), + originalURL: '/mock-server-basepath/', + customBranding: undefined, + }); + }); + + it('when login page is available', async () => { + mockSetupAuthenticationParams.config = createConfig( + ConfigSchema.validate({ + authc: { + selector: { enabled: true }, + }, + }), + loggingSystemMock.create().get(), + { isTLSEnabled: false } + ); + const { authenticator, onPreResponseHandler } = getService(); + authenticator.getRequestOriginalURL.mockReturnValue('/mock-server-basepath/app/some'); + mockCanRedirectRequest.mockReturnValue(true); + + await expect( + onPreResponseHandler( + httpServerMock.createKibanaRequest({ + path: '/app/some', + query: { param: 'one two' }, + routeTags: [ROUTE_TAG_AUTH_FLOW], + }), + { statusCode: 500 }, + mockOnPreResponseToolkit + ) + ).resolves.toBe(mockReturnedValue); + + expect(mockOnPreResponseToolkit.render).toHaveBeenCalledWith({ + body: '
', + headers: { + 'Content-Security-Policy': CspConfig.DEFAULT.header, + Refresh: + '0;url=/mock-server-basepath/login?msg=UNAUTHENTICATED&next=%2Fmock-server-basepath%2F', + }, + }); + }); + + it('when logout redirect fails', async () => { + const { authenticator, onPreResponseHandler } = getService(); + authenticator.getRequestOriginalURL.mockReturnValue('/mock-server-basepath/app/some'); + mockCanRedirectRequest.mockReturnValue(true); + const mockRenderUnauthorizedPage = jest + .requireMock('./unauthenticated_page') + .renderUnauthenticatedPage.mockReturnValue('rendered-view'); + + await expect( + onPreResponseHandler( + httpServerMock.createKibanaRequest({ + path: '/api/security/logout', + routeTags: [ROUTE_TAG_AUTH_FLOW], + }), + { statusCode: 500 }, + mockOnPreResponseToolkit + ) + ).resolves.toBe(mockReturnedValue); + + expect(mockRenderUnauthorizedPage).toHaveBeenCalledWith({ + basePath: mockSetupAuthenticationParams.http.basePath, + staticAssets: expect.any(Object), + originalURL: '/mock-server-basepath/', + customBranding: undefined, + }); + }); + }); }); }); diff --git a/x-pack/plugins/security/server/authentication/authentication_service.ts b/x-pack/plugins/security/server/authentication/authentication_service.ts index 8a40e59480dc2..299ca2d7d3bc5 100644 --- a/x-pack/plugins/security/server/authentication/authentication_service.ts +++ b/x-pack/plugins/security/server/authentication/authentication_service.ts @@ -187,30 +187,44 @@ export class AuthenticationService { }); http.registerOnPreResponse(async (request, preResponse, toolkit) => { - if (preResponse.statusCode !== 401 || !canRedirectRequest(request)) { - return toolkit.next(); - } - if (!this.authenticator) { // Core doesn't allow returning error here. this.logger.error('Authentication sub-system is not fully initialized yet.'); return toolkit.next(); } + const isAuthRoute = request.route.options.tags.includes(ROUTE_TAG_AUTH_FLOW); + const isLogoutRoute = + request.route.path === '/api/security/logout' || + request.route.path === '/api/v1/security/logout'; + // If users can eventually re-login we want to redirect them directly to the page they tried // to access initially, but we only want to do that for routes that aren't part of the various // authentication flows that wouldn't make any sense after successful authentication. - const originalURL = !request.route.options.tags.includes(ROUTE_TAG_AUTH_FLOW) - ? this.authenticator.getRequestOriginalURL(request) - : `${http.basePath.get(request)}/`; - if (!isLoginPageAvailable) { + const originalURL = isAuthRoute + ? `${http.basePath.get(request)}/` + : this.authenticator.getRequestOriginalURL(request); + + // Let API responses or <400 responses pass through as we can let their handlers deal with them. + if (preResponse.statusCode < 400 || !canRedirectRequest(request)) { + return toolkit.next(); + } + + if (preResponse.statusCode !== 401 && !isAuthRoute) { + return toolkit.next(); + } + + // Now we are only dealing with authentication flow errors or 401 errors in non-authentication routes. + // Additionally, if logout fails for any reason, we also want to show an error page. + // At this point we redirect users to the login page if it's available, or render a dedicated unauthenticated error page. + if (!isLoginPageAvailable || isLogoutRoute) { const customBrandingValue = await customBranding.getBrandingFor(request, { unauthenticated: true, }); return toolkit.render({ body: renderUnauthenticatedPage({ - basePath: http.basePath, staticAssets: http.staticAssets, + basePath: http.basePath, originalURL, customBranding: customBrandingValue, }), diff --git a/x-pack/plugins/security/server/index.ts b/x-pack/plugins/security/server/index.ts index 140f7657b8084..48ef9cfb24514 100644 --- a/x-pack/plugins/security/server/index.ts +++ b/x-pack/plugins/security/server/index.ts @@ -24,7 +24,7 @@ export { HTTPAuthorizationHeader } from './authentication'; export type { CasesSupportedOperations } from './authorization'; export type { SecurityPluginSetup, SecurityPluginStart }; export type { AuthenticatedUser } from '../common'; -export { ROUTE_TAG_CAN_REDIRECT } from './routes/tags'; +export { ROUTE_TAG_CAN_REDIRECT, ROUTE_TAG_AUTH_FLOW } from './routes/tags'; // Re-export types from the plugin directly to enhance the developer experience for consumers of the Security plugin. export type { diff --git a/x-pack/test/security_api_integration/tests/http_bearer/access_token.ts b/x-pack/test/security_api_integration/tests/http_bearer/access_token.ts index 3f6d89fb5ff9d..3e94adfc3ece8 100644 --- a/x-pack/test/security_api_integration/tests/http_bearer/access_token.ts +++ b/x-pack/test/security_api_integration/tests/http_bearer/access_token.ts @@ -98,5 +98,55 @@ export default function ({ getService }: FtrProviderContext) { .set('authorization', `Bearer ${accessToken}`) .expect(401); }); + + describe('Post-authentication', () => { + it('correctly handles unexpected post-authentication errors', async () => { + const { accessToken } = await createToken(); + + await supertest + .get('/authentication/app/not_auth_flow') + .set('authorization', `Bearer ${accessToken}`) + .expect(200); + + await supertest + .get('/authentication/app/not_auth_flow?statusCode=400') + .set('authorization', `Bearer ${accessToken}`) + .expect(400); + + const { text: nonauthFlow500ResponseText } = await supertest + .get('/authentication/app/not_auth_flow?statusCode=500') + .set('authorization', `Bearer ${accessToken}`) + .expect(500); + expect(nonauthFlow500ResponseText).to.eql( + '{"statusCode":500,"error":"Internal Server Error","message":"500 response"}' + ); + + // Auth-flow routes + await supertest + .get('/authentication/app/auth_flow') + .set('authorization', `Bearer ${accessToken}`) + .expect(200); + + const { + text: authFlow401ResponseText, + headers: { refresh: refresh401Header }, + } = await supertest + .get('/authentication/app/auth_flow?statusCode=401') + .set('authorization', `Bearer ${accessToken}`) + .expect(401); + expect(authFlow401ResponseText).to.contain('
'); + expect(refresh401Header).to.contain('url=/login'); + + const { + text: authFlow500ResponseText, + headers: { refresh: refresh500Header }, + } = await supertest + .get('/authentication/app/auth_flow?statusCode=500') + .set('authorization', `Bearer ${accessToken}`) + .expect(500); + expect(authFlow500ResponseText).to.contain('
'); + expect(refresh500Header).to.contain('url=/login'); + }); + }); }); } diff --git a/x-pack/test/security_api_integration/tests/saml/saml_login.ts b/x-pack/test/security_api_integration/tests/saml/saml_login.ts index 66cc721512c3e..893f2fcf3b0bd 100644 --- a/x-pack/test/security_api_integration/tests/saml/saml_login.ts +++ b/x-pack/test/security_api_integration/tests/saml/saml_login.ts @@ -23,6 +23,7 @@ import { FileWrapper } from '../audit/file_wrapper'; export default function ({ getService }: FtrProviderContext) { const randomness = getService('randomness'); const supertest = getService('supertestWithoutAuth'); + const config = getService('config'); const retry = getService('retry'); @@ -892,5 +893,60 @@ export default function ({ getService }: FtrProviderContext) { expect(auditEvents[0].kibana.authentication_provider).to.be('saml'); }); }); + + describe('Post-authentication failures', () => { + it('correctly handles unexpected post-authentication errors', async () => { + const samlAuthenticationResponse = await supertest + .post('/api/security/saml/callback') + .send({ SAMLResponse: await createSAMLResponse() }) + .expect(302); + + // User should be redirected to the base URL. + expect(samlAuthenticationResponse.headers.location).to.be('/'); + + const cookies = samlAuthenticationResponse.headers['set-cookie']; + expect(cookies).to.have.length(1); + + await checkSessionCookie(parseCookie(cookies[0])!); + + const sessionCookie = parseCookie(cookies[0])!.cookieString(); + // Non-auth flow routes + await supertest + .get('/authentication/app/not_auth_flow') + .set('Cookie', sessionCookie) + .expect(200); + + await supertest + .get('/authentication/app/not_auth_flow?statusCode=400') + .set('Cookie', sessionCookie) + .expect(400); + + const { text: nonauthFlow500ResponseText } = await supertest + .get('/authentication/app/not_auth_flow?statusCode=500') + .set('Cookie', sessionCookie) + .expect(500); + expect(nonauthFlow500ResponseText).to.eql( + '{"statusCode":500,"error":"Internal Server Error","message":"500 response"}' + ); + + // Auth-flow routes + await supertest + .get('/authentication/app/auth_flow') + .set('Cookie', sessionCookie) + .expect(200); + + const { text: authFlow401ResponseText } = await supertest + .get('/authentication/app/auth_flow?statusCode=401') + .set('Cookie', sessionCookie) + .expect(401); + expect(authFlow401ResponseText).to.contain('We hit an authentication error'); + + const { text: authFlow500ResponseText } = await supertest + .get('/authentication/app/auth_flow?statusCode=500') + .set('Cookie', sessionCookie) + .expect(500); + expect(authFlow500ResponseText).to.contain('We hit an authentication error'); + }); + }); }); } diff --git a/x-pack/test/security_functional/plugins/test_endpoints/server/init_routes.ts b/x-pack/test/security_functional/plugins/test_endpoints/server/init_routes.ts index 54c2f2500ff63..70a077305ba39 100644 --- a/x-pack/test/security_functional/plugins/test_endpoints/server/init_routes.ts +++ b/x-pack/test/security_functional/plugins/test_endpoints/server/init_routes.ts @@ -14,6 +14,7 @@ import type { BulkUpdateTaskResult, } from '@kbn/task-manager-plugin/server'; import { restApiKeySchema } from '@kbn/security-plugin-types-server'; +import { ROUTE_TAG_AUTH_FLOW } from '@kbn/security-plugin/server'; import { PluginStartDependencies } from '.'; export const SESSION_INDEX_CLEANUP_TASK_NAME = 'session_cleanup'; @@ -39,6 +40,32 @@ export function initRoutes( ); const router = core.http.createRouter(); + + for (const isAuthFlow of [true, false]) { + router.get( + { + path: `/authentication/app/${isAuthFlow ? 'auth_flow' : 'not_auth_flow'}`, + validate: { + query: schema.object({ + statusCode: schema.maybe(schema.number()), + message: schema.maybe(schema.string()), + }), + }, + options: { tags: isAuthFlow ? [ROUTE_TAG_AUTH_FLOW] : [], authRequired: !isAuthFlow }, + }, + (context, request, response) => { + if (request.query.statusCode) { + return response.customError({ + statusCode: request.query.statusCode, + body: request.query.message ?? `${request.query.statusCode} response`, + }); + } + + return response.ok({ body: isAuthFlow ? 'Auth flow complete' : 'Not auth flow complete' }); + } + ); + } + router.post( { path: '/authentication/app/setup', diff --git a/x-pack/test/security_functional/tests/login_selector/basic_functionality.ts b/x-pack/test/security_functional/tests/login_selector/basic_functionality.ts index 727edd341a65f..2c60d50650647 100644 --- a/x-pack/test/security_functional/tests/login_selector/basic_functionality.ts +++ b/x-pack/test/security_functional/tests/login_selector/basic_functionality.ts @@ -209,5 +209,29 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { await testSubjects.click('loginBackToLoginLink'); await PageObjects.security.loginSelector.verifyLoginSelectorIsVisible(); }); + + it('correctly handles unexpected post-authentication errors', async () => { + const supertest = getService('supertest'); + await PageObjects.security.loginSelector.login('basic', 'basic1'); + + await supertest.get('/authentication/app/not_auth_flow').expect(200); + await supertest.get('/authentication/app/not_auth_flow?statusCode=400').expect(400); + await supertest.get('/authentication/app/not_auth_flow?statusCode=500').expect(500); + await browser.navigateTo( + `${deployment.getHostPort()}/authentication/app/not_auth_flow?statusCode=401` + ); + await PageObjects.security.loginSelector.verifyLoginSelectorIsVisible(); + + await supertest.get('/authentication/app/auth_flow').expect(200); + await browser.navigateTo( + `${deployment.getHostPort()}/authentication/app/auth_flow?statusCode=400` + ); + await PageObjects.security.loginSelector.verifyLoginSelectorIsVisible(); + + await browser.navigateTo( + `${deployment.getHostPort()}/authentication/app/auth_flow?statusCode=500` + ); + await PageObjects.security.loginSelector.verifyLoginSelectorIsVisible(); + }); }); }