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
HIVE-25468: Create/Drop functions are authorized in HMS #2595
Conversation
80b0d18
to
15abd74
Compare
...re/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
Outdated
Show resolved
Hide resolved
...ore-server/src/main/java/org/apache/hadoop/hive/metastore/events/PreCreateFunctionEvent.java
Outdated
Show resolved
Hide resolved
try { | ||
func.setOwnerName(SecurityUtils.getUGI().getShortUserName()); | ||
} catch (Exception ex) { | ||
LOG.error("Cannot obtain username", ex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Please improve this error message. If I look at this error message in vaccum, it does not indicate where it occurred.
public class CreateFunctionEvent extends HiveMetaStoreAuthorizableEvent { | ||
private static final Logger LOG = LoggerFactory.getLogger(CreateFunctionEvent.class); | ||
|
||
private String COMMAND_STR = "create function"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static final ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other events related to the database and table are also using the static final LOG variable, so I thought I would follow the same.
...pache/hadoop/hive/ql/security/authorization/plugin/metastore/events/CreateFunctionEvent.java
Outdated
Show resolved
Hide resolved
Function function = event.getFunction(); | ||
List<ResourceUri> uris = function.getResourceUris(); | ||
ret.add(new HivePrivilegeObject(HivePrivilegeObject.HivePrivilegeObjectType.FUNCTION, function.getDbName(), function.getFunctionName(), null, | ||
null, HivePrivilegeObject.HivePrivObjectActionType.OTHER, null, function.getClassName(), function.getOwnerName(), function.getOwnerType())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont we need to check the DELETE permissions for the functions here? Shouldnt the ActionType be HivePrivilegeObject.HivePrivObjectActionType.DELETE ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to send HivePrivilegeObject.HivePrivObjectActionType as OTHER since Ranger is expecting OTHER as action type. If the action type is other, Ranger would check HivePrivilege Object type and do the required action.
public class DropFunctionEvent extends HiveMetaStoreAuthorizableEvent { | ||
private static final Logger LOG = LoggerFactory.getLogger(DropFunctionEvent.class); | ||
|
||
private String COMMAND_STR = "drop function"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static final ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other events related to the database and table are also using the static final LOG variable, so I thought I would follow the same.
.../apache/hadoop/hive/ql/security/authorization/plugin/metastore/events/DropFunctionEvent.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me. +1
Fix has been committed to master. Please close the PR and the jira. |
What changes were proposed in this pull request?
Created authorizable events for create and drop function commands in HMS.
Why are the changes needed?
This addresses the security issue in HMS.
Does this PR introduce any user-facing change?
Yeah, respective policies should be added in ranger/sentry
How was this patch tested?
Local machine, and remote cluster.