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

Add RBAC support for MC executed actions #18264

Merged
merged 4 commits into from
Mar 1, 2021

Conversation

kwart
Copy link
Member

@kwart kwart commented Feb 22, 2021

This PR adds a new permission type for management MessageTasks.

Newly the client used in Management Center will need to have either the all-permission granted or the new permission type management-permission. E.g.

<hazelcast xmlns="http://www.hazelcast.com/schema/config"
           xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
           xsi:schemaLocation="http://www.hazelcast.com/schema/config
           http://www.hazelcast.com/schema/config/hazelcast-config-4.2.xsd">
    <security enabled="true">
        <!--
           define security realm and client-authentication here
        -->
        <client-permissions>
            <management-permission principal="management">
                <endpoints>
                    <endpoint>127.0.0.1</endpoint>
                </endpoints>
            </management-permission>
        </client-permissions>
    </security>
</hazelcast>
hazelcast:
  security:
    enabled: true
    client-permissions:
      management:
        - principal: management
          endpoints:
            - 127.0.0.1

This change can affect Hazelcast Enterprise users with security enabled but without the management client having the all-permission granted. In such a case ManagementCenter operations on the cluster will fail with the AccessControlException thrown.

@kwart kwart added this to the 4.2 milestone Feb 22, 2021
@kwart kwart self-assigned this Feb 22, 2021
@kwart kwart requested a review from a team as a code owner February 22, 2021 15:12
Copy link
Contributor

@blazember blazember left a comment

Choose a reason for hiding this comment

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

LGTM in general, left only a few minors. Also, the Spring config needs to be updated/tested as well.

Copy link
Contributor

@emre-aydin emre-aydin left a comment

Choose a reason for hiding this comment

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

ReadMetricsMessageTask is also used by MC and needs a permission.

AFAICT this PR doesn't change the default behavior, i.e. Management Center will be able to execute all operations on a target cluster with default configuration. Can you please confirm this is the case?

Since we didn't have these permissions before, we had added scripting enabled to Management Center config before. Do you think it's still needed? If not, maybe we can mark it as deprecated and remove it in 5.0. WDYT?

@emre-aydin
Copy link
Contributor

Will you send a PR to the docs as well? I think we should link to that section from the Management Center docs.

@kwart
Copy link
Member Author

kwart commented Feb 24, 2021

@blazember & @emre-aydin Thank you for your reviews and valuable comments. I'll go through them and fix (or comment back).

@hazelcast hazelcast deleted a comment from hz-devops-test Feb 24, 2021
@kwart
Copy link
Member Author

kwart commented Feb 24, 2021

LGTM in general, left only a few minors. Also, the Spring config needs to be updated/tested as well.

The Spring configuration was added too (I somehow missed it completely in the first iteration - someone is getting old here :) )

ReadMetricsMessageTask is also used by MC and needs a permission.

Required permission was added to the ReadMetricsMessageTask too.

AFAICT this PR doesn't change the default behavior, i.e. Management Center will be able to execute all operations on a target cluster with default configuration. Can you please confirm this is the case?

The behavior is only changed for cases where the security is enabled (i.e. Hazelcast Enterprise only) and the client used by the MC doesn't have the all-permission granted.

Since we didn't have these permissions before, we had added scripting enabled to Management Center config before. Do you think it's still needed? If not, maybe we can mark it as deprecated and remove it in 5.0. WDYT?

I don't think we can simply remove the protection as the real implementation is on the member protocol level (RunScriptOperation). We would need to put some thoughts into it and come up with a secure redesign of the feature.

Will you send a PR to the docs as well? I think we should link to that section from the Management Center docs.

I'll sync with @Serdaro to decide how to cover it in the 4.2 documentation.

@kwart kwart requested review from emre-aydin and blazember and removed request for sancar February 24, 2021 17:44
Copy link
Contributor

@emre-aydin emre-aydin left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all the comments!

Sorry for missing this in the first review, but is the new config reported via the ConfigXmlGenerator or do you also need to add it there as well?

(I think there should be some checklist or so for checking all the places that needs to be updated for a new config - too easy to miss something).

@kwart
Copy link
Member Author

kwart commented Feb 25, 2021

Sorry for missing this in the first review, but is the new config reported via the ConfigXmlGenerator or do you also need to add it there as well?

The ConfigXmlGenerator handles client permissions by using PermissionConfig.PermissionType enum so I didn't need to touch the generator directly.

The config checklist lives in the GH wiki: https://github.com/hazelcast/hazelcast/wiki/Checklist-for-Hazelcast-config-changes

@kwart kwart merged commit a9e401f into hazelcast:master Mar 1, 2021
@kwart kwart deleted the fix/4.2/ee-3946-rbac branch March 1, 2021 09:22
@jgardiner68
Copy link

@AyberkSorgun can you please create an MC Jira task for how MC behaves when when it does not have sufficient permissions on the IMDG size.

We should show a helpful message which explains what configuration options should be added to the members.

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

Successfully merging this pull request may close these issues.

5 participants