You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/05/04 22:44:38 UTC
[GitHub] [ozone] errose28 opened a new pull request #2217: HDDS-5138. admin check in auth. Upgrade related RPC calls should be allowed only for admins.
errose28 opened a new pull request #2217:
URL: https://github.com/apache/ozone/pull/2217
## What changes were proposed in this pull request?
Add admin checks for the following upgrade commands:
- ozone admin om prepare
- ozone admin om cancelprepare
- ozone admin om finalizeupgrade [--takeover]
- ozone admin scm finalizeupgrade [--takeover]
These read only upgrade commands should still be allowed for regular users.
- ozone admin om finalizationstatus
- ozone admin scm finalizationstatus
## What is the link to the Apache JIRA
HDDS-5138
## How was this patch tested?
Manual testing of the above listed commands as admin and non-admin user in the ozonesecure-ha docker-compose cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #2217: HDDS-5138. Upgrade related RPC calls should be allowed only for admins.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #2217:
URL: https://github.com/apache/ozone/pull/2217#discussion_r638130370
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -3617,9 +3618,11 @@ public OzoneDelegationTokenSecretManager getDelegationTokenMgr() {
return ozAdmins;
}
- public boolean isAdmin(String username) {
+ public boolean isAdmin(String username) throws IOException {
if (isAclEnabled) {
- return accessAuthorizer != null && accessAuthorizer.isAdmin(username);
Review comment:
We don't expose isAdmin to the IAccessAuthorizer but remove all the isAdmin call from the namespace access check. This way checkAccess will include the admin check inside it. The s3 secret revoke brings back the isAdmin check to ozone manager with a similar assumption that the operation is not directly mapped into namespace check. I would suggest add additional comments to the isAdmin check in OzoneManager for only non-namespace related admin operations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a change in pull request #2217: HDDS-5138. Upgrade related RPC calls should be allowed only for admins.
Posted by GitBox <gi...@apache.org>.
errose28 commented on a change in pull request #2217:
URL: https://github.com/apache/ozone/pull/2217#discussion_r631206687
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -663,13 +663,33 @@ public boolean getReplicationManagerStatus() {
@Override
public StatusAndMessages finalizeScmUpgrade(String upgradeClientID) throws
IOException {
+ // check admin authorization
+ String remoteUser = getRpcRemoteUsername();
+ try {
+ getScm().checkAdminAccess(remoteUser);
+ } catch (IOException e) {
+ LOG.error("Authorisation failed for finalize scm upgrade", e);
Review comment:
Fixed, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a change in pull request #2217: HDDS-5138. Upgrade related RPC calls should be allowed only for admins.
Posted by GitBox <gi...@apache.org>.
errose28 commented on a change in pull request #2217:
URL: https://github.com/apache/ozone/pull/2217#discussion_r631216191
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -663,13 +663,33 @@ public boolean getReplicationManagerStatus() {
@Override
public StatusAndMessages finalizeScmUpgrade(String upgradeClientID) throws
IOException {
+ // check admin authorization
+ String remoteUser = getRpcRemoteUsername();
+ try {
+ getScm().checkAdminAccess(remoteUser);
+ } catch (IOException e) {
+ LOG.error("Authorisation failed for finalize scm upgrade", e);
+ throw e;
+ }
return scm.finalizeUpgrade(upgradeClientID);
}
@Override
public StatusAndMessages queryUpgradeFinalizationProgress(
String upgradeClientID, boolean force, boolean readonly)
- throws IOException {
+ throws IOException {
+ if (!readonly) {
Review comment:
This isn't a problem for OM (`OzoneManager#queryUpgradeFinalizationProgress`) due to differences in where admin access is checked between OM and SCM. We check admin access in the finalize upgrade OM request, where the readonly flag is false. Other calls just querying the status have the readonly flag set to true.
So in SCM we have to authenticate in this method, but in the OM equivalent we can authenticate before this method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] avijayanhwx merged pull request #2217: HDDS-5138. Upgrade related RPC calls should be allowed only for admins.
Posted by GitBox <gi...@apache.org>.
avijayanhwx merged pull request #2217:
URL: https://github.com/apache/ozone/pull/2217
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a change in pull request #2217: HDDS-5138. Upgrade related RPC calls should be allowed only for admins.
Posted by GitBox <gi...@apache.org>.
errose28 commented on a change in pull request #2217:
URL: https://github.com/apache/ozone/pull/2217#discussion_r637330969
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -3617,9 +3618,11 @@ public OzoneDelegationTokenSecretManager getDelegationTokenMgr() {
return ozAdmins;
}
- public boolean isAdmin(String username) {
+ public boolean isAdmin(String username) throws IOException {
if (isAclEnabled) {
- return accessAuthorizer != null && accessAuthorizer.isAdmin(username);
Review comment:
The Ranger plugin is for OM only, not for SCM. Existing SCM admin operations only use the admins defined by the ozone administrators configuration. If we use the Ranger defined admins for command line operations on OM we have a divergence, because some admins may be able to run upgrade commands on OM, but not the corresponding upgrade commands on SCM, or any other existing SCM admin operations like decommission.
The structure of the existing IAccessAuthorizer seems intended for admin related filesystem operations, like volume creation, that exist only on the OM, since it currently only exposes a `checkAccess` method corresponding to an `IOzoneObj`.
This is the first command line admin operation we are adding to OM, so we have a choice fo implementation. Roles is readonly and does not require admin privileges currently. We have a choice whether or not to use Ranger admins on the OM, but in this case I think it would be best not to so that behavior is consistent across OM and SCM CLI. Let me know if you disagree, I have misunderstood something here, or if there is an easy way to check ranger admins for SCM as well that we could add.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #2217: HDDS-5138. Upgrade related RPC calls should be allowed only for admins.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #2217:
URL: https://github.com/apache/ozone/pull/2217#discussion_r630693887
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -663,13 +663,33 @@ public boolean getReplicationManagerStatus() {
@Override
public StatusAndMessages finalizeScmUpgrade(String upgradeClientID) throws
IOException {
+ // check admin authorization
+ String remoteUser = getRpcRemoteUsername();
+ try {
+ getScm().checkAdminAccess(remoteUser);
+ } catch (IOException e) {
+ LOG.error("Authorisation failed for finalize scm upgrade", e);
+ throw e;
+ }
return scm.finalizeUpgrade(upgradeClientID);
}
@Override
public StatusAndMessages queryUpgradeFinalizationProgress(
String upgradeClientID, boolean force, boolean readonly)
- throws IOException {
+ throws IOException {
+ if (!readonly) {
Review comment:
why do we need readonly flag here? This seems to be readonly operation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] avijayanhwx commented on a change in pull request #2217: HDDS-5138. Upgrade related RPC calls should be allowed only for admins.
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on a change in pull request #2217:
URL: https://github.com/apache/ozone/pull/2217#discussion_r638275281
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -3624,6 +3625,16 @@ public OzoneDelegationTokenSecretManager getDelegationTokenMgr() {
return ozAdmins;
}
+ public boolean isAdmin(String username) throws IOException {
Review comment:
Thank you @xiaoyuyao. We will be bringing in changes from master later this week. We will resolve the conflicts then.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #2217: HDDS-5138. Upgrade related RPC calls should be allowed only for admins.
Posted by GitBox <gi...@apache.org>.
errose28 commented on pull request #2217:
URL: https://github.com/apache/ozone/pull/2217#issuecomment-839025830
Latest CI failure is unrelated and will be fixed by merge of master into upgrade branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #2217: HDDS-5138. Upgrade related RPC calls should be allowed only for admins.
Posted by GitBox <gi...@apache.org>.
errose28 commented on pull request #2217:
URL: https://github.com/apache/ozone/pull/2217#issuecomment-840704611
Thanks for the review @xiaoyuyao. After some offline discussion the new isAdmin check for OM was moved out of the IAccessAuthorizer and fully within the OzoneManager, so it will always use the OM admins specified in the ozone configuration. This makes it consistent with how SCM checks admins.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #2217: HDDS-5138. Upgrade related RPC calls should be allowed only for admins.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #2217:
URL: https://github.com/apache/ozone/pull/2217#discussion_r637325475
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -3617,9 +3618,11 @@ public OzoneDelegationTokenSecretManager getDelegationTokenMgr() {
return ozAdmins;
}
- public boolean isAdmin(String username) {
+ public boolean isAdmin(String username) throws IOException {
if (isAclEnabled) {
- return accessAuthorizer != null && accessAuthorizer.isAdmin(username);
Review comment:
Here we honor admin defined by the pluggable authorizers such as Ranger, if we remove this. The admin defined outside the ozone.administrators configuration key in Ranger will not be honored.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a change in pull request #2217: HDDS-5138. Upgrade related RPC calls should be allowed only for admins.
Posted by GitBox <gi...@apache.org>.
errose28 commented on a change in pull request #2217:
URL: https://github.com/apache/ozone/pull/2217#discussion_r631195733
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -663,13 +663,33 @@ public boolean getReplicationManagerStatus() {
@Override
public StatusAndMessages finalizeScmUpgrade(String upgradeClientID) throws
IOException {
+ // check admin authorization
+ String remoteUser = getRpcRemoteUsername();
+ try {
+ getScm().checkAdminAccess(remoteUser);
+ } catch (IOException e) {
+ LOG.error("Authorisation failed for finalize scm upgrade", e);
+ throw e;
+ }
return scm.finalizeUpgrade(upgradeClientID);
}
@Override
public StatusAndMessages queryUpgradeFinalizationProgress(
String upgradeClientID, boolean force, boolean readonly)
- throws IOException {
+ throws IOException {
+ if (!readonly) {
Review comment:
When readonly=true, the caller gets the status of the upgrade only. When readonly=false, the caller gets the status of the upgrade as well as any queued messages about the upgrade progress (see `StorageContainerManager#queryUpgradeFinalizationProgress`). These messages are only delivered once, since we don't keep track of which client has received which messages. Anyone should be able to query the status of the upgrade, but we do not want to allow regular users to steal upgrade messages intended for admins.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #2217: HDDS-5138. Upgrade related RPC calls should be allowed only for admins.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #2217:
URL: https://github.com/apache/ozone/pull/2217#discussion_r638134732
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -3624,6 +3625,16 @@ public OzoneDelegationTokenSecretManager getDelegationTokenMgr() {
return ozAdmins;
}
+ public boolean isAdmin(String username) throws IOException {
Review comment:
You might want to rebase to pick up the changes from HDDS-5206, which includes a similar isAdmin change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #2217: HDDS-5138. Upgrade related RPC calls should be allowed only for admins.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #2217:
URL: https://github.com/apache/ozone/pull/2217#discussion_r630693661
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -663,13 +663,33 @@ public boolean getReplicationManagerStatus() {
@Override
public StatusAndMessages finalizeScmUpgrade(String upgradeClientID) throws
IOException {
+ // check admin authorization
+ String remoteUser = getRpcRemoteUsername();
+ try {
+ getScm().checkAdminAccess(remoteUser);
+ } catch (IOException e) {
+ LOG.error("Authorisation failed for finalize scm upgrade", e);
Review comment:
NIT: typo: Authorisation => Authorization
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org