-
Notifications
You must be signed in to change notification settings - Fork 365
Introduces plugins.security.superadmin.secret for superadmin access when https is disabled
#6073
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.
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
3ba87a7
e97a44a
51f78df
68e63c3
4564e9f
9a27345
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,8 @@ | |
|
|
||
| import java.net.InetAddress; | ||
| import java.net.InetSocketAddress; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.security.MessageDigest; | ||
| import java.util.Arrays; | ||
| import java.util.Collection; | ||
| import java.util.Collections; | ||
|
|
@@ -46,6 +48,7 @@ | |
| import com.google.common.cache.RemovalListener; | ||
| import com.google.common.cache.RemovalNotification; | ||
| import com.google.common.collect.Multimap; | ||
| import org.apache.commons.lang3.ObjectUtils; | ||
| import org.apache.http.HttpHeaders; | ||
| import org.apache.logging.log4j.LogManager; | ||
| import org.apache.logging.log4j.Logger; | ||
|
|
@@ -108,6 +111,7 @@ public class BackendRegistry { | |
| private final ThreadPool threadPool; | ||
| private final UserInjector userInjector; | ||
| private final ClusterInfoHolder clusterInfoHolder; | ||
| private final String superadminSecret; | ||
| private int ttlInMin; | ||
| private Cache<AuthCredentials, User> userCache; // rest standard | ||
| private Cache<String, User> restImpersonationCache; // used for rest impersonation | ||
|
|
@@ -169,6 +173,7 @@ public BackendRegistry( | |
| this.threadPool = threadPool; | ||
| this.clusterInfoHolder = clusterInfoHolder; | ||
| this.userInjector = new UserInjector(settings, threadPool, auditLog, xffResolver); | ||
| this.superadminSecret = SecuritySettings.SECURITY_SUPERADMIN_SECRET_SETTING.get(settings).toString(); | ||
| this.restAuthDomains = Collections.emptySortedSet(); | ||
| this.ipAuthFailureListeners = Collections.emptyList(); | ||
|
|
||
|
|
@@ -278,6 +283,24 @@ public boolean authenticate(final SecurityRequestChannel request) { | |
| return true; | ||
| } | ||
|
|
||
| /* | ||
| Authenticates superuser based on superadmin secret. The secret is read from thread context | ||
| and compared against the configured superadmin secret. If superuser is authenticated here we skip the remaining | ||
| authentication flow. This mechanism is independent of the security index and serves as an out-of-band recovery | ||
| path for HTTP deployments. | ||
| */ | ||
| if (!gRPC) { | ||
|
itsmevichu marked this conversation as resolved.
Outdated
|
||
| final String providedSecret = request.header(ConfigConstants.SECURITY_SUPERADMIN_SECRET_HEADER); | ||
| if (isSuperadminSecretValid(providedSecret)) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to audit log on a failed check since this is similar to a failed basic auth attempt? |
||
| log.debug("Superadmin authentication successful via secret"); | ||
| User superuser = new User(ConfigConstants.SECURITY_SUPERADMIN_SECRET_USER); | ||
| UserSubject subject = new UserSubjectImpl(threadPool, superuser); | ||
| threadContext.putPersistent(ConfigConstants.OPENDISTRO_SECURITY_AUTHENTICATED_USER, subject); | ||
| threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, superuser); | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| /* | ||
| Authenticate users injected in the thread context by internal components (plugins). | ||
| */ | ||
|
|
@@ -613,6 +636,27 @@ private boolean checkRemoteAddrBlocked(SecurityRequestChannel request) { | |
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Validates given superadmin secret against configured secret. | ||
| * @param providedSecret the secret from the request header | ||
| * @return true if valid, false otherwise | ||
| */ | ||
| private boolean isSuperadminSecretValid(String providedSecret) { | ||
| if (ObjectUtils.isEmpty(superadminSecret) || ObjectUtils.isEmpty(providedSecret)) { | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| return MessageDigest.isEqual( | ||
| superadminSecret.getBytes(StandardCharsets.UTF_8), | ||
| providedSecret.getBytes(StandardCharsets.UTF_8) | ||
| ); | ||
| } catch (Exception e) { | ||
| log.debug("Error comparing superadmin secret", e); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Resolve and stash client IP in thread context. | ||
| * @param request with remote address. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -271,7 +271,7 @@ public RestHandler wrap(RestHandler original, AdminDNs adminDNs, Set<RestHeaderD | |
| * Checks if a given user is a SuperAdmin | ||
| */ | ||
| boolean userIsSuperAdmin(User user, AdminDNs adminDNs) { | ||
| return user != null && adminDNs.isAdmin(user); | ||
| return user != null && (adminDNs.isAdmin(user) || ConfigConstants.SECURITY_SUPERADMIN_SECRET_USER.equals(user.getName())); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if I configure a user on my cluster with the same name as this hard coded |
||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,13 @@ | |
|
|
||
| package org.opensearch.security.support; | ||
|
|
||
| import org.apache.logging.log4j.LogManager; | ||
| import org.apache.logging.log4j.Logger; | ||
|
|
||
| import org.opensearch.common.settings.SecureSetting; | ||
| import org.opensearch.common.settings.Setting; | ||
| import org.opensearch.common.settings.Settings; | ||
| import org.opensearch.core.common.settings.SecureString; | ||
|
|
||
| public class SecuritySettings { | ||
| public static final Setting<Boolean> LEGACY_OPENDISTRO_SSL_DUAL_MODE_SETTING = Setting.boolSetting( | ||
|
|
@@ -57,4 +63,40 @@ public class SecuritySettings { | |
| Setting.Property.Dynamic, | ||
| Setting.Property.Sensitive | ||
| ); | ||
|
|
||
| public static final Setting<SecureString> SECURITY_SUPERADMIN_SECRET_INSECURE_SETTING = new InsecureFallbackStringSetting( | ||
| ConfigConstants.SECURITY_SUPERADMIN_SECRET | ||
| ); | ||
|
|
||
| public static final Setting<SecureString> SECURITY_SUPERADMIN_SECRET_SETTING = SecureSetting.secureString( | ||
| ConfigConstants.SECURITY_SUPERADMIN_SECRET_SECURE, | ||
| SECURITY_SUPERADMIN_SECRET_INSECURE_SETTING | ||
| ); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO, this should be accompanied with a very prominent note that using such a setting is inherently insecure and should be only if there is no other way. It should be noted that using TLS client cert auth should be always preferred. |
||
|
|
||
| /** | ||
| * Alternative to InsecureStringSetting, which doesn't raise an exception if allow_insecure_settings is false, but | ||
| * instead logs a warning. This is to allow insecure settings for container-based deployments while recommending | ||
| * secure keystore usage. | ||
| */ | ||
| private static class InsecureFallbackStringSetting extends Setting<SecureString> { | ||
|
itsmevichu marked this conversation as resolved.
Outdated
|
||
| private static final Logger LOG = LogManager.getLogger(InsecureFallbackStringSetting.class); | ||
| private final String name; | ||
|
|
||
| private InsecureFallbackStringSetting(String name) { | ||
| super(name, "", s -> new SecureString(s.toCharArray()), Property.NodeScope, Property.Deprecated, Property.Filtered); | ||
| this.name = name; | ||
| } | ||
|
|
||
| public SecureString get(Settings settings) { | ||
| if (this.exists(settings)) { | ||
| LOG.warn( | ||
| "Setting [{}] has a secure counterpart [{}] which should be used instead - allowing for container-based deployments", | ||
| this.name, | ||
| ConfigConstants.SECURITY_SUPERADMIN_SECRET_SECURE | ||
| ); | ||
| } | ||
|
|
||
| return super.get(settings); | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.