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.

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

[BUG] Fix roles verification for roles mapping and internal users #3278

Merged
merged 1 commit into from
Sep 1, 2023

Conversation

willyborankin
Copy link
Collaborator

@willyborankin willyborankin commented Aug 31, 2023

Description

The resent refactoring of the REST APIs: #3123 introduce a regression in how roles-mapping verification has worked before.
The old solution verified only hidden roles both for internal users and roles mapping, while new was too strict and forbid to do it for both.

This PR fixes the problem and uses the same logic as it was before.

  • In case of roles-mapping it verifies only a role associated with it that the role is not hidden.
  • In case of internal users it verifies that a role is not hidden and roles-mapping associated with the role is mutable

So verification was split and added to the corresponding ActionApi class which is more convenient as it was before.

Tests for such functionality were added as well.

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

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.

@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Merging #3278 (17c9e2e) into main (0338cdd) will increase coverage by 0.02%.
Report is 4 commits behind head on main.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #3278      +/-   ##
============================================
+ Coverage     63.15%   63.18%   +0.02%     
  Complexity     3448     3448              
============================================
  Files           263      263              
  Lines         20024    20038      +14     
  Branches       3341     3346       +5     
============================================
+ Hits          12647    12661      +14     
+ Misses         5748     5747       -1     
- Partials       1629     1630       +1     
Files Changed Coverage Δ
...security/dlic/rest/api/InternalUsersApiAction.java 77.45% <100.00%> (+2.45%) ⬆️
.../security/dlic/rest/api/RolesMappingApiAction.java 97.05% <100.00%> (ø)
...curity/dlic/rest/validation/EndpointValidator.java 94.20% <100.00%> (-0.25%) ⬇️

... and 4 files with indirect coverage changes

@willyborankin
Copy link
Collaborator Author

So the logic was like this:

protected boolean isValidRolesMapping(final RestChannel channel, final String role) {
        final SecurityDynamicConfiguration<?> rolesConfiguration = load(CType.ROLES, false);
        final SecurityDynamicConfiguration<?> rolesMappingConfiguration = load(CType.ROLESMAPPING, false);

        if (!rolesConfiguration.exists(role)) {
            notFound(channel, "Role '" + role + "' is not available for role-mapping.");
            return false;
        }

        if (isHidden(rolesConfiguration, role)) {
            notFound(channel, "Role '" + role + "' is not available for role-mapping.");
            return false;
        }

        return isWriteable(channel, rolesMappingConfiguration, role);
    }

@willyborankin willyborankin force-pushed the role-mappings-fix branch 2 times, most recently from fb956de to 4708582 Compare August 31, 2023 20:02
import static org.opensearch.security.dlic.rest.api.Responses.forbiddenMessage;
import static org.opensearch.security.dlic.rest.api.Responses.notFoundMessage;
import static org.opensearch.security.dlic.rest.support.Utils.withIOException;
import static org.opensearch.security.dlic.rest.api.Responses.*;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this star import crept in. There's an open issue to add enforcement to forbid * imports: #3260

Copy link
Collaborator Author

@willyborankin willyborankin Aug 31, 2023

Choose a reason for hiding this comment

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

yeap looks like that. fixed manually

}

@Test
public void validateSecurityRolesWithImmutableRolesMappingConfig() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Its really confusing me that there are 2 ways that roles mapping can be done to map a user directly to a role.

  1. Modify the opendistro_security_roles attribute of a user - this modifies the internaluser config entry and not the roles_mapping config entry
  2. Update the roles_mapping and add the username in the list of users.

I had to spend a little time digesting that this is simulating the behavior of a PUT _plugins/_security/api/internalusers/<username> call where the config entries for the role and roles_mapping already exist. In this case in the test setup it creates 4 roles where the mapping will fail:

  1. some_hidden_role - The role is hidden
  2. some_role_with_hidden_mapping - The role is fine, but the roles mapping is hidden
  3. some_role_with_reserved_mapping - The role is fine, but the roles mapping is reserved
  4. some_role_with_static_mapping - The role is fine, but the roles mapping is static

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not exactly but closer. In any case a tests on PATCH should be added.

Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

Thank you @willyborankin. This PR looks good to me.

Overall feedback is that it uses var a lot instead of the datatype. I recommend to minimize the usage of var. It may be clear to the compiler what the datatype is, but sometimes var makes it hard to glean the datatype at a glance.

@peternied
Copy link
Member

Overall feedback is that it uses var a lot instead of the datatype. I recommend to minimize the usage of var. It may be clear to the compiler what the datatype is, but something var makes it hard to glean the datatype at a glance.

@cwperks Want to make an issue / checkstyle rule? IMO I don't mind var when locals have understandable names, but I'd be curious about your thoughts

@peternied peternied merged commit 53f64b9 into opensearch-project:main Sep 1, 2023
@peternied peternied added the backport 2.x backport to 2.x branch label Sep 1, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 1, 2023
)

The resent refactoring of the REST APIs:
#3123 introduce a
regression in how roles-mapping verification has worked before.
The old solution verified only hidden roles both for internal users and
roles mapping, while new was too strict and forbid to do it for both.

This PR fixes the problem and uses the same logic as it was before.

- In case of roles-mapping it verifies only a role associated with it
that the role is not hidden.
- In case of internal users it verifies that a role is not hidden and
roles-mapping associated with the role is mutable

So verification was split and added to the corresponding ActionApi class
which is more convenient as it was before.

Signed-off-by: Andrey Pleskach <[email protected]>
(cherry picked from commit 53f64b9)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@peternied peternied mentioned this pull request Sep 1, 2023
31 tasks
cwperks pushed a commit that referenced this pull request Sep 1, 2023
…ernal users (#3282)

Backport 53f64b9 from #3278.

Signed-off-by: Andrey Pleskach <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@cwperks
Copy link
Member

cwperks commented Sep 1, 2023

Want to make an issue / checkstyle rule? IMO I don't mind var when locals have understandable names, but I'd be curious about your thoughts

Sure, I plan to check if core has any restrictions and will file an issue shortly for discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants