Skip to content

Commit

Permalink
Prevent Kerberos and PKI providers from initiating a new session for …
Browse files Browse the repository at this point in the history 
…unauthenticated XHR/API requests. (elastic#82817)

* Prevent Kerberos and PKI providers from initiating a new session for unauthenticated XHR requests.

* Review#1: fix comment.
# Conflicts:
#	.github/CODEOWNERS
#	x-pack/scripts/functional_tests.js
  • Loading branch information
@azasypkin
azasypkin committed Nov 9, 2020
1 parent 93d6068 commit 7442883
Show file tree
Hide file tree
Showing 48 changed files with 271 additions and 287 deletions.
63 changes: 39 additions & 24 deletions x-pack/plugins/security/server/authentication/providers/kerberos.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,16 @@ describe('KerberosAuthenticationProvider', () => {
expect(mockOptions.client.callAsInternalUser).not.toHaveBeenCalled();
});

it('does not start SPNEGO for Ajax requests.', async () => {
const request = httpServerMock.createKibanaRequest({ headers: { 'kbn-xsrf': 'xsrf' } });
await expect(provider.authenticate(request)).resolves.toEqual(
AuthenticationResult.notHandled()
);

expect(mockOptions.client.asScoped).not.toHaveBeenCalled();
expect(mockOptions.client.callAsInternalUser).not.toHaveBeenCalled();
});

it('succeeds if state contains a valid token.', async () => {
const user = mockAuthenticatedUser();
const request = httpServerMock.createKibanaRequest({ headers: {} });
Expand Down Expand Up @@ -442,9 +452,6 @@ describe('KerberosAuthenticationProvider', () => {
});

it('fails with `Negotiate` challenge if both access and refresh tokens from the state are expired and backend supports Kerberos.', async () => {
const request = httpServerMock.createKibanaRequest();
const tokenPair = { accessToken: 'expired-token', refreshToken: 'some-valid-refresh-token' };

const failureReason = LegacyElasticsearchErrorHelpers.decorateNotAuthorizedError(
new (errors.AuthenticationException as any)('Unauthorized', {
body: { error: { header: { 'WWW-Authenticate': 'Negotiate' } } },
Expand All @@ -456,37 +463,45 @@ describe('KerberosAuthenticationProvider', () => {

mockOptions.tokens.refresh.mockResolvedValue(null);

await expect(provider.authenticate(request, tokenPair)).resolves.toEqual(
const nonAjaxRequest = httpServerMock.createKibanaRequest();
const nonAjaxTokenPair = {
accessToken: 'expired-token',
refreshToken: 'some-valid-refresh-token',
};
await expect(provider.authenticate(nonAjaxRequest, nonAjaxTokenPair)).resolves.toEqual(
AuthenticationResult.failed(failureReason, {
authResponseHeaders: { 'WWW-Authenticate': 'Negotiate' },
})
);

expect(mockOptions.tokens.refresh).toHaveBeenCalledTimes(1);
expect(mockOptions.tokens.refresh).toHaveBeenCalledWith(tokenPair.refreshToken);
});

it('does not re-start SPNEGO if both access and refresh tokens from the state are expired.', async () => {
const request = httpServerMock.createKibanaRequest({ routeAuthRequired: false });
const tokenPair = { accessToken: 'expired-token', refreshToken: 'some-valid-refresh-token' };

const failureReason = LegacyElasticsearchErrorHelpers.decorateNotAuthorizedError(
new (errors.AuthenticationException as any)('Unauthorized', {
body: { error: { header: { 'WWW-Authenticate': 'Negotiate' } } },
const ajaxRequest = httpServerMock.createKibanaRequest({ headers: { 'kbn-xsrf': 'xsrf' } });
const ajaxTokenPair = {
accessToken: 'expired-token',
refreshToken: 'ajax-some-valid-refresh-token',
};
await expect(provider.authenticate(ajaxRequest, ajaxTokenPair)).resolves.toEqual(
AuthenticationResult.failed(failureReason, {
authResponseHeaders: { 'WWW-Authenticate': 'Negotiate' },
})
);
const mockScopedClusterClient = elasticsearchServiceMock.createLegacyScopedClusterClient();
mockScopedClusterClient.callAsCurrentUser.mockRejectedValue(failureReason);
mockOptions.client.asScoped.mockReturnValue(mockScopedClusterClient);

mockOptions.tokens.refresh.mockResolvedValue(null);

await expect(provider.authenticate(request, tokenPair)).resolves.toEqual(
AuthenticationResult.notHandled()
const optionalAuthRequest = httpServerMock.createKibanaRequest({ routeAuthRequired: false });
const optionalAuthTokenPair = {
accessToken: 'expired-token',
refreshToken: 'optional-some-valid-refresh-token',
};
await expect(
provider.authenticate(optionalAuthRequest, optionalAuthTokenPair)
).resolves.toEqual(
AuthenticationResult.failed(failureReason, {
authResponseHeaders: { 'WWW-Authenticate': 'Negotiate' },
})
);

expect(mockOptions.tokens.refresh).toHaveBeenCalledTimes(1);
expect(mockOptions.tokens.refresh).toHaveBeenCalledWith(tokenPair.refreshToken);
expect(mockOptions.tokens.refresh).toHaveBeenCalledTimes(3);
expect(mockOptions.tokens.refresh).toHaveBeenCalledWith(nonAjaxTokenPair.refreshToken);
expect(mockOptions.tokens.refresh).toHaveBeenCalledWith(ajaxTokenPair.refreshToken);
expect(mockOptions.tokens.refresh).toHaveBeenCalledWith(optionalAuthTokenPair.refreshToken);
});
});

Expand Down
37 changes: 20 additions & 17 deletions x-pack/plugins/security/server/authentication/providers/kerberos.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
import { AuthenticationResult } from '../authentication_result';
import { DeauthenticationResult } from '../deauthentication_result';
import { HTTPAuthorizationHeader } from '../http_authentication';
import { canRedirectRequest } from '../can_redirect_request';
import { Tokens, TokenPair } from '../tokens';
import { BaseAuthenticationProvider } from './base';

Expand All @@ -32,8 +33,9 @@ const WWWAuthenticateHeaderName = 'WWW-Authenticate';
* @param request Request instance.
*/
function canStartNewSession(request: KibanaRequest) {
// We should try to establish new session only if request requires authentication.
return request.route.options.authRequired === true;
// We should try to establish new session only if request requires authentication and it's not an XHR request.
// Technically we can authenticate XHR requests too, but we don't want these to create a new session unintentionally.
return canRedirectRequest(request) && request.route.options.authRequired === true;
}

/**
Expand Down Expand Up @@ -75,11 +77,8 @@ export class KerberosAuthenticationProvider extends BaseAuthenticationProvider {
return AuthenticationResult.notHandled();
}

let authenticationResult = authorizationHeader
? await this.authenticateWithNegotiateScheme(request)
: AuthenticationResult.notHandled();

if (state && authenticationResult.notHandled()) {
let authenticationResult = AuthenticationResult.notHandled();
if (state) {
authenticationResult = await this.authenticateViaState(request, state);
if (
authenticationResult.failed() &&
Expand All @@ -89,11 +88,15 @@ export class KerberosAuthenticationProvider extends BaseAuthenticationProvider {
}
}

// If we couldn't authenticate by means of all methods above, let's try to check if Elasticsearch can
// start authentication mechanism negotiation, otherwise just return authentication result we have.
return authenticationResult.notHandled() && canStartNewSession(request)
? await this.authenticateViaSPNEGO(request, state)
: authenticationResult;
if (!authenticationResult.notHandled() || !canStartNewSession(request)) {
return authenticationResult;
}

// If we couldn't authenticate by means of all methods above, let's check if we're already at the authentication
// mechanism negotiation stage, otherwise check with Elasticsearch if we can start it.
return authorizationHeader
? await this.authenticateWithNegotiateScheme(request)
: await this.authenticateViaSPNEGO(request, state);
}

/**
Expand Down Expand Up @@ -264,12 +267,12 @@ export class KerberosAuthenticationProvider extends BaseAuthenticationProvider {
return AuthenticationResult.failed(err);
}

// If refresh token is no longer valid, then we should clear session and renegotiate using SPNEGO.
// If refresh token is no longer valid, let's try to renegotiate new tokens using SPNEGO. We
// allow this because expired underlying token is an implementation detail and Kibana user
// facing session is still valid.
if (refreshedTokenPair === null) {
this.logger.debug('Both access and refresh tokens are expired.');
return canStartNewSession(request)
? this.authenticateViaSPNEGO(request, state)
: AuthenticationResult.notHandled();
this.logger.debug('Both access and refresh tokens are expired. Re-authenticating...');
return this.authenticateViaSPNEGO(request, state);
}

try {
Expand Down
138 changes: 97 additions & 41 deletions x-pack/plugins/security/server/authentication/providers/pki.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,22 @@ describe('PKIAuthenticationProvider', () => {
expect(mockOptions.client.callAsInternalUser).not.toHaveBeenCalled();
});

it('does not exchange peer certificate to access token for Ajax requests.', async () => {
const request = httpServerMock.createKibanaRequest({
headers: { 'kbn-xsrf': 'xsrf' },
socket: getMockSocket({
authorized: true,
peerCertificate: getMockPeerCertificate(['2A:7A:C2:DD', '3B:8B:D3:EE']),
}),
});
await expect(provider.authenticate(request)).resolves.toEqual(
AuthenticationResult.notHandled()
);

expect(mockOptions.client.asScoped).not.toHaveBeenCalled();
expect(mockOptions.client.callAsInternalUser).not.toHaveBeenCalled();
});

it('fails with non-401 error if state is available, peer is authorized, but certificate is not available.', async () => {
const request = httpServerMock.createKibanaRequest({
socket: getMockSocket({ authorized: true }),
Expand Down Expand Up @@ -383,14 +399,7 @@ describe('PKIAuthenticationProvider', () => {
});

it('gets a new access token even if existing token is expired.', async () => {
const user = mockAuthenticatedUser();
const request = httpServerMock.createKibanaRequest({
socket: getMockSocket({
authorized: true,
peerCertificate: getMockPeerCertificate(['2A:7A:C2:DD', '3B:8B:D3:EE']),
}),
});
const state = { accessToken: 'existing-token', peerCertificateFingerprint256: '2A:7A:C2:DD' };
const user = mockAuthenticatedUser({ authentication_provider: { type: 'pki', name: 'pki' } });

const mockScopedClusterClient = elasticsearchServiceMock.createLegacyScopedClusterClient();
mockScopedClusterClient.callAsCurrentUser
Expand All @@ -399,55 +408,102 @@ describe('PKIAuthenticationProvider', () => {
LegacyElasticsearchErrorHelpers.decorateNotAuthorizedError(new Error())
)
// In response to a call with a new token.
.mockResolvedValueOnce(user) // In response to call with an expired token.
.mockRejectedValueOnce(
LegacyElasticsearchErrorHelpers.decorateNotAuthorizedError(new Error())
)
// In response to a call with a new token.
.mockResolvedValueOnce(user) // In response to call with an expired token.
.mockRejectedValueOnce(
LegacyElasticsearchErrorHelpers.decorateNotAuthorizedError(new Error())
)
// In response to a call with a new token.
.mockResolvedValueOnce(user);
mockOptions.client.asScoped.mockReturnValue(mockScopedClusterClient);
mockOptions.client.callAsInternalUser.mockResolvedValue({ access_token: 'access-token' });

await expect(provider.authenticate(request, state)).resolves.toEqual(
AuthenticationResult.succeeded(
{ ...user, authentication_provider: { type: 'pki', name: 'pki' } },
{
authHeaders: { authorization: 'Bearer access-token' },
state: { accessToken: 'access-token', peerCertificateFingerprint256: '2A:7A:C2:DD' },
}
)
const nonAjaxRequest = httpServerMock.createKibanaRequest({
socket: getMockSocket({
authorized: true,
peerCertificate: getMockPeerCertificate(['2A:7A:C2:DD', '3B:8B:D3:EE']),
}),
});
const nonAjaxState = {
accessToken: 'existing-token',
peerCertificateFingerprint256: '2A:7A:C2:DD',
};
await expect(provider.authenticate(nonAjaxRequest, nonAjaxState)).resolves.toEqual(
AuthenticationResult.succeeded(user, {
authHeaders: { authorization: 'Bearer access-token' },
state: { accessToken: 'access-token', peerCertificateFingerprint256: '2A:7A:C2:DD' },
})
);

expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1);
expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledWith('shield.delegatePKI', {
body: {
x509_certificate_chain: [
'fingerprint:2A:7A:C2:DD:base64',
'fingerprint:3B:8B:D3:EE:base64',
],
},
const ajaxRequest = httpServerMock.createKibanaRequest({
headers: { 'kbn-xsrf': 'xsrf' },
socket: getMockSocket({
authorized: true,
peerCertificate: getMockPeerCertificate(['3A:7A:C2:DD', '3B:8B:D3:EE']),
}),
});
const ajaxState = {
accessToken: 'existing-token',
peerCertificateFingerprint256: '3A:7A:C2:DD',
};
await expect(provider.authenticate(ajaxRequest, ajaxState)).resolves.toEqual(
AuthenticationResult.succeeded(user, {
authHeaders: { authorization: 'Bearer access-token' },
state: { accessToken: 'access-token', peerCertificateFingerprint256: '3A:7A:C2:DD' },
})
);

expect(request.headers).not.toHaveProperty('authorization');
});

it('does not exchange peer certificate to a new access token even if existing token is expired and request does not require authentication.', async () => {
const request = httpServerMock.createKibanaRequest({
const optionalAuthRequest = httpServerMock.createKibanaRequest({
routeAuthRequired: false,
socket: getMockSocket({
authorized: true,
peerCertificate: getMockPeerCertificate(['2A:7A:C2:DD', '3B:8B:D3:EE']),
peerCertificate: getMockPeerCertificate(['4A:7A:C2:DD', '3B:8B:D3:EE']),
}),
});
const state = { accessToken: 'existing-token', peerCertificateFingerprint256: '2A:7A:C2:DD' };

const mockScopedClusterClient = elasticsearchServiceMock.createLegacyScopedClusterClient();
mockScopedClusterClient.callAsCurrentUser.mockRejectedValueOnce(
LegacyElasticsearchErrorHelpers.decorateNotAuthorizedError(new Error())
const optionalAuthState = {
accessToken: 'existing-token',
peerCertificateFingerprint256: '4A:7A:C2:DD',
};
await expect(provider.authenticate(optionalAuthRequest, optionalAuthState)).resolves.toEqual(
AuthenticationResult.succeeded(user, {
authHeaders: { authorization: 'Bearer access-token' },
state: { accessToken: 'access-token', peerCertificateFingerprint256: '4A:7A:C2:DD' },
})
);
mockOptions.client.asScoped.mockReturnValue(mockScopedClusterClient);

await expect(provider.authenticate(request, state)).resolves.toEqual(
AuthenticationResult.notHandled()
);
expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(3);
expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledWith('shield.delegatePKI', {
body: {
x509_certificate_chain: [
'fingerprint:2A:7A:C2:DD:base64',
'fingerprint:3B:8B:D3:EE:base64',
],
},
});
expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledWith('shield.delegatePKI', {
body: {
x509_certificate_chain: [
'fingerprint:3A:7A:C2:DD:base64',
'fingerprint:3B:8B:D3:EE:base64',
],
},
});
expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledWith('shield.delegatePKI', {
body: {
x509_certificate_chain: [
'fingerprint:4A:7A:C2:DD:base64',
'fingerprint:3B:8B:D3:EE:base64',
],
},
});

expect(mockOptions.client.callAsInternalUser).not.toHaveBeenCalled();
expect(request.headers).not.toHaveProperty('authorization');
expect(nonAjaxRequest.headers).not.toHaveProperty('authorization');
expect(ajaxRequest.headers).not.toHaveProperty('authorization');
expect(optionalAuthRequest.headers).not.toHaveProperty('authorization');
});

it('fails with 401 if existing token is expired, but certificate is not present.', async () => {
Expand Down
20 changes: 11 additions & 9 deletions x-pack/plugins/security/server/authentication/providers/pki.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { KibanaRequest } from '../../../../../../src/core/server';
import { AuthenticationResult } from '../authentication_result';
import { DeauthenticationResult } from '../deauthentication_result';
import { HTTPAuthorizationHeader } from '../http_authentication';
import { canRedirectRequest } from '../can_redirect_request';
import { Tokens } from '../tokens';
import { BaseAuthenticationProvider } from './base';

Expand All @@ -33,8 +34,9 @@ interface ProviderState {
* @param request Request instance.
*/
function canStartNewSession(request: KibanaRequest) {
// We should try to establish new session only if request requires authentication.
return request.route.options.authRequired === true;
// We should try to establish new session only if request requires authentication and it's not an XHR request.
// Technically we can authenticate XHR requests too, but we don't want these to create a new session unintentionally.
return canRedirectRequest(request) && request.route.options.authRequired === true;
}

/**
Expand Down Expand Up @@ -75,27 +77,27 @@ export class PKIAuthenticationProvider extends BaseAuthenticationProvider {
authenticationResult = await this.authenticateViaState(request, state);

// If access token expired or doesn't match to the certificate fingerprint we should try to get
// a new one in exchange to peer certificate chain assuming request can initiate new session.
// a new one in exchange to peer certificate chain. Since we know that we had a valid session
// before we can safely assume that it's desired to automatically re-create session even for XHR
// requests.
const invalidAccessToken =
authenticationResult.notHandled() ||
(authenticationResult.failed() &&
Tokens.isAccessTokenExpiredError(authenticationResult.error));
if (invalidAccessToken && canStartNewSession(request)) {
if (invalidAccessToken) {
authenticationResult = await this.authenticateViaPeerCertificate(request);
// If we have an active session that we couldn't use to authenticate user and at the same time
// we couldn't use peer's certificate to establish a new one, then we should respond with 401
// and force authenticator to clear the session.
if (authenticationResult.notHandled()) {
return AuthenticationResult.failed(Boom.unauthorized());
}
} else if (invalidAccessToken) {
return AuthenticationResult.notHandled();
}
}

// If we couldn't authenticate by means of all methods above, let's try to check if we can authenticate
// request using its peer certificate chain, otherwise just return authentication result we have.
// We shouldn't establish new session if authentication isn't required for this particular request.
// If we couldn't authenticate by means of all methods above, let's check if the request is allowed
// to start a new session, and if so try to authenticate request using its peer certificate chain,
// otherwise just return authentication result we have.
return authenticationResult.notHandled() && canStartNewSession(request)
? await this.authenticateViaPeerCertificate(request)
: authenticationResult;
Expand Down
Loading

0 comments on commit 7442883

Please sign in to comment.