Migrate authentication subsystem to the new platform. #39446

azasypkin · 2019-06-21T15:56:22Z

In this PR we migrate authentication subsystem to the new platform. Authentication source code will now live within a new platform plugin that will coexist with the legacy Security plugin until we fully migrate to the new platform.

Here is the list of blockers we need to resolve before we can merge this (PR includes all these changes as a reference of what we need, but we need a help of the @elastic/kibana-platform to merge them or get alternative proposals):

Changes in @kbn/config-schema:

The schema.literal type should support null values (needed for sessionTimeout config value) Extend @kbn/config-schema #40118
The schema.conditional should support schemas within a right operand in addition to references and plain values (needed for our not-very-simple validation of authc.* Extend @kbn/config-schema #40118
[Optional] Maybe introduce schema.forbidden (currently can be emulated with schema.any({ validate: () => 'error' }), used mostly with schema.conditional when we'd like to forbid some configuration values in case some condition is met)? Extend @kbn/config-schema #40118

Changes in the Core:

Config validation code should inject dist context variable during config validation, so that we can use it with contextRef('dist') (needed for automatic generation of encryptionKey in dev environment) New platform should inject "is distributable" flag in package info #40187
The http setup contract should expose isTLSEnabled or something like this (needed to automatically set secureCookies if TLS is enabled and/or warn about insecure cookie transmission) Expose "is TLS enabled" flag for Kibana HTTP Server #40336
The elasticsearch setup contract should expose createClient (we need it to create our own security client with "esShield" plugin). Another problem here is that createClient expects full Elasticsearch config, so we have to rely on setup.elasticsearch.legacy.config$. To be honest, I don't remember already why I didn't use default Elasticsearch config in createClient internally when I implemented this method (it must be something related to the way legacy elasticsearch plugin works currently). This would allow consumers to just override or add some config values if needed. createCluster requires a partial elasticsearch config #40405 and expose ES createClient to plugins #40717

Other changes:

We need to include new platform x-pack plugins into release build, [core] Update .kibana-plugin-helpers.json #39677
We need to include xpack/plugins into Jest config Include new platform x-pack plugin paths to Jest roots. #40422
[Optional] We use RecursivelyReadonly-like constructions in many places, especially for the contracts we return, would it make sense to officially expose Core's deepFreeze 「DISCUSS」TypeScript utility types #39881?
[Optional] Would it make sense for Core to expose mocks for commonly used stuff (e.g. mock for Logger, ClusterClient, ScopedClusterClient and BasePath, Mocks for CoreStart, CoreSetup and PluginInitializerContext #39351 and Provide Mocks for New Platform concepts #40186) ?

Related to: #33775
Fixes: #40756
Blocked by: ~~#39366~~, ~~#40118~~, ~~#40187~~, ~~#40405~~, ~~#40717~~

/cc @restrry

elasticmachine · 2019-06-21T15:56:24Z

Pinging @elastic/kibana-security

azasypkin · 2019-06-26T17:45:37Z

Hey @kobelb and @restrry, would be great if you can give any preliminary feedback on the approach I've taken here whenever you have time (PR is based on #39366, so please ignore the first commit).

In #39446 (comment) I've outlined all the blockers we have right now, PR includes workarounds for all of them which I'll remove as soon as they aren't needed anymore. Right now I'm cleaning up/adding comments/fixing tests/removing leftovers.

src/core/server/config/env.ts

x-pack/plugins/security/server/authentication/providers/basic.ts

x-pack/plugins/security/server/authentication/providers/base.ts

mshustov · 2019-06-27T10:45:12Z

x-pack/plugins/security/server/config.ts

+import { schema, Type, TypeOf } from '@kbn/config-schema';
+import { PluginInitializerContext } from '../../../../src/core/server';
+
+export type ConfigType = ReturnType<typeof createConfig> extends Promise<infer P>


should core expose Unpromise type? https://github.com/restrry/kibana/blob/8c72cead14333a583f55416c2fbf43a87f418378/src/legacy/server/kbn_server.d.ts#L89
even local declaration should simplify reading:
Unpromise<ReturnType<typeof createConfig>>

JFYI: we are going to introduce type utilities library and move all type helpers there #39881

JFYI: we are going to introduce type utilities library and move all type helpers there #39881

That'd be awesome!

x-pack/plugins/security/server/config.ts

x-pack/plugins/security/server/authentication/providers/basic.ts

mshustov · 2019-06-27T13:23:53Z

x-pack/plugins/security/server/authentication/providers/kerberos.ts

+    }
+
+    const challenges = ([] as string[]).concat(
+      get<string | string[]>(authenticationError, 'output.headers[WWW-Authenticate]') || ''


I'm wondering if we should expose Boom errors from the core as a part of contract. even worse it is an implicit contract. @elastic/kibana-platform
https://github.com/restrry/kibana/blob/8c72cead14333a583f55416c2fbf43a87f418378/src/core/server/elasticsearch/cluster_client.ts#L100-L106

I don't love that we're exposing Boom but I can also see the argument that this will make migrating easier.

I think when we add the new elasticsearch-js client (#35508), we should not use Boom.

In this case I'd suggest to introduce ESClientError type (which is a Boom object atm) and document it in migration guide as a part of ES Client contract.
later we can switch all such places to a custom error and have more or less clear understanding what should be fixed.

x-pack/legacy/plugins/security/server/routes/api/v1/users.js

mshustov · 2019-06-27T13:49:30Z

x-pack/legacy/plugins/security/server/routes/views/login.js

@@ -8,9 +8,8 @@ import { get } from 'lodash';

 import { parseNext } from '../../lib/parse_next';

-export function initLoginView(server, xpackMainPlugin) {
+export function initLoginView({ config: { cookieName } }, server, xpackMainPlugin) {


Shouldn't this method call isAuthenticated instead of manually check a cookie presence? the same in logged out view

We definitely should somehow improve that (maybe with something like Hapi's try auth mode? we should prevent redirects during authentication for such mode though). But the route itself opts out of authentication and hence isAuthenticated will return false

But the route itself opts out of authentication and hence isAuthenticated will return false

yeah. that's kinda expected...because we don't run interceptor we cannot say for sure whether cookies aren't stale. The logic in legacy platform has the same problem.

because we don't run interceptor we cannot say for sure whether cookies aren't stale.

It has more problems than that :) Some providers (e.g. SAML) relies on intermediate cookies that used during SAML handshake, so if you start handshake you can no longer get back to login page. That's something I'm going to solve soon, but not in this PR.

created #41959

x-pack/plugins/security/server/__fixtures__/request.ts

mshustov · 2019-06-27T15:09:43Z

We use RecursivelyReadonly-like constructions in many places, especially for the contracts we return, would it make sense to officially expose Core's deepFreeze?

It's already available in 'src/core/utils'. Where would you suggest to export it from?

Would it make sense for Core to expose mocks for commonly used stuff (e.g. mock for Logger, ClusterClient, ScopedClusterClient and BasePath

I believe so. The only problem that most of those mocks don't exist yet.

mshustov · 2019-06-28T08:57:45Z

The elasticsearch setup contract should expose createClient (we need it to create our own security client with "esShield" plugin). Another problem here is that createClient expects full Elasticsearch config, so we have to rely on setup.elasticsearch.legacy.config$. To be honest, I don't remember already why I didn't use default Elasticsearch config in createClient internally when I implemented this method (it must be something related to the way legacy elasticsearch plugin works currently). This would allow consumers to just override or add some config values if needed.

I believe we can change the API safely. In practice it already expects a partial config in Legacy platform where all createClient external consumers are.
https://github.com/restrry/kibana/blob/06dbcfbe96ef768ac73700ec888e4b1165682ee3/src/legacy/core_plugins/elasticsearch/index.js#L84-L85

I managed to find only monitoring plugin declaring a full elasticsearch config. Others use default elasticsearch config.

mshustov · 2019-07-01T09:54:27Z

Config validation code should inject dist context variable during config validation, so that we can use it with contextRef('dist') (needed for automatic generation of encryptionKey in dev environment)

We already inject it https://github.com/restrry/kibana/blob/8c72cead14333a583f55416c2fbf43a87f418378/src/core/server/config/config_service.ts#L145 Don't we?

mshustov · 2019-07-02T13:23:18Z

x-pack/plugins/security/server/authentication/providers/base.mock.ts

+export function mockAuthenticationProviderOptions() {
+  return {
+    client: { callAsInternalUser: stub(), asScoped: stub(), close: stub() },
+    logger: {


Should cover cases for Logger and BasePath:

import { loggingServiceMock, httpServiceMock } from '../../../../../../src/core/server/mocks'; export function mockAuthenticationProviderOptions() { const logger = loggingServiceMock.create(); const basePath = httpServiceMock.createSetupContract().basePath; basePath.get.mockReturnValue('/base-path'); return { client: { callAsInternalUser: stub(), asScoped: stub(), close: stub() }, logger, basePath, tokens: createStubInstance(Tokens), }; }

I will add mocks for ClusterClient, ScopedClusterClient and KibanaRequest in a separate PR

azasypkin · 2019-07-02T14:56:40Z

We already inject it https://github.com/restrry/kibana/blob/8c72cead14333a583f55416c2fbf43a87f418378/src/core/server/config/config_service.ts#L145 Don't we?

Hmm, no we don't. I mean I inject it in this PR just to unblock myself, but not in master :)

mshustov · 2019-07-02T15:01:07Z

Hmm, no we don't. I mean I inject it in this PR just to unblock myself, but not in master :)

ok, I meant we inject anything added to packageInfo. do you want me to add dist in a separate PR?

azasypkin · 2019-07-02T15:09:37Z

ok, I meant we inject anything added to packageInfo. do you want me to add dist in a separate PR?

That'd be great! I can do that, but ideally such changes should go through Platform review funnel :)

kobelb · 2019-07-02T15:36:39Z

Ack: reviewing now, it's taking me a bit

mshustov · 2019-07-02T17:37:57Z

That'd be great! I can do that, but ideally such changes should go through Platform review funnel :)

created #40187

mshustov · 2019-07-02T17:52:04Z

I believe we can change the API safely. In practice it already expects a partial config in Legacy platform where all createClient external consumers are.

created #40188

kobelb

Separating out "login" and "authenticate" makes it much easier to understand this providers, I really like this change.

kobelb · 2019-07-01T22:50:03Z

x-pack/plugins/security/server/config.ts

+      );
+    }
+  } else if (!secureCookies) {
+    secureCookies = true;


comment: this is rather awkward, but it mirrors the behavior in the LP, so... 🤷

Can you please elaborate on this a bit? Do you have any suggestions on how we can improve that?

kobelb · 2019-07-01T23:11:25Z

x-pack/plugins/security/server/index.ts

+    this.logger = this.initializerContext.logger.get();
+  }
+
+  public async setup(core: CoreSetup): Promise<RecursiveReadonly<SecuritySetupContract>> {


Within the setup method, we're grabbing the first instance of the config, and the clusterClient and using them within the authenticator for all subsequent http requests. Are we not concerned about these values changing or is this just temporary?

as I understand an answer depends on a decision in #40188
at the current moment, it's a correct assumption.

kobelb · 2019-07-02T16:12:40Z

x-pack/plugins/security/server/config.ts

+
+export async function createConfig(context: PluginInitializerContext, isTLSEnabled: boolean) {
+  const logger = context.logger.get('config');
+  const config = await context.config


question: will this work with the various LP deprecations? For example, we've renamed xpack.security.authProviders to authc.providers in 7.x using the LP deprecations, will these be reflected here or do we need to duplicate these?

That's a good question, I believe NP core doesn't provide any config deprecation support right now (cc @restrry). I haven't looked into this problem yet, but that will probably involve some duplicated code as a temporary workaround.

not provided yet. created #40255

kobelb · 2019-07-02T16:18:44Z

x-pack/plugins/security/server/authentication/authenticator.ts

+    this.logger = options.loggers.get('authenticator');
+
+    const providerCommonOptions = {
+      client: this.options.clusterClient,


When we perform the backport, we'll want to ensure that we're passing additional properties here:

kibana/x-pack/legacy/plugins/security/server/lib/authentication/authenticator.ts

Lines 67 to 73 in 2ceab54

protocol: server.info.protocol,

hostname: config.get<string>('server.host'),

port: config.get<number>('server.port'),

basePath: config.get<string>('server.basePath'),

tokens: new Tokens({ client, log }),

...config.get('xpack.security.public'),

Yeah, that won't be an easy backport :/

kobelb · 2019-07-02T16:21:15Z

x-pack/plugins/security/server/authentication/authenticator.ts

+} from './providers';
+import { AuthenticationResult } from './authentication_result';
+import { DeauthenticationResult } from './deauthentication_result';
+import { AuthenticationProviderSpecificOptions } from './providers/base';


nit: should we move this to being exported by `./providers'?

kobelb · 2019-07-02T17:21:36Z

x-pack/plugins/security/server/authentication/providers/basic.ts

+
+    if (!attempt || !attempt.username || !attempt.password) {
+      this.logger.debug('Username and/or password not provided.');
+      return AuthenticationResult.notHandled();


nit: it feels like we should be throwing an Error here as the consumer should be performing the validation on the end-user provided data before calling login.

Yeah, you're right, it probably makes sense to just remove this check entirely. We shouldn't do such checks for every argument we expect, especially since they are defined as required arguments.

kobelb · 2019-07-02T17:30:29Z

x-pack/plugins/security/server/authentication/providers/basic.ts

+      const user = await this.getUser(request);
+
+      this.logger.debug('Request has been authenticated via header.');
+      return { authenticationResult: AuthenticationResult.succeeded(user) };


question: should we be passing the authorization headers as the second parameter? They're already on the request, does that mean they'll automatically forward them to ES based on the elasticsearch.requestHeadersWhitelist?

They're already on the request, does that mean they'll automatically forward them to ES based on the elasticsearch.requestHeadersWhitelist?

Yeah, I believe so, existing headers + authHeaders returned from auth handler should be provided to ClusterClient that will filter them based on elasticsearch.requestHeadersWhitelist. @restrry can you please confirm?

right, we merge request and authHeaders headers, filter them and pass to ES Client only ones with names declared in elasticsearch.requestHeadersWhitelist
https://github.com/restrry/kibana/blob/0f25401fe4caaf97ff1b701d245455a0ef71ec3a/src/core/server/elasticsearch/cluster_client.ts#L206

kobelb · 2019-07-02T17:49:25Z

x-pack/plugins/security/server/authentication/authentication_result.ts

   * @param [state] Optional state to be stored and reused for the next request.
   */
-  public static succeeded(user: AuthenticatedUser, state?: unknown) {
+  public static succeeded(


question: we almost always should be passing the authHeaders to the succeeded method, do you think changing this method signature to the following would make this more apparent to consumers and potentially prevent us from forgetting to do so?

public static succeeded( user: AuthenticatedUser, authHeaders: Record<string, string> | null, state?: unknown ) {

Actually, that's how I did it in the beginning, but eventually abandoned this idea as it became hard to read and interpret, e.g.:

return AuthenticationResult.succeeded(user, { authorization }, { authorization });

In the snippet above we pass both authHeaders and state and it's hard to tell what we're doing here exactly. Also TS won't complain if we decide to switch the order we pass state and authHeaders in since types of the args are allowing that (and I managed to spend some unreasonable amount of time looking at the code and trying to understand what was wrong when I did that 🙈 ).

So I'm leaning towards keeping it as is, but would like to hear what you think?

That makes sense... let's leave it how it is then.

kobelb · 2019-07-02T18:31:07Z

x-pack/plugins/security/server/authentication/providers/kerberos.ts

+      const user = await this.getUser(request, authHeaders);
+
+      this.logger.debug('Request has been authenticated via state.');
+      return AuthenticationResult.succeeded(user, { authHeaders });


question: we're no longer mutating request.headers for this and all other token based auth providers which is great. have we ensured that we aren't breaking reporting by doing so?

I'll double check before I ask for review, but I believe we should be fine since we maintain BWC behavior with this https://github.com/elastic/kibana/blob/master/src/core/server/http/http_server.ts#L238

kobelb · 2019-07-02T18:33:50Z

x-pack/plugins/security/server/authentication/providers/token.ts

+  ) {
+    this.logger.debug('Trying to perform a login.');
+
+    if (!credentials || !credentials.username || !credentials.password) {


nit: same as above, should we be throwing an error here also?

Yep, let's just remove this check.

kobelb

Outside of the changes that you've proposed for login's use of updateSessionStorage, this is looking great! Just one question, but everything seems to be functioning perfectly.

kobelb · 2019-07-12T16:46:55Z

x-pack/plugins/security/server/authentication/providers/kerberos.test.ts

+        headers: { authorization: 'negotiate spnego' },
+      });
+
+      mockScopedClusterClient(mockOptions.client)


question: Did we intend to add this for this test? The test passes without it, and it's not immediately obvious what it's adding.

Hmm, good catch! I believe it's just a stupid copy-paste mistake 🙈 This stub should not even be called as we have one with more specific withArgs below.

…nt hapi-auth-cookie deps from x-pack package.json, migrate to new core sessionStorage API, integrate latest Kerberos provider changes from upstream

elasticmachine · 2019-07-16T17:03:14Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

azasypkin · 2019-07-16T17:07:19Z

CI will be failing until #41263 is fixed. But apart from that PR should be ready for review @kobelb .

elasticmachine · 2019-07-16T19:57:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

kobelb

Nice work!

elasticmachine · 2019-07-19T07:41:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin · 2019-07-19T09:09:16Z

Thanks everyone for review, let's see if it sticks :)

azasypkin · 2019-07-22T08:48:26Z

7.x/7.4.0: e19a03b

azasypkin added Feature:New Platform Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:Security/Authentication Platform Security - Authentication release_note:skip Skip the PR/issue when compiling release notes v7.3.0 labels Jun 21, 2019

azasypkin force-pushed the issue-xxx-np-authc branch 4 times, most recently from e901c8f to 8c72cea Compare June 26, 2019 17:38

azasypkin added blocked feedback_needed labels Jun 26, 2019

mshustov reviewed Jun 27, 2019

View reviewed changes

mshustov mentioned this pull request Jun 27, 2019

「AppArch」Interpreter 👉 New Platform #39329

Merged

kobelb self-requested a review July 1, 2019 21:32

mshustov mentioned this pull request Jul 2, 2019

Extend @kbn/config-schema #40118

Merged

3 tasks

mshustov reviewed Jul 2, 2019

View reviewed changes

mshustov mentioned this pull request Jul 2, 2019

Unify Header interface in New Platform #40185

Closed

kobelb reviewed Jul 2, 2019

View reviewed changes

spalger added the v7.4.0 label Jul 3, 2019

kobelb reviewed Jul 12, 2019

View reviewed changes

joshdover mentioned this pull request Jul 12, 2019

Initial implementation of server licensing service #41026

Closed

9 tasks

azasypkin added 4 commits July 15, 2019 17:40

Simplify session handling for login.

e09958c

Merge branch 'master' into issue-xxx-np-authc

a20b939

Merge branch 'master' into issue-xxx-np-authc

99606ef

Review#3: properly handle session updates for login, remove redunda…

07507cd

…nt hapi-auth-cookie deps from x-pack package.json, migrate to new core sessionStorage API, integrate latest Kerberos provider changes from upstream

azasypkin requested a review from kobelb July 16, 2019 17:08

azasypkin added 2 commits July 16, 2019 20:26

Do not clear session on login if it does not exist.

8980946

Merge branch 'master' into issue-xxx-np-authc

653b34a

azasypkin requested a review from mshustov July 17, 2019 06:37

kobelb approved these changes Jul 18, 2019

View reviewed changes

mshustov approved these changes Jul 19, 2019

View reviewed changes

Merge branch 'master' into issue-xxx-np-authc

d8fdb13

azasypkin merged commit 91e1d9c into elastic:master Jul 19, 2019

azasypkin deleted the issue-xxx-np-authc branch July 19, 2019 09:09

azasypkin added the backport pending label Jul 19, 2019

azasypkin mentioned this pull request Jul 19, 2019

[7.x] Migrate authentication subsystem to the new platform. (#39446) #41593

Merged

azasypkin added backported and removed backport pending labels Jul 22, 2019

mshustov mentioned this pull request Jul 25, 2019

Add an optional authentication mode for HTTP resources #41959

Closed

azasypkin added Feature:NP Migration and removed Feature:New Platform labels Jul 26, 2019

azasypkin mentioned this pull request Aug 16, 2019

Properly validate current user password during password change. #43447

Merged

azasypkin mentioned this pull request Sep 16, 2019

[7.x] Properly apply rename deprecation to xpack.security.authProviders. #45755

Merged

	protocol: server.info.protocol,
	hostname: config.get<string>('server.host'),
	port: config.get<number>('server.port'),
	basePath: config.get<string>('server.basePath'),
	tokens: new Tokens({ client, log }),

	...config.get('xpack.security.public'),

Migrate authentication subsystem to the new platform. #39446

Migrate authentication subsystem to the new platform. #39446

Conversation

azasypkin commented Jun 21, 2019 • edited Loading

elasticmachine commented Jun 21, 2019

azasypkin commented Jun 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Jun 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Jun 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

azasypkin Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov commented Jun 27, 2019

mshustov commented Jun 28, 2019 • edited Loading

mshustov commented Jul 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin commented Jul 2, 2019

mshustov commented Jul 2, 2019

azasypkin commented Jul 2, 2019

kobelb commented Jul 2, 2019

mshustov commented Jul 2, 2019

mshustov commented Jul 2, 2019

kobelb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kobelb Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kobelb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jul 16, 2019

💔 Build Failed

azasypkin commented Jul 16, 2019 • edited Loading

elasticmachine commented Jul 16, 2019

💚 Build Succeeded

kobelb left a comment

Choose a reason for hiding this comment

elasticmachine commented Jul 19, 2019

💚 Build Succeeded

azasypkin commented Jul 19, 2019

azasypkin commented Jul 22, 2019

azasypkin commented Jun 21, 2019 •

edited

Loading

mshustov Jun 27, 2019 •

edited

Loading

mshustov Jun 28, 2019 •

edited

Loading

mshustov Jul 2, 2019 •

edited

Loading

azasypkin Jul 2, 2019 •

edited

Loading

mshustov commented Jun 28, 2019 •

edited

Loading

mshustov commented Jul 1, 2019 •

edited

Loading

kobelb Jul 2, 2019 •

edited

Loading

azasypkin commented Jul 16, 2019 •

edited

Loading