From c6fb0bc7f2233de90e4b45156212b758d7662845 Mon Sep 17 00:00:00 2001 From: Kurt Date: Tue, 6 Sep 2022 11:54:57 -0400 Subject: [PATCH] Adding Global Access Agreement (#139217) * Adding Global Access Agreement * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * Adding docs * PR Review changes * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * PR Review changes * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * Fixing global access agreement redirect logic * Fixing unit test * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * Changing verbiage * Changing local -> provider specific * Update x-pack/plugins/security/server/routes/views/access_agreement.ts Co-authored-by: Aleh Zasypkin * PR Feedback * removing `only` * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * Changing the accessAgreement to be optional * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * Adding docker config * Update docs/user/security/access-agreement.asciidoc Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com> * Update docs/user/security/access-agreement.asciidoc Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com> * Adding PR Review feedback Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Aleh Zasypkin Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com> --- docs/settings/security-settings.asciidoc | 11 +++++ docs/user/security/access-agreement.asciidoc | 3 ++ .../resources/base/bin/kibana-docker | 1 + .../authentication/authentication_service.ts | 5 ++- .../authentication/authenticator.test.ts | 45 ++++++++++++++++++- .../server/authentication/authenticator.ts | 39 +++++++++------- x-pack/plugins/security/server/config.test.ts | 26 +++++++++++ x-pack/plugins/security/server/config.ts | 11 ++++- .../routes/views/access_agreement.test.ts | 40 ++++++++++++++++- .../server/routes/views/access_agreement.ts | 19 ++++++-- .../security_usage_collector.test.ts | 31 +++++++++++++ .../security_usage_collector.ts | 3 +- 12 files changed, 209 insertions(+), 25 deletions(-) diff --git a/docs/settings/security-settings.asciidoc b/docs/settings/security-settings.asciidoc index 9174995c5d9e1..941cfca92e603 100644 --- a/docs/settings/security-settings.asciidoc +++ b/docs/settings/security-settings.asciidoc @@ -162,6 +162,17 @@ Adds a message accessible at the login UI with additional help information for t xpack.security.authc.selector.enabled {ess-icon}:: Determines if the login selector UI should be enabled. By default, this setting is set to `true` if more than one authentication provider is configured. +[float] +[[authentication-access-agreement-settings]] +==== Configure a default access agreement + +You can configure the following settings in the `kibana.yml` file. + +xpack.security.accessAgreement.message {ess-icon}:: +This setting specifies the access agreement text in Markdown format that will be used as the default access agreement for all providers that do not +specify a value for `xpack.security.authc.providers...accessAgreement.message`. +For more information, refer to <>. + [float] [[security-session-and-cookie-settings]] ==== Session and cookie security settings diff --git a/docs/user/security/access-agreement.asciidoc b/docs/user/security/access-agreement.asciidoc index 9d9a0bb61a90b..03e5a312937a5 100644 --- a/docs/user/security/access-agreement.asciidoc +++ b/docs/user/security/access-agreement.asciidoc @@ -6,6 +6,9 @@ Access agreement is a https://www.elastic.co/subscriptions[subscription feature] agreement before accessing {kib}. The agreement text supports Markdown format and can be specified using the `xpack.security.authc.providers...accessAgreement.message` setting. +You can specify a default access agreement using the `xpack.security.accessAgreement.message` setting. +This message will be used for each provider who doesn't specify an access agreement. + [NOTE] ============================================================================ You need to acknowledge the access agreement only once per session, and {kib} reports the acknowledgement in the audit logs. diff --git a/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker b/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker index 3774676288c7f..9a73f1f295492 100755 --- a/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker +++ b/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker @@ -342,6 +342,7 @@ kibana_vars=( xpack.reporting.roles.allow xpack.reporting.roles.enabled xpack.ruleRegistry.write.enabled + xpack.security.accessAgreement.message xpack.security.audit.appender.fileName xpack.security.audit.appender.layout.highlight xpack.security.audit.appender.layout.pattern diff --git a/x-pack/plugins/security/server/authentication/authentication_service.ts b/x-pack/plugins/security/server/authentication/authentication_service.ts index 50797c821e884..c22ac5fceecb6 100644 --- a/x-pack/plugins/security/server/authentication/authentication_service.ts +++ b/x-pack/plugins/security/server/authentication/authentication_service.ts @@ -335,7 +335,10 @@ export class AuthenticationService { loggers, clusterClient, basePath: http.basePath, - config: { authc: config.authc }, + config: { + authc: config.authc, + accessAgreement: config.accessAgreement, + }, getCurrentUser, featureUsageService, userProfileService, diff --git a/x-pack/plugins/security/server/authentication/authenticator.test.ts b/x-pack/plugins/security/server/authentication/authenticator.test.ts index 98accaf987a99..1c62b5ae44dd9 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.test.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.test.ts @@ -48,14 +48,21 @@ function getMockOptions({ providers, http = {}, selector, + accessAgreementMessage, }: { providers?: Record | string[]; http?: Partial; selector?: AuthenticatorOptions['config']['authc']['selector']; + accessAgreementMessage?: string; } = {}) { const auditService = auditServiceMock.create(); auditLogger = auditLoggerMock.create(); auditService.asScoped.mockReturnValue(auditLogger); + + const accessAgreementObj = accessAgreementMessage + ? { accessAgreement: { message: accessAgreementMessage } } + : null; + return { audit: auditService, getCurrentUser: jest.fn(), @@ -65,7 +72,10 @@ function getMockOptions({ loggers: loggingSystemMock.create(), getServerBaseURL: jest.fn(), config: createConfig( - ConfigSchema.validate({ authc: { selector, providers, http } }), + ConfigSchema.validate({ + authc: { selector, providers, http }, + ...accessAgreementObj, + }), loggingSystemMock.create().get(), { isTLSEnabled: false } ), @@ -1921,6 +1931,39 @@ describe('Authenticator', () => { ); expect(auditLogger.log).not.toHaveBeenCalled(); }); + + it('redirects to global Access Agreement when provider specific Access Agreement is not configured.', async () => { + mockOptions = getMockOptions({ + providers: { + basic: { basic1: { order: 0 } }, + }, + accessAgreementMessage: 'Foo', + }); + + mockOptions.license.getFeatures.mockReturnValue({ + allowAccessAgreement: true, + } as SecurityLicenseFeatures); + + authenticator = new Authenticator(mockOptions); + + mockOptions.session.get.mockResolvedValue(mockSessVal); + mockOptions.session.extend.mockResolvedValue(mockSessVal); + + mockBasicAuthenticationProvider.authenticate.mockResolvedValue( + AuthenticationResult.succeeded(mockUser, { + authResponseHeaders: { 'WWW-Authenticate': 'Negotiate' }, + }) + ); + + const request = httpServerMock.createKibanaRequest(); + await expect(authenticator.authenticate(request)).resolves.toEqual( + AuthenticationResult.redirectTo( + '/mock-server-basepath/security/access_agreement?next=%2Fmock-server-basepath%2Fpath', + { user: mockUser, authResponseHeaders: { 'WWW-Authenticate': 'Negotiate' } } + ) + ); + expect(auditLogger.log).not.toHaveBeenCalled(); + }); }); describe('with Overwritten Session', () => { diff --git a/x-pack/plugins/security/server/authentication/authenticator.ts b/x-pack/plugins/security/server/authentication/authenticator.ts index 94bb89f631d2b..735224fd83720 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.ts @@ -82,7 +82,7 @@ export interface AuthenticatorOptions { featureUsageService: SecurityFeatureUsageServiceStart; userProfileService: UserProfileServiceStartInternal; getCurrentUser: (request: KibanaRequest) => AuthenticatedUser | null; - config: Pick; + config: Pick; basePath: IBasePath; license: SecurityLicense; loggers: LoggerFactory; @@ -831,21 +831,27 @@ export class Authenticator { * @param sessionValue Current session value if any. */ private shouldRedirectToAccessAgreement(sessionValue: SessionValue | null) { - // Request should be redirected to Access Agreement UI only if all following conditions are met: - // 1. Request can be redirected (not API call) - // 2. Request is authenticated, but user hasn't acknowledged access agreement in the current - // session yet (based on the flag we store in the session) - // 3. Request is authenticated by the provider that has `accessAgreement` configured - // 4. Current license allows access agreement - // 5. And it's not a request to the Access Agreement UI itself - return ( - sessionValue != null && - !sessionValue.accessAgreementAcknowledged && - (this.options.config.authc.providers as Record)[sessionValue.provider.type]?.[ - sessionValue.provider.name - ]?.accessAgreement && - this.options.license.getFeatures().allowAccessAgreement - ); + // If user doesn't have an active session or if they already acknowledged + // access agreement (based on the flag we store in the session) - bail out. + if (sessionValue == null || sessionValue.accessAgreementAcknowledged) { + return false; + } + + // If access agreement is neither enabled globally (for all providers) + // nor for the provider that authenticated user request - bail out. + const providerConfig = (this.options.config.authc.providers as Record)[ + sessionValue.provider.type + ]?.[sessionValue.provider.name]; + + if ( + !this.options.config.accessAgreement?.message && + !providerConfig?.accessAgreement?.message + ) { + return false; + } + + // Check if the current license allows access agreement. + return this.options.license.getFeatures().allowAccessAgreement; } /** @@ -874,6 +880,7 @@ export class Authenticator { const isUpdatedSessionAuthenticated = isSessionAuthenticated(sessionUpdateResult?.value); let preAccessRedirectURL; + if (isUpdatedSessionAuthenticated && sessionUpdateResult?.overwritten) { this.logger.debug('Redirecting user to the overwritten session UI.'); preAccessRedirectURL = `${this.options.basePath.serverBasePath}${OVERWRITTEN_SESSION_ROUTE}`; diff --git a/x-pack/plugins/security/server/config.test.ts b/x-pack/plugins/security/server/config.test.ts index f0db13bc773d6..1c50c82d3a0f6 100644 --- a/x-pack/plugins/security/server/config.test.ts +++ b/x-pack/plugins/security/server/config.test.ts @@ -2204,4 +2204,30 @@ describe('createConfig()', () => { `); }); }); + + describe('Global Access Agreement', () => { + it('should require `message` for globally configured `accessAgreement`', () => { + expect(() => { + createConfig( + ConfigSchema.validate({ + accessAgreement: {}, + }), + loggingSystemMock.create().get(), + { isTLSEnabled: true } + ); + }).toThrow('[accessAgreement.message]: expected value of type [string] but got [undefined]'); + }); + + it('should accept string `message` for globally configured `accessAgreement`', () => { + expect( + createConfig( + ConfigSchema.validate({ + accessAgreement: { message: 'Foo' }, + }), + loggingSystemMock.create().get(), + { isTLSEnabled: true } + )?.accessAgreement?.message + ).toEqual('Foo'); + }); + }); }); diff --git a/x-pack/plugins/security/server/config.ts b/x-pack/plugins/security/server/config.ts index 8d31e8ec95bfd..aa7e3c0964ba8 100644 --- a/x-pack/plugins/security/server/config.ts +++ b/x-pack/plugins/security/server/config.ts @@ -83,6 +83,7 @@ function getUniqueProviderSchema>>( } type ProvidersConfigType = TypeOf; + const providersConfigSchema = schema.object( { basic: getUniqueProviderSchema('basic', { @@ -235,6 +236,7 @@ export const ConfigSchema = schema.object({ hostname: schema.maybe(schema.string({ hostname: true })), port: schema.maybe(schema.number({ min: 0, max: 65535 })), }), + accessAgreement: schema.maybe(schema.object({ message: schema.string() })), authc: schema.object({ selector: schema.object({ enabled: schema.maybe(schema.boolean()) }), providers: schema.oneOf([schema.arrayOf(schema.string()), providersConfigSchema], { @@ -306,6 +308,7 @@ export function createConfig( } let secureCookies = config.secureCookies; + if (!isTLSEnabled) { if (secureCookies) { logger.warn( @@ -322,6 +325,7 @@ export function createConfig( } const isUsingLegacyProvidersFormat = Array.isArray(config.authc.providers); + const providers = ( isUsingLegacyProvidersFormat ? [...new Set(config.authc.providers as Array)].reduce( @@ -332,6 +336,7 @@ export function createConfig( ? { enabled: true, showInSelector: true, order, ...config.authc[providerType] } : { enabled: true, showInSelector: true, order }, }; + return legacyProviders; }, {} as Record @@ -346,16 +351,19 @@ export function createConfig( order: number; hasAccessAgreement: boolean; }> = []; + for (const [type, providerGroup] of Object.entries(providers)) { for (const [name, { enabled, order, accessAgreement }] of Object.entries(providerGroup ?? {})) { if (!enabled) { delete providerGroup![name]; } else { + const hasAccessAgreement: boolean = !!accessAgreement?.message; + sortedProviders.push({ type: type as any, name, order, - hasAccessAgreement: !!accessAgreement?.message, + hasAccessAgreement, }); } } @@ -391,6 +399,7 @@ export function createConfig( max: 10, }, } as AppenderConfigType); + return { ...config, audit: { diff --git a/x-pack/plugins/security/server/routes/views/access_agreement.test.ts b/x-pack/plugins/security/server/routes/views/access_agreement.test.ts index 9b4bdb4d4329d..5635dceace0b6 100644 --- a/x-pack/plugins/security/server/routes/views/access_agreement.test.ts +++ b/x-pack/plugins/security/server/routes/views/access_agreement.test.ts @@ -31,6 +31,7 @@ describe('Access agreement view routes', () => { let session: jest.Mocked>; let license: jest.Mocked; let mockContext: SecurityRequestHandlerContext; + beforeEach(() => { const routeParamsMock = routeDefinitionParamsMock.create(); router = routeParamsMock.router; @@ -138,7 +139,7 @@ describe('Access agreement view routes', () => { }); }); - it('returns non-empty `accessAgreement` only if it is configured.', async () => { + it('returns non-empty provider specific `accessAgreement` only if it is configured.', async () => { const request = httpServerMock.createKibanaRequest(); config.authc = routeDefinitionParamsMock.create({ @@ -172,5 +173,42 @@ describe('Access agreement view routes', () => { }); } }); + + it('returns global `accessAgreement` when provider specific `accessAgreement` is not configured', async () => { + const request = httpServerMock.createKibanaRequest(); + + config.authc = routeDefinitionParamsMock.create({ + authc: { + providers: { + basic: { basic1: { order: 0 } }, + saml: { + saml1: { + order: 1, + realm: 'realm1', + accessAgreement: { message: 'Some access agreement' }, + }, + }, + }, + }, + }).config.authc; + + config.accessAgreement = { message: 'Foo' }; + + const cases: Array<[AuthenticationProvider, string]> = [ + [{ type: 'basic', name: 'basic1' }, 'Foo'], + [{ type: 'saml', name: 'saml1' }, 'Some access agreement'], + [{ type: 'unknown-type', name: 'unknown-name' }, 'Foo'], + ]; + + for (const [sessionProvider, expectedAccessAgreement] of cases) { + session.get.mockResolvedValue(sessionMock.createValue({ provider: sessionProvider })); + + await expect(routeHandler(mockContext, request, kibanaResponseFactory)).resolves.toEqual({ + options: { body: { accessAgreement: expectedAccessAgreement } }, + payload: { accessAgreement: expectedAccessAgreement }, + status: 200, + }); + } + }); }); }); diff --git a/x-pack/plugins/security/server/routes/views/access_agreement.ts b/x-pack/plugins/security/server/routes/views/access_agreement.ts index 83e640f43dee9..49a3984e181ac 100644 --- a/x-pack/plugins/security/server/routes/views/access_agreement.ts +++ b/x-pack/plugins/security/server/routes/views/access_agreement.ts @@ -47,12 +47,23 @@ export function defineAccessAgreementRoutes({ // authenticated with the help of HTTP authentication), that means we should safely check if // we have it and can get a corresponding configuration. const sessionValue = await getSession().get(request); - const accessAgreement = - (sessionValue && + + let accessAgreement = ''; + + if (sessionValue) { + const providerSpecificAccessAgreement = config.authc.providers[ sessionValue.provider.type as keyof ConfigType['authc']['providers'] - ]?.[sessionValue.provider.name]?.accessAgreement?.message) || - ''; + ]?.[sessionValue.provider.name]?.accessAgreement?.message; + + const globalAccessAgreement = config.accessAgreement?.message; + + if (providerSpecificAccessAgreement) { + accessAgreement = providerSpecificAccessAgreement; + } else if (globalAccessAgreement) { + accessAgreement = globalAccessAgreement; + } + } return response.ok({ body: { accessAgreement } }); }) diff --git a/x-pack/plugins/security/server/usage_collector/security_usage_collector.test.ts b/x-pack/plugins/security/server/usage_collector/security_usage_collector.test.ts index 15a0713d80326..286aed84527af 100644 --- a/x-pack/plugins/security/server/usage_collector/security_usage_collector.test.ts +++ b/x-pack/plugins/security/server/usage_collector/security_usage_collector.test.ts @@ -200,6 +200,37 @@ describe('Security UsageCollector', () => { }); describe('access agreement', () => { + it('reports if the global access agreement message is configured', async () => { + const config = createSecurityConfig( + ConfigSchema.validate({ + accessAgreement: { message: 'Bar' }, + authc: { + providers: { + saml: { + saml1: { + realm: 'foo', + order: 1, + }, + }, + }, + }, + }) + ); + const usageCollection = usageCollectionPluginMock.createSetupContract(); + const license = createSecurityLicense({ isLicenseAvailable: true }); + registerSecurityUsageCollector({ usageCollection, config, license }); + + const usage = await usageCollection + .getCollectorByType('security') + ?.fetch(collectorFetchContext); + + expect(usage).toEqual({ + ...DEFAULT_USAGE, + accessAgreementEnabled: true, + enabledAuthProviders: ['saml'], + }); + }); + it('reports if the access agreement message is configured for any provider', async () => { const config = createSecurityConfig( ConfigSchema.validate({ diff --git a/x-pack/plugins/security/server/usage_collector/security_usage_collector.ts b/x-pack/plugins/security/server/usage_collector/security_usage_collector.ts index 4050e70bbcfed..d33ff8617e453 100644 --- a/x-pack/plugins/security/server/usage_collector/security_usage_collector.ts +++ b/x-pack/plugins/security/server/usage_collector/security_usage_collector.ts @@ -161,7 +161,8 @@ export function registerSecurityUsageCollector({ usageCollection, config, licens ]; const accessAgreementEnabled = allowAccessAgreement && - config.authc.sortedProviders.some((provider) => provider.hasAccessAgreement); + (!!config.accessAgreement?.message || + config.authc.sortedProviders.some((provider) => provider.hasAccessAgreement)); const httpAuthSchemes = config.authc.http.schemes.filter((scheme) => WELL_KNOWN_AUTH_SCHEMES.includes(scheme.toLowerCase())