Enable license feature usage for Security Realms #61963

tvernum · 2020-09-04T08:16:35Z

Changes the way that license checking for realms is performed so that
it is compatible with feature usage reporting.

The detailed changes are:

Replace the old license features of "ALL_REALMS" and
"STANDARD_REALMS" with a new feature per realm type (+"CUSTOM")
When evaluating realms against the license, check the configured
realms against their coresponding feature. We now only perform
checks for realms that have been configured
Switched the previous license check that was performed in
Realms.asList() to a new filtering process in a license state
listener. This was needed due to the increased number of feature
types that we were checking against.

Because these checks are done on license change, the "last used" in
the feature usage reflects the more recent of "node startup" or
"license updated". This is not strictly accurate, since a realm might
(and typically will) be used more frequently than that to perform
authentications. However, for SAML, OIDC, and Delegated PKI the
authentication flow uses specific Rest actions that do explicit
license checking (primarily to provide client and explicit error
messages) and these will report an accurate "last used" time.

For LDAP, AD, PKI, Kerberos and Custom realms, we might consider a
followup change to update the last-used time each time authentication
is successful.

Relates: #59342

Changes the way that license checking for realms is performed so that it is compatible with feature usage reporting. The detailed changes are: 1. Replace the old license features of "ALL_REALMS" and "STANDARD_REALMS" with a new feature per realm type (+"CUSTOM") 2. When evaluating realms against the license, check the configured realms against their coresponding feature. We know only perform checks for realms that have been configured 3. Switched the previous license check that was performed in Realms.asList() to a new filtering process in a license state listener. This was needed due to the increased number of feature types that we were checking against. Because these checks are done on license change, the "last used" in the feature usage reflects the more recent of "node startup" or "license updated". This is not strictly accurate, since a realm might (and typically will) be used more frequently than that to perform authentications. However, for SAML, OIDC, and Delegated PKI the authentication flow uses specific Rest actions that do explicit license checking (primarily to provide client and explicit error messages) and these will report an accurate "last used" time. For LDAP, AD, PKI, Kerberos and Custom realms, we might consider a followup change to update the last-used time each time authenticated is successful.

elasticmachine · 2020-09-04T08:16:38Z

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

rjernst

LGTM. Thanks for picking it up so quickly!

ywangd

I still need read the tests more closely. But here are a few comments to begin with.

ywangd · 2020-09-10T05:30:49Z

x-pack/plugin/core/src/main/java/org/elasticsearch/license/XPackLicenseState.java

        SECURITY_AUTHORIZATION_ENGINE(OperationMode.PLATINUM, true),
        SECURITY_STATS_AND_HEALTH(OperationMode.MISSING, true),

+


Nit: extra blank line

ywangd · 2020-09-10T05:42:39Z

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

+    // All realms that were explicitly configured in the settings, some of these may not be enabled due to licensing
+    private final List<Realm> allConfiguredRealms;
+    // the default native realms to enable if no other realms are permitted under the current license
+    private final List<Realm> fallbackNativeRealms;


This is not strictly a problem of this PR since it is carried from the existing code. But can we use a different word other than "native"? It is pretty confusing since we do have an actual "native" realm. It might be good enough to just call it "fallbackRealms".

ywangd · 2020-09-10T05:48:03Z

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

+        if (ReservedRealm.TYPE.equals(type)) {
+            return XPackLicenseState.Feature.SECURITY;
+        }
+        return XPACK_TYPES.getOrDefault(type, XPackLicenseState.Feature.SECURITY_CUSTOM_REALM);


Nit: It is a bit weird that a method of "Internal"Realms can check for custom realm type.

ywangd · 2020-09-10T06:04:19Z

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

-        }
+        this.allConfiguredRealms = initRealms();
+        this.allConfiguredRealms.forEach(r -> r.initialize(this, licenseState));
+        this.activeRealms = allConfiguredRealms;


Why not call recomputeActiveRealms here? Is it because licenseState may not be ready at this point? It also assumes that the first usage of activeRealms will be after the licenseState is ready (and listener invoked). Will it be possible that somehow an unlicensed realm is enabled because of racing condition?

ywangd

LGTM.

A few more comments. Nothing major.

ywangd · 2020-09-14T04:53:18Z

...ecurity/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java

        verify(auditTrail).authenticationFailed(reqId, firstRealm.name(), token, "_action", transportRequest);
-        verify(realms).asList();
-        verifyNoMoreInteractions(realms);
+        verify(realms, atLeastOnce()).asList();


realms.asList() is now called in the init() method. So should this be atLeast(2)?

Should it? Testing how many times a method is called tends to be an anti-pattern. Why do we can if it was called once or twice?

The concern is different here because realms.asList() is called once in init() as:

assertThat(this.realms.asList(), equalTo(realmList));

So either we don't test for this interaction at all. Or we should test for at least twice because the first time is done as part of test setup and there is no piont to test it.

ywangd · 2020-09-14T05:32:56Z

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

+        verify(licenseState, atLeastOnce()).checkFeature(Feature.SECURITY);
+        verify(licenseState, atLeastOnce()).checkFeature(Feature.SECURITY_CUSTOM_REALM);


Took me a while to understand the purpose of these two checks: IIUC, they are the alternative to ensure the licenseState listener work. Since the listener is a protected method and the its downstream method isRealmTypeAvailable is static, so the interactions have to be checked like this. I think it could use some comments for the underlying logic to help future readers (future me included).

I'll add comments, but the explanation isn't what you've described.
The purpose of this PR is to add these calls. This is how we track feature usage. So, the test verifies that they've been called.

ywangd · 2020-09-14T05:40:33Z

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

+    private static final Map<String, XPackLicenseState.Feature> XPACK_TYPES = Map.ofEntries(
+        Map.entry(NativeRealmSettings.TYPE, XPackLicenseState.Feature.SECURITY),
+        Map.entry(FileRealmSettings.TYPE, XPackLicenseState.Feature.SECURITY),
+        Map.entry(LdapRealmSettings.AD_TYPE, XPackLicenseState.Feature.SECURITY_AD_REALM),
+        Map.entry(LdapRealmSettings.LDAP_TYPE, XPackLicenseState.Feature.SECURITY_LDAP_REALM),
+        Map.entry(PkiRealmSettings.TYPE, XPackLicenseState.Feature.SECURITY_PKI_REALM),
+        Map.entry(SamlRealmSettings.TYPE, XPackLicenseState.Feature.SECURITY_SAML_REALM),
+        Map.entry(OpenIdConnectRealmSettings.TYPE, XPackLicenseState.Feature.SECURITY_OIDC_REALM),
+        Map.entry(KerberosRealmSettings.TYPE, XPackLicenseState.Feature.SECURITY_KERBEROS_REALM)
+    );


Other than InternalRealms#getConfigurableRealmsTypes, is there any other reason for this map to not include the ReservedRealm? In other places where this map is used, e.g. isXPackRealm, it feels better if ReversedRealm is in it.

getConfigurableRealmsTypes seems to be the only exception. It is only used for tests as far as I can tell. Would it be better to remove ReservedRealm inside this method only instead of not having it in the Map?

Probably. There's a lot that could be cleaned up here. I tried to keep this PR as self contained as possible and didn't want to change the meaning of XPACK_TYPES unnecessarily.

tvernum · 2020-10-06T00:35:10Z

I'm going to hold this over to 7.11
While investigating the current CI failure, I found that the change had broken something that was only adequately tested in a SAML QA test, and not in any unit or integ tests.
I have a fix, but I'm not confident pushing this right before FF given the lack of test coverage.

tvernum · 2020-10-06T01:57:19Z

@elasticmachine update branch

tvernum · 2020-12-08T23:37:37Z

I've dropped the 7.11.0 label from this PR. There are a number of open questions about feature usage tracking, and we don't anticipate having answers in time for 7.11, we'll select a new release target when we know more.

tvernum · 2021-08-25T01:54:29Z

Replaced by #76592 and #76476

tvernum added >refactoring :Security/License License functionality for commercial features v8.0.0 v7.10.0 labels Sep 4, 2020

tvernum requested review from rjernst and ywangd September 4, 2020 08:16

elasticmachine added the Team:Security Meta label for security team label Sep 4, 2020

rjernst approved these changes Sep 4, 2020

View reviewed changes

ywangd reviewed Sep 10, 2020

View reviewed changes

ywangd approved these changes Sep 14, 2020

View reviewed changes

tvernum added 2 commits October 5, 2020 16:08

Merge branch 'master' into realm-feature-track

f215cff

Address feedback

9086265

Fix delegated authoriztion

f52ec43

Merge branch 'master' into realm-feature-track

5039fbc

andreidan added v7.11.0 and removed v7.10.0 labels Oct 7, 2020

tvernum mentioned this pull request Nov 23, 2020

Support "on-by-config" features in usage tracking #65345

Closed

tvernum removed the v7.11.0 label Dec 8, 2020

tvernum closed this Aug 25, 2021

jakelandis added v8.0.0-alpha2 and removed v8.0.0 labels Sep 15, 2021

		SECURITY_AUTHORIZATION_ENGINE(OperationMode.PLATINUM, true),
		SECURITY_STATS_AND_HEALTH(OperationMode.MISSING, true),

		verify(licenseState, atLeastOnce()).checkFeature(Feature.SECURITY);
		verify(licenseState, atLeastOnce()).checkFeature(Feature.SECURITY_CUSTOM_REALM);

Enable license feature usage for Security Realms #61963

Enable license feature usage for Security Realms #61963

Uh oh!

Conversation

tvernum commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 4, 2020

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvernum commented Oct 6, 2020

Uh oh!

tvernum commented Oct 6, 2020

Uh oh!

tvernum commented Dec 8, 2020

Uh oh!

tvernum commented Aug 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tvernum commented Sep 4, 2020 •

edited

Loading