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.

Sign up for GitHub

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

Jump to bottom

Add secondary authentication to Core ES client #184901

Merged
merged 15 commits into from
Jun 13, 2024
Merged

Add secondary authentication to Core ES client #184901

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
1fa0e05
Add secondary authentication to Core ES client
pgayvallet Jun 6, 2024
23e979d
fix client wrapper
pgayvallet Jun 6, 2024
75466ef
fix another client wrapper
pgayvallet Jun 6, 2024
4ba071d
fix another client wrapper
pgayvallet Jun 6, 2024
0c536f6
fix another client wrapper
pgayvallet Jun 6, 2024
ca2d6d3
Merge remote-tracking branch 'upstream/main' into kbn-179458-es-secon…
pgayvallet Jun 10, 2024
fd77242
Merge remote-tracking branch 'upstream/main' into kbn-179458-es-secon…
pgayvallet Jun 10, 2024
02c3819
rename `asSecondaryAuth` to `asSecondaryAuthUser`
pgayvallet Jun 10, 2024
7ad6f90
Merge remote-tracking branch 'upstream/main' into kbn-179458-es-secon…
pgayvallet Jun 10, 2024
cfa29a9
throw error when calling `asSecondaryAuthUser` for a request without …
pgayvallet Jun 10, 2024
7ab4779
Merge remote-tracking branch 'upstream/main' into kbn-179458-es-secon…
pgayvallet Jun 11, 2024
2b27b2d
change the client wrappers to be lazy
pgayvallet Jun 11, 2024
cecc102
Merge remote-tracking branch 'upstream/main' into kbn-179458-es-secon…
pgayvallet Jun 13, 2024
0c014fb
Merge branch 'main' into kbn-179458-es-secondary-auth
kibanamachine Jun 13, 2024
f5d2f01
Merge branch 'main' into kbn-179458-es-secondary-auth
kibanamachine Jun 13, 2024
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
367 changes: 349 additions & 18 deletions ...s/core/elasticsearch/core-elasticsearch-client-server-internal/src/cluster_client.test.ts
View file
Open in desktop

Large diffs are not rendered by default.

60 changes: 48 additions & 12 deletions packages/core/elasticsearch/core-elasticsearch-client-server-internal/src/cluster_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import type {
import type { ElasticsearchClientConfig } from '@kbn/core-elasticsearch-server';
import { configureClient } from './configure_client';
import { ScopedClusterClient } from './scoped_cluster_client';
import { getDefaultHeaders } from './headers';
import { getDefaultHeaders, AUTHORIZATION_HEADER, ES_SECONDARY_AUTH_HEADER } from './headers';
import {
createInternalErrorHandler,
type InternalUnauthorizedErrorHandler,
Expand Down Expand Up @@ -78,28 +78,43 @@ export class ClusterClient implements ICustomClusterClient {
kibanaVersion,
});
this.rootScopedClient = configureClient(config, {
scoped: true,
logger,
type,
getExecutionContext,
scoped: true,
agentFactoryProvider,
kibanaVersion,
});
}

asScoped(request: ScopeableRequest) {
const scopedHeaders = this.getScopedHeaders(request);
const createScopedClient = () => {
const scopedHeaders = this.getScopedHeaders(request);

const transportClass = createTransport({
getExecutionContext: this.getExecutionContext,
getUnauthorizedErrorHandler: this.createInternalErrorHandlerAccessor(request),
});
const transportClass = createTransport({
getExecutionContext: this.getExecutionContext,
getUnauthorizedErrorHandler: this.createInternalErrorHandlerAccessor(request),
});

return this.rootScopedClient.child({
headers: scopedHeaders,
Transport: transportClass,
});
};

const createSecondaryScopedClient = () => {
const secondaryAuthHeaders = this.getSecondaryAuthHeaders(request);

return this.asInternalUser.child({
headers: secondaryAuthHeaders,
});
Comment on lines +108 to +110
Copy link
Member

Choose a reason for hiding this comment

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

question: I see we don't use a custom transport here, does it mean we won't support refresh & re-try for expired secondary authentication credentials (access token, when request is "real") like we do for asCurrentUser?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

Do you have any idea of what of error ES returns when the secondary auth is expired? Because I don't, but I suspect it's different than errors for the primary auth, right?

Also we won't be able to use the current logic of the custom transport, because it only handles primary authentication credentials, so it would mean move specific development for second auth. I'm not opposed doing so, but then it probably can be done in a later stage (in which case I will open an issue for it), would that be alright with you?

Copy link
Member

Choose a reason for hiding this comment

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

Do you have any idea of what of error ES returns when the secondary auth is expired? Because I don't, but I suspect it's different than errors for the primary auth, right?

I'd expect 401 for expired tokens even if the token is a part of the secondary auth, but it's a good question, we need to confirm. Let me see if I can quickly check it locally.

Also we won't be able to use the current logic of the custom transport, because it only handles primary authentication credentials, so it would mean move specific development for second auth. I'm not opposed doing so, but then it probably can be done in a later stage (in which case I will open an issue for it), would that be alright with you?

Oh, I didn't realize we can't reuse the custom transport logic we already have. In any case, I think it's fine to treat it as an enhancement and handle it separately 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't realize we can't reuse the custom transport logic we already have

I mean, looking at the code in createTransport

kibana/packages/core/elasticsearch/core-elasticsearch-client-server-internal/src/create_transport.ts

Lines 59 to 74 in 3508350

if (isUnauthorizedError(e)) {
const unauthorizedErrorHandler = getUnauthorizedErrorHandler
? getUnauthorizedErrorHandler()
: undefined;
if (unauthorizedErrorHandler) {
const result = await unauthorizedErrorHandler(e);
if (isRetryResult(result)) {
this.headers = {
...this.headers,
...result.authHeaders,
};
const retryOpts = { ...opts };
retryOpts.headers = {
...this.headers,
...options?.headers,
};

It should work "out of the box" on Core's side, but I think on the side of the security's handler we would need modifications, to be able to detect which token/header is expired to renew it and to then update the right header?

Copy link
Member

Choose a reason for hiding this comment

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

but I think on the side of the security's handler we would need modifications, to be able to detect which token/header is expired to renew it and to then update the right header?

We might need to tweak our code slightly to remove the es-secondary-authorization header before re-authentication (see here, unless Core does that for us). However, Security doesn't use headers from the request to extend the access token. Instead, we fetch the session for the request that Core provides us in the "unauthorized error handler" from ES and extract credentials to extend from there. So the only thing that matters to Security is what is located in the session.

The problem, I guess, is that Security will return a new access token within the Authorization HTTP header value. We need to decide whether Security should return it within the proper header, or if Core should take the value from Authorization and use it as ES-Secondary-Authorization instead. I'm leaning towards the latter since Core has a better chance of knowing that the request is performed via asSecondaryAuthUser, but we can discuss.

Copy link
Member

Choose a reason for hiding this comment

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

I'd expect 401 for expired tokens even if the token is a part of the secondary auth, but it's a good question, we need to confirm. Let me see if I can quickly check it locally.

Okay, checked:

  1. Run ES with yarn es snapshot --license trial --ssl -E xpack.security.authc.token.timeout=15s to have short-lived tokens
  2. Create an access token with
POST https://localhost:9200/_security/oauth2/token
Authorization: Basic ZWxhc3RpYzpjaGFuZ2VtZQ==
Content-Type: application/json
Accept: application/json

{
  "grant_type" : "client_credentials"
}
---
{
  "access_token": "wOSRBBj15MpZT8HbQdvoEkQFHAlcNC4J2gPqDZEAAAA=",
  "type": "Bearer",
  "expires_in": 15,
  ...
}
  1. Request cluster state API while token is valid
GET https://localhost:9200/_cluster/state
Authorization: Basic ZWxhc3RpYzpjaGFuZ2VtZQ==
ES-Secondary-Authorization: Bearer wOSRBBj15MpZT8HbQdvoEkQFHAlcNC4J2gPqDZEAAAA=
---
200
---
{
  "cluster_name":"elasticsearch",
  ...
}
  1. Request cluster state API after token has expired
GET https://localhost:9200/_cluster/state
Authorization: Basic ZWxhc3RpYzpjaGFuZ2VtZQ==
ES-Secondary-Authorization: Bearer wOSRBBj15MpZT8HbQdvoEkQFHAlcNC4J2gPqDZEAAAA=
---
401
---
{
  "error": {
    "root_cause": [
      {
        "type": "security_exception",
        "reason": "token expired",
        "header": {
          "WWW-Authenticate": "Bearer realm=\"security\", error=\"invalid_token\", error_description=\"The access token expired\""
        }
      }
    ],
    "type": "security_exception",
    "reason": "Failed to authenticate secondary user",
    "caused_by": {
      "type": "security_exception",
      "reason": "token expired",
      "header": {
        "WWW-Authenticate": "Bearer realm=\"security\", error=\"invalid_token\", error_description=\"The access token expired\""
      }
    }
  },
  "status": 401
}

The behavior seems to be exactly the same as for "primary" auth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"exactly the same" seems pretty bad for us though, given we'd ideally like a way to differentiate the two scenarios from within the reauth hook...

Copy link
Member

Choose a reason for hiding this comment

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

"exactly the same" seems pretty bad for us though, given we'd ideally like a way to differentiate the two scenarios from within the reauth hook...

Is it the custom transport that handles re-authentication/re-try on the Core side? I admit I don't know this code, so please correct me if I'm wrong, but if so, couldn't we create transports with different parameters for asCurrentUser and asSecondaryAuthUser to indicate which specific scenario we're handling?

};

const scopedClient = this.rootScopedClient.child({
headers: scopedHeaders,
Transport: transportClass,
return new ScopedClusterClient({
asInternalUser: this.asInternalUser,
asCurrentUserFactory: createScopedClient,
asSecondaryAuthUserFactory: createSecondaryScopedClient,
});
return new ScopedClusterClient(this.asInternalUser, scopedClient);
}

public async close() {
Expand Down Expand Up @@ -129,7 +144,7 @@ export class ClusterClient implements ICustomClusterClient {
if (isRealRequest(request)) {
const requestHeaders = ensureRawRequest(request).headers ?? {};
const requestIdHeaders = isKibanaRequest(request) ? { 'x-opaque-id': request.id } : {};
const authHeaders = this.authHeaders ? this.authHeaders.get(request) : {};
const authHeaders = this.authHeaders?.get(request) ?? {};

scopedHeaders = {
...filterHeaders(requestHeaders, this.config.requestHeadersWhitelist),
Expand All @@ -146,4 +161,25 @@ export class ClusterClient implements ICustomClusterClient {
...scopedHeaders,
};
}

private getSecondaryAuthHeaders(request: ScopeableRequest): Headers {
const headerSource = isRealRequest(request)
? this.authHeaders?.get(request) ?? {}
: request.headers;
const authorizationHeader = Object.entries(headerSource).find(([key, value]) => {
return key.toLowerCase() === AUTHORIZATION_HEADER && value !== undefined;
});

if (!authorizationHeader) {
throw new Error(
`asSecondaryAuthUser called from a client scoped to a request without 'authorization' header.`
);
}

return {
...getDefaultHeaders(this.kibanaVersion),
...this.config.customHeaders,
[ES_SECONDARY_AUTH_HEADER]: authorizationHeader[1],
};
}
}
10 changes: 10 additions & 0 deletions packages/core/elasticsearch/core-elasticsearch-client-server-internal/src/headers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,16 @@ export const PRODUCT_ORIGIN_HEADER = 'x-elastic-product-origin';
*/
export const USER_AGENT_HEADER = 'user-agent';

/**
* @internal
*/
export const AUTHORIZATION_HEADER = 'authorization';

/**
* @internal
*/
export const ES_SECONDARY_AUTH_HEADER = 'es-secondary-authorization';

/**
* @internal
*/
Expand Down
66 changes: 62 additions & 4 deletions ...elasticsearch/core-elasticsearch-client-server-internal/src/scoped_cluster_client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,24 +9,82 @@
import type { ElasticsearchClient } from '@kbn/core-elasticsearch-server';
import { ScopedClusterClient } from './scoped_cluster_client';

const createEsClient = () => ({} as unknown as ElasticsearchClient);
const createEsClient = () => Symbol('client') as unknown as ElasticsearchClient;

describe('ScopedClusterClient', () => {
it('uses the internal client passed in the constructor', () => {
const internalClient = createEsClient();
const scopedClient = createEsClient();
const secondaryAuthClient = createEsClient();

const scopedClusterClient = new ScopedClusterClient(internalClient, scopedClient);
const scopedClusterClient = new ScopedClusterClient({
asInternalUser: internalClient,
asCurrentUserFactory: () => scopedClient,
asSecondaryAuthUserFactory: () => secondaryAuthClient,
});

expect(scopedClusterClient.asInternalUser).toBe(internalClient);
});

it('uses the scoped client passed in the constructor', () => {
it('uses the primary-auth scoped client factory passed in the constructor', () => {
const internalClient = createEsClient();
const scopedClient = createEsClient();
const secondaryAuthClient = createEsClient();

const scopedClusterClient = new ScopedClusterClient(internalClient, scopedClient);
const scopedClusterClient = new ScopedClusterClient({
asInternalUser: internalClient,
asCurrentUserFactory: () => scopedClient,
asSecondaryAuthUserFactory: () => secondaryAuthClient,
});

expect(scopedClusterClient.asCurrentUser).toBe(scopedClient);
});

it('uses the secondary-auth scoped client factory passed in the constructor', () => {
const internalClient = createEsClient();
const scopedClient = createEsClient();
const secondaryAuthClient = createEsClient();

const scopedClusterClient = new ScopedClusterClient({
asInternalUser: internalClient,
asCurrentUserFactory: () => scopedClient,
asSecondaryAuthUserFactory: () => secondaryAuthClient,
});

expect(scopedClusterClient.asSecondaryAuthUser).toBe(secondaryAuthClient);
});

it('returns the same instance when calling `asCurrentUser` multiple times', () => {
const internalClient = createEsClient();
const scopedClient = createEsClient();
const secondaryAuthClient = createEsClient();

const scopedClusterClient = new ScopedClusterClient({
asInternalUser: internalClient,
asCurrentUserFactory: () => scopedClient,
asSecondaryAuthUserFactory: () => secondaryAuthClient,
});

const userClient1 = scopedClusterClient.asCurrentUser;
const userClient2 = scopedClusterClient.asCurrentUser;

expect(userClient1).toBe(userClient2);
});

it('returns the same instance when calling `asSecondaryAuthUser` multiple times', () => {
const internalClient = createEsClient();
const scopedClient = createEsClient();
const secondaryAuthClient = createEsClient();

const scopedClusterClient = new ScopedClusterClient({
asInternalUser: internalClient,
asCurrentUserFactory: () => scopedClient,
asSecondaryAuthUserFactory: () => secondaryAuthClient,
});

const secondaryAuth1 = scopedClusterClient.asSecondaryAuthUser;
const secondaryAuth2 = scopedClusterClient.asSecondaryAuthUser;

expect(secondaryAuth1).toBe(secondaryAuth2);
});
});
39 changes: 35 additions & 4 deletions ...core/elasticsearch/core-elasticsearch-client-server-internal/src/scoped_cluster_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,39 @@ import type { ElasticsearchClient, IScopedClusterClient } from '@kbn/core-elasti

/** @internal **/
export class ScopedClusterClient implements IScopedClusterClient {
constructor(
public readonly asInternalUser: ElasticsearchClient,
public readonly asCurrentUser: ElasticsearchClient
) {}
public readonly asInternalUser;

readonly #asCurrentUserFactory: () => ElasticsearchClient;
readonly #asSecondaryAuthUserFactory: () => ElasticsearchClient;

#asCurrentUserClient?: ElasticsearchClient;
#asSecondaryAuthUserClient?: ElasticsearchClient;

constructor({
Copy link
Contributor

Choose a reason for hiding this comment

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

It seemed so much simpler before lazily creating clients!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it sure was. But I remember that the time spent in client creation is not negligible, so unfortunatly we had to optimize that thing, and usually perf optimization is at the expense of code simplicity.

asInternalUser,
asCurrentUserFactory,
asSecondaryAuthUserFactory,
}: {
asInternalUser: ElasticsearchClient;
asCurrentUserFactory: () => ElasticsearchClient;
asSecondaryAuthUserFactory: () => ElasticsearchClient;
}) {
this.asInternalUser = asInternalUser;
this.#asCurrentUserFactory = asCurrentUserFactory;
this.#asSecondaryAuthUserFactory = asSecondaryAuthUserFactory;
}

public get asCurrentUser() {
if (this.#asCurrentUserClient === undefined) {
this.#asCurrentUserClient = this.#asCurrentUserFactory();
}
return this.#asCurrentUserClient;
}

public get asSecondaryAuthUser() {
if (this.#asSecondaryAuthUserClient === undefined) {
this.#asSecondaryAuthUserClient = this.#asSecondaryAuthUserFactory();
}
return this.#asSecondaryAuthUserClient;
}
}
2 changes: 2 additions & 0 deletions packages/core/elasticsearch/core-elasticsearch-client-server-mocks/src/mocks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,12 +190,14 @@ const createClientMock = (res?: Promise<unknown>): ElasticsearchClientMock =>
export interface ScopedClusterClientMock {
asInternalUser: ElasticsearchClientMock;
asCurrentUser: ElasticsearchClientMock;
asSecondaryAuthUser: ElasticsearchClientMock;
}

const createScopedClusterClientMock = () => {
const mock: ScopedClusterClientMock = {
asInternalUser: createClientMock(),
asCurrentUser: createClientMock(),
asSecondaryAuthUser: createClientMock(),
};

return mock;
Expand Down
11 changes: 11 additions & 0 deletions packages/core/elasticsearch/core-elasticsearch-server/src/client/scoped_cluster_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,17 @@ export interface IScopedClusterClient {
* on behalf of the internal Kibana user.
*/
readonly asInternalUser: ElasticsearchClient;

/**
* A {@link ElasticsearchClient | client} to be used to query the elasticsearch cluster
* with the internal Kibana user as primary auth and the current user as secondary auth
* (using the `es-secondary-authorization` header).
*
* Note that only a subset of Elasticsearch APIs support secondary authentication, and that only those endpoints
* should be called with this client.
Copy link
Contributor

@TinaHeiligers TinaHeiligers Jun 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

with the internal Kibana user as primary auth and the current user as secondary auth (using the es-secondary-authorization header).

Does this mean the current user can switch between primary auth and secondary auth clients or only ever use one?

Copy link
Contributor Author

@pgayvallet pgayvallet Jun 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If by current user you mean current active user, then they have no control in which client is used (similar to how they do not get to choose if we're using the internal one of the primary-scoped one).

API consumers will be able to switch between those three clients the same way they did between the two original ones (clusterClient.asInternalUser, clusterClient.asCurrentUser and clusterClient.asSecondaryAuthUser)

*/
readonly asSecondaryAuthUser: ElasticsearchClient;

/**
* A {@link ElasticsearchClient | client} to be used to query the elasticsearch cluster
* on behalf of the user that initiated the request to the Kibana server.
Expand Down
4 changes: 4 additions & 0 deletions x-pack/plugins/alerting/server/lib/wrap_scoped_cluster_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@ function wrapScopedClusterClient(opts: WrapScopedClusterClientOpts): IScopedClus
...rest,
esClient: scopedClusterClient.asCurrentUser,
}),
asSecondaryAuthUser: wrapEsClient({
...rest,
esClient: scopedClusterClient.asSecondaryAuthUser,
}),
};
}

Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/ml/server/shared_services/shared_services.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ function getRequestItemsProvider(
scopedClient = {
asInternalUser,
asCurrentUser: asInternalUser,
asSecondaryAuthUser: asInternalUser,
};
mlSavedObjectService = getSobSavedObjectService(scopedClient);
mlClient = getMlClient(scopedClient, mlSavedObjectService);
Expand Down
6 changes: 5 additions & 1 deletion x-pack/plugins/observability_solution/slo/server/lib/rules/slo_burn_rate/executor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,11 @@ describe('BurnRateRuleExecutor', () => {
loggerMock = loggingSystemMock.createLogger();
servicesMock = {
savedObjectsClient: soClientMock,
scopedClusterClient: { asCurrentUser: esClientMock, asInternalUser: esClientMock },
scopedClusterClient: {
asCurrentUser: esClientMock,
asInternalUser: esClientMock,
asSecondaryAuthUser: esClientMock,
},
alertsClient: publicAlertsClientMock.create(),
alertFactory: {
create: jest.fn(),
Expand Down
4 changes: 4 additions & 0 deletions .../server/lib/detection_engine/rule_preview/api/preview_rules/wrap_scoped_cluster_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ export function wrapScopedClusterClient(opts: WrapScopedClusterClientOpts): ISco
...rest,
esClient: scopedClusterClient.asCurrentUser,
}),
asSecondaryAuthUser: wrapEsClient({
...rest,
esClient: scopedClusterClient.asSecondaryAuthUser,
}),
};
}

Expand Down