Add ability for plugins to inject roles #560

skkosuri-amzn · 2020-07-14T06:15:18Z

Issue #, if available:
opendistro-for-elasticsearch/alerting#215

Description of changes:
This is used to inject opendistro-roles into the request when there is no user involved, like periodic plugin background jobs. The roles injection is done using thread-context at transport layer only. You can't inject roles using REST api. Using this we can enforce fine-grained-access-control for the transport layer calls plugins make.

Format for the injected string: user_name|role_1,role_2. User name is ignored. And roles are opendistro-roles.
threadContext.putTransient("opendistro_security_injected_roles", "bones|role_1,role_2"); The request will be evaluated using role1 and role2. These changes are similar to User injection functionality.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov · 2020-07-14T06:47:46Z

Codecov Report

Merging #560 into master will increase coverage by 0.10%.
The diff coverage is 86.11%.

@@             Coverage Diff              @@
##             master     #560      +/-   ##
============================================
+ Coverage     63.15%   63.25%   +0.10%     
- Complexity     3050     3064      +14     
============================================
  Files           233      234       +1     
  Lines         16560    16592      +32     
  Branches       2989     2996       +7     
============================================
+ Hits          10458    10496      +38     
+ Misses         4551     4544       -7     
- Partials       1551     1552       +1

Impacted Files	Coverage Δ	Complexity Δ
...lasticsearch/security/support/ConfigConstants.java	`93.75% <ø> (ø)`	`5.00 <0.00> (ø)`
...earch/security/privileges/PrivilegesEvaluator.java	`65.64% <50.00%> (-0.26%)`	`74.00 <0.00> (+2.00)`	⬇️
...oforelasticsearch/security/auth/RolesInjector.java	`84.61% <84.61%> (ø)`	`7.00 <7.00> (?)`
...arch/security/filter/OpenDistroSecurityFilter.java	`72.18% <100.00%> (+4.21%)`	`48.00 <0.00> (+3.00)`
...earch/security/resolver/IndexResolverReplacer.java	`58.60% <0.00%> (+0.26%)`	`80.00% <0.00%> (+1.00%)`
...security/configuration/DlsFlsFilterLeafReader.java	`58.91% <0.00%> (+0.74%)`	`58.00% <0.00%> (ø%)`
.../dlic/auth/ldap2/LDAPConnectionFactoryFactory.java	`57.25% <0.00%> (+0.76%)`	`23.00% <0.00%> (ø%)`
...ecurity/configuration/ConfigurationRepository.java	`70.49% <0.00%> (+1.09%)`	`22.00% <0.00%> (ø%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5be858c...295ee18. Read the comment docs.

vrozov · 2020-07-25T02:31:56Z

src/main/java/com/amazon/opendistroforelasticsearch/security/support/ConfigConstants.java

@@ -100,7 +100,10 @@
    public static final String OPENDISTRO_SECURITY_USER_HEADER = OPENDISTRO_SECURITY_CONFIG_PREFIX+"user_header";

    public static final String OPENDISTRO_SECURITY_INJECTED_USER = "injected_user";
-
+
+    public static final String OPENDISTRO_SECURITY_INJECTED_ROLES_ENABLED = "opendistro_security_injected_roles_enabled";


Please follow the common format for other settings (opendistro_security.enable_inject_roles)

vrozov · 2020-07-25T02:35:03Z

src/main/java/com/amazon/opendistroforelasticsearch/security/OpenDistroSecurityPlugin.java

@@ -961,6 +961,7 @@ public Settings additionalSettings() {
            settings.add(Setting.boolSetting(ConfigConstants.OPENDISTRO_SECURITY_UNSUPPORTED_LOAD_STATIC_RESOURCES, true, Property.NodeScope, Property.Filtered));
            settings.add(Setting.boolSetting(ConfigConstants.OPENDISTRO_SECURITY_SSL_CERT_RELOAD_ENABLED, false, Property.NodeScope, Property.Filtered));
            settings.add(Setting.boolSetting(ConfigConstants.OPENDISTRO_SECURITY_UNSUPPORTED_ACCEPT_INVALID_CONFIG, false, Property.NodeScope, Property.Filtered));
+            settings.add(Setting.boolSetting(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_ENABLED, true, Property.NodeScope, Property.Filtered));


What is the risk of enabling role injection by default? If there is no risk, why is it necessary to introduce enable settings?

Removed OPENDISTRO_SECURITY_INJECTED_ROLES_ENABLED, its enabled all the time.With that Settings are not required, removed.

src/main/java/com/amazon/opendistroforelasticsearch/security/auth/RolesInjector.java

vrozov · 2020-07-25T02:38:05Z

src/main/java/com/amazon/opendistroforelasticsearch/security/auth/RolesInjector.java

+    public final User getUser() {
+        return user;
+    }
+}


nit: new line

...ain/java/com/amazon/opendistroforelasticsearch/security/filter/OpenDistroSecurityFilter.java

src/test/java/com/amazon/opendistroforelasticsearch/security/RolesInjectorIntegTest.java

src/main/java/com/amazon/opendistroforelasticsearch/security/auth/RolesInjector.java

...est/java/com/amazon/opendistroforelasticsearch/security/test/plugin/RolesInjectorPlugin.java

…he time.With that Settings are not required, removed.Refactored RolesInjector.java. Moved RolesInjectorPlugin class to the RolesInjectorPlugin. Added exception message for roles injection missing permissions.

skkosuri-amzn · 2020-07-29T06:18:57Z

@vrozov addressed the comments and published revision.

vrozov · 2020-07-30T18:11:47Z

src/main/java/com/amazon/opendistroforelasticsearch/security/auth/RolesInjector.java

+    public Set<String> injectUserAndRoles(final ThreadContext ctx) {
+
+        if(log.isDebugEnabled()){
+            log.debug("Injected role str: "+ ctx.getTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES));


Use smart logging. Log after checking for null.

vrozov · 2020-07-30T18:13:16Z

src/main/java/com/amazon/opendistroforelasticsearch/security/auth/RolesInjector.java

+            log.debug("Injected role str: "+ ctx.getTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES));
+        }
+        String injectedStr = ctx.getTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES);
+        if (Strings.isNullOrEmpty(injectedStr)) {


Empty string should probably be considered the way as malformed, only check for null is necessary.

vrozov · 2020-07-30T18:14:55Z

src/main/java/com/amazon/opendistroforelasticsearch/security/auth/RolesInjector.java

+        if(log.isDebugEnabled()){
+            log.debug("Injected role str: "+ ctx.getTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES));
+        }
+        String injectedStr = ctx.getTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES);


rename to injectedUserAndRoles

vrozov · 2020-07-30T18:16:55Z

src/main/java/com/amazon/opendistroforelasticsearch/security/auth/RolesInjector.java

+        if (Strings.isNullOrEmpty(injectedStr)) {
+            return null;
+        }
+        User user = parseUser(injectedStr);


Can the string be parsed once?

vrozov · 2020-07-30T18:32:54Z

src/main/java/com/amazon/opendistroforelasticsearch/security/auth/RolesInjector.java

+            log.error("Roles must not be null, injected string was '{}.' Roles injection failed.", injectedStr);
+            return null;
+        }
+        return new HashSet<>(Arrays.asList(strs[1].split(",")));


Use ImmutableSet.copyOf().

vrozov · 2020-07-30T18:36:03Z

src/main/java/com/amazon/opendistroforelasticsearch/security/auth/RolesInjector.java

+            return;
+
+        threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS,
+                new TransportAddress(InetAddress.getLoopbackAddress(), 9300));


What if it is not a default port? Is it necessary to set remote address when roles are injected?

vrozov · 2020-08-06T16:49:41Z

src/main/java/com/amazon/opendistroforelasticsearch/security/auth/RolesInjector.java

+        }
+
+        if(log.isDebugEnabled()){
+            log.debug("Injected roles: "+ injectedUserAndRoles);


nit: please use smart logging:

Suggested change

log.debug("Injected roles: "+ injectedUserAndRoles);

log.debug("Injected roles: {}", injectedUserAndRoles);

vrozov · 2020-08-06T16:50:04Z

src/main/java/com/amazon/opendistroforelasticsearch/security/auth/RolesInjector.java

+            return null;
+        }
+
+        if(log.isDebugEnabled()){


nit: the check is not necessary with smart logging.

vrozov · 2020-08-06T17:26:22Z

src/main/java/com/amazon/opendistroforelasticsearch/security/auth/RolesInjector.java

+                    " Roles injection failed.", injectedUserAndRoles);
+            return null;
+        }
+        if (Strings.isNullOrEmpty(strs[0].trim())) {


nit: trim() does not return null.

Good catch, thanks.

vrozov · 2020-08-06T17:28:01Z

...ain/java/com/amazon/opendistroforelasticsearch/security/filter/OpenDistroSecurityFilter.java

-                listener.onFailure(new ElasticsearchSecurityException("no permissions for " + pres.getMissingPrivileges()+ " and "+user, RestStatus.FORBIDDEN));
+                String err = (injectedRoles == null) ?
+                        String.format("no permissions for %s and %s" , pres.getMissingPrivileges(), user) :
+                        String.format("no permissions for %s and associated roles %s ", pres.getMissingPrivileges(), injectedRoles);


nit: trim extra space in the error message.

vrozov · 2020-08-06T17:29:41Z

src/test/java/com/amazon/opendistroforelasticsearch/security/auth/RolesInjectorTest.java

+                "invalid",
+                "role_1,role_2",
+                " | ",
+                "  "


nit: add "|" use case.

vrozov · 2020-08-06T18:16:51Z

src/main/java/com/amazon/opendistroforelasticsearch/security/auth/RolesInjector.java

+            log.error("Roles must not be null, injected string was '{}.' Roles injection failed.", injectedUserAndRoles);
+            return null;
+        }
+        Set<String> roles = new HashSet<>(ImmutableSet.copyOf(strs[1].split(",")));


Suggested change

Set<String> roles = new HashSet<>(ImmutableSet.copyOf(strs[1].split(",")));

Set<String> roles = ImmutableSet.copyOf(strs[1].split(","));

vrozov · 2020-08-06T18:18:18Z

src/main/java/com/amazon/opendistroforelasticsearch/security/auth/RolesInjector.java

+        }
+
+        if (StringUtils.isEmpty(StringUtils.trim(strs[0]))) {
+            log.error("Username must not be null, injected string was '{}.' Roles injection failed.", injectedUserAndRoles);


Suggested change

log.error("Username must not be null, injected string was '{}.' Roles injection failed.", injectedUserAndRoles);

log.error("Username must be provided, injected string was '{}.' Roles injection failed.", injectedUserAndRoles);

vrozov · 2020-08-06T18:18:34Z

src/main/java/com/amazon/opendistroforelasticsearch/security/auth/RolesInjector.java

+        User user = new User(strs[0]);
+
+        if (strs.length < 2 || StringUtils.isEmpty(StringUtils.trim(strs[0]))) {
+            log.error("Roles must not be null, injected string was '{}.' Roles injection failed.", injectedUserAndRoles);


Suggested change

log.error("Roles must not be null, injected string was '{}.' Roles injection failed.", injectedUserAndRoles);

log.error("Roles must be provided, injected string was '{}.' Roles injection failed.", injectedUserAndRoles);

vrozov

LGTM

sujithvm · 2020-08-06T19:01:25Z

...ain/java/com/amazon/opendistroforelasticsearch/security/filter/OpenDistroSecurityFilter.java

@@ -112,6 +114,7 @@ public OpenDistroSecurityFilter(final Settings settings, final PrivilegesEvaluat
        this.compatConfig = compatConfig;
        this.indexResolverReplacer = indexResolverReplacer;
        this.immutableIndicesMatcher = WildcardMatcher.from(settings.getAsList(ConfigConstants.OPENDISTRO_SECURITY_COMPLIANCE_IMMUTABLE_INDICES, Collections.emptyList()));
+        this.rolesInjector = new RolesInjector();


nit: This can be injected via constructor ? or the class function can be made static as it has no state (other than logger) ?

Initially we had injected via constructor, then we separated to make it clean. Would prefer not to change this.

…arch-project#560)

Add ability for plugins to inject roles

b7944eb

skkosuri-amzn requested a review from a team as a code owner July 14, 2020 06:15

skkosuri-amzn self-assigned this Jul 14, 2020

skkosuri-amzn requested review from dinusX, vrozov and hardik-k-shah July 14, 2020 06:52

vrozov reviewed Jul 25, 2020

View reviewed changes

Removed OPENDISTRO_SECURITY_INJECTED_ROLES_ENABLED, its enabled all t…

e9c0a0f

…he time.With that Settings are not required, removed.Refactored RolesInjector.java. Moved RolesInjectorPlugin class to the RolesInjectorPlugin. Added exception message for roles injection missing permissions.

vrozov reviewed Jul 30, 2020

View reviewed changes

skkosuri-amzn force-pushed the master branch 2 times, most recently from 6119e1d to e9c0a0f Compare August 5, 2020 14:55

skkosuri-amzn added 2 commits August 5, 2020 08:11

merge from master

31b265c

refactor and removed remote-addr

2a96cb7

vrozov reviewed Aug 6, 2020

View reviewed changes

minor changes and cleanup

45da444

vrozov reviewed Aug 6, 2020

View reviewed changes

minor changes and cleanup

295ee18

vrozov approved these changes Aug 6, 2020

View reviewed changes

sujithvm reviewed Aug 6, 2020

View reviewed changes

sujithvm approved these changes Aug 6, 2020

View reviewed changes

vrozov merged commit 6304e78 into opensearch-project:master Aug 6, 2020

skkosuri-amzn mentioned this pull request Sep 17, 2020

Integrate Security with Alerting opendistro-for-elasticsearch/alerting#6

Closed

lbreinig pushed a commit to lbreinig/security that referenced this pull request Dec 23, 2021

Add ability for plugins to inject roles (opensearch-project#560)

4f670c2

zhongnansu mentioned this pull request May 5, 2022

[DRAFT][Proof of Concept] Enable Observability scheduling to send ppl query opensearch-project/observability#677

Closed

6 tasks

wuychn pushed a commit to ochprince/security that referenced this pull request Mar 16, 2023

Add ability for plugins to inject roles (opensearch-project#560)

5a1e66a

gaobinlong pushed a commit to gaobinlong/security that referenced this pull request Aug 30, 2023

extend session using session ttl config instead of cookie ttl (opense…

5a48aad

…arch-project#560)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability for plugins to inject roles #560

Add ability for plugins to inject roles #560

skkosuri-amzn commented Jul 14, 2020

codecov bot commented Jul 14, 2020 •

edited

Loading

vrozov Jul 25, 2020

vrozov Jul 25, 2020

skkosuri-amzn Jul 29, 2020

vrozov Jul 25, 2020

skkosuri-amzn commented Jul 29, 2020

vrozov Jul 30, 2020

vrozov Jul 30, 2020

vrozov Jul 30, 2020

vrozov Jul 30, 2020

vrozov Jul 30, 2020

vrozov Jul 30, 2020

vrozov Aug 6, 2020

vrozov Aug 6, 2020

vrozov Aug 6, 2020

skkosuri-amzn Aug 6, 2020

vrozov Aug 6, 2020

vrozov Aug 6, 2020

vrozov Aug 6, 2020

vrozov Aug 6, 2020

vrozov Aug 6, 2020

vrozov left a comment

sujithvm Aug 6, 2020

skkosuri-amzn Aug 6, 2020

	log.debug("Injected roles: "+ injectedUserAndRoles);
	log.debug("Injected roles: {}", injectedUserAndRoles);

	Set<String> roles = new HashSet<>(ImmutableSet.copyOf(strs[1].split(",")));
	Set<String> roles = ImmutableSet.copyOf(strs[1].split(","));

	log.error("Username must not be null, injected string was '{}.' Roles injection failed.", injectedUserAndRoles);
	log.error("Username must be provided, injected string was '{}.' Roles injection failed.", injectedUserAndRoles);

	log.error("Roles must not be null, injected string was '{}.' Roles injection failed.", injectedUserAndRoles);
	log.error("Roles must be provided, injected string was '{}.' Roles injection failed.", injectedUserAndRoles);

Add ability for plugins to inject roles #560

Add ability for plugins to inject roles #560

Conversation

skkosuri-amzn commented Jul 14, 2020

codecov bot commented Jul 14, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skkosuri-amzn commented Jul 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrozov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 14, 2020 •

edited

Loading