Use a licensed feature per realm-type (+custom) #78810

tvernum · 2021-10-07T03:55:07Z

This commit changes the licensed feature usage tracking for realms to
record each realm type as its own separate feature.
Custom realms continue to fall under a single catch-all feature.

This commit changes the licensed feature usage tracking for realms to record each realm type as its own separate feature. Custom realms continue to fall under a single catch-all feature.

elasticmachine · 2021-10-07T03:55:11Z

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

ywangd

LGTM

I don't think it belongs to this PR. But those names used in Realms and friends confused me again. There are "internal", "builtin", "native", "fallback", "standard", "basic" etc. We really need formalize and clean them up in a future PR.

ywangd · 2021-10-07T07:39:26Z

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

-    // Standard realms (LDAP, AD, PKI, etc) are Gold+
+    // Some realms (LDAP, AD, PKI) are Gold+
+    public static final LicensedFeature.Persistent LDAP_REALM_FEATURE =
+        LicensedFeature.persistentLenient(null, "security_ldap_realm", License.OperationMode.GOLD);


I believe @rjernst purposely uses dash (-) instead of (underscore) to different the new names from old ones.

I also think we should have a family of “security” or “security-realms”, and the feature name can just be eg “ldap-realm”.

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

ywangd · 2021-10-07T08:50:45Z

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

+     * @throws IllegalArgumentException if the provided type is not an {@link #isInternalRealm(String) internal realm}
+     */
+    static LicensedFeature.Persistent getLicensedFeature(String type) {
+        if (Strings.isNullOrEmpty(type)) {


Should we use Strings.hasText here similar to what is used in Realms#getLicensedFeatureForRealm. It has better coverage for blank strings.

Blanks will still fail below when opt is null, as will any other string with whitespace (e.g. " ldap").

This code forces clear failures in things that don't implement a type (e.g. poorly configured mocks), but I don't think we need to go out of our way to deal with whitespace there.

ywangd · 2021-10-07T09:03:35Z

...lugin/security/src/test/java/org/elasticsearch/xpack/security/authc/InternalRealmsTests.java

+                // In theory this could be "standard" too, but there aren't any realms on that license level
+                assertThat(feature.getMinimumOperationMode(), is(License.OperationMode.GOLD));


I don't quite get what this comment try to say. Could you please elaborate?

ywangd · 2021-10-07T09:04:08Z

...lugin/security/src/test/java/org/elasticsearch/xpack/security/authc/InternalRealmsTests.java

+                assertThat(feature.getMinimumOperationMode(), is(License.OperationMode.GOLD));
+            } else {
+                assertThat(feature, notNullValue());
+                // In theory this could be "enterprise" too, but there aren't any realms on that license level


Same as above.

ywangd · 2021-10-07T09:11:02Z

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/RealmsTests.java

+        assertThat(
+            iae.getMessage(),
+            is(
+                equalTo(
+                    "multiple realms [realm_1, realm_2] configured of type [kerberos], [kerberos] can only have one such realm configured"
+                )
+            )
+        );


Is this spotless format change?

ywangd · 2021-10-07T09:24:13Z

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

+     * @return The licensed feature for the given realm type, or {@code null} if the realm does not require a specific license type
+     * @throws IllegalArgumentException if the provided type is not an {@link #isInternalRealm(String) internal realm}
+     */
+    static LicensedFeature.Persistent getLicensedFeature(String type) {


I am not sure about this method. It exists really just for Relams.getLicensedFeatureForRealm and is not meant to be used standalone.

I'd personally move it as a private method in Realms. This also removes the need to have XPACK_TYPES as a Map which could be a good thing beause using Optional.of() to accomodate null value seems to be an indication of forcing things together.

My concern in that we already have too many places that try and keep track of all the builtin realm types. I don't think it's wise for both Realms and InternalRealms to know about what realm types exist.

I can rework InternalRealms to have multiple maps instead if that helps, but I think it's a bad idea to force Realms to be aware of each internal realm type and the mapping to a feature.

I changed things around a bit.
The separation between Realms and InternalRealms is a bit weird, and many of the methods on InternalRealms are redundant in production code, but are used widely in tests.
We ought to refactor things a bit, but this isn't the PR for it.

One thing to note is that getFactories duplicates the list of realm types, so I think we might want something like:

private static class InternalRealm { String type; Realm.Factory factory; @Nullable LicensedFeature.Persistent licensedFeature; }

And then have a Map of those.

Thanks Tim. You proposal of having a class of InternalRealm to represent the Realm at the concept level* sounds good to me. And I agree it does not belong to this PR.

* "Concept level" may not be really descriptive for its purpose. My intention is to differentiate it from the runtime class Realm and the configuration class RealmConfig.

rjernst

A couple general thoughts but I defer to Yang for details.

rjernst · 2021-10-08T13:27:07Z

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

-    // Standard realms (LDAP, AD, PKI, etc) are Gold+
+    // Some realms (LDAP, AD, PKI) are Gold+
+    public static final LicensedFeature.Persistent LDAP_REALM_FEATURE =
+        LicensedFeature.persistentLenient(null, "security_ldap_realm", License.OperationMode.GOLD);


I also think we should have a family of “security” or “security-realms”, and the feature name can just be eg “ldap-realm”.

rjernst · 2021-10-08T13:31:37Z

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

-                } else {
-                    Security.ALL_REALMS_FEATURE.stopTracking(licenseStateSnapshot, realm.name());
+                final LicensedFeature.Persistent feature = getLicensedFeatureForRealm(realm.type());
+                if (feature != null) {


When is null possible? Shouldn’t it always have a feature object since unknown should fallback to custom?

It will be null if it's basic licensed.

In master that shouldn't happen because a basic licensed realm cannot become inactive at runtime. In 7.x it can because of the hoops we jump through to provide a default set of basic licensed realms if the configured realms are unlicensed (and the reverse behaviour when the configured realms become licensed again will mean that those basic realms are no longer active).

I'll switch to an assert here instead, but convert it to an if on backport.

rjernst · 2021-10-11T18:47:34Z

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

+    public static final LicensedFeature.Persistent LDAP_REALM_FEATURE =
+        LicensedFeature.persistentLenient(REALMS_FEATURE_FAMILY, "ldap", License.OperationMode.GOLD);
+    public static final LicensedFeature.Persistent AD_REALM_FEATURE =
+        LicensedFeature.persistentLenient(REALMS_FEATURE_FAMILY, "active_directory", License.OperationMode.GOLD);


Can we please use active-directory here for consistency with other new features that are using dashes instead of underscore?

I can, but the realm type in settings is active_directory so we'll lose that consistency.

elasticsearchmachine · 2021-10-13T05:41:30Z

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 78810

This commit changes the licensed feature usage tracking for realms to record each realm type as its own separate feature. Custom realms continue to fall under a single catch-all feature. Backport of: elastic#78810

This commit changes the licensed feature usage tracking for realms to record each realm type as its own separate feature. Custom realms continue to fall under a single catch-all feature. Backport of: #78810

Use a licensed feature per realm-type (+custom)

e4df2a1

This commit changes the licensed feature usage tracking for realms to record each realm type as its own separate feature. Custom realms continue to fall under a single catch-all feature.

tvernum added >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) :Security/License License functionality for commercial features v8.0.0 v7.16.0 labels Oct 7, 2021

tvernum requested review from rjernst and ywangd October 7, 2021 03:55

elasticmachine added Team:Security Meta label for security team labels Oct 7, 2021

This comment has been minimized.

Sign in to view

ywangd approved these changes Oct 7, 2021

View reviewed changes

rjernst reviewed Oct 8, 2021

View reviewed changes

tvernum added 3 commits October 11, 2021 12:08

Merge branch 'master' into licensed-feature-per-realm-type

c0c27a4

Address feedback

61beeec

Rework InternalRealms

d3add15

rjernst reviewed Oct 11, 2021

View reviewed changes

tvernum added 2 commits October 12, 2021 16:58

Switch to hyphen

3388c54

Merge branch 'master' into licensed-feature-per-realm-type

88951d6

tvernum added the auto-backport-and-merge Automatically create backport pull requests and merge when ready label Oct 13, 2021

tvernum merged commit 327b8bd into elastic:master Oct 13, 2021

tvernum mentioned this pull request Oct 14, 2021

[Backport 7.x] Use a licensed feature per realm-type (+custom) #79121

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

Use a licensed feature per realm-type (+custom) #78810

Use a licensed feature per realm-type (+custom) #78810

tvernum commented Oct 7, 2021

elasticmachine commented Oct 7, 2021

This comment has been minimized.

ywangd left a comment

ywangd Oct 7, 2021

rjernst Oct 8, 2021

ywangd Oct 7, 2021

tvernum Oct 11, 2021

ywangd Oct 7, 2021

ywangd Oct 7, 2021

ywangd Oct 7, 2021

ywangd Oct 7, 2021

tvernum Oct 11, 2021

tvernum Oct 11, 2021

tvernum Oct 11, 2021

ywangd Oct 11, 2021 •

edited

Loading

rjernst left a comment

rjernst Oct 8, 2021

rjernst Oct 8, 2021

tvernum Oct 11, 2021

rjernst Oct 11, 2021

tvernum Oct 12, 2021

elasticsearchmachine commented Oct 13, 2021

		// In theory this could be "standard" too, but there aren't any realms on that license level
		assertThat(feature.getMinimumOperationMode(), is(License.OperationMode.GOLD));

Use a licensed feature per realm-type (+custom) #78810

Use a licensed feature per realm-type (+custom) #78810

Conversation

tvernum commented Oct 7, 2021

elasticmachine commented Oct 7, 2021

This comment has been minimized.

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

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

ywangd Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

rjernst 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

elasticsearchmachine commented Oct 13, 2021

💔 Backport failed

ywangd Oct 11, 2021 •

edited

Loading