Migrate custom role providers to licensed feature #79127

tvernum · 2021-10-14T08:42:39Z

This changes the license checking for custom role providers to use the
new LicensedFeature model with feature tracking.

To support this, and reduce code complexity the logic for managing
role providers has been moved out of the CompositeRolesStore into a
new RoleProviders class.

This changes the license checking for custom role providers to use the new `LicensedFeature` model with feature tracking. To support this, and reduce code complexity the logic for managing role providers has been moved out of the `CompositeRolesStore` into a new `RoleProviders` test.

elasticmachine · 2021-10-14T08:42:42Z

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

rjernst

LGTM. Could we have unit tests for the new RoleProviders class?

rjernst · 2021-10-14T20:50:55Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

-        List<BiConsumer<Set<String>, ActionListener<RoleRetrievalResult>>> rolesProviders = new ArrayList<>();
+
+        final Map<String, List<BiConsumer<Set<String>, ActionListener<RoleRetrievalResult>>>> customRoleProviders = new HashMap<>(
+            securityExtensions.size()


I wouldn't worry about setting the size since it will need some larger number of empty entries internally anyways.

ywangd

LGTM

ywangd · 2021-10-14T13:19:47Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

+                extensionComponents
+            );
+            if (providers != null && providers.isEmpty() == false) {
+                customRoleProviders.put(extension.toString(), providers);


The generic toString include the hashCode of the object. Is that really an useful information to include? It is not stable. I'd think the class name is sufficient and easier to read.

I agree in principle, however

elasticsearch/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

Lines 677 to 683 in f435488

if (extensionEngine != null && authorizationEngine != null) {

throw new IllegalStateException("Extensions [" + extensionName + "] and [" + extension.toString() + "] "

+ "both set an authorization engine");

}

authorizationEngine = extensionEngine;

extensionName = extension.toString();

}

we already rely on extension.toString().

I want to follow up by adding a name() method on extension and replace all of these uses with that instead.

ywangd · 2021-10-14T13:33:36Z

...lugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/RoleProviders.java

+        if (activeRoleProviders.equals(previousProviders) == false) {
+            changeListeners.forEach(ChangeListener::providersChanged);
+        }


Does this mean we currently don't invalidate roles from custom roles provider on license downgrade or expiry?

I think so.
It looks like the multiple instances of CompositeRolesStore in testCustomRolesProvidersLicensing were needed to avoid the cached values.

...lugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/RoleProviders.java

ywangd · 2021-10-14T23:53:42Z

...lugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/RoleProviders.java

+        providers.addAll(builtInRoleProviders);
+
+        final XPackLicenseState fixedLicenseState = this.licenseState.copyCurrentLicenseState();
+        this.customRoleProviders.forEach((name, customProviders) -> {


Nit: The map is unordered, so in theory it is possible to get a providers that has the same entries as the old one but with different orders on license change. This will in turn lead to an unnecessary invalidateAll() invocation for CompositeRolesStore.

Good point.
Worse, it means that the order of role retrieval can potentially change which could have a semantic difference.

I'll fix it.

I think this actually means that the use of a Map here is wrong.
I'm going to switch it to be based on a LinkedHashMap for now, but I think the right answer is to pass in a list of paired objects (extension, providers) which will be easier to do when extension has a name() method.

* upstream/master: (109 commits) Migrate custom role providers to licensed feature (elastic#79127) Remove stale AwaitsFix in InternalEngineTests (elastic#79323) Fix errors in RefreshListenersTests (elastic#79324) Reeable BwC Tests after elastic#79318 (elastic#79320) Mute BwC Tests for elastic#79318 (elastic#79319) Reenable BwC Tests after elastic#79308 (elastic#79313) Disable BwC Tests for elastic#79308 (elastic#79310) Adjust BWC for node-level field cap requests (elastic#79301) Allow total memory to be overridden (elastic#78750) Fix SnapshotBasedIndexRecoveryIT#testRecoveryIsCancelledAfterDeletingTheIndex (elastic#79269) Disable BWC tests Mute GeoIpDownloaderCliIT.testStartWithNoDatabases (elastic#79299) Add alias support to fleet search API (elastic#79285) Create a coordinating node level reader for tsdb (elastic#79197) Route documents to the correct shards in tsdb (elastic#77731) Inject migrate action regardless of allocate action (elastic#79090) Migrate to data tiers should always ensure a TIER_PREFERENCE is set (elastic#79100) Skip building of BWC distributions when building release artifacts (elastic#79180) Default ENFORCE_DEFAULT_TIER_PREFERENCE to true (elastic#79275) Deprecation of transient cluster settings (elastic#78794) ... # Conflicts: # server/src/main/java/org/elasticsearch/index/IndexMode.java # server/src/test/java/org/elasticsearch/index/TimeSeriesModeTests.java

* upstream/master: (521 commits) Migrate custom role providers to licensed feature (elastic#79127) Remove stale AwaitsFix in InternalEngineTests (elastic#79323) Fix errors in RefreshListenersTests (elastic#79324) Reeable BwC Tests after elastic#79318 (elastic#79320) Mute BwC Tests for elastic#79318 (elastic#79319) Reenable BwC Tests after elastic#79308 (elastic#79313) Disable BwC Tests for elastic#79308 (elastic#79310) Adjust BWC for node-level field cap requests (elastic#79301) Allow total memory to be overridden (elastic#78750) Fix SnapshotBasedIndexRecoveryIT#testRecoveryIsCancelledAfterDeletingTheIndex (elastic#79269) Disable BWC tests Mute GeoIpDownloaderCliIT.testStartWithNoDatabases (elastic#79299) Add alias support to fleet search API (elastic#79285) Create a coordinating node level reader for tsdb (elastic#79197) Route documents to the correct shards in tsdb (elastic#77731) Inject migrate action regardless of allocate action (elastic#79090) Migrate to data tiers should always ensure a TIER_PREFERENCE is set (elastic#79100) Skip building of BWC distributions when building release artifacts (elastic#79180) Default ENFORCE_DEFAULT_TIER_PREFERENCE to true (elastic#79275) Deprecation of transient cluster settings (elastic#78794) ... # Conflicts: # rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/tsdb/10_settings.yml # server/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java # server/src/main/java/org/elasticsearch/common/settings/Setting.java # server/src/main/java/org/elasticsearch/index/IndexMode.java # server/src/test/java/org/elasticsearch/index/TimeSeriesModeTests.java

This changes the license checking for custom role providers to use the new `LicensedFeature` model with feature tracking. To support this, and reduce code complexity the logic for managing role providers has been moved out of the `CompositeRolesStore` into a new `RoleProviders` test. Backport of: elastic#79127

This changes the license checking for custom role providers to use the new `LicensedFeature` model with feature tracking. To support this, and reduce code complexity the logic for managing role providers has been moved out of the `CompositeRolesStore` into a new `RoleProviders` test. Backport of: #79127

tvernum added >enhancement :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC :Security/License License functionality for commercial features v8.0.0 v7.16.0 labels Oct 14, 2021

tvernum requested review from rjernst and ywangd October 14, 2021 08:42

elasticmachine added the Team:Security Meta label for security team label Oct 14, 2021

rjernst approved these changes Oct 14, 2021

View reviewed changes

ywangd approved these changes Oct 15, 2021

View reviewed changes

tvernum added 3 commits October 15, 2021 17:26

Merge branch 'master' into licensed-feature-role-providers

a599361

Add test & other feedback

842f0de

Merge branch 'master' into licensed-feature-role-providers

18ea781

tvernum mentioned this pull request Oct 18, 2021

Add extensionName() to security extension #79329

Merged

tvernum merged commit fceacfe into elastic:master Oct 18, 2021

tvernum mentioned this pull request Oct 18, 2021

Migrate custom role providers to licensed feature #79349

Merged

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate custom role providers to licensed feature #79127

Migrate custom role providers to licensed feature #79127

tvernum commented Oct 14, 2021

elasticmachine commented Oct 14, 2021

rjernst left a comment

rjernst Oct 14, 2021

ywangd left a comment

ywangd Oct 14, 2021

tvernum Oct 15, 2021

ywangd Oct 14, 2021

tvernum Oct 15, 2021

ywangd Oct 14, 2021

tvernum Oct 15, 2021

tvernum Oct 15, 2021

	if (extensionEngine != null && authorizationEngine != null) {
	throw new IllegalStateException("Extensions [" + extensionName + "] and [" + extension.toString() + "] "
	+ "both set an authorization engine");
	}
	authorizationEngine = extensionEngine;
	extensionName = extension.toString();
	}

Migrate custom role providers to licensed feature #79127

Migrate custom role providers to licensed feature #79127

Conversation

tvernum commented Oct 14, 2021

elasticmachine commented Oct 14, 2021

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd 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