Convert security auth feature objects to LicensedFeature #79213

rjernst · 2021-10-15T03:02:51Z

This commit moves the security auth realm and engine license
checks to use the new LicensedFeature class.

This commit moves the security auth realm and engine license checks to use the new LicensedFeature class.

elasticmachine · 2021-10-15T03:02:55Z

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

tvernum

The code changes seem fine, but

I don't think we want to group these 2 features under a family
I don't think the behaviour we'll get (which is the same as we currently have) is actually what we'll want.

I guess we can tackle item 2 later, but probably need to decide on item 1 now so that we introduce stable names into the API.

tvernum · 2021-10-15T03:38:40Z

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

@@ -376,6 +376,12 @@
    public static final LicensedFeature.Persistent CUSTOM_REALMS_FEATURE =
        LicensedFeature.persistentLenient(REALMS_FEATURE_FAMILY, "custom", License.OperationMode.PLATINUM);

+    private static final String AUTHORIZATION_FEATURE_FAMILY = "security-authorization";
+    public static final LicensedFeature.Momentary AUTHORIZATION_REALM_FEATURE =
+        LicensedFeature.momentary(AUTHORIZATION_FEATURE_FAMILY, "realm", License.OperationMode.PLATINUM);


I suspect authorization realms should be persistent.
They're "in use" by virtue of a setting on a realm, more than because someone authenticated using one. I think we would prefer to have them track the same way realms do (by configuration not active use).

I agree we should use explicit tracking, I'm just not sure all that will entail. At this point I'm just trying to remove remaining uses of XPackLicenseState.Feature so that there is no confusion or possibility of adding new values there for the life of 7.16.

tvernum · 2021-10-15T03:40:53Z

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

@@ -376,6 +376,12 @@
    public static final LicensedFeature.Persistent CUSTOM_REALMS_FEATURE =
        LicensedFeature.persistentLenient(REALMS_FEATURE_FAMILY, "custom", License.OperationMode.PLATINUM);

+    private static final String AUTHORIZATION_FEATURE_FAMILY = "security-authorization";


I don't think bundling these under a feature family is meaningful (unless we bundle all of security under a common family).
Authorization realms and Authorization engines don't have any relationship with one another other than both being security features (and having similar words in the name).

Ok, I'll remove the family, the similarity of the name made me think they were related. With that in mind, is there a more descriptive name we can use for these? "security-authorization-realm" is going to be confusing with the security realms feature?

The public Yaml setting is authorization_realms, and the internal classes refer to it as DelegatedAuthorization

The docs uses both terms (they refer to the concept as "Delegating Authorization ...", but also reference the name of the setting.

My feeling was that authorization-realm[s] is more helpful because it ties into something tangible in the docs (a specific setting), but delegated-authorization would be fine.

tvernum · 2021-10-15T03:42:40Z

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

+    public static final LicensedFeature.Momentary AUTHORIZATION_REALM_FEATURE =
+        LicensedFeature.momentary(AUTHORIZATION_FEATURE_FAMILY, "realm", License.OperationMode.PLATINUM);
+    public static final LicensedFeature.Momentary AUTHORIZATION_ENGINE_FEATURE =
+        LicensedFeature.momentary(AUTHORIZATION_FEATURE_FAMILY, "engine", License.OperationMode.PLATINUM);


I think the same is true for an AuthorizationEngine. It's a type of plugin, and I think we want to consider it to be in use any time one is installed in the cluster.

ywangd · 2021-10-15T04:14:10Z

I am reomving myself as a reviewer since @tvernum is already on it.

tvernum

LGTM.
If you want to change the feature name from security-authorization-realm to security-delegated-authorization (or a variant on that) that's fine too.

rjernst · 2021-10-18T13:20:08Z

Thanks, I updated the name as suggested.

elasticsearchmachine · 2021-10-18T16:17:53Z

💔 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 79213

This commit moves the security auth realm and engine license checks to use the new LicensedFeature class.

…9386) This commit moves the security auth realm and engine license checks to use the new LicensedFeature class.

Convert security auth feature objects to LicensedFeature

b2f823b

This commit moves the security auth realm and engine license checks to use the new LicensedFeature class.

rjernst added >refactoring :Security/License License functionality for commercial features v8.0.0 v7.16.0 labels Oct 15, 2021

rjernst requested a review from ywangd October 15, 2021 03:02

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

rjernst added the auto-backport-and-merge label Oct 15, 2021

tvernum reviewed Oct 15, 2021

View reviewed changes

ywangd removed their request for review October 15, 2021 04:14

rjernst added 2 commits October 15, 2021 06:19

remove family

6eb9763

Merge branch 'master' into license/security_auth

f709883

tvernum approved these changes Oct 18, 2021

View reviewed changes

Merge branch 'master' into license/security_auth

2466cca

rjernst merged commit 444d7ca into elastic:master Oct 18, 2021

rjernst deleted the license/security_auth branch October 18, 2021 16:16

rjernst added a commit to rjernst/elasticsearch that referenced this pull request Oct 18, 2021

Convert security auth feature objects to LicensedFeature (elastic#79213)

91d16b4

This commit moves the security auth realm and engine license checks to use the new LicensedFeature class.

elasticsearchmachine pushed a commit that referenced this pull request Oct 18, 2021

Convert security auth feature objects to LicensedFeature (#79213) (#7…

40c02ef

…9386) This commit moves the security auth realm and engine license checks to use the new LicensedFeature class.

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

Convert security auth feature objects to LicensedFeature #79213

Convert security auth feature objects to LicensedFeature #79213

rjernst commented Oct 15, 2021

elasticmachine commented Oct 15, 2021

tvernum left a comment

tvernum Oct 15, 2021

rjernst Oct 15, 2021

tvernum Oct 15, 2021

rjernst Oct 15, 2021

tvernum Oct 18, 2021 •

edited

Loading

tvernum Oct 15, 2021

ywangd commented Oct 15, 2021

tvernum left a comment

rjernst commented Oct 18, 2021

elasticsearchmachine commented Oct 18, 2021

Convert security auth feature objects to LicensedFeature #79213

Convert security auth feature objects to LicensedFeature #79213

Conversation

rjernst commented Oct 15, 2021

elasticmachine commented Oct 15, 2021

tvernum left a comment

Choose a reason for hiding this comment

tvernum Oct 15, 2021

Choose a reason for hiding this comment

rjernst Oct 15, 2021

Choose a reason for hiding this comment

tvernum Oct 15, 2021

Choose a reason for hiding this comment

rjernst Oct 15, 2021

Choose a reason for hiding this comment

tvernum Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

tvernum Oct 15, 2021

Choose a reason for hiding this comment

ywangd commented Oct 15, 2021

tvernum left a comment

Choose a reason for hiding this comment

rjernst commented Oct 18, 2021

elasticsearchmachine commented Oct 18, 2021

💔 Backport failed

tvernum Oct 18, 2021 •

edited

Loading