From 9763717c9025a7afdb54ea8bc996b471dd4b5310 Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Tue, 10 Dec 2019 11:07:40 +0100 Subject: [PATCH] Review#1: do not perform license check for the `/logout` endpoint, keep `/api/security/v1/me` in 7.x, validate OIDC `authenticationResponseURI` as a proper URI, explain the need for `allowUnknowns` in OIDC routes validation, localize error message, update APM dev script with new `users` internal routes. --- .../setup-custom-kibana-user-role.ts | 6 ++-- .../server/routes/authentication/common.ts | 34 ++++++++++++------- .../server/routes/authentication/oidc.ts | 20 ++++++++--- 3 files changed, 40 insertions(+), 20 deletions(-) diff --git a/x-pack/legacy/plugins/apm/scripts/kibana-security/setup-custom-kibana-user-role.ts b/x-pack/legacy/plugins/apm/scripts/kibana-security/setup-custom-kibana-user-role.ts index 9591dfdecbfef..66f2a8d1ac79f 100644 --- a/x-pack/legacy/plugins/apm/scripts/kibana-security/setup-custom-kibana-user-role.ts +++ b/x-pack/legacy/plugins/apm/scripts/kibana-security/setup-custom-kibana-user-role.ts @@ -183,7 +183,7 @@ async function createOrUpdateUser(newUser: User) { async function createUser(newUser: User) { const user = await callKibana({ method: 'POST', - url: `/api/security/v1/users/${newUser.username}`, + url: `/internal/security/users/${newUser.username}`, data: { ...newUser, enabled: true, @@ -209,7 +209,7 @@ async function updateUser(existingUser: User, newUser: User) { // assign role to user await callKibana({ method: 'POST', - url: `/api/security/v1/users/${username}`, + url: `/internal/security/users/${username}`, data: { ...existingUser, roles: allRoles } }); @@ -219,7 +219,7 @@ async function updateUser(existingUser: User, newUser: User) { async function getUser(username: string) { try { return await callKibana({ - url: `/api/security/v1/users/${username}` + url: `/internal/security/users/${username}` }); } catch (e) { const err = e as AxiosError; diff --git a/x-pack/plugins/security/server/routes/authentication/common.ts b/x-pack/plugins/security/server/routes/authentication/common.ts index 017061b08f6f9..cb4ec196459ee 100644 --- a/x-pack/plugins/security/server/routes/authentication/common.ts +++ b/x-pack/plugins/security/server/routes/authentication/common.ts @@ -24,7 +24,7 @@ export function defineCommonRoutes({ router, authc, basePath, logger }: RouteDef validate: { query: schema.object({}, { allowUnknowns: true }) }, options: { authRequired: false }, }, - createLicensedRouteHandler(async (context, request, response) => { + async (context, request, response) => { const serverBasePath = basePath.serverBasePath; if (path === '/api/security/v1/logout') { logger.warn( @@ -51,18 +51,28 @@ export function defineCommonRoutes({ router, authc, basePath, logger }: RouteDef } catch (error) { return response.customError(wrapIntoCustomErrorResponse(error)); } - }) + } ); } - router.get( - { path: '/internal/security/me', validate: false }, - createLicensedRouteHandler(async (context, request, response) => { - try { - return response.ok({ body: (await authc.getCurrentUser(request)) as any }); - } catch (error) { - return response.customError(wrapIntoCustomErrorResponse(error)); - } - }) - ); + // Generate two identical routes with new and deprecated URL and issue a warning if route with deprecated URL is ever used. + for (const path of ['/internal/security/me', '/api/security/v1/me']) { + router.get( + { path, validate: false }, + createLicensedRouteHandler(async (context, request, response) => { + if (path === '/api/security/v1/me') { + logger.warn( + `The "${basePath.serverBasePath}${path}" endpoint is deprecated and will be removed in the next major version.`, + { tags: ['deprecation'] } + ); + } + + try { + return response.ok({ body: (await authc.getCurrentUser(request)) as any }); + } catch (error) { + return response.customError(wrapIntoCustomErrorResponse(error)); + } + }) + ); + } } diff --git a/x-pack/plugins/security/server/routes/authentication/oidc.ts b/x-pack/plugins/security/server/routes/authentication/oidc.ts index 277eac799c513..8483630763ae6 100644 --- a/x-pack/plugins/security/server/routes/authentication/oidc.ts +++ b/x-pack/plugins/security/server/routes/authentication/oidc.ts @@ -5,6 +5,7 @@ */ import { schema } from '@kbn/config-schema'; +import { i18n } from '@kbn/i18n'; import { KibanaRequest, KibanaResponseFactory } from '../../../../../../src/core/server'; import { OIDCAuthenticationFlow } from '../../authentication'; import { createCustomResourceResponse } from '.'; @@ -95,7 +96,7 @@ export function defineOIDCRoutes({ validate: { query: schema.object( { - authenticationResponseURI: schema.maybe(schema.string({ minLength: 1 })), + authenticationResponseURI: schema.maybe(schema.uri()), code: schema.maybe(schema.string()), error: schema.maybe(schema.string()), error_description: schema.maybe(schema.string()), @@ -105,6 +106,9 @@ export function defineOIDCRoutes({ target_link_uri: schema.maybe(schema.uri()), state: schema.maybe(schema.string()), }, + // The client MUST ignore unrecognized response parameters according to + // https://openid.net/specs/openid-connect-core-1_0.html#AuthResponseValidation and + // https://tools.ietf.org/html/rfc6749#section-4.1.2. { allowUnknowns: true } ), }, @@ -178,6 +182,8 @@ export function defineOIDCRoutes({ login_hint: schema.maybe(schema.string()), target_link_uri: schema.maybe(schema.uri()), }, + // Other parameters MAY be sent, if defined by extensions. Any parameters used that are not understood MUST + // be ignored by the Client according to https://openid.net/specs/openid-connect-core-1_0.html#ThirdPartyInitiatedLogin. { allowUnknowns: true } ), }, @@ -215,6 +221,8 @@ export function defineOIDCRoutes({ login_hint: schema.maybe(schema.string()), target_link_uri: schema.maybe(schema.uri()), }, + // Other parameters MAY be sent, if defined by extensions. Any parameters used that are not understood MUST + // be ignored by the Client according to https://openid.net/specs/openid-connect-core-1_0.html#ThirdPartyInitiatedLogin. { allowUnknowns: true } ), }, @@ -235,7 +243,7 @@ export function defineOIDCRoutes({ loginAttempt: ProviderLoginAttempt ) { try { - // We handle the fact that the user might get redirected to Kibana while already having an session + // We handle the fact that the user might get redirected to Kibana while already having a session // Return an error notifying the user they are already logged in. const authenticationResult = await authc.login(request, { provider: 'oidc', @@ -244,9 +252,11 @@ export function defineOIDCRoutes({ if (authenticationResult.succeeded()) { return response.forbidden({ - body: - 'Sorry, you already have an active Kibana session. ' + - 'If you want to start a new one, please logout from the existing session first.', + body: i18n.translate('xpack.security.conflictingSessionError', { + defaultMessage: + 'Sorry, you already have an active Kibana session. ' + + 'If you want to start a new one, please logout from the existing session first.', + }), }); }