-
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
Add secondary authentication to Core ES client #184901
Conversation
/ci |
/ci |
/ci |
/ci |
/ci |
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.
self-review
/** | ||
* 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. | ||
*/ | ||
readonly asSecondaryAuth: ElasticsearchClient; |
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.
So we now have asInternalUser
, asCurrentUser
, and asSecondaryAuth
. I don't really like that name, but I couldn't find something better (without renaming asCurrentUser
which is out of the question). If you have a better idea, please voice it!
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.
asSecondaryAuthUser
for consistency?
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.
++, asSecondaryAuthUser
is the best trade-off I can think of too.
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.
Thank you both! Done in 02c3819
return new ScopedClusterClient({ | ||
asInternalUser: this.asInternalUser, | ||
asCurrentUserClientFactory: createScopedClient, | ||
asSecondaryAuthClientFactory: createSecondaryScopedClient, |
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.
Creating child clients is somewhat costly. Did some quick refactoring to have them lazily instantiated instead of eagerly. (it's especially important now that we have 2 different instantiations that can happen with asCurrentUser
and asSecondaryAuth
)
private getSecondaryAuthHeaders(request: ScopeableRequest): Headers { | ||
const headerSource = isRealRequest(request) | ||
? this.authHeaders?.get(request) ?? {} | ||
: request.headers; | ||
const authorizationHeader = Object.entries(headerSource).find(([key, _]) => { | ||
return key.toLowerCase() === AUTHORIZATION_HEADER; | ||
}); | ||
const scopedHeaders = authorizationHeader | ||
? { [ES_SECONDARY_AUTH_HEADER]: authorizationHeader[1] } | ||
: {}; | ||
|
||
return { | ||
...getDefaultHeaders(this.kibanaVersion), | ||
...this.config.customHeaders, | ||
...scopedHeaders, | ||
}; |
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.
@azasypkin added you as reviewer to double check this.
This is the approach I took after our slack and GH discussion. For secondary-auth scoped clients, we're using the exact same credentials, configuration, and headers as the internal client. The only exception is the addition of the es-secondary-authorization
header, with the value resolved from the request's authorization
header.
Pinging @elastic/kibana-core (Team:Core) |
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.
obs-ux-manaement changes LGTM
#asCurrentUserClient?: ElasticsearchClient; | ||
#asSecondaryAuthClient?: ElasticsearchClient; | ||
|
||
constructor({ |
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.
It seemed so much simpler before lazily creating clients!
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.
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.
* (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 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?
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.
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
)
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.
ML changes LGTM
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.
DE changes LGTM
/** | ||
* 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. | ||
*/ | ||
readonly asSecondaryAuth: ElasticsearchClient; |
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.
++, asSecondaryAuthUser
is the best trade-off I can think of too.
return this.asInternalUser.child({ | ||
headers: secondaryAuthHeaders, | ||
}); |
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.
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 👍
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.
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?
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.
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.
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.
Okay, checked:
- Run ES with
yarn es snapshot --license trial --ssl -E xpack.security.authc.token.timeout=15s
to have short-lived tokens - 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,
...
}
- 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",
...
}
- 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.
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.
"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 scopedHeaders = authorizationHeader | ||
? { [ES_SECONDARY_AUTH_HEADER]: authorizationHeader[1] } | ||
: {}; |
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'm wondering if we should throw (or at least log) an error here to ensure we're not silently ignoring invalid usage?
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.
As discussed in slack: we're not doing it for the logic returning the primary auth headers, so I'm scared of changing the behavior there, but we can absolutely do it for the new API. Will add.
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.
Done in cfa29a9
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.
Thanks!
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.
LGTM, thanks!
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.
LGTM
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Public APIs missing comments
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: |
…ion result when HTTP authentication is used (#190998) ## Summary When Kibana tries to authenticate a request that already has an `Authorization` header (not a cookie, client certificate, or Kerberos ticket), the authentication is performed by the [HTTP authentication provider](https://www.elastic.co/guide/en/kibana/current/kibana-authentication.html#http-authentication). Unlike session/Kerberos/PKI providers, this provider returns an authentication result that doesn't explicitly tell Core which authorization headers should be used (e.g., `t.authenticated({ state: authenticationResult.user, --> requestHeaders: authenticationResult.authHeaders <-- ... });`), assuming that Core will just use the headers from the request. The `Authorization` header is forwarded to Elasticsearch by default, no additional configuration is required. This worked well previously, but I think with the introduction of the the [`getSecondaryAuthHeaders`](#184901) method this is the first time where this assumption doesn't hold. Internally, this method tries to reuse authentication headers that were provided to Core by the authentication provider during the request authentication stage — headers that the HTTP authentication provider never provided before. This PR makes the HTTP authentication provider behave consistently with the rest of the providers we support today.
…ion result when HTTP authentication is used (elastic#190998) ## Summary When Kibana tries to authenticate a request that already has an `Authorization` header (not a cookie, client certificate, or Kerberos ticket), the authentication is performed by the [HTTP authentication provider](https://www.elastic.co/guide/en/kibana/current/kibana-authentication.html#http-authentication). Unlike session/Kerberos/PKI providers, this provider returns an authentication result that doesn't explicitly tell Core which authorization headers should be used (e.g., `t.authenticated({ state: authenticationResult.user, --> requestHeaders: authenticationResult.authHeaders <-- ... });`), assuming that Core will just use the headers from the request. The `Authorization` header is forwarded to Elasticsearch by default, no additional configuration is required. This worked well previously, but I think with the introduction of the the [`getSecondaryAuthHeaders`](elastic#184901) method this is the first time where this assumption doesn't hold. Internally, this method tries to reuse authentication headers that were provided to Core by the authentication provider during the request authentication stage — headers that the HTTP authentication provider never provided before. This PR makes the HTTP authentication provider behave consistently with the rest of the providers we support today. (cherry picked from commit 3ac931f)
…entication result when HTTP authentication is used (#190998) (#191051) # Backport This will backport the following commits from `main` to `8.15`: - [fix(security, http): expose authentication headers in the authentication result when HTTP authentication is used (#190998)](#190998) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Aleh Zasypkin","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-08-22T07:48:35Z","message":"fix(security, http): expose authentication headers in the authentication result when HTTP authentication is used (#190998)\n\n## Summary\r\n\r\nWhen Kibana tries to authenticate a request that already has an\r\n`Authorization` header (not a cookie, client certificate, or Kerberos\r\nticket), the authentication is performed by the [HTTP authentication\r\nprovider](https://www.elastic.co/guide/en/kibana/current/kibana-authentication.html#http-authentication).\r\n\r\nUnlike session/Kerberos/PKI providers, this provider returns an\r\nauthentication result that doesn't explicitly tell Core which\r\nauthorization headers should be used (e.g., `t.authenticated({ state:\r\nauthenticationResult.user, --> requestHeaders:\r\nauthenticationResult.authHeaders <-- ... });`), assuming that Core will\r\njust use the headers from the request. The `Authorization` header is\r\nforwarded to Elasticsearch by default, no additional configuration is\r\nrequired.\r\n\r\nThis worked well previously, but I think with the introduction of the\r\nthe\r\n[`getSecondaryAuthHeaders`](https://github.com/elastic/kibana/pull/184901)\r\nmethod this is the first time where this assumption doesn't hold.\r\nInternally, this method tries to reuse authentication headers that were\r\nprovided to Core by the authentication provider during the request\r\nauthentication stage — headers that the HTTP authentication provider\r\nnever provided before.\r\n\r\nThis PR makes the HTTP authentication provider behave consistently with\r\nthe rest of the providers we support today.","sha":"3ac931fb796b2053c68b454804e643788dc598d9","branchLabelMapping":{"^v8.16.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Security","release_note:skip","backport:prev-minor","v8.16.0"],"title":"fix(security, http): expose authentication headers in the authentication result when HTTP authentication is used","number":190998,"url":"https://github.com/elastic/kibana/pull/190998","mergeCommit":{"message":"fix(security, http): expose authentication headers in the authentication result when HTTP authentication is used (#190998)\n\n## Summary\r\n\r\nWhen Kibana tries to authenticate a request that already has an\r\n`Authorization` header (not a cookie, client certificate, or Kerberos\r\nticket), the authentication is performed by the [HTTP authentication\r\nprovider](https://www.elastic.co/guide/en/kibana/current/kibana-authentication.html#http-authentication).\r\n\r\nUnlike session/Kerberos/PKI providers, this provider returns an\r\nauthentication result that doesn't explicitly tell Core which\r\nauthorization headers should be used (e.g., `t.authenticated({ state:\r\nauthenticationResult.user, --> requestHeaders:\r\nauthenticationResult.authHeaders <-- ... });`), assuming that Core will\r\njust use the headers from the request. The `Authorization` header is\r\nforwarded to Elasticsearch by default, no additional configuration is\r\nrequired.\r\n\r\nThis worked well previously, but I think with the introduction of the\r\nthe\r\n[`getSecondaryAuthHeaders`](https://github.com/elastic/kibana/pull/184901)\r\nmethod this is the first time where this assumption doesn't hold.\r\nInternally, this method tries to reuse authentication headers that were\r\nprovided to Core by the authentication provider during the request\r\nauthentication stage — headers that the HTTP authentication provider\r\nnever provided before.\r\n\r\nThis PR makes the HTTP authentication provider behave consistently with\r\nthe rest of the providers we support today.","sha":"3ac931fb796b2053c68b454804e643788dc598d9"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/190998","number":190998,"mergeCommit":{"message":"fix(security, http): expose authentication headers in the authentication result when HTTP authentication is used (#190998)\n\n## Summary\r\n\r\nWhen Kibana tries to authenticate a request that already has an\r\n`Authorization` header (not a cookie, client certificate, or Kerberos\r\nticket), the authentication is performed by the [HTTP authentication\r\nprovider](https://www.elastic.co/guide/en/kibana/current/kibana-authentication.html#http-authentication).\r\n\r\nUnlike session/Kerberos/PKI providers, this provider returns an\r\nauthentication result that doesn't explicitly tell Core which\r\nauthorization headers should be used (e.g., `t.authenticated({ state:\r\nauthenticationResult.user, --> requestHeaders:\r\nauthenticationResult.authHeaders <-- ... });`), assuming that Core will\r\njust use the headers from the request. The `Authorization` header is\r\nforwarded to Elasticsearch by default, no additional configuration is\r\nrequired.\r\n\r\nThis worked well previously, but I think with the introduction of the\r\nthe\r\n[`getSecondaryAuthHeaders`](https://github.com/elastic/kibana/pull/184901)\r\nmethod this is the first time where this assumption doesn't hold.\r\nInternally, this method tries to reuse authentication headers that were\r\nprovided to Core by the authentication provider during the request\r\nauthentication stage — headers that the HTTP authentication provider\r\nnever provided before.\r\n\r\nThis PR makes the HTTP authentication provider behave consistently with\r\nthe rest of the providers we support today.","sha":"3ac931fb796b2053c68b454804e643788dc598d9"}}]}] BACKPORT--> Co-authored-by: Aleh Zasypkin <[email protected]>
Summary
Fix #179458
Add a third method to
IScopedClusterClient
,asSecondaryAuth
which allow performing requests on behalf of the kibana system users with the current user as secondary authentication (via thees-secondary-authorization
header)