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

[New platform] Security plugin integration #33775

Closed
29 of 34 tasks
mshustov opened this issue Mar 25, 2019 · 17 comments
Closed
29 of 34 tasks

[New platform] Security plugin integration #33775

mshustov opened this issue Mar 25, 2019 · 17 comments
Assignees
@mshustov
Labels
blocker chore 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 Mar 25, 2019 •
edited by azasypkin
Loading

This issue is used to track progress and blockers for Security plugin New Platform migration:

Status

  • ✔️ Client-side
    • Everything is migrated except for two request interceptors to support plugins that still use Angular HTTP client
  • ✔️ Server-side
    • Everything is migrated

Blockers (in order of priority)

Possible improvements

Previous description Security needs New Platform to provide a way to intercept all incoming requests to perform auth check. Security plugin is optional, thus flow shouldn't be affected if it is disabled. We can provide **async handler** that supports pausing request handling and finishing with one of the next scenarios: - resume execution if auth is succeeded. - interrupt in case of rejection. - redirect

Security plugin operates low-level primitives and needs a way to controls:

  • cookie header (get/set/clear operations), cookie params are configured by Security plugin as well.
  • auth header (get/set/clear operations).
    Downstream plugins shouldn't have access to user credentials stored via those mechanisms. It's not a problem while we don't expose raw request to plugins.

Original summary from #18024 (comment)

To summarize or discussion on the "http filters". The platform extension point that security needs should be able to:

  • Intercept all incoming requests;
  • Get/set/clear cookies and pass request through to the route handlers (e.g. authenticate user and save token to the cookies);
  • Abort request and return error (e.g. failed authentication);
  • Abort request and return 302 with redirect (e.g. SAML handshake);
  • Possibly get some route-metadata (e.g we want to skip authentication for certain routes like login/logout should not go through authentication filter).

Won't be implemented in the first iteration

  • The ability to render hidden apps. NP doesn't provide rendering mechanism at the moment. We still can relay on LP doing it for us.

Related: #18024

Implementation This auth handler can be Security specific, because it specifies cookie parameters - name, password(encryptionKey), path, secure, cookie validation. ```js setup(core, dependencies){ // If we don't want to expose registerAuth as a core capability // we can provide it only to Security plugin core.http.configureCookie({ name, password ...}); core.http.registerAuthMiddleware(async function(({cookie, headers}), authToolkit){ cookie.set(...) return authToolkit.succeeded(); ```

Or the platform can provide the ability to register a request middleware. Middleware specifies to what request headers it wants to have access to

setup(core, dependencies){
   core.http.registerMiddleware(
    '*', // apply to all requests
    {   // declare access to
       headers: ['cookie', 'authorization'], // only one middleware has access to a specified header 
     },
     async function(({method, headers}), authToolkit){
       // in this case Security plugin has to manage session cookie and parse it
       // In the legacy platform that functionality is implemented by 'hapi-auth-cookie' hapi plugin.
@mshustov mshustov added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels Mar 25, 2019
@mshustov mshustov self-assigned this Mar 25, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@kimjoar kimjoar mentioned this issue Mar 25, 2019
Closed
15 tasks
@mshustov mshustov added the enhancement New value added to drive a business result label Mar 25, 2019
@mshustov
Copy link
Contributor Author

mshustov commented Mar 26, 2019 •
edited
Loading

Right now legacy platform defines 2 auth strategies:

@mshustov
Copy link
Contributor Author

@elastic/kibana-security would you mind to check everything here makes sense 😄?

@kobelb
Copy link
Contributor

kobelb commented Mar 26, 2019

@restrry thanks for writing this up! Given @azasypkin's experience with the new platform, and his knowledge of our various authentication providers, I'd like to hear his opinion on this as well.

Were you intending for this extension point to be utilized for both authentication and authorization, or would we have two different "lifecycle methods" that the new platform exposes?

@mshustov
Copy link
Contributor Author

mshustov commented Mar 27, 2019 •
edited
Loading

Were you intending for this extension point to be utilized for both authentication and authorization, or would we have two different "lifecycle methods" that the new platform exposes?

Does the authorization mechanism in the current state require incoming request interception?

As I understand Security plugin provides tools for other plugins to check user privileges but doesn't provide user data & roles directly.
https://github.com/elastic/kibana/blob/master/x-pack/plugins/security/index.js#L124-L125
Those tools heavily relay on request headers, which we don't expose right now in NP route handler. We are going to solve this problem in #33783, for now we can provide raw request if needed

@kobelb
Copy link
Contributor

kobelb commented Mar 27, 2019

Does the authorization mechanism in the current state require incoming request interception?

Spaces does currently in the master branch https://github.com/elastic/kibana/blob/master/x-pack/plugins/spaces/server/lib/space_request_interceptors.ts#L41

With the introduction of Feature Controls we're using the following code to intercept some API calls https://github.com/elastic/kibana/blob/granular-app-privileges/x-pack/plugins/security/server/lib/authorization/api_authorization.ts#L14 and we're doing something similar to block certain applications from being loaded https://github.com/elastic/kibana/blob/granular-app-privileges/x-pack/plugins/security/index.js#L231

@azasypkin
Copy link
Member

Or the platform can provide the ability to register a request middleware. Middleware specifies to what request headers it wants to have access to

Do we have any plans on defining framework-agnostic Kibana request lifecycle with lifecycle event based hooks similar to what Hapi has or there will be express-js-like mechanism?

With the introduction of Feature Controls we're using the following code to intercept some API calls https://github.com/elastic/kibana/blob/granular-app-privileges/x-pack/plugins/security/server/lib/authorization/api_authorization.ts#L14 and we're doing something similar to block certain applications from being loaded https://github.com/elastic/kibana/blob/granular-app-privileges/x-pack/plugins/security/index.js#L231

It looks like we can group authc + all authz based interceptors internally and just register one security interceptor/pre-request-handler with the core, so that security does everything in one go before any other app route handler is invoked. It's kind of what we do already with multiple onPostAuth handlers.

Also it may depend on whether core will be treating API calls as a separate group of requests, IIRC there was a conversation about having something like core.http.regiterAPIRoute and core.http.registerSomethingElse and hence potentially two different extension points for interceptors (so that we can get rid of this a bit fragile !request.path.startsWith('/api/') check), but don't quote me on that.

@mshustov
Copy link
Contributor Author

mshustov commented Apr 1, 2019 •
edited
Loading

Sorry for a long comment, I spent several days wrapping my head around current implementation and want to structure my thoughts.

Right now we rely on 3 different stages for every request (in execution order in hapi framework, more info about hapi request lifecycle)

As a bottom line there are 2 main mechanics:

  • intercept a request and implement the context-dependent operation, interceptors do not share any information between them and do not have access to user credentials., that makes them closer to event handlers than middlewares.
    Possible implementation:
interface InterceptorToolkit {
   next: () => ({ type: 'next' }),
   redirected: (url: string) => ({ type: 'redirected', url }),
   rejected: (error?: Error) => ({ type: 'rejected', error }),
}
core.http.registerResourceInterceptor((req: KibanaRequest, interceptorToolkit: InterceptorToolkit){
   if(req.method === 'GET') return interceptorToolkit.redirect('/');
   return interceptorToolkit.next();
});
  • intercept a request and perform authc/authz operation. Those 2 are dependent on order and have different meaning and interfaces. The very first step performs authentication and provides a result to the next authorization steps, which have only read access to given user credentials. Potentially we can merge them into one step, but we need to decide where 3rd party plugins (dashboard_mode e.g.) register access to authz - via HTTP server or via security plugin (I'm inclined to this option). But want to hear from @elastic/kibana-security
    An option where we merge authc/authz steps.
// 3rd party plugin, for example dashboard_mode
plugins.security.addAuthorizationInterceptor(async function(req: KibanaRequest, interceptorToolkit: InterceptorToolkit, readonly user: UserStore, scopedTools){
   if(user.roles.includes(...) || scopedTools.getSpaces(...)){
      return interceptorToolkit.redirected(...)
   }
});
// plugins/security.js
// in following steps we can restrict an access only to required auth headers instead of whole hapi request
core.http.registerAuthorizationInterceptor((req: HapiRequest, interceptorToolkit: InterceptorToolkit) => {
 const authenticationResult = await authenticate(req);
 if(authenticationResult.succeeded()){ 
   for (const interceptor of this.getAuthorizationInterceptors()){
     const authorizationResult = await interceptor(KibanaRequest.from(req), interceptorToolkit, authenticationResult.user, scopeToolsToRequest(req));
     if(authorizationResult.redirected()) {
        return interceptorToolkit.redirected(authorizationResult.redirectURL)
     }
});

Or the implementation option where we separate authc/authz steps.

interface AuthToolkit {
   authenticated: () => ({ type: 'authenticated' }),
   unauthenticated: (error?) => ({ type: 'unauthenticated', error }),
   reject: (error: Error) => ({ type: 'reject', error }),
}
// in following steps we can restrict an access only to required auth headers instead of whole hapi request
core.http.registerAuthenticationInterceptor((req: HapiRequest, authToolkit: AuthToolkit, user: UserStore){
   const authenticationResult = authenticate(req);
   if(authenticationResult.succeeded()){
      user.set(authenticationResult);
   }
   return authToolkit.authenticated();
});

core.http.registerAuthorizationInterceptor((req: HapiRequest, interceptorToolkit: InterceptorToolkit, readonly user: UserStore){
  // user is scoped to the request. on the very first step we can support old approach with plugin.secuity.checkPriveledgesFor(request, user);
   const result  = plugin.secuity.checkPriveledgesFor(user);
   if(result) {
      return interceptorToolkit.next();
   } else {
      return interceptorToolkit.reject(new Error('not found'));
   }
});

And note if we migrate Security plugin to NP we should change dependent parts (like dashboard_mode) in legacy platform to restrict an access to hapi request object. Because request objects in New and legacy platform are different objects with different lifecycles.

@kobelb
Copy link
Contributor

kobelb commented Apr 1, 2019

Thanks for giving this so much thought, and a clear description of the current situation @restrry.

I think it makes sense for the security plugin itself to handle all authentication and authorization, and provide extension points to the various plugins to "add" their own authorization. This allows the security plugin to determine when authentication/authorization should be performed, without requiring other plugin authors to understand the details of when security is enabled and auth should be performed.

@epixa
Copy link
Contributor

epixa commented Apr 3, 2019

I'm comfortable with the merged approach through the security plugin as well. It's worth noting that one way or another the underlying ability to build this auth stuff through the http service is there and available to plugins, so the security plugin just makes it more explicit.

@mshustov mshustov mentioned this issue Apr 9, 2019
Merged
4 tasks
@mshustov mshustov mentioned this issue May 7, 2019
Closed
3 tasks
@mshustov
Copy link
Contributor Author

I'm back to the problem. After talking with the Security team we figured out that several lifecycle stages are required.

  • a stage that is performed before auth. Specificity: can perform request forwarding to another URL, before auth is performed and request hits a route handler. Analog in the LP - server.ext('onRequest',...). There is only one consumer as of now. But some extension points in LP can be placed here. one, two
  • auth stage. Specificity: can perform authc/authz logic and store user credentials in memory.
  • a stage that is performed after auth. Specificity: has access to auth status. Analog in the LP - server.ext('onPostAuth',...).

@mshustov mshustov mentioned this issue Jun 4, 2019
Merged
3 tasks
@mshustov
Copy link
Contributor Author

mshustov commented Jun 7, 2019 •
edited
Loading

I'm going to work on restriction access to raw hapi request object in registerAuth((req: hapi.Request, ... and want Security team to clarify some moments

Authorization header

Kibana relays on Elasticsearch to do all heavy lifting for user authentication. Security plugin tries to apply different providers and authenticate a user by means of extending request headers with a custom authorization header.

// some provider specific logic
request.headers.authorization = providerSpecificHeader;
try {
  esClient.callWithRequest(request, 'shield.authenticate');
  ...
} catch(e) {
  delete request.headers.authorization;
}

Headers are mutated in order to retain authorization header for downstream plugins interacting with Elasticsearch. It means Elasticsearch server expects this header to set on KibanaRequest, Legacy.Request or whatever.
https://github.com/restrry/kibana/blob/f753474423c973aba2b9c35a68a22b031991e6e6/src/core/server/elasticsearch/cluster_client.ts#L155

public asScoped(req: { headers?: Headers } = {}) {...}

Here I have a question: how critical if we expose authorization header as a part of KibanaRequest abstraction? Should be able any plugin (including 3rd party ones) to have read/write access to this header? Depending on an answer we either use KibanaRequest in registerAuth or introduce our own abstraction (KibanaRequestSecurity?) which is only one that has access to read/write this authz header. Introducing additional abstraction also will require ClusterClient refactoring, because asScoped is a separate plugin and won't have direct access to request headers anymore.

Request body & query validation

SAML provider expects a request to contain some provider specific information.
https://github.com/restrry/kibana/blob/f753474423c973aba2b9c35a68a22b031991e6e6/x-pack/plugins/security/server/lib/authentication/providers/saml.ts#L80-L92
The new platform does try to enforce input validation on system boundaries and requires schema declaration for request body, query, params. Can Security plugin use router API to enforce request validation? https://github.com/restrry/kibana/blob/f753474423c973aba2b9c35a68a22b031991e6e6/src/core/server/http/router/route.ts#L61
Should we enforce request validation for Security plugin at all? It shouldn't be a problem as we already know the schema
https://github.com/restrry/kibana/blob/f753474423c973aba2b9c35a68a22b031991e6e6/x-pack/plugins/security/server/lib/authentication/providers/saml.ts#L46-L56

@kobelb
Copy link
Contributor

kobelb commented Jun 7, 2019

Here I have a question: how critical if we expose authorization header as a part of KibanaRequest abstraction? Should be able any plugin (including 3rd party ones) to have read/write access to this header?

I would absolutely prefer that they don't. The way that we do this today has always bothered me, and as far as I'm aware it's only being done this way because security was introduced after a significant portion of Kibana was already written. I'm open to reconsidering this entire approach, if you are :) Would it be possible for the registerAuth implementation itself to respond with the headers that can be used to "authenticate against Elasticsearch as the end-user" and then the new platform could store this somewhere, perhaps in a weakmap with the request and the authentication headers, and then the Elasticsearch plugin could use this weakmap? I'd love to be able to rename callWithRequest to something like callImpersonatingAuthenticatedEndUser.

The new platform does try to enforce input validation on system boundaries and requires schema declaration for request body, query, params. Can Security plugin use router API to enforce request validation?

I believe we should be able to as we have this route where I believe we could perform this.

@azasypkin you mind double-checking everything I've said here for correctness and to ensure you agree with my recommendations?

@mshustov
Copy link
Contributor Author

mshustov commented Jun 7, 2019 •
edited
Loading

I believe we should be able to as we have this route where I believe we could perform this.

In this case we might need to change api slightly:

await server.plugins.security.authenticate(request, {
   type: 'saml',
   SAMLRequest: query.SAMLRequest,
   SAMLResponse: body.SAMLResponse,
});

it could be useful, because we can use the same pattern for LoginAttempt

const { username, password } = request.body;
await server.plugins.security.authenticate(request, {
  type: 'basic',
  credentials: { username, password }
});

@kobelb
Copy link
Contributor

kobelb commented Jun 7, 2019

@restrry sounds reasonable to me!

@azasypkin
Copy link
Member

azasypkin commented Jun 11, 2019 •
edited
Loading

I believe we should be able to as we have this route where I believe we could perform this.

++, the validation schema should allow us to expect "unknown" keys as well (see OIDC route with Joi.object.unknown), don't remember if kbn/config-schema supports this already or not.

@azasypkin you mind double-checking everything I've said here for correctness and to ensure you agree with my recommendations?

Yes, I agree, thanks!

In this case we might need to change api slightly:

Exactly! That's what we're discussing for some time already: second argument for authenticate should be an optional free form LoginAttempt that will consolidate all these query string/body parameters and be created at the route handler level (e.g. api/security/v1/saml).

@azasypkin azasypkin mentioned this issue Jun 21, 2019
Merged
10 tasks
This was referenced Jul 9, 2019
Merged
Closed
@mshustov
Copy link
Contributor Author

Authentication mechanism was migrated to the New Platform

@mshustov mshustov added chore and removed enhancement New value added to drive a business result labels Jul 25, 2019
@mshustov mshustov reopened this Jul 29, 2019
@azasypkin azasypkin added the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label Sep 4, 2019
This was referenced Sep 13, 2019
Merged
Merged
@mattapperson mattapperson mentioned this issue Oct 4, 2019
Open
13 tasks
@legrego legrego closed this as completed Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mshustov mshustov

Labels
blocker chore 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
Milestone
No milestone
Development

No branches or pull requests
7 participants
@epixa @kobelb @azasypkin @joshdover @mshustov @legrego @elasticmachine