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

Enables preventing access to internal APIs #156935

Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
2f8af4e
Adds check to ensure the internal product header is provided, adds he…
TinaHeiligers Apr 28, 2023
34a0e5f
Adds internal origin header in fetch
TinaHeiligers Apr 30, 2023
3adc1df
Adds header to other request constructors that do not use core fetch
TinaHeiligers May 1, 2023
a8ca696
adds validation on not injecting internal product header
TinaHeiligers May 3, 2023
dc0f209
Adds api_integration tests for enforcing restrictInternalApis to http…
TinaHeiligers May 4, 2023
5a7463f
Enables restricting access to internal APIs in serverless mode
TinaHeiligers May 5, 2023
e200669
adds header to isKibanaRequest in spaces plugin
TinaHeiligers May 5, 2023
4d90466
Adds new header to security redirect request checked
TinaHeiligers May 5, 2023
0a0cb44
Adds header to x-pack/dev_tools
TinaHeiligers May 5, 2023
e823131
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine May 5, 2023
6070213
Fix types
TinaHeiligers May 6, 2023
7d6038a
Type fixes
TinaHeiligers May 6, 2023
59583ba
more type fixes
TinaHeiligers May 6, 2023
78a69ea
fix types and tests 1
TinaHeiligers May 6, 2023
8a5fedd
fix core usage stats client unit tests
TinaHeiligers May 6, 2023
12a46cb
Fix typo, comment out for documenting intent only
TinaHeiligers May 6, 2023
30c441f
fix test
TinaHeiligers May 6, 2023
5324c20
Adds config setting to docker list
TinaHeiligers May 7, 2023
3cae610
removes header from core createKibanaRequest RouterMock
TinaHeiligers May 7, 2023
78c0fc0
deletes debug console
TinaHeiligers May 7, 2023
1a9247b
Merge branch 'main' into kbn-152282-http-handle-access-param-in-core-…
kibanamachine May 7, 2023
01e4f1e
updates codeowners file
TinaHeiligers May 8, 2023
f5b2a27
Merge branch 'main' into kbn-152282-http-handle-access-param-in-core-…
TinaHeiligers May 8, 2023
cb86813
update types
TinaHeiligers May 9, 2023
e321d1b
tests happy path for retriction enforced on internal routes
TinaHeiligers May 9, 2023
f38d8bd
Removes internal product origin header from canRedirectRequest
TinaHeiligers May 9, 2023
24ff4c3
Merge branch 'main' into kbn-152282-http-handle-access-param-in-core-…
kibanamachine May 10, 2023
0093f59
Merge branch 'main' into kbn-152282-http-handle-access-param-in-core-…
kibanamachine May 10, 2023
3bd27de
Merge branch 'main' into kbn-152282-http-handle-access-param-in-core-…
mistic May 10, 2023
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
1 change: 1 addition & 0 deletions .github/CODEOWNERS
Original file line number Diff line number Diff line change
Expand Up @@ -922,6 +922,7 @@ x-pack/test/observability_functional @elastic/actionable-observability
/WORKSPACE.bazel @elastic/kibana-operations
/.buildkite/ @elastic/kibana-operations
/kbn_pm/ @elastic/kibana-operations
/x-pack/dev-tools @elastic/kibana-operations

# Appex QA
/src/dev/code_coverage @elastic/appex-qa
Expand Down
2 changes: 2 additions & 0 deletions config/serverless.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ xpack.license_management.enabled: false
#xpack.canvas.enabled: false #only disabable in dev-mode
xpack.reporting.enabled: false

# Enforce restring access to internal APIs see https://github.com/elastic/kibana/issues/151940
# server.restrictInternalApis: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added here for intended purpose. Once all intra-stack components and Kibana browser-side code not using Core's http service are updated, we can enable the restriction.

# Telemetry enabled by default and not disableable via UI
telemetry.optIn: true
telemetry.allowChangingOptInStatus: false
21 changes: 20 additions & 1 deletion packages/core/http/core-http-browser-internal/src/fetch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ describe('Fetch', () => {
expect(fetchMock.lastOptions()!.headers).toMatchObject({
'content-type': 'application/json',
'kbn-version': 'VERSION',
'x-elastic-internal-origin': 'Kibana',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're not concerned about the content here, only that the header is present.

myheader: 'foo',
});
});
Expand All @@ -168,13 +169,30 @@ describe('Fetch', () => {
fetchMock.get('*', {});
await expect(
fetchInstance.fetch('/my/path', {
headers: { myHeader: 'foo', 'kbn-version': 'CUSTOM!' },
headers: {
myHeader: 'foo',
'kbn-version': 'CUSTOM!',
},
})
).rejects.toThrowErrorMatchingInlineSnapshot(
`"Invalid fetch headers, headers beginning with \\"kbn-\\" are not allowed: [kbn-version]"`
);
});

it('should not allow overwriting of x-elastic-internal-origin header', async () => {
fetchMock.get('*', {});
await expect(
fetchInstance.fetch('/my/path', {
headers: {
myHeader: 'foo',
'x-elastic-internal-origin': 'anything',
},
})
).rejects.toThrowErrorMatchingInlineSnapshot(
`"Invalid fetch headers, headers beginning with \\"x-elastic-internal-\\" are not allowed: [x-elastic-internal-origin]"`
);
});

it('should not set kbn-system-request header by default', async () => {
fetchMock.get('*', {});
await fetchInstance.fetch('/my/path', {
Expand Down Expand Up @@ -310,6 +328,7 @@ describe('Fetch', () => {
headers: {
'content-type': 'application/json',
'kbn-version': 'VERSION',
'x-elastic-internal-origin': 'Kibana',
},
});
});
Expand Down
23 changes: 19 additions & 4 deletions packages/core/http/core-http-browser-internal/src/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ import type {
HttpResponse,
HttpFetchOptionsWithPath,
} from '@kbn/core-http-browser';
import { ELASTIC_HTTP_VERSION_HEADER } from '@kbn/core-http-common';
import {
ELASTIC_HTTP_VERSION_HEADER,
X_ELASTIC_INTERNAL_ORIGIN_REQUEST,
} from '@kbn/core-http-common';
import { HttpFetchError } from './http_fetch_error';
import { HttpInterceptController } from './http_intercept_controller';
import { interceptRequest, interceptResponse } from './intercept';
Expand Down Expand Up @@ -131,6 +134,7 @@ export class Fetch {
...options.headers,
'kbn-version': this.params.kibanaVersion,
[ELASTIC_HTTP_VERSION_HEADER]: version,
[X_ELASTIC_INTERNAL_ORIGIN_REQUEST]: 'Kibana',
...(!isEmpty(context) ? new ExecutionContextContainer(context).toHeader() : {}),
}),
};
Expand Down Expand Up @@ -223,12 +227,23 @@ const validateFetchArguments = (
);
}

const invalidHeaders = Object.keys(fullOptions.headers ?? {}).filter((headerName) =>
const invalidKbnHeaders = Object.keys(fullOptions.headers ?? {}).filter((headerName) =>
headerName.startsWith('kbn-')
);
if (invalidHeaders.length) {
const invalidInternalOriginProducHeader = Object.keys(fullOptions.headers ?? {}).filter(
(headerName) => headerName.includes(X_ELASTIC_INTERNAL_ORIGIN_REQUEST)
);

if (invalidKbnHeaders.length) {
throw new Error(
`Invalid fetch headers, headers beginning with "kbn-" are not allowed: [${invalidKbnHeaders.join(
','
)}]`
);
}
if (invalidInternalOriginProducHeader.length) {
throw new Error(
`Invalid fetch headers, headers beginning with "kbn-" are not allowed: [${invalidHeaders.join(
`Invalid fetch headers, headers beginning with "x-elastic-internal-" are not allowed: [${invalidInternalOriginProducHeader.join(
','
)}]`
);
Expand Down
1 change: 1 addition & 0 deletions packages/core/http/core-http-browser/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ export interface IAnonymousPaths {
/**
* Headers to append to the request. Any headers that begin with `kbn-` are considered private to Core and will cause
* {@link HttpHandler} to throw an error.
* Includes the required Header that validates internal requests to internal APIs
* @public
*/
export interface HttpHeadersInit {
Expand Down
2 changes: 1 addition & 1 deletion packages/core/http/core-http-common/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,4 @@
export type { IExternalUrlPolicy } from './src/external_url_policy';

export type { ApiVersion } from './src/versioning';
export { ELASTIC_HTTP_VERSION_HEADER } from './src/constants';
export { ELASTIC_HTTP_VERSION_HEADER, X_ELASTIC_INTERNAL_ORIGIN_REQUEST } from './src/constants';
2 changes: 2 additions & 0 deletions packages/core/http/core-http-common/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,5 @@

/** @internal */
export const ELASTIC_HTTP_VERSION_HEADER = 'elastic-api-version' as const;

export const X_ELASTIC_INTERNAL_ORIGIN_REQUEST = 'x-elastic-internal-origin' as const;

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ const configSchema = schema.object(
},
}
),
restrictInternalApis: schema.boolean({ defaultValue: false }), // allow access to internal routes by default to prevent breaking changes in current offerings
Copy link
Contributor Author

@TinaHeiligers TinaHeiligers May 8, 2023

Choose a reason for hiding this comment

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

Code comment to explain the intent.

},
{
validate: (rawConfig) => {
Expand Down Expand Up @@ -223,6 +224,7 @@ export class HttpConfig implements IHttpConfig {
public xsrf: { disableProtection: boolean; allowlist: string[] };
public requestId: { allowFromAnyIp: boolean; ipAllowlist: string[] };
public shutdownTimeout: Duration;
public restrictInternalApis: boolean;

/**
* @internal
Expand Down Expand Up @@ -263,6 +265,7 @@ export class HttpConfig implements IHttpConfig {
this.xsrf = rawHttpConfig.xsrf;
this.requestId = rawHttpConfig.requestId;
this.shutdownTimeout = rawHttpConfig.shutdownTimeout;
this.restrictInternalApis = rawHttpConfig.restrictInternalApis;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,12 @@ import type {
OnPreResponseToolkit,
OnPostAuthToolkit,
OnPreRoutingToolkit,
OnPostAuthHandler,
} from '@kbn/core-http-server';
import { mockRouter } from '@kbn/core-http-router-server-mocks';
import {
createCustomHeadersPreResponseHandler,
createRestrictInternalRoutesPostAuthHandler,
createVersionCheckPostAuthHandler,
createXsrfPostAuthHandler,
} from './lifecycle_handlers';
Expand Down Expand Up @@ -242,6 +244,108 @@ describe('versionCheck post-auth handler', () => {
});
});

describe('restrictInternal post-auth handler', () => {
let toolkit: ToolkitMock;
let responseFactory: ReturnType<typeof mockRouter.createResponseFactory>;

beforeEach(() => {
toolkit = createToolkit();
responseFactory = mockRouter.createResponseFactory();
});
const createForgeRequest = (
access: 'internal' | 'public',
headers: Record<string, string> | undefined = {}
) => {
return forgeRequest({
method: 'get',
headers,
path: `/${access}/some-path`,
kibanaRouteOptions: {
xsrfRequired: false,
access,
},
});
};

const createForwardSuccess = (handler: OnPostAuthHandler, request: KibanaRequest) => {
toolkit.next.mockReturnValue('next' as any);
const result = handler(request, responseFactory, toolkit);

expect(toolkit.next).toHaveBeenCalledTimes(1);
expect(responseFactory.badRequest).not.toHaveBeenCalled();
expect(result).toBe('next');
};

describe('when restriction is enabled', () => {
const config = createConfig({
name: 'my-server-name',
restrictInternalApis: true,
});
it('returns a bad request if called without internal origin header for internal API', () => {
const handler = createRestrictInternalRoutesPostAuthHandler(config as HttpConfig);
const request = createForgeRequest('internal');

responseFactory.badRequest.mockReturnValue('badRequest' as any);

const result = handler(request, responseFactory, toolkit);

expect(toolkit.next).not.toHaveBeenCalled();
expect(responseFactory.badRequest.mock.calls[0][0]?.body).toMatch(
/uri \[.*\/internal\/some-path\] with method \[get\] exists but is not available with the current configuration/
);
expect(result).toBe('badRequest');
});

it('forward the request to the next interceptor if called with internal origin header for internal API', () => {
const handler = createRestrictInternalRoutesPostAuthHandler(config as HttpConfig);
const request = createForgeRequest('internal', { 'x-elastic-internal-origin': 'Kibana' });
createForwardSuccess(handler, request);
});

it('forward the request to the next interceptor if called with internal origin header for public APIs', () => {
const handler = createRestrictInternalRoutesPostAuthHandler(config as HttpConfig);
const request = createForgeRequest('public', { 'x-elastic-internal-origin': 'Kibana' });
createForwardSuccess(handler, request);
});

it('forward the request to the next interceptor if called without internal origin header for public APIs', () => {
const handler = createRestrictInternalRoutesPostAuthHandler(config as HttpConfig);
const request = createForgeRequest('public');
createForwardSuccess(handler, request);
});
});

describe('when restriction is not enabled', () => {
const config = createConfig({
name: 'my-server-name',
restrictInternalApis: false,
});
it('forward the request to the next interceptor if called without internal origin header for internal APIs', () => {
const handler = createRestrictInternalRoutesPostAuthHandler(config as HttpConfig);
const request = createForgeRequest('internal');
createForwardSuccess(handler, request);
});

it('forward the request to the next interceptor if called with internal origin header for internal API', () => {
const handler = createRestrictInternalRoutesPostAuthHandler(config as HttpConfig);
const request = createForgeRequest('internal', { 'x-elastic-internal-origin': 'Kibana' });
createForwardSuccess(handler, request);
});

it('forward the request to the next interceptor if called without internal origin header for public APIs', () => {
const handler = createRestrictInternalRoutesPostAuthHandler(config as HttpConfig);
const request = createForgeRequest('public');
createForwardSuccess(handler, request);
});

it('forward the request to the next interceptor if called with internal origin header for public APIs', () => {
const handler = createRestrictInternalRoutesPostAuthHandler(config as HttpConfig);
const request = createForgeRequest('public', { 'x-elastic-internal-origin': 'Kibana' });
createForwardSuccess(handler, request);
});
});
});

describe('customHeaders pre-response handler', () => {
let toolkit: ToolkitMock;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import { Env } from '@kbn/config';
import type { OnPostAuthHandler, OnPreResponseHandler } from '@kbn/core-http-server';
import { isSafeMethod } from '@kbn/core-http-router-server-internal';
import { X_ELASTIC_INTERNAL_ORIGIN_REQUEST } from '@kbn/core-http-common/src/constants';
import { HttpConfig } from './http_config';
import { LifecycleRegistrar } from './http_server';

Expand Down Expand Up @@ -39,6 +40,27 @@ export const createXsrfPostAuthHandler = (config: HttpConfig): OnPostAuthHandler
};
};

export const createRestrictInternalRoutesPostAuthHandler = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This takes a similar approach to createVersionCheckPostAuthHandler.

config: HttpConfig
): OnPostAuthHandler => {
const isRestrictionEnabled = config.restrictInternalApis;

return (request, response, toolkit) => {
const isInternalRoute = request.route.options.access === 'internal';

// only check if the header is present, not it's content.
const hasInternalKibanaRequestHeader = X_ELASTIC_INTERNAL_ORIGIN_REQUEST in request.headers;

if (isRestrictionEnabled && isInternalRoute && !hasInternalKibanaRequestHeader) {
TinaHeiligers marked this conversation as resolved.
Show resolved Hide resolved
// throw 400
return response.badRequest({
body: `uri [${request.url}] with method [${request.route.method}] exists but is not available with the current configuration`,
TinaHeiligers marked this conversation as resolved.
Show resolved Hide resolved
});
}
return toolkit.next();
};
};

export const createVersionCheckPostAuthHandler = (kibanaVersion: string): OnPostAuthHandler => {
return (request, response, toolkit) => {
const requestVersion = request.headers[VERSION_HEADER];
Expand All @@ -60,7 +82,6 @@ export const createVersionCheckPostAuthHandler = (kibanaVersion: string): OnPost
};
};

// TODO: implement header required for accessing internal routes. See https://github.com/elastic/kibana/issues/151940
export const createCustomHeadersPreResponseHandler = (config: HttpConfig): OnPreResponseHandler => {
const {
name: serverName,
Expand All @@ -76,7 +97,6 @@ export const createCustomHeadersPreResponseHandler = (config: HttpConfig): OnPre
'Content-Security-Policy': cspHeader,
[KIBANA_NAME_HEADER]: serverName,
};

return toolkit.next({ headers: additionalHeaders });
};
};
Expand All @@ -86,7 +106,12 @@ export const registerCoreHandlers = (
config: HttpConfig,
env: Env
) => {
// add headers based on config
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't often update these and I added comments to more easily follow where they're applied.

registrar.registerOnPreResponse(createCustomHeadersPreResponseHandler(config));
// add extra request checks stuff
registrar.registerOnPostAuth(createXsrfPostAuthHandler(config));
// add check on version
registrar.registerOnPostAuth(createVersionCheckPostAuthHandler(env.packageInfo.version));
// add check on header if the route is internal
registrar.registerOnPostAuth(createRestrictInternalRoutesPostAuthHandler(config)); // strictly speaking, we should have access to route.options.access from the request on postAuth
};
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ const createConfigService = () => {
shutdownTimeout: moment.duration(30, 'seconds'),
keepaliveTimeout: 120_000,
socketTimeout: 120_000,
restrictInternalApis: false,
} as any);
}
if (path === 'externalUrl') {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,11 @@ describe('throwIfAnyTypeNotVisibleByAPI', () => {

describe('logWarnOnExternalRequest', () => {
let logger: MockedLogger;
const firstPartyRequestHeaders = { 'kbn-version': 'a', referer: 'b' };
const firstPartyRequestHeaders = {
'kbn-version': 'a',
referer: 'b',
'x-elastic-internal-origin': 'foo',
};
const kibRequest = httpServerMock.createKibanaRequest({ headers: firstPartyRequestHeaders });
const extRequest = httpServerMock.createKibanaRequest();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,9 @@ export interface BulkGetItem {
export function isKibanaRequest({ headers }: KibanaRequest) {
// The presence of these two request headers gives us a good indication that this is a first-party request from the Kibana client.
// We can't be 100% certain, but this is a reasonable attempt.
return headers && headers['kbn-version'] && headers.referer;
return (
headers && headers['kbn-version'] && headers.referer && headers['x-elastic-internal-origin']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ATM, there are 4: core usage stats client, cases plugin, rule registry, and spaces usage stats client. I've added the header to core's implementation.

Are the concerns Joe raised still val a valid reason to have 4 implementations of isKibanaRequest or can we add the extra header to that check?

A while back, Joe raised concerns about adding a header to all Kibana requests:

  1. other integrations might attempt to use this header
  2. adding a new header might present problems for users with reverse proxies that may strip headers not in an allow-list; this would cause false negative for first-party request detection

If these are still valid, then having different implementations is warrented. @response-ops @kibana-security would a single method fullfil your needs or are concerns around false negatives be an issue for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO (1) and (2) should be mitigated because we will not be restricting access to internal endpoints by default so everything should continue working as-is for on-prem.

);
}

export interface LogWarnOnExternalRequest {
Expand Down
Loading