-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add secondary authentication to Core ES client #184901
Changes from all commits
1fa0e05
23e979d
75466ef
4ba071d
0c536f6
ca2d6d3
fd77242
02c3819
7ad6f90
cfa29a9
7ab4779
2b27b2d
cecc102
0c014fb
f5d2f01
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seemed so much simpler before lazily creating clients! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Does this mean the current user can switch between primary auth and secondary auth clients or only ever use one? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( |
||
*/ | ||
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. | ||
|
There was a problem hiding this comment.
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
?There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 fromAuthorization
and use it asES-Secondary-Authorization
instead. I'm leaning towards the latter since Core has a better chance of knowing that the request is performed viaasSecondaryAuthUser
, but we can discuss.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, checked:
yarn es snapshot --license trial --ssl -E xpack.security.authc.token.timeout=15s
to have short-lived tokensThe behavior seems to be exactly the same as for "primary" auth.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andasSecondaryAuthUser
to indicate which specific scenario we're handling?