Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose session invalidation API. #92376

Merged
merged 16 commits into from
Mar 24, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import type { ObjectType } from '@kbn/config-schema';
import type { PublicMethodsOf } from '@kbn/utility-types';
import type { SecurityRequestHandlerContext, SecurityRouter } from '../../types';
import {
kibanaResponseFactory,
RequestHandler,
RouteConfig,
} from '../../../../../../src/core/server';
import type { Session } from '../../session_management';
import { defineDeleteAllSessionsRoutes } from './delete_all';

import { httpServerMock } from '../../../../../../src/core/server/mocks';
import { routeDefinitionParamsMock } from '../index.mock';
import { sessionMock } from '../../session_management/session.mock';

describe('Delete all sessions routes', () => {
let router: jest.Mocked<SecurityRouter>;
let session: jest.Mocked<PublicMethodsOf<Session>>;
beforeEach(() => {
const routeParamsMock = routeDefinitionParamsMock.create();
router = routeParamsMock.router;

session = sessionMock.create();
routeParamsMock.getSession.mockReturnValue(session);

defineDeleteAllSessionsRoutes(routeParamsMock);
});

describe('delete sessions', () => {
let routeHandler: RequestHandler<any, any, any, SecurityRequestHandlerContext>;
let routeConfig: RouteConfig<any, any, any, any>;
beforeEach(() => {
const [extendRouteConfig, extendRouteHandler] = router.delete.mock.calls.find(
([{ path }]) => path === '/internal/security/session/_all'
)!;

routeConfig = extendRouteConfig;
routeHandler = extendRouteHandler;
});

it('correctly defines route.', () => {
expect(routeConfig.options).toEqual({ tags: ['access:sessionManagement'] });

const querySchema = (routeConfig.validate as any).query as ObjectType;
expect(() =>
querySchema.validate({ providerName: 'basic1' })
).toThrowErrorMatchingInlineSnapshot(
`"[request query.providerType]: expected value of type [string] but got [undefined]"`
);
expect(() => querySchema.validate({ username: 'user' })).toThrowErrorMatchingInlineSnapshot(
`"[request query.providerType]: expected value of type [string] but got [undefined]"`
);
expect(() =>
querySchema.validate({ providerName: 'basic1', username: 'user' })
).toThrowErrorMatchingInlineSnapshot(
`"[request query.providerType]: expected value of type [string] but got [undefined]"`
);

expect(querySchema.validate(undefined)).toBeUndefined();
expect(querySchema.validate({})).toEqual({});
expect(querySchema.validate({ providerType: 'basic' })).toEqual({ providerType: 'basic' });
expect(querySchema.validate({ providerType: 'basic', providerName: 'basic1' })).toEqual({
providerType: 'basic',
providerName: 'basic1',
});
expect(querySchema.validate({ providerType: 'basic', username: 'user' })).toEqual({
providerType: 'basic',
username: 'user',
});
expect(
querySchema.validate({ providerType: 'basic', providerName: 'basic1', username: 'user' })
).toEqual({
providerType: 'basic',
providerName: 'basic1',
username: 'user',
});
});

it('uses query string to construct filter.', async () => {
session.clearAll.mockResolvedValue(30);

const mockRequest = httpServerMock.createKibanaRequest({
query: { providerType: 'basic', providerName: 'basic1', username: 'user' },
});
await expect(
routeHandler(
({} as unknown) as SecurityRequestHandlerContext,
mockRequest,
kibanaResponseFactory
)
).resolves.toEqual({
status: 200,
options: { body: { total: 30 } },
payload: { total: 30 },
});

expect(session.clearAll).toHaveBeenCalledTimes(1);
expect(session.clearAll).toHaveBeenCalledWith(mockRequest, {
provider: { type: 'basic', name: 'basic1' },
username: 'user',
});
});

it('does not specify filter if it is not specified in the query.', async () => {
session.clearAll.mockResolvedValue(30);

const mockRequest = httpServerMock.createKibanaRequest();
await expect(
routeHandler(
({} as unknown) as SecurityRequestHandlerContext,
mockRequest,
kibanaResponseFactory
)
).resolves.toEqual({
status: 200,
options: { body: { total: 30 } },
payload: { total: 30 },
});

expect(session.clearAll).toHaveBeenCalledTimes(1);
expect(session.clearAll).toHaveBeenCalledWith(mockRequest, undefined);
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import typeDetect from 'type-detect';
import { schema } from '@kbn/config-schema';
import { RouteDefinitionParams } from '..';

/**
* Defines routes required for the deleting of all sessions.
*/
export function defineDeleteAllSessionsRoutes({ router, getSession }: RouteDefinitionParams) {
router.delete(
{
path: '/internal/security/session/_all',
validate: {
query: schema.maybe(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I was also considering POST + body + /internal/security/session/_invalidate, but didn't find enough benefits comparing to DELETE + query. I know some ES APIs use body with DELETE, but IIRC it's not widely recommended and some proxies/tools may not be happy about it (e.g. the REST tool I use 🙂 ).

Let me know what approach you prefer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm learning towards POST + body + /internal/security/session/_invalidate. This name gives us more flexibility to expand its functionality over time. The _all suffix we're using above confused me initially -- at first glance, I assumed it would always invalidate all sessions, but we also accept a query parameter to optionally invalidate a subset of sessions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you've noted in the PR description that this is an internal route for now. Is there a reason not to document this as a public endpoint? We intend for this to be used externally, and marking it as public won't preclude us from making a breaking change in a minor release if we need to. In other words, it could be an experimental/beta public API, much like our SO, Space, and Role APIs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _all suffix we're using above confused me initially -- at first glance, I assumed it would always invalidate all sessions, but we also accept a query parameter to optionally invalidate a subset of sessions.

Yeah, that's because I added filter at a later iteration 🙂 Agree it's confusing now.

and marking it as public won't preclude us from making a breaking change in a minor release if we need to. In other words, it could be an experimental/beta public API, much like our SO, Space, and Role APIs.

That's a good point, I completely forgot about the experimental APIs! The "no guarantees yet" was my main motivator behind keeping this API as internal.

schema.object(
{
// `providerType` is actually required when any other options are specified, but such schema isn't
// currently supported by the Core for `query` string parameters. To workaround that we mark this property
// as optional and do custom validation instead: https://github.com/elastic/kibana/issues/92201
providerType: schema.maybe(schema.string()),
providerName: schema.maybe(schema.string()),
username: schema.maybe(schema.string()),
},
{
validate(value) {
if (typeof value?.providerType !== 'string' && Object.keys(value).length > 0) {
return `[request query.providerType]: expected value of type [string] but got [${typeDetect(
value?.providerType
)}]`;
}
},
}
)
),
},
options: { tags: ['access:sessionManagement'] },
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: do we have any precedent\ability to relax superuser-only requirement and, for example, give access to kibana_admin?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have the ability to grant access to any user with the "Global All" privilege (aka kibana_admin), but I don't know that these users should have the ability to force-logout everyone else.

IMO this feels like something that only a superuser (or someone with manage_security, which is effectively superuser) should be able to do. Are there benefits to relaxing this requirement that I'm overlooking?

I could see us eventually offering a way to "invalidate all of my sessions", but we aren't at a place to do that just yet, and adding this capability in would increase the scope of this PR quite a bit, I'd imagine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there benefits to relaxing this requirement that I'm overlooking?

Nothing concrete yet, I'm just not very happy that we require a "cluster superpower" to invalidate Kibana-specific sessions and Kibana Admin sounded like a role that could be assumed to be responsible for all Kibana-specific things. I'm totally fine to keep it as is right now, but if we can eventually have a Kibana Operator role or something, I'd vote for it instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there benefits to relaxing this requirement that I'm overlooking?

Nothing concrete yet, I'm just not very happy that we require a "cluster superpower" to invalidate Kibana-specific sessions and Kibana Admin sounded like a role that could be assumed to be responsible for all Kibana-specific things. I'm totally fine to keep it as is right now, but if we can eventually have a Kibana Operator role or something, I'd vote for it instead.

Yeah what you're saying makes sense. My fear is that kibana_admin would be granted to too many folks, especially without an "operator" role readily available. More than happy to revisit this in the future!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My fear is that kibana_admin would be granted to too many folks, especially without an "operator" role readily available.

Yeah, that's a good point, let's see how it goes with superuser then.

},
async (_context, request, response) => {
return response.ok({
body: {
total: await getSession().clearAll(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to my comment about _all above, clearAll is potentially misleading too, since it's not always clearing all sessions, but rather sessions matching a optional criteria. What do you think about aligning on invalidate? The one thing I don't like about this is that it's not immediately clear that this will invalidate multiple sessions, but perhaps that could be mitigated by requiring criteria, even if the criteria is empty?

What do you think?

Copy link
Member Author

@azasypkin azasypkin Feb 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will become a bit confusing since we'd have a clear and invalidate at the same time. How do you feel about joining clear and clearAll to a single clear or invalidate method instead?

export type InvalidateSessionFilter =
  | { type: 'all' }
  | { type: 'by-sid'; sid: string }
  | { type: 'by-query'; query: { provider: { type: string; name?: string }; usernameHash?: string } };

Or keep them separate, let me see if I can come up with non-confusing names for both.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you feel about joining clear and clearAll to a single clear or invalidate method instead?

Or keep them separate, let me see if I can come up with non-confusing names for both.

I would be fine with either of these approaches!

request,
request.query?.providerType
? {
provider: { type: request.query.providerType, name: request.query.providerName },
username: request.query.username,
}
: undefined
),
},
});
}
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@
* 2.0.
*/

import { defineDeleteAllSessionsRoutes } from './delete_all';
import { defineSessionExtendRoutes } from './extend';
import { defineSessionInfoRoutes } from './info';
import { RouteDefinitionParams } from '..';
import type { RouteDefinitionParams } from '..';

export function defineSessionManagementRoutes(params: RouteDefinitionParams) {
defineSessionInfoRoutes(params);
defineSessionExtendRoutes(params);
defineDeleteAllSessionsRoutes(params);
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import type { PublicMethodsOf } from '@kbn/utility-types';
import { mockAuthenticatedUser } from '../../common/model/authenticated_user.mock';
import { Session, SessionValue } from './session';
import type { Session, SessionValue } from './session';
import { sessionIndexMock } from './session_index.mock';

export const sessionMock = {
Expand All @@ -18,6 +18,7 @@ export const sessionMock = {
update: jest.fn(),
extend: jest.fn(),
clear: jest.fn(),
clearAll: jest.fn(),
}),

createValue: (sessionValue: Partial<SessionValue> = {}): SessionValue => ({
Expand Down
57 changes: 57 additions & 0 deletions x-pack/plugins/security/server/session_management/session.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -807,4 +807,61 @@ describe('Session', () => {
expect(mockSessionCookie.clear).toHaveBeenCalledWith(mockRequest);
});
});

describe('#clearAll', () => {
beforeEach(() => {
mockSessionCookie.get.mockResolvedValue(sessionCookieMock.createValue());
mockSessionIndex.clearAll.mockResolvedValue(10);
});

it('clears all sessions even if current initiator request does not have a session', async () => {
mockSessionCookie.get.mockResolvedValue(null);

await expect(session.clearAll(httpServerMock.createKibanaRequest())).resolves.toBe(10);

expect(mockSessionCookie.clear).not.toHaveBeenCalled();
expect(mockSessionIndex.clearAll).toHaveBeenCalledTimes(1);
expect(mockSessionIndex.clearAll).toHaveBeenCalledWith(undefined);
});

it('properly forwards filter with the provider type to the session index', async () => {
await expect(
session.clearAll(httpServerMock.createKibanaRequest(), { provider: { type: 'basic' } })
).resolves.toBe(10);

expect(mockSessionCookie.clear).not.toHaveBeenCalled();
expect(mockSessionIndex.clearAll).toHaveBeenCalledTimes(1);
expect(mockSessionIndex.clearAll).toHaveBeenCalledWith({ provider: { type: 'basic' } });
});

it('properly forwards filter with the provider type and provider name to the session index', async () => {
await expect(
session.clearAll(httpServerMock.createKibanaRequest(), {
provider: { type: 'basic', name: 'basic1' },
})
).resolves.toBe(10);

expect(mockSessionCookie.clear).not.toHaveBeenCalled();
expect(mockSessionIndex.clearAll).toHaveBeenCalledTimes(1);
expect(mockSessionIndex.clearAll).toHaveBeenCalledWith({
provider: { type: 'basic', name: 'basic1' },
});
});

it('properly forwards filter with the provider type, provider name, and username hash to the session index', async () => {
await expect(
session.clearAll(httpServerMock.createKibanaRequest(), {
provider: { type: 'basic', name: 'basic1' },
username: 'elastic',
})
).resolves.toBe(10);

expect(mockSessionCookie.clear).not.toHaveBeenCalled();
expect(mockSessionIndex.clearAll).toHaveBeenCalledTimes(1);
expect(mockSessionIndex.clearAll).toHaveBeenCalledWith({
provider: { type: 'basic', name: 'basic1' },
usernameHash: 'eb28536c8ead72bf81a0a9226e38fc9bad81f5e07c2081bb801b2a5c8842924e',
});
});
});
});
53 changes: 50 additions & 3 deletions x-pack/plugins/security/server/session_management/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,22 @@ export interface SessionValueContentToEncrypt {
state: unknown;
}

/**
* Parameters provided for the `SessionIndex.clearAll` method that determine which session index
* values should be cleared (removed from the index).
*/
export interface ClearAllSessionFilter {
/**
* Descriptor of the authentication provider that created sessions that should be cleared. Provider
* name is optional.
*/
provider: { type: string; name?: string };
/**
* Optional name of the user whose sessions should be cleared.
*/
username?: string;
}

/**
* The SIDs and AAD must be unpredictable to prevent guessing attacks, where an attacker is able to
* guess or predict the ID of a valid session through statistical analysis techniques. That's why we
Expand Down Expand Up @@ -375,6 +391,37 @@ export class Session {
sessionLogger.debug('Successfully invalidated session.');
}

/**
* Clears all existing session values.
* @param request Request instance to clear session value for.
* @param [filter] Filter that narrows down the list of the sessions that should be cleared.
*/
async clearAll(request: KibanaRequest, filter?: ClearAllSessionFilter) {
// For this case method we don't require request to have the associated session, but nevertheless
// we still want to log the SID if session is available.
const sessionCookieValue = await this.options.sessionCookie.get(request);
const sessionLogger = this.getLoggerForSID(sessionCookieValue?.sid);
sessionLogger.debug('Invalidating sessions.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it might be nice to include the filters used as part of this invalidate operation. I think even a simple JSON.stringify would be good enough

Copy link
Member Author

@azasypkin azasypkin Feb 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, we can do that. I was a bit concerned about logging a username if it's provided as afaik we never logged it anywhere before. It's probably too paranoid though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. Feel free to omit for now then


const clearIndexFilter = filter?.username
? {
provider: filter.provider,
usernameHash: createHash('sha3-256').update(filter.username).digest('hex'),
}
: filter;

// There are two things to be aware of here:
// 1. We don't clear the cookie for the current session as we cannot be sure that we removed the
// session index value for this session, but it's not a big deal since it will be automatically
// cleared as soon as it's reused anyway.
// 2. We only remove session index values and don't invalidate any Elasticsearch tokens that
// may have been stored there since we cannot decrypt the session content. To decrypt it we need
// AAD string that is separately stored in the user browser cookie.
const invalidatedSessionsCount = await this.options.sessionIndex.clearAll(clearIndexFilter);
sessionLogger.debug(`Successfully invalidated ${invalidatedSessionsCount} session(s).`);
return invalidatedSessionsCount;
}

private calculateExpiry(
provider: AuthenticationProvider,
currentLifespanExpiration?: number | null
Expand Down Expand Up @@ -411,9 +458,9 @@ export class Session {

/**
* Creates logger scoped to a specified session ID.
* @param sid Session ID to create logger for.
* @param [sid] Session ID to create logger for.
*/
private getLoggerForSID(sid: string) {
return this.options.logger.get(sid?.slice(-10));
private getLoggerForSID(sid?: string) {
return this.options.logger.get(sid?.slice(-10) ?? 'x'.repeat(10));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: alternatively we can just use parent "context" if SID isn't provided.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be nice to explicitly denote that this is a session-less request. What do you think about *no_session* or similar?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I like no_session and it's exactly 10 chars, thanks! 🙂

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@
*/

import type { PublicMethodsOf } from '@kbn/utility-types';
import { SessionIndex, SessionIndexValue } from './session_index';
import type { SessionIndex, SessionIndexValue } from './session_index';

export const sessionIndexMock = {
create: (): jest.Mocked<PublicMethodsOf<SessionIndex>> => ({
get: jest.fn(),
create: jest.fn(),
update: jest.fn(),
clear: jest.fn(),
clearAll: jest.fn(),
initialize: jest.fn(),
cleanUp: jest.fn(),
}),
Expand Down
Loading