Introduces plugins.security.superadmin.secret for superadmin access when https is disabled#6073
Introduces plugins.security.superadmin.secret for superadmin access when https is disabled#6073itsmevichu wants to merge 6 commits intoopensearch-project:mainfrom
plugins.security.superadmin.secret for superadmin access when https is disabled#6073Conversation
Signed-off-by: Vishnutheep B <vishnutheep@gmail.com>
|
@itsmevichu wherever possible we should abstract Can you also include in the PR description how a caller uses the superadmin secret to make a request to the cluster? |
|
Sure, will do the changes. |
| public static final Setting<SecureString> SECURITY_SUPERADMIN_SECRET_SETTING = SecureSetting.secureString( | ||
| ConfigConstants.SECURITY_SUPERADMIN_SECRET_SECURE, | ||
| SECURITY_SUPERADMIN_SECRET_INSECURE_SETTING | ||
| ); |
There was a problem hiding this comment.
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.
| */ | ||
| 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())); |
There was a problem hiding this comment.
What happens if I configure a user on my cluster with the same name as this hard coded SECURITY_SUPERADMIN_SECRET_USER? Is there a path to escalate privileges here?
| */ | ||
| if (!gRPC) { | ||
| final String providedSecret = request.header(ConfigConstants.SECURITY_SUPERADMIN_SECRET_HEADER); | ||
| if (isSuperadminSecretValid(providedSecret)) { |
There was a problem hiding this comment.
Do we want to audit log on a failed check since this is similar to a failed basic auth attempt?
Signed-off-by: Vishnutheep B <vishnutheep@gmail.com>
Signed-off-by: Vishnutheep B <vishnutheep@gmail.com>
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit 9a27345.
The table above displays the top 10 most important findings. Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
Signed-off-by: Vishnutheep B <vishnutheep@gmail.com>
Description
Old Behavior
New Behaviour
plugins.security.superadmin.secretfor plaintext secret configuration.plugins.security.superadmin.secret_securefor keystore-based storage, which is the recommended approach:X-OpenSearch-Superadmin-Secret-sas or --superadmin-secretIssues Resolved
#6051
To do:
Testing
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.