ACCESS-95: Remove restrictions on create/drop function

Review Request #2913 - Created April 23, 2013 and updated

Prasad Mujumdar
old-access
master
ACCESS-95
access
brock, shreepadma
Thought the ticket is specific to functions, the implementation of that privilege is also applicable for switch db privilege. Hence the two are combined into the single patch. I will log a separate ticket to track switch db and close it with reference to this one.

- Implicit privilege for create/drop temp functions for a user that has access to any object in the system
- Implicit privilege for use <db> to any user that access to any object in that database
  + An authz config level option to allow any user to access the 'default' if you have access to any object in the system. This is added since its common for hive application to connect to 'default' at the start. without this config, the admins will need add workaround (eg a dummy object just to simulate a connect privilege on default db) for 'use default' to work for every user.
  + The default for the above configuration is to allow 'use default'
- Implemented as an implicit connect privilege that checks if the the user has privilege on any object under the given hierarchy.
- Policy handler allows Authorizable.* to match any authorizable object of the given type

The wiki docs will be updated to reflect the changes and configs after the patch is committed.
Added tests for various use <db> scenarios
Added tests for various create/drop function scenarios
  • 0
  • 0
  • 7
  • 1
  • 8
Description From Last Updated
  1. 
      
  2. This test TestPrivilegesAtFunctionScope is pretty much rewritten. Please ignore the old file and treat it as new.
  3. 
      
  1. Prasad, looks good overall, I do have a few questions below.
    1. Thanks for the review. please see the comments inline
  2. Since create/drop function require ALL on server, I don't fully understand why we are sending db=* down to the policy engine?
    1. sorry I didn't make it clear. The create/drop function is NOT going to be ALL on server. That's one of the things the patch is changing. The reason is that hive only support temporary UDFs. That means if you can't create a function then you can't use it.
      The patch is trying to make create/drop function and use <db> as implicit privilege, pretty much a CONNECT equivalent. If you are allowed to access any object in system then you can create/drop UDF. If have access to any object in a database, then you can switch to that database.
    2. This would take a UDF from ALL on Server to ANY on Server, correct? What are the impacts of this on security and how would the jar's be handled?
    3. Correct. 
      The JAR are still handled externally.
         - ADD JAR will still be restricted since that can to used to inject arbitrary code in Hive.
         - The required jars for UDFs should be in the class, which would require OS level Hive userid or root access to make it available for users. The patch only allows users to register a UDF for a class that's available in Hive's classpath.
    1. oh, I missed that. Thanks!
      changed to AccessConstants.ALL
  3. At this place in the code, we know that: policyPart.getKey().equalsIgnoreCase(requestPart.getKey()) is true. See assertion directly above if statement.
    
    The requesting coming with * means that "do we have access to any object"?
    
    
    1. Right. The key check is unnecessary. Removed it.
      yes, its trying to see if the requester has access to any object of the given type.
  4. 
      
  1. Hi Prasad,
    
    Looks good! But why are we doing the system property substitution?
    
    Brock
    1. Brock, Thanks for the review.
      I saw the problem during testing as the Hive Jars are loaded from maven cache. Hence supporting system property was very convenient. It will also be useful for deployments as you don't have to specify absolute path for the jars.
    2. The URI from getPath() had a unresolved system property in the form of ${sys.prop}?
    1. As an alternative to hardcoding full path for the jar
  2. 
      
  1. 
      
  2. It's possible for getCodeSource to return null. It's also possible for getPath to return an empty string. I think that both these cases should be handled by throwing an exception with a good error message so we can different these cases should we see them in the wild.
    1. Agreed. Added specif error messages for both cases.
  3. 
      
  1. 
      
    1. ah right. Thanks for catching that.
  2. 
      
Review request changed
  1. Ship It!
  2. 
      
Loading...