Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 6 commits into from
Oct 13, 2021
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e4df2a1
Use a licensed feature per realm-type (+custom)
tvernum Oct 7, 2021
c0c27a4
Merge branch 'master' into licensed-feature-per-realm-type
tvernum Oct 11, 2021
61beeec
Address feedback
tvernum Oct 11, 2021
d3add15
Rework InternalRealms
tvernum Oct 11, 2021
3388c54
Switch to hyphen
tvernum Oct 12, 2021
88951d6
Merge branch 'master' into licensed-feature-per-realm-type
tvernum Oct 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,13 +355,25 @@ public class Security extends Plugin implements SystemIndexPlugin, IngestPlugin,
public static final LicensedFeature.Momentary AUDITING_FEATURE =
LicensedFeature.momentaryLenient(null, "security_auditing", License.OperationMode.GOLD);

private static final String REALMS_FEATURE_FAMILY = "security-realms";
// Builtin realms (file/native) realms are Basic licensed, so don't need to be checked or tracked
// 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(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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public static final LicensedFeature.Persistent PKI_REALM_FEATURE =
LicensedFeature.persistentLenient(REALMS_FEATURE_FAMILY, "pki", License.OperationMode.GOLD);
// SSO realms are Platinum+
public static final LicensedFeature.Persistent STANDARD_REALMS_FEATURE =
LicensedFeature.persistentLenient(null, "security_standard_realms", License.OperationMode.GOLD);
public static final LicensedFeature.Persistent ALL_REALMS_FEATURE =
LicensedFeature.persistentLenient(null, "security_all_realms", License.OperationMode.PLATINUM);
public static final LicensedFeature.Persistent SAML_REALM_FEATURE =
LicensedFeature.persistentLenient(REALMS_FEATURE_FAMILY, "saml", License.OperationMode.PLATINUM);
public static final LicensedFeature.Persistent OIDC_REALM_FEATURE =
LicensedFeature.persistentLenient(REALMS_FEATURE_FAMILY, "oidc", License.OperationMode.PLATINUM);
public static final LicensedFeature.Persistent KERBEROS_REALM_FEATURE =
LicensedFeature.persistentLenient(REALMS_FEATURE_FAMILY, "kerberos", License.OperationMode.PLATINUM);
// Custom realms are Platinum+
public static final LicensedFeature.Persistent CUSTOM_REALMS_FEATURE =
LicensedFeature.persistentLenient(REALMS_FEATURE_FAMILY, "custom", License.OperationMode.PLATINUM);

private static final Logger logger = LogManager.getLogger(Security.class);

Expand Down
71 changes: 51 additions & 20 deletions .../plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/InternalRealms.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,12 @@
package org.elasticsearch.xpack.security.authc;

import org.elasticsearch.bootstrap.BootstrapCheck;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.set.Sets;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.env.Environment;
import org.elasticsearch.license.LicensedFeature;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.watcher.ResourceWatcherService;
import org.elasticsearch.xpack.core.security.authc.Realm;
Expand All @@ -23,6 +26,7 @@
import org.elasticsearch.xpack.core.security.authc.pki.PkiRealmSettings;
import org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings;
import org.elasticsearch.xpack.core.ssl.SSLService;
import org.elasticsearch.xpack.security.Security;
import org.elasticsearch.xpack.security.authc.esnative.NativeRealm;
import org.elasticsearch.xpack.security.authc.esnative.NativeUsersStore;
import org.elasticsearch.xpack.security.authc.esnative.ReservedRealm;
Expand All @@ -37,7 +41,6 @@
import org.elasticsearch.xpack.security.support.SecurityIndexManager;

import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Objects;
Expand All @@ -52,36 +55,64 @@
*/
public final class InternalRealms {

static final String RESERVED_TYPE = ReservedRealm.TYPE;
static final String NATIVE_TYPE = NativeRealmSettings.TYPE;
static final String FILE_TYPE = FileRealmSettings.TYPE;
static final String LDAP_TYPE = LdapRealmSettings.LDAP_TYPE;
static final String AD_TYPE = LdapRealmSettings.AD_TYPE;
static final String PKI_TYPE = PkiRealmSettings.TYPE;
static final String SAML_TYPE = SamlRealmSettings.TYPE;
static final String OIDC_TYPE = OpenIdConnectRealmSettings.TYPE;
static final String KERBEROS_TYPE = KerberosRealmSettings.TYPE;

private static final Set<String> BUILTIN_TYPES = Set.of(NATIVE_TYPE, FILE_TYPE);

/**
* The list of all <em>internal</em> realm types, excluding {@link ReservedRealm#TYPE}.
* The map of all <em>licensed</em> internal realm types to their licensed feature
*/
private static final Set<String> XPACK_TYPES = Collections
.unmodifiableSet(Sets.newHashSet(NativeRealmSettings.TYPE, FileRealmSettings.TYPE, LdapRealmSettings.AD_TYPE,
LdapRealmSettings.LDAP_TYPE, PkiRealmSettings.TYPE, SamlRealmSettings.TYPE, KerberosRealmSettings.TYPE,
OpenIdConnectRealmSettings.TYPE));
private static final Map<String, LicensedFeature.Persistent> LICENSED_REALMS = Map.ofEntries(
Map.entry(AD_TYPE, Security.AD_REALM_FEATURE),
Map.entry(LDAP_TYPE, Security.LDAP_REALM_FEATURE),
Map.entry(PKI_TYPE, Security.PKI_REALM_FEATURE),
Map.entry(SAML_TYPE, Security.SAML_REALM_FEATURE),
Map.entry(KERBEROS_TYPE, Security.KERBEROS_REALM_FEATURE),
Map.entry(OIDC_TYPE, Security.OIDC_REALM_FEATURE)
);

/**
* The list of all standard realm types, which are those provided by x-pack and do not have extensive
* interaction with third party sources
* The set of all <em>internal</em> realm types, excluding {@link ReservedRealm#TYPE}
* @deprecated Use of this method (other than in tests) is discouraged.
*/
private static final Set<String> STANDARD_TYPES = Collections.unmodifiableSet(Sets.newHashSet(NativeRealmSettings.TYPE,
FileRealmSettings.TYPE, LdapRealmSettings.AD_TYPE, LdapRealmSettings.LDAP_TYPE, PkiRealmSettings.TYPE));

@Deprecated
public static Collection<String> getConfigurableRealmsTypes() {
return Collections.unmodifiableSet(XPACK_TYPES);
return Set.copyOf(Sets.union(BUILTIN_TYPES, LICENSED_REALMS.keySet()));
}

/**
* Determines whether <code>type</code> is an internal realm-type that is provided by x-pack,
* excluding the {@link ReservedRealm} and realms that have extensive interaction with
* third party sources
*/
static boolean isStandardRealm(String type) {
return STANDARD_TYPES.contains(type);
static boolean isInternalRealm(String type) {
return RESERVED_TYPE.equals(type) || BUILTIN_TYPES.contains(type) || LICENSED_REALMS.containsKey(type);
}

static boolean isBuiltinRealm(String type) {
return FileRealmSettings.TYPE.equals(type) || NativeRealmSettings.TYPE.equals(type);
return BUILTIN_TYPES.contains(type);
}

/**
* @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}
*/
@Nullable
static LicensedFeature.Persistent getLicensedFeature(String type) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ywangd ywangd Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (Strings.isNullOrEmpty(type)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

throw new IllegalArgumentException("Empty realm type [" + type + "]");
}
if (type.equals(RESERVED_TYPE) || isBuiltinRealm(type)) {
return null;
}
final LicensedFeature.Persistent feature = LICENSED_REALMS.get(type);
if (feature == null) {
throw new IllegalArgumentException("Unsupported realm type [" + type + "]");
}
return feature;
}

/**
Expand Down
56 changes: 34 additions & 22 deletions x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/Realms.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.concurrent.CountDown;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.env.Environment;
import org.elasticsearch.license.LicensedFeature;
import org.elasticsearch.license.XPackLicenseState;
import org.elasticsearch.xpack.core.XPackSettings;
import org.elasticsearch.xpack.core.security.authc.Realm;
Expand Down Expand Up @@ -76,9 +78,10 @@ public Realms(Settings settings, Environment env, Map<String, Realm.Factory> fac
assert factories.get(ReservedRealm.TYPE) == null;

final List<RealmConfig> realmConfigs = buildRealmConfigs();
this.allConfiguredRealms = initRealms(realmConfigs);
this.allConfiguredRealms.forEach(r -> r.initialize(allConfiguredRealms, licenseState));
assert allConfiguredRealms.get(0) == reservedRealm : "the first realm must be reserved realm";
final List<Realm> initialRealms = initRealms(realmConfigs);
this.allConfiguredRealms = initialRealms;
this.allConfiguredRealms.forEach(r -> r.initialize(this.allConfiguredRealms, licenseState));
assert this.allConfiguredRealms.get(0) == reservedRealm : "the first realm must be reserved realm";

recomputeActiveRealms();
licenseState.addListener(this::recomputeActiveRealms);
Expand All @@ -93,18 +96,25 @@ protected void recomputeActiveRealms() {
Strings.collectionToCommaDelimitedString(licensedRealms)
);

stopTrackingInactiveRealms(licenseStateSnapshot, licensedRealms);

activeRealms = licensedRealms;
}

// Can be overridden in testing
protected void stopTrackingInactiveRealms(XPackLicenseState licenseStateSnapshot, List<Realm> licensedRealms) {
// Stop license-tracking for any previously-active realms that are no longer allowed
if (activeRealms != null) {
activeRealms.stream().filter(r -> licensedRealms.contains(r) == false).forEach(realm -> {
if (InternalRealms.isStandardRealm(realm.type())) {
Security.STANDARD_REALMS_FEATURE.stopTracking(licenseStateSnapshot, realm.name());
} else {
ywangd marked this conversation as resolved.
Show resolved Hide resolved
Security.ALL_REALMS_FEATURE.stopTracking(licenseStateSnapshot, realm.name());
}
final LicensedFeature.Persistent feature = getLicensedFeatureForRealm(realm.type());
assert feature != null : "Realm ["
+ realm
+ "] with no licensed feature became inactive due to change to license mode ["
+ licenseStateSnapshot.getOperationMode()
+ "]";
feature.stopTracking(licenseStateSnapshot, realm.name());
});
}

activeRealms = licensedRealms;
}

@Override
Expand Down Expand Up @@ -142,27 +152,29 @@ protected List<Realm> calculateLicensedRealms(XPackLicenseState licenseStateSnap
}

private static boolean checkLicense(Realm realm, XPackLicenseState licenseState) {
if (isBasicLicensedRealm(realm.type())) {
final LicensedFeature.Persistent feature = getLicensedFeatureForRealm(realm.type());
if (feature == null) {
return true;
}
if (InternalRealms.isStandardRealm(realm.type())) {
return Security.STANDARD_REALMS_FEATURE.checkAndStartTracking(licenseState, realm.name());
}
return Security.ALL_REALMS_FEATURE.checkAndStartTracking(licenseState, realm.name());
return feature.checkAndStartTracking(licenseState, realm.name());
}

public static boolean isRealmTypeAvailable(XPackLicenseState licenseState, String type) {
if (Security.ALL_REALMS_FEATURE.checkWithoutTracking(licenseState)) {
final LicensedFeature.Persistent feature = getLicensedFeatureForRealm(type);
if (feature == null) {
return true;
} else if (Security.STANDARD_REALMS_FEATURE.checkWithoutTracking(licenseState)) {
return InternalRealms.isStandardRealm(type) || ReservedRealm.TYPE.equals(type);
} else {
return isBasicLicensedRealm(type);
}
return feature.checkWithoutTracking(licenseState);
}

private static boolean isBasicLicensedRealm(String type) {
return ReservedRealm.TYPE.equals(type) || InternalRealms.isBuiltinRealm(type);
@Nullable
private static LicensedFeature.Persistent getLicensedFeatureForRealm(String realmType) {
assert Strings.hasText(realmType) : "Realm type must be provided (received [" + realmType + "])";
if (InternalRealms.isInternalRealm(realmType)) {
return InternalRealms.getLicensedFeature(realmType);
} else {
return Security.CUSTOM_REALMS_FEATURE;
}
}

public Realm realm(String name) {
Expand Down
2 changes: 1 addition & 1 deletion ...ava/org/elasticsearch/xpack/security/rest/action/RestDelegatePkiAuthenticationAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ protected Exception checkFeatureAvailable(RestRequest request) {
Exception failedFeature = super.checkFeatureAvailable(request);
if (failedFeature != null) {
return failedFeature;
} else if (Security.STANDARD_REALMS_FEATURE.checkWithoutTracking(licenseState)) {
} else if (Security.PKI_REALM_FEATURE.checkWithoutTracking(licenseState)) {
return null;
} else {
logger.info("The '{}' realm is not available under the current license", PkiRealmSettings.TYPE);
Expand Down
42 changes: 38 additions & 4 deletions ...rity/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import org.elasticsearch.index.seqno.SequenceNumbers;
import org.elasticsearch.index.shard.ShardId;
import org.elasticsearch.license.License;
import org.elasticsearch.license.LicensedFeature;
import org.elasticsearch.license.MockLicenseState;
import org.elasticsearch.license.XPackLicenseState;
import org.elasticsearch.license.XPackLicenseState.Feature;
Expand Down Expand Up @@ -79,6 +80,7 @@
import org.elasticsearch.xpack.core.security.authc.DefaultAuthenticationFailureHandler;
import org.elasticsearch.xpack.core.security.authc.Realm;
import org.elasticsearch.xpack.core.security.authc.Realm.Factory;
import org.elasticsearch.xpack.core.security.authc.RealmConfig;
import org.elasticsearch.xpack.core.security.authc.esnative.NativeRealmSettings;
import org.elasticsearch.xpack.core.security.authc.file.FileRealmSettings;
import org.elasticsearch.xpack.core.security.authc.support.AuthenticationContextSerializer;
Expand Down Expand Up @@ -220,8 +222,13 @@ public void init() throws Exception {
.put(XPackSettings.API_KEY_SERVICE_ENABLED_SETTING.getKey(), true)
.build();
MockLicenseState licenseState = mock(MockLicenseState.class);
when(licenseState.isAllowed(Security.ALL_REALMS_FEATURE)).thenReturn(true);
when(licenseState.isAllowed(Security.STANDARD_REALMS_FEATURE)).thenReturn(true);
for (String realmType : InternalRealms.getConfigurableRealmsTypes()) {
final LicensedFeature.Persistent feature = InternalRealms.getLicensedFeature(realmType);
if (feature != null) {
when(licenseState.isAllowed(feature)).thenReturn(true);
}
}
when(licenseState.isAllowed(Security.CUSTOM_REALMS_FEATURE)).thenReturn(true);
when(licenseState.checkFeature(Feature.SECURITY_TOKEN_SERVICE)).thenReturn(true);
when(licenseState.copyCurrentLicenseState()).thenReturn(licenseState);
when(licenseState.checkFeature(Feature.SECURITY_AUDITING)).thenReturn(true);
Expand All @@ -231,7 +238,7 @@ public void init() throws Exception {
when(reservedRealm.type()).thenReturn("reserved");
when(reservedRealm.name()).thenReturn("reserved_realm");
realms = spy(new TestRealms(Settings.EMPTY, TestEnvironment.newEnvironment(settings),
Map.of(FileRealmSettings.TYPE, config -> mock(FileRealm.class), NativeRealmSettings.TYPE, config -> mock(NativeRealm.class)),
Map.of(FileRealmSettings.TYPE, this::mockRealm, NativeRealmSettings.TYPE, this::mockRealm),
licenseState, threadContext, reservedRealm, Arrays.asList(firstRealm, secondRealm),
Arrays.asList(firstRealm)));

Expand Down Expand Up @@ -300,6 +307,25 @@ threadPool, new AnonymousUser(settings), tokenService, apiKeyService, serviceAcc
operatorPrivilegesService);
}

private Realm mockRealm(RealmConfig config) {
Class<? extends Realm> cls;
switch (config.type()) {
case InternalRealms.FILE_TYPE:
cls = FileRealm.class;
break;
case InternalRealms.NATIVE_TYPE:
cls = NativeRealm.class;
break;
default:
throw new IllegalArgumentException("No factory for realm " + config);
}
final Realm mock = mock(cls);
when(mock.type()).thenReturn(config.type());
when(mock.name()).thenReturn(config.name());
when(mock.order()).thenReturn(config.order());
return mock;
}

@After
public void shutdownThreadpool() throws InterruptedException {
if (threadPool != null) {
Expand Down Expand Up @@ -399,6 +425,7 @@ public void testAuthenticateBothSupportSecondSucceeds() throws Exception {
verify(realms, atLeastOnce()).recomputeActiveRealms();
verify(realms, atLeastOnce()).calculateLicensedRealms(any(XPackLicenseState.class));
verify(realms, atLeastOnce()).getActiveRealms();
verify(realms, atLeastOnce()).stopTrackingInactiveRealms(any(XPackLicenseState.class), any());
// ^^ We don't care how many times these methods are called, we just check it here so that we can verify no more interactions below.
verifyNoMoreInteractions(realms);
}
Expand Down Expand Up @@ -2132,7 +2159,9 @@ protected List<Realm> calculateLicensedRealms(XPackLicenseState licenseState) {
// This can happen because the realms are recalculated during construction
return super.calculateLicensedRealms(licenseState);
}
if (Security.STANDARD_REALMS_FEATURE.checkWithoutTracking(licenseState)) {

// Use custom as a placeholder for all non-internal realm
if (Security.CUSTOM_REALMS_FEATURE.checkWithoutTracking(licenseState)) {
return allRealms;
} else {
return internalRealms;
Expand All @@ -2143,6 +2172,11 @@ protected List<Realm> calculateLicensedRealms(XPackLicenseState licenseState) {
public void recomputeActiveRealms() {
super.recomputeActiveRealms();
}

@Override
protected void stopTrackingInactiveRealms(XPackLicenseState licenseStateSnapshot, List<Realm> licensedRealms) {
// Ignore
}
}

private void logAndFail(Exception e) {
Expand Down
Loading