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

azasypkin · 2019-07-19T17:28:05Z

Backports the following commits to 7.x:

Migrate authentication subsystem to the new platform. (Migrate authentication subsystem to the new platform. #39446)

* Temporary Core workarounds. * Move files to NP Security Plugin. * Fix references. * Migrate to the New Platform. * Review#1: remove unused `loginAttempt` from provider iterator, rely more on RecursiveReadonly, etc. * Integrate latest core changes: isTlsEnabled and get rid of legacy ES config. * Revert `deepFreeze` changes and rely on `src/core/utils`. * Review#2: do not mutate injectedVars in onInit. Integrate latest upstream changes. * Use mocks provided by the Core. * Expect ElasticsearchError instead of Boom errors as 401 Cluster client errors. * Simplify session handling for `login`. * Review#3: properly handle session updates for `login`, remove redundant hapi-auth-cookie deps from x-pack package.json, migrate to new core sessionStorage API, integrate latest Kerberos provider changes from upstream * Do not clear session on login if it does not exist.

azasypkin · 2019-07-19T17:29:28Z

x-pack/legacy/plugins/security/index.js

+      encryptionKey: Joi.any().description('This key is handled in the new platform security plugin ONLY'),
+      sessionTimeout: Joi.any().description('This key is handled in the new platform security plugin ONLY'),
+      secureCookies: Joi.any().description('This key is handled in the new platform security plugin ONLY'),
+      public: Joi.any().description('This key is handled in the new platform security plugin ONLY'),


note: diff#1: keeping public in 7.x

azasypkin · 2019-07-19T17:30:13Z

x-pack/legacy/plugins/security/index.js


    const config = server.config();
    const xpackMainPlugin = server.plugins.xpack_main;
    const xpackInfo = xpackMainPlugin.info;
+    securityPlugin.registerLegacyAPI({
+      xpackInfo,
+      serverConfig: {


note: diff#3: need to pass server config that's not available for NP plugins (we could ask platform team to expose it, but we don't need these in master yet, so chose to shim it),

azasypkin · 2019-07-19T17:32:18Z

x-pack/legacy/plugins/security/index.js

-        oidc: providerOptionsSchema('oidc', Joi.object({ realm: Joi.string().required() }).required()),
-        saml: providerOptionsSchema('saml', Joi.object({ realm: Joi.string() })),
-      }).default()
+      authc: Joi.any().description('This key is handled in the new platform security plugin ONLY')
    }).default();
  },



note: diff#2: had to keep deprecations untouched since NP doesn't support this functionality yet, and fortunately we don't need it in master yet.

azasypkin · 2019-07-19T17:38:44Z

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

+  clusterClient: PublicMethodsOf<ClusterClient>;
+  sessionStorageFactory: SessionStorageFactory<ProviderSession>;
+  isSystemAPIRequest: (request: KibanaRequest) => boolean;
+  getServerBaseURL: () => string;


note: diff#4: instead of passing protocol/hostname/port tripple I just pass a function that generates server base URL. It's required since we get necessary info from the legacy plugin and it's actually shows the intention better.

azasypkin · 2019-07-19T17:40:21Z

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

+    });
+  });
+
+  describe('getServerBaseURL()', () => {


note: diff#5: added few tests to cover this new (well, old, since we don't need it in master anymore) function

azasypkin · 2019-07-19T17:40:56Z

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

+   * Retrieves server protocol name/host name/port and merges it with `xpack.security.public` config
+   * to construct a server base URL (deprecated, used by the SAML provider only).
+   */
+  const getServerBaseURL = () => {


note: diff#6: same note as for diff#5.

azasypkin · 2019-07-19T17:41:23Z

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

+  basePath.get.mockReturnValue('/base-path');
+
+  return {
+    getServerBaseURL: () => 'test-protocol://test-hostname:1234',


note: diff#7: mock to cover this new function.

azasypkin · 2019-07-19T17:43:01Z

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

@@ -0,0 +1,1010 @@
+/*


note: diff#7: removed test that checks that provider throws if realm isn't provided (it's optional in 7.x) and migrated tests that verify acs we pass in prepare and invalidate.

azasypkin · 2019-07-19T17:43:51Z

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

-   */
-  private debug(message: string) {
-    this.options.log(['debug', 'security', 'saml'], message);
+  private getACS(request: KibanaRequest) {


note: diff#8: migrated this function to use getServerBaseURL.

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

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

+describe('config schema', () => {
+  it('generates proper defaults', () => {
+    expect(ConfigSchema.validate({})).toMatchInlineSnapshot(`
+                                                                                                            Object {


note: diff#9: have no idea why eslint formats inline snapshots in 7.x in a such weird way 😞 But it what it does automatically 🤷‍♂️

azasypkin · 2019-07-19T17:46:37Z

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

+    );
+  });
+
+  describe('public', () => {


note: diff#10: added suite to test public.

spalger · 2019-07-19T17:47:26Z

retest

azasypkin · 2019-07-19T17:47:50Z

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

+    expect(loggingServiceMock.collect(contextMock.logger).warn).toEqual([]);
+  });
+
+  it('should set authProviders to authc.providers', async () => {


note: diff#11: added test for the "rename deprecation transformation" ugly hacky workaround I had to add in this backport (see below).

azasypkin · 2019-07-19T17:49:35Z

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

+    }),
+    // This property is deprecated, but we include it here since new platform doesn't support config deprecation
+    // transformations yet (e.g. `rename`), so we handle it manually for now.
+    authProviders: schema.maybe(schema.arrayOf(schema.string(), { minSize: 1 })),


note: diff#12: that's the hack, I didn't find a better way to handle this "rename" deprecation use case with the primitives core provides currently. The config deprecations in NP are on the short-term Mikhail's roadmap, but we're not there yet. Let me know if you have a better idea or just don't want to see this hack at all :)

azasypkin · 2019-07-19T17:50:51Z

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

+          ...config.authc,
+          // If deprecated `authProviders` is specified that most likely means that `authc.providers` isn't, but if it's
+          // specified then this case should be caught by the config deprecation subsystem in the legacy platform.
+          providers: config.authProviders || config.authc.providers,


note: diff#13: continuation of the hack from diff#12.

azasypkin · 2019-07-19T17:51:59Z

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

+ */
+export interface LegacyAPI {
+  xpackInfo: Pick<XPackInfo, 'isAvailable' | 'feature'>;
+  serverConfig: { protocol: string; hostname: string; port: number };


note: diff#14: declared one more property we expect from the legacy plugin

azasypkin · 2019-07-19T17:55:26Z

Hey @kobelb, I had to make a number of changes in this backport that I'm not super happy about, but couldn't find a better way. I've outlined all of them in the comments (14 diffs). Let me know how you feel about them!

elasticmachine · 2019-07-19T19:16:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-22T08:11:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin · 2019-07-22T08:47:27Z

@kobelb I've just realized you're OOO for a couple of next days, I'll go ahead and merge this PR to make it easier to backport upcoming changes, but feel free to raise any concerns you have regarding the additional changes I had to make in the backport once you're back - I'll handle them.

azasypkin added the backport label Jul 19, 2019

azasypkin commented Jul 19, 2019

View reviewed changes

azasypkin requested a review from kobelb July 19, 2019 17:55

Merge branch '7.x' into backport/7.x/pr-39446

25c587f

Merge branch '7.x' into backport/7.x/pr-39446

8a2ad43

azasypkin merged commit e19a03b into elastic:7.x Jul 22, 2019

azasypkin deleted the backport/7.x/pr-39446 branch July 22, 2019 08:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

azasypkin commented Jul 19, 2019

azasypkin Jul 19, 2019 •

edited

Loading

azasypkin Jul 19, 2019 •

edited

Loading

azasypkin Jul 19, 2019

azasypkin Jul 19, 2019

azasypkin Jul 19, 2019

azasypkin Jul 19, 2019

azasypkin Jul 19, 2019

azasypkin Jul 19, 2019

azasypkin Jul 19, 2019

azasypkin Jul 19, 2019

azasypkin Jul 19, 2019

spalger commented Jul 19, 2019

azasypkin Jul 19, 2019

azasypkin Jul 19, 2019 •

edited

Loading

azasypkin Jul 19, 2019

azasypkin Jul 19, 2019

azasypkin commented Jul 19, 2019

elasticmachine commented Jul 19, 2019

elasticmachine commented Jul 22, 2019

azasypkin commented Jul 22, 2019

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

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

Conversation

azasypkin commented Jul 19, 2019

azasypkin Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

azasypkin Jul 19, 2019 • edited Loading

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

spalger commented Jul 19, 2019

Choose a reason for hiding this comment

azasypkin Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin commented Jul 19, 2019

elasticmachine commented Jul 19, 2019

💚 Build Succeeded

elasticmachine commented Jul 22, 2019

💚 Build Succeeded

azasypkin commented Jul 22, 2019

azasypkin Jul 19, 2019 •

edited

Loading

azasypkin Jul 19, 2019 •

edited

Loading

azasypkin Jul 19, 2019 •

edited

Loading