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

Scope-able elasticsearch clients #39430

Open
mshustov opened this issue Jun 21, 2019 · 27 comments
Open

Scope-able elasticsearch clients #39430

mshustov opened this issue Jun 21, 2019 · 27 comments
Labels
discuss Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!

Comments

@mshustov
Copy link
Contributor

mshustov commented Jun 21, 2019

Every time when Kibana needs to get access to data saved in elasticsearch it should perform a check whether an end user has access to the data.

Regular mode

This check relays heavily on Security that authenticates a user via elasticsearch API and set authorization header accordingly. Having user authenticated Kibana performs all requests to elasticsearch on behalf of the user by attaching authorization headers to an incoming request:

callWithRequest(request: Request)

Note: the whole Request object is not required to hit elasticsearch API, but something with interface { headers: { authorization: 'token, [...other authorization headers: string]: string } }.

FakeRequest mode

There are a couple of cases when the current model doesn't play well

  • When we need to check whether headers.authorization contains valid credentials.
// in legacy platform
try{
  request.headers.authorization = tokenCandidate;
  callWithRequest(request)
} catch(e){
  delete request.headers.authorization
}
// in new platform
asScoped({headers: {authorization: tokenCandidate }}.callAsCurrentUser()
  • When elasticsearch api is called outside of Http request context.

In Reporting plugin

const decryptedHeaders: string = await crypto.decrypt(job.headers);

And in Upgrade Assistant

const fakeRequest = { headers: credential } as Request;

In all those cases we 'mimic' Request object, although we need only authorization headers to be present.

Discuss

  • What core services need to support this "scoping" mechanic? Obviously, it works for elasticsearch and other services that wrap it, like SavedObjects. Probably there is something else that I missed.
  • What should be considered as a valid input for asScoped()? Are those use cases for FakeRequest are going to stay in a long-term? Should we operate Request term at all? Probably we could rename it to something less confusing, for example OwnerCredentials to emphasise that something with scope-able interface is a valid input.
  • Will Alerting plugin use the same FakeRequest pattern to perform access data? @mikecote
  • Should elasticsearch ClusterClient support headers re-configuring? At the moment it's not possible. On the one hand it's a good thing because we explicitly declare the creation of FakeRequest to perform a call and declare dependencies on the passed credentials.
const data = await dataClient.asScoped({headers: { authorization: ....}}).callAsInternalUser()

On the other hand, the ergonomic suffers as we have to use asScoped and cannot leverage coming Handler RFC API to extend a request for custom needs

// or in registerHandler
const dataClient = await dataClient$.pipe(take(1)).toPromise();
const dataClient = dataClient.asScoped(request);
// somewhere below
const data = await dataClient.callAsInternalUser({headers: { authorization: ....}})
@mshustov mshustov added discuss Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Jun 21, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@mshustov mshustov added the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label Jun 21, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security

@kobelb
Copy link
Contributor

kobelb commented Jun 21, 2019

What core services need to support this "scoping" mechanic? Obviously, it works for elasticsearch and other services that wrap it, like SavedObjects. Probably there is something else that I missed.

I can't think of anything that doesn't end up going through the elasticsearch service that needs this ability.

What should be considered as a valid input for asScoped()? Are those use cases for FakeRequest are going to stay in a long-term? Should we operate Request term at all? Probably we could rename it to something less confusing, for example OwnerCredentials to emphasise that something with scope-able interface is a valid input.

As far as I know, they exist because we don't have API Keys for background tasks and we're essentially emulating this behavior. Once we start using api keys, we'll need some mechanism of supplying this api key when using the Elasticsearch client, similar to the "fake request". Renaming it to something like OwnerCredentials makes sense to me.

Will Alerting plugin use the same FakeRequest pattern to perform access data?

Once we get API keys, they will be doing so. We aren't able to use the hacky solutions like what Reporting does now because of limitations with the token based auth providers.

One other thing we might have to take into consideration is we currently support a elasticsearch.requestHeadersWhitelist, which allows administrators to choose arbitrary headers which should be forwarded to Elasticsearch. The only usage of this that I'm aware of is when users hand-roll their own authentication using a proxy, similar to the approach described here: https://www.elastic.co/blog/user-impersonation-with-x-pack-integrating-third-party-auth-with-kibana

@azasypkin
Copy link
Member

Not much to add here from my side, just one note:

This check relays heavily on Security that authenticates a user via elasticsearch API and set authorization header accordingly.

The authorization header is just a specific case of a more general authHeaders header dictionary that we may be need to pass to Elasticsearch with the request to authenticate the user. These are defined by the handler registered through registerAuth and represent a part of the ^scope^ of the ^scoped^ request to Elasticsearch (we call it as callAsCurrentUser right now).

It happened so that scope consists of mainly authentication information right now (+anything UA sent to Kibana and that's allowed by elasticsearch.requestHeadersWhitelist), but I can't see any reason why we may not want to introduce other ^things^ that will contribute into ^scope^ as well (e.g. something very random/nonsense like handler specified in registerTelemetry that adds telemetry headers to every request, just for the sake of example).

I mention this because I think that if we want to override just one header (or one specific part of the ^scope^) we can't just do something like this :

asScoped({headers: {authorization}}).callAsCurrentUser()

We'll need to preserve any other headers that could have been added to the ^scope^:

asScoped({headers: {...originalRequest.headers, authorization }}).callAsCurrentUser()

We're doing this in "authc in NP" PR right now and if we have just a few places where we redefine scope that way - not a big deal. But if this boilerplate code will be all over the place, I'd rather reconsider our decision to forbid scope-header overrides on a call basis, e.g. allow this call even if we override authorization defined by one of the ^scope^ forming interceptors:

scopedClientThatCameFromNPHandler.callAsCurrentUser(...., { headers: {authorization }})

@mshustov
Copy link
Contributor Author

mshustov commented Jun 28, 2019

But if this boilerplate code will be all over the place, I'd rather reconsider our decision to forbid scope-header overrides

We didn't forbid them.
Before #34610: if ES client was scoped to a request, other headers passed directly in callAsCurrentUser were ignored.
After #34610: headers passed in callAsCurrentUser aren't ignored anymore, but scoped request headers have precedence over them.

elasticsearch-js has the opposite logic, but aligning our code with it means we introduce breaking changes.
If we are planning to switch to elasticsearch-js we can start adopting ElasticseachService already now, still preserve the current behavior for the legacy platform via existing adapter

@azasypkin
Copy link
Member

We didn't forbid them.

I meant we forbid overriding them (default headers, passed through asScoped) on a call-basis.

elasticsearch-js has the opposite logic, but aligning our code with it means we introduce breaking changes.

I don't think it'd be a breaking change, it seems we never backported #34610 to 7.x.

@mshustov
Copy link
Contributor Author

I don't think it'd be a breaking change, it seems we never backported #34610 to 7.x.

#34610 is not related to breaking changes per se.
Now in v7: if ES client was scoped to a request, other headers passed directly in callAsCurrentUser were ignored.
if we adopt elasticsearch-js logic and backport it to 7.x: if ES client was scoped to a request, other headers passed directly in callAsCurrentUser rewrite scoped request headers.

@joshdover
Copy link
Contributor

In order to ease migration to the NP, I'd rather keep the current behavior of not allowing these headers to be overridden.

We can change this behavior when we introduce the new elasticsearch-js client along side the legacy one in #35508. We plan to do this in 7.5. This breaks up the migration process for existing plugins so they can deal with this change (and any other breaking changes) all at once.

@Bamieh I am curious if #34610 was not backported to 7.x intentionally. If this was intentional, I am worried about the inconsistency between master and 7.x.

@Bamieh
Copy link
Member

Bamieh commented Jul 5, 2019

@joshdover it was not intentional. Backported here: #40429

@mshustov
Copy link
Contributor Author

When Security migrated to the NP they stumbled upon a requirement that FakeRequest has to emulate KibanaRequest interface. e0c8981#r339997510
It steams from SavedObject allowing plugins to add client wrapper factory.
And now it becomes a plugin responsibility to handle all possible scope-able interfaces:

  • KibanaRequest
  • Legacy.Request
  • FakeRequest

Also, SavedObject service doesn't specify possible types for a Request, so plugins don't even know what they can receive as an argument.
There are 2 possible solutions:

@pgayvallet
Copy link
Contributor

pgayvallet commented Jan 22, 2020

I'm discovering this part of the code, so please tell me if I'm saying anything wrong.

And now it becomes a plugin responsibility to handle all possible scope-able interfaces:
KibanaRequest
Legacy.Request
FakeRequest

I will not talk about 'legacy' request here, as they should become core internal implementation details post 8.0

Also, SavedObject service doesn't specify possible types for a Request, so plugins don't even know what they can receive as an argument.

This is only true with the legacy SO service. NP one expects a KibanaRequest (even if effectively working with other types if force-casted)

forbid FakeRequest usage and provide API allowing plugins to create KibanaRequest with configured headers.

This one seems tricky with current implementation, as we are keeping reference of the original request inside KibanaRequest

export const ensureRawRequest = (request: KibanaRequest | LegacyRequest) =>
isKibanaRequest(request) ? request[requestSymbol] : request;

We make the assumption that this is the actual original HAPI request, and relies on it on some places, such as

const headers = ensureRawRequest(request).headers;

Also current implementation to retrieve the auth headers is based on the fact that the http server / auth hook keep track of them

private getHeaders(
request?: KibanaRequest | LegacyRequest | FakeRequest
): Record<string, string | string[] | undefined> {
if (!isRealRequest(request)) {
return request && request.headers ? request.headers : {};
}
const authHeaders = this.getAuthHeaders(request);
const headers = ensureRawRequest(request).headers;
return { ...headers, ...authHeaders };
}

(this.getAuthHeaders retrieves the headers from HttpServer.authRequestHeaders populated by the auth hook)

With current implementation, a 'cloned' KibanaRequest ( { ...request } ) will not provides the same headers as it's source when used to create a scopedClient, as the 'header registry' (authRequestHeaders) will not be aware of of the clone reference.
EDIT: this is false, as the reference to the hapi request will be preserved and is used as the registry key

As the behavior between actual requests 'received' by the http server and any other kind of requests differ (see the this.getAuthHeaders(request) part) I think I'm leaning to @restrry first proposal, and introduce an explicit interface for fake request. Would love to avoid this name though.

Maybe

interface AuthorizationContext {
  headers: Headers;
}

and then adapt the ES/SO(UI?) APIs to accept KibanaRequest | AuthorizationContext

Another option would be to have the 'request' be the single source of truth regarding authz headers. This would mean getting rid of the httpServer.authRequestHeaders and injecting the headers to the actual request instead to be able to retrieve them from there.

I would use a slightly different interface in that case:

interface AuthorizationContext {
  getAuthorizationHeaders: () => Headers;
}

.This way, we could have ES/SO APIs only accept the new AuthorizationContext interface.

KibanaRequest would simply implement it with what is currently done in ClusterClient.getHeaders, and creating a 'fake' request implementing this interface would be explicit and easy. Also the non-override issue is preserved, as the getAuthorizationHeaders implementation is internal to KibanaRequest

We would have a fallback mechanism for legacy until 8.0, by checking the presence of getAuthorizationHeaders from input parameter and fallbacking to 'legacy' (current) implementation if not present

@joshdover
Copy link
Contributor

joshdover commented Jan 22, 2020

As far as I know, they exist because we don't have API Keys for background tasks and we're essentially emulating this behavior. Once we start using api keys, we'll need some mechanism of supplying this api key when using the Elasticsearch client, similar to the "fake request". Renaming it to something like OwnerCredentials makes sense to me.

@kobelb It appears that Alerting is now using API keys. Is there any reason we can't or shouldn't change the Upgrade Assistant and Reporting code to use API keys as well?

Another option would be to have the 'request' be the single source of truth regarding authz headers. This would mean getting rid of the httpServer.authRequestHeaders and injecting the headers to the actual request instead to be able to retrieve them from there.

From what I understand the original reason for having the indirect way of getting auth headers was so that we could hide all of that information from plugin code. We have had at least one security vulnerability in the past where a plugin exposed auth credentials to other users. Making this impossible would be ideal.

If we can leverage API keys instead, I believe some of the FakeRequest concerns go away. All the plugins that need to use Elasticsearch out-of-band from a HTTP request can source API keys from the original requester. The Scoped Elasticsearch client then doesn't need to worry about FakeRequest and can instead accept KibanaRequest | ApiKey (where ApiKey is a string) on the asScoped method.

class Plugin {
  private readonly apiKeys = new Map<string, CreateApiKeyResult>();

  setup(core, plugins) {
    core.http.createRouter().post(
      { path: '/start_job/', }, 
      async (context, req, res) => {
        // Note this could be stored in ES if using encrypted saved objects
        this.apiKeys.set(jobId, await plugins.security.createApiKey(req));
        /* ... */
      }
    });
  }
 
  start(core) { 
    const esClient = core.elasticsearch.dataClient;
    
    // Very naive polling mechanism for demonstration purposes
    setInterval(async () => {
      const job = await getJob(esClient);
      const apiKeyResult = this.apiKeys.get(job.id);
      // TODO: check api key exists and is not expired
      // Use API key
      const response = await esClient
        .asScoped(apiKeyResult.api_key)
        .callAsCurrentUser(/* args as normal */);
      // TODO: update job
    });
  }
}

@kobelb
Copy link
Contributor

kobelb commented Jan 22, 2020

@kobelb It appears that Alerting is now using API keys. Is there any reason we can't or shouldn't change the Upgrade Assistant and Reporting code to use API keys as well?

API Keys are only available when Kibana is communicating with Elasticsearch over TLS. This hasn't been a requirement to use Reporting, so we can't make this switch in a minor version.

@mshustov
Copy link
Contributor Author

mshustov commented Jan 23, 2020

From what I understand the original reason for having the indirect way of getting auth headers was so that we could hide all of that information from plugin code. We have had at least one security vulnerability in the past where a plugin exposed auth credentials to other users. Making this impossible would be ideal.

correct. we need to restore this logic #55670

Also current implementation to retrieve the auth headers is based on the fact that the http server / auth hook keep track of them

getHeaders falls back to request headers if getAuthHeaders hasn't got any headers associated with an incoming request. It's required to cover the next case:

One other thing we might have to take into consideration is we currently support a elasticsearch.requestHeadersWhitelist, which allows administrators to choose arbitrary headers which should be forwarded to Elasticsearch. The only usage of this that I'm aware of is when users hand-roll their own authentication using a proxy, similar to the approach described here: https://www.elastic.co/blog/user-impersonation-with-x-pack-integrating-third-party-auth-with-kibana

AuthorizationContext

another option could be - OwnerCredentials or even just Credentials

and then adapt the ES/SO(UI?) APIs to accept KibanaRequest | AuthorizationContext

We need to separate them because Spaces & Security plugins require KibanaRequest for work?
AFAIK it's required for


getBasePath: () => job.basePath || serverBasePath,

Any other examples? Probably we can refactor this code and narrow KibanaRequest | AuthorizationContext to AuthorizationContext ?

The Scoped Elasticsearch client then doesn't need to worry about FakeRequest and can instead accept KibanaRequest | ApiKey

Just to note: FakeRequest uses ApiKey to create AuthorizationContext compatible interface

requestHeaders.authorization = `ApiKey ${apiKey}`;

@kobelb
Copy link
Contributor

kobelb commented Jan 24, 2020

For what it's worth, I'd much prefer we find an alternative to what is being done here

, @legrego can you provide some context of why this was required?

The two pieces of information from the HTTP request which the security and spaces plugin depend upon is the "base path", as this allows us to determine the current space transparently to end-users, and the Authorization headers, as this is how we later authenticate users against Elasticsearch.

I don't want to interject yet another opinion in regard to the specifics of how we want to implement this if it's not helpful. However, if I can be of assistance or you'd prefer I take a different role than commenting on our general needs, please just let me know.

@legrego
Copy link
Member

legrego commented Jan 24, 2020

disclaimer: I haven't caught up on the conversation in this issue yet

For what it's worth, I'd much prefer we find an alternative to what is being done here

, @legrego can you provide some context of why this was required?

++ I also want to find an alternative here. We put this logic in place when migrating Spaces to the NP in order to maintain compatibility with Alerting and Reporting, which at the time were still entirely in the LP IIRC, and therefore relied on the LP convention of retrieving the base path, which is via the request.getBasePath().

Since these background/async interactions don't have a true HTTP request associated with them, they end up crafting this fake request. Prior to the NP conversion efforts, the way to fake the request was to create an object with the getBasePath function, along with a couple of other properties.

@joshdover
Copy link
Contributor

for now is the key for me. I'm ok with this in the interim, but I'd rather not get into a situation where we have to maintain BWC with a fake request implementation once scope-able clients come around.

Totally agree. Are we planning to make these alerting APIs "stable"-ish for 3rd parties yet? If not, then we can use fakerequest for now and break the API once we introduce scoped clients.

@mikecote
Copy link
Contributor

mikecote commented May 6, 2020

Totally agree. Are we planning to make these alerting APIs "stable"-ish for 3rd parties yet? If not, then we can use fakerequest for now and break the API once we introduce scoped clients.

We're planning to have the server side APIs stable / GA in 7.10.

By exposing a fake request, the best the alerting team could do is making it clear in the documentation what changes are coming and why it's exposed in the meantime. That way developers are aware how much debt they'll take on until 8.0 or something.

@legrego
Copy link
Member

legrego commented Oct 13, 2020

This came up in conversation today with @lukeelmers. Some of the initiatives the AppArch team is working on will rely on authenticating via API Keys, so without scope-able clients they'll have to take a similar approach to what Alerting has done with FakeRequests, at least for the time being

@lukeelmers
Copy link
Member

Based on the latest discussion in #87990 (comment), it sounds like alerting may soon be providing executors access to the FakeRequest to facilitate the alerting-from-discover effort.

In this particular case it is because some of the data plugin's services are now scoped using a KibanaRequest (as they need access to scoped ES clients, SO clients, authenticated user info). So in order for data services to be used in an alert executor, plugins will need this information.

@mshustov
Copy link
Contributor Author

mshustov commented May 5, 2021

Un-assigning myself as I'm not actively working on it.

@mshustov mshustov removed their assignment May 5, 2021
@exalate-issue-sync exalate-issue-sync bot added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels Aug 4, 2021
@mshustov
Copy link
Contributor Author

mshustov commented Sep 2, 2021

As far as I know, they exist because we don't have API Keys for background tasks and we're essentially emulating this behavior. Once we start using api keys, we'll need some mechanism of supplying this api key when using the Elasticsearch client, similar to the "fake request". Renaming it to something like OwnerCredentials makes sense to me.

Elasticsearch relaxed restrictions on API keys usage without HTTPS elastic/elasticsearch#76801
API keys can now be widely adapted in Kibana. We might need to start formalizing the OwnerCredentials interface.

@exalate-issue-sync exalate-issue-sync bot added loe:medium Medium Level of Effort and removed loe:small Small Level of Effort labels Sep 29, 2021
@exalate-issue-sync exalate-issue-sync bot added loe:small Small Level of Effort and removed loe:medium Medium Level of Effort labels Feb 14, 2022
@legrego legrego removed EnableJiraSync loe:small Small Level of Effort impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. labels Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!
Projects
None yet
Development

No branches or pull requests