[Security into Core] expose `authc.getCurrentUser` from `core.security` #177976

pgayvallet · 2024-03-05T08:46:17Z

Summary

First part of #174578

Introduce the new security core service, both on the browser and server-side.
- In this first stage, this service only has a single API: authc.getCurrentUser
Have the security plugin register its API to Core for ex-exposition

pgayvallet · 2024-03-05T08:49:29Z

/ci

pgayvallet · 2024-03-05T08:59:59Z

/ci

pgayvallet · 2024-03-05T09:09:21Z

/ci

pgayvallet · 2024-03-05T09:20:52Z

/ci

pgayvallet · 2024-03-05T11:10:42Z

/ci

pgayvallet · 2024-03-05T11:46:45Z

/ci

pgayvallet · 2024-03-05T14:54:28Z

/ci

pgayvallet · 2024-03-05T15:03:29Z

/ci

pgayvallet · 2024-03-05T16:03:45Z

/ci

pgayvallet · 2024-03-05T17:13:43Z

/ci

pgayvallet · 2024-03-06T07:18:45Z

/ci

pgayvallet · 2024-03-06T08:26:25Z

/ci

pgayvallet · 2024-03-06T08:41:57Z

/ci

pgayvallet · 2024-03-06T10:09:43Z

/ci

pgayvallet · 2024-03-06T10:43:18Z

/ci

pgayvallet · 2024-03-06T11:29:50Z

/ci

pgayvallet · 2024-03-06T12:25:49Z

/ci

pgayvallet · 2024-03-06T13:22:48Z

/ci

pgayvallet · 2024-03-06T13:33:43Z

/ci

…fix'

elasticmachine · 2024-03-08T08:16:26Z

Pinging @elastic/kibana-core (Team:Core)

elasticmachine · 2024-03-08T08:16:27Z

Pinging @elastic/kibana-security (Team:Security)

juliaElastic

Fleet change LGTM

elasticmachine · 2024-03-08T08:36:56Z

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

jgowdyelastic

ML changes LGTM

smith

Obs Exploratory view change looks good.

TinaHeiligers

Great start to moving security to core and thanks for making it easier to implement future changes to the packages.

I left a few non-blocking comments and questions around co-owners.

Nothing stands out that would prevent merging once all reviews are in.

LGTM

.github/CODEOWNERS

TinaHeiligers · 2024-03-08T21:58:09Z

packages/core/security/core-security-browser-internal/src/utils/convert_security_api.ts

+export const convertSecurityApi = (
+  privateApi: CoreInternalSecurityContract
+): InternalSecurityServiceStart => {
+  // shapes are the same for now given we only have one API exposed.
+  return privateApi;
+};


Double checking if I'm still following our intension: The contracts will diverge because we're not going to expose all of the security API's, correct?

TinaHeiligers · 2024-03-08T22:01:00Z

packages/core/security/core-security-browser-internal/src/internal_contracts.ts

+export type InternalSecurityServiceSetup = SecurityServiceSetup;
+export type InternalSecurityServiceStart = SecurityServiceStart;


Double checking: They'll diverge because we only intend to expose authc and authz API's and not the full set that the security plugin currently exposes.

TinaHeiligers · 2024-03-08T22:25:21Z

packages/core/security/core-security-browser/src/api_provider.ts

+/**
+ * The internal contract exposed by the security provider for Core to
+ * consume and re-expose via its security service.
+ *
+ * @public
+ */
+export interface CoreInternalSecurityContract {
+  authc: InternalAuthenticationServiceContract;
+}


The types exposed from *-browser and *-server packages are intended to be public. Why then are we including Internal in CoreInternalSecurityContract and not using CoreSecurityContract?

TinaHeiligers · 2024-03-08T23:09:05Z

x-pack/plugins/data_visualizer/public/application/data_drift/data_drift_app_state.tsx

  const services = {
+    ...coreStart,
    data,
    maps,


Should we anticipate side effects from having security accessible in two ways? The reordering makes sense here because we'll overwrite core.security with security from the plugin. Once we start diverging the API's exposed from these, there might be some confusion as to why the API's (and types etc) are accessible from both. Unless, of course, we're moving the API's from the security plugin to core's security package.

TinaHeiligers · 2024-03-08T23:20:30Z

...vability_solution/exploratory_view/public/components/shared/exploratory_view/rtl_helpers.tsx

  const core: Partial<CoreStart & ExploratoryViewPublicPluginsStart> = {
    ...defaultCore,
+    security: undefined,


We're not going to expose all the types anyway. It's best to do that now.

TinaHeiligers · 2024-03-08T23:28:42Z

packages/core/security/core-security-server/src/contracts.ts

+ */
+export interface SecurityServiceSetup {
+  /**
+   * Register the security implementation that then will be used and re-exposed by Core.


Suggested change

* Register the security implementation that then will be used and re-exposed by Core.

* Register the security implementation that will be used by core and re-exposed from Core's security service.

Minor change to specify the intention of moving some pieces to core

TinaHeiligers · 2024-03-08T23:30:11Z

packages/core/security/core-security-server/src/contracts.ts

+  /**
+   * Register the security implementation that then will be used and re-exposed by Core.
+   *
+   * @remark this should **exclusively** be used by the security plugin.


I wish we had a simple-ish way of enforcing this....
💡 can we use a hash map perhaps?

When we discussed about this, (iirc) we came to the conclusion that it was fine to not do any hard-checks, because

it's already possible to hook up into security by mimicking what the security plugin does today

the implementation remains in "xpack licensed" code

ideally we would be avoided doing hard checks on plugin ids (and also it isn't really protecting against 3rd party replacement as they could just the same pluginId as our internal security plugin...

I'll let @elastic/kibana-security confirms that it's still fine

I'll let @elastic/kibana-security confirms that it's still fine

Yeah, I believe it's fine to keep what we have now. There are so many hurdles and restrictions at various levels to replace the built-in Security plugin with any alternative implementation that it sounds impractical for anyone to even consider.

TinaHeiligers · 2024-03-08T23:55:16Z

...ore/http/core-http-request-handler-context-server-internal/src/core_route_handler_context.ts

+  #elasticsearch?: CoreElasticsearchRouteHandlerContext;
+  #savedObjects?: CoreSavedObjectsRouteHandlerContext;
+  #uiSettings?: CoreUiSettingsRouteHandlerContext;
+  #deprecations?: CoreDeprecationsRouteHandlerContext;
+  #security?: CoreSecurityRouteHandlerContext;


TinaHeiligers · 2024-03-09T00:08:45Z

packages/core/security/core-security-common/kibana.jsonc

+{
+  "type": "shared-common",
+  "id": "@kbn/core-security-common",
+  "owner": "@elastic/kibana-core"


Kibana security owns @kbn/security-plugin-types-common. Should we add them as co-owners to catch unintended changes to the types?

Good remark, Any objections @elastic/kibana-security?

Ah, commented in a different place. Yes, I agree, let's make our team as co-owners for this package.

azasypkin · 2024-03-13T09:56:56Z

ACK: I'll start reviewing today, sorry for the delay!

azasypkin

Looks great, thank you! Just left a few notes.

azasypkin · 2024-03-13T13:19:19Z

...ore/http/core-http-request-handler-context-server-internal/src/core_route_handler_context.ts

+  #elasticsearch?: CoreElasticsearchRouteHandlerContext;
+  #savedObjects?: CoreSavedObjectsRouteHandlerContext;
+  #uiSettings?: CoreUiSettingsRouteHandlerContext;
+  #deprecations?: CoreDeprecationsRouteHandlerContext;
+  #security?: CoreSecurityRouteHandlerContext;


Off-topic: Oh, native private fields. I remember there was some performance hit (around 2-3 years ago) related to them. Have things improved, and are private fields ready for wider adoption in Kibana now?

azasypkin · 2024-03-13T13:28:33Z

packages/core/http/core-http-request-handler-context-server/src/request_handler_context.ts

@@ -31,6 +32,7 @@ export interface CoreRequestHandlerContext {
  elasticsearch: ElasticsearchRequestHandlerContext;
  uiSettings: UiSettingsRequestHandlerContext;
  deprecations: DeprecationsRequestHandlerContext;
+  security: SecurityRequestHandlerContext;


question: Thanks for adding security to the request context! Would you be open, in this PR, to update a single route (/me) in the Security plugin to use the Core's request context so that we have a real usage example (literally almost all tests in Security and beyond rely on this route, so it'd be a great indicator that everything works as expected)?

I was somewhat concerned that the PR didn't add/change any FTR tests, so I think this is a great idea.

azasypkin · 2024-03-13T13:38:23Z

packages/core/security/core-security-common/kibana.jsonc

+{
+  "type": "shared-common",
+  "id": "@kbn/core-security-common",
+  "owner": "@elastic/kibana-core"


Ah, commented in a different place. Yes, I agree, let's make our team as co-owners for this package.

azasypkin · 2024-03-13T14:57:00Z

packages/core/security/core-security-server/src/contracts.ts

+  /**
+   * Register the security implementation that then will be used and re-exposed by Core.
+   *
+   * @remark this should **exclusively** be used by the security plugin.


I'll let @elastic/kibana-security confirms that it's still fine

Yeah, I believe it's fine to keep what we have now. There are so many hurdles and restrictions at various levels to replace the built-in Security plugin with any alternative implementation that it sounds impractical for anyone to even consider.

…-into-core-v1

…text

…-fix'

kibana-ci · 2024-03-14T12:31:18Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`core`	504	509	+5
`security`	502	503	+1
total			+6

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/core-http-request-handler-context-server`	11	12	+1
`@kbn/core-security-browser`	-	3	+3
`@kbn/core-security-browser-internal`	-	8	+8
`@kbn/core-security-browser-mocks`	-	6	+6
`@kbn/core-security-common`	-	6	+6
`@kbn/core-security-server`	-	7	+7
`@kbn/core-security-server-internal`	-	14	+14
`@kbn/core-security-server-mocks`	-	7	+7
total			+52

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`core`	387.8KB	388.6KB	+829.0B
`security`	65.0KB	65.1KB	+117.0B
total			+946.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-http-request-handler-context-server`	14	15	+1
`@kbn/core-lifecycle-browser`	32	34	+2
`@kbn/core-lifecycle-server`	36	38	+2
`@kbn/core-security-browser`	-	10	+10
`@kbn/core-security-browser-internal`	-	8	+8
`@kbn/core-security-browser-mocks`	-	6	+6
`@kbn/core-security-common`	-	20	+20
`@kbn/core-security-server`	-	15	+15
`@kbn/core-security-server-internal`	-	14	+14
`@kbn/core-security-server-mocks`	-	7	+7
total			+85

History

💔 Build #197617 failed d803545
💚 Build #196556 succeeded 81feecf
💔 Build #196364 failed 55bf242
💔 Build #196339 failed 3e16e3b
💔 Build #196336 failed 6f79f96

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

pgayvallet mentioned this pull request Mar 5, 2024

Making security a first class citizen of Kibana Core #174578

Open

pgayvallet and others added 5 commits March 7, 2024 08:02

creating server-side core security packages

d0190e4

draft of the core security types

97b156f

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

26c2cb2

…fix'

re-expose security types from Core server entrypoint

60d825b

fix import

78eac76

pgayvallet requested review from a team as code owners March 8, 2024 08:16

juliaElastic approved these changes Mar 8, 2024

View reviewed changes

botelastic bot added the Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team label Mar 8, 2024

jgowdyelastic approved these changes Mar 8, 2024

View reviewed changes

legrego requested a review from azasypkin March 8, 2024 15:49

smith approved these changes Mar 8, 2024

View reviewed changes

TinaHeiligers approved these changes Mar 9, 2024

View reviewed changes

azasypkin approved these changes Mar 13, 2024

View reviewed changes

pgayvallet and others added 5 commits March 14, 2024 08:35

Merge remote-tracking branch 'upstream/main' into kbn-174578-security…

9c5be95

…-into-core-v1

rename CoreInternalSecurityContract to CoreSecurityContract

4bb7af6

add security team as owner of the common type package

a2fb006

change /internal/security/me handler to use core security handler con…

c34c290

…text

[CI] Auto-commit changed files from 'node scripts/generate codeowners'

717734a

pgayvallet enabled auto-merge (squash) March 14, 2024 07:55

kibanamachine and others added 2 commits March 14, 2024 08:35

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

d803545

…-fix'

fix endpoint unit tests

50b5007

pgayvallet merged commit b816af4 into elastic:main Mar 14, 2024
41 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Mar 14, 2024

This was referenced Mar 19, 2024

[security-in-core] Expose userProfile APIs from Core #178932

Closed

[security-in-core] Expose auditLogging APIs from Core #178934

Closed

Dosant mentioned this pull request Mar 20, 2024

[POC] Dashboard filter by user #179034

Closed

legrego mentioned this pull request May 20, 2024

[security-in-core] Expose authz APIs from Core #183849

Open

legrego mentioned this pull request Jun 4, 2024

[Security-in-core] Expose API Key service under core.security.authc #184764

Closed

TinaHeiligers mentioned this pull request Jun 20, 2024

Making security a first class citizen of Kibana Core examples #186574

Closed

		export type InternalSecurityServiceSetup = SecurityServiceSetup;
		export type InternalSecurityServiceStart = SecurityServiceStart;

	* Register the security implementation that then will be used and re-exposed by Core.
	* Register the security implementation that will be used by core and re-exposed from Core's security service.

[Security into Core] expose authc.getCurrentUser from core.security #177976

[Security into Core] expose authc.getCurrentUser from core.security #177976

Conversation

pgayvallet commented Mar 5, 2024 • edited Loading

Summary

pgayvallet commented Mar 5, 2024

pgayvallet commented Mar 5, 2024

pgayvallet commented Mar 5, 2024

pgayvallet commented Mar 5, 2024

pgayvallet commented Mar 5, 2024

pgayvallet commented Mar 5, 2024

pgayvallet commented Mar 5, 2024

pgayvallet commented Mar 5, 2024

pgayvallet commented Mar 5, 2024

pgayvallet commented Mar 5, 2024

pgayvallet commented Mar 6, 2024

pgayvallet commented Mar 6, 2024

pgayvallet commented Mar 6, 2024

pgayvallet commented Mar 6, 2024

pgayvallet commented Mar 6, 2024

pgayvallet commented Mar 6, 2024

pgayvallet commented Mar 6, 2024

pgayvallet commented Mar 6, 2024

pgayvallet commented Mar 6, 2024

elasticmachine commented Mar 8, 2024

elasticmachine commented Mar 8, 2024

juliaElastic left a comment

Choose a reason for hiding this comment

elasticmachine commented Mar 8, 2024

jgowdyelastic left a comment

Choose a reason for hiding this comment

smith left a comment

Choose a reason for hiding this comment

TinaHeiligers 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

azasypkin commented Mar 13, 2024

azasypkin 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

kibana-ci commented Mar 14, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Page load bundle

API count

History

[Security into Core] expose `authc.getCurrentUser` from `core.security` #177976

[Security into Core] expose `authc.getCurrentUser` from `core.security` #177976

pgayvallet commented Mar 5, 2024 •

edited

Loading