[GitHub] accumulo pull request #273: ACCUMULO-4665 Use UGI with real Kerberos credent...

classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #273: ACCUMULO-4665 Use UGI with real Kerberos credent...

ctubbsii
GitHub user joshelser opened a pull request:

    https://github.com/apache/accumulo/pull/273

    ACCUMULO-4665 Use UGI with real Kerberos credentials

    UGI supports the notion of users without credentials being
    "proxied" (riding on top of) another user which does have
    credentials. This is authorized via configuration. These
    changes allow this scenario more naturally and remove
    unnecessarily strict assertions in KerberosToken.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/joshelser/accumulo 4665-krb-real-user

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/accumulo/pull/273.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #273
   
----
commit fd666f6ee0087a6a8dec78673f319217464fd820
Author: Josh Elser <[hidden email]>
Date:   2017-06-26T19:00:29Z

    ACCUMULO-4665 Use UGI with real Kerberos credentials
   
    UGI supports the notion of users without credentials being
    "proxied" (riding on top of) another user which does have
    credentials. This is authorized via configuration. These
    changes allow this scenario more naturally and remove
    unnecessarily strict assertions in KerberosToken.

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #273: ACCUMULO-4665 Use UGI with real Kerberos credentials

ctubbsii
Github user joshelser commented on the issue:

    https://github.com/apache/accumulo/pull/273
 
    I'd appreciate a review of these changes (instead of just merging) since they are security related.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #273: ACCUMULO-4665 Use UGI with real Kerberos credent...

ctubbsii
In reply to this post by ctubbsii
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/273#discussion_r124108676
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java ---
    @@ -282,21 +283,39 @@ public static TTransport createClientTransport(HostAndPort address, int timeout,
             try {
               // Log in via UGI, ensures we have logged in with our KRB credentials
               final UserGroupInformation currentUser = UserGroupInformation.getCurrentUser();
    +          final UserGroupInformation userForRpc;
    +          if (AuthenticationMethod.PROXY == currentUser.getAuthenticationMethod()) {
    +            // A "proxy" user is when the real (Kerberos) credentials are for a user
    +            // other than the one we're acting as. When we make an RPC though, we need to make sure
    +            // that the current user is the user that has some credentials.
    +            if (currentUser.getRealUser() != null) {
    +              userForRpc = currentUser.getRealUser();
    +              log.trace("{} is a proxy user, using real user instead {}", currentUser, userForRpc);
    +            } else {
    +              // The current user has no credentials, let it fail naturally at the RPC layer (no ticket)
    +              // We know this won't work, but we can't do anything else
    +              log.warn("The current user is a proxy user but there is no underlying real user (RPCs will fail): {}", currentUser);
    --- End diff --
   
    Why not throw an exception here?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #273: ACCUMULO-4665 Use UGI with real Kerberos credent...

ctubbsii
In reply to this post by ctubbsii
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/273#discussion_r124110937
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java ---
    @@ -70,18 +71,12 @@ public KerberosToken(String principal) throws IOException {
        *          Should the current Hadoop user be replaced with this user
        */
       public KerberosToken(String principal, File keytab, boolean replaceCurrentUser) throws IOException {
    -    requireNonNull(principal, "Principal was null");
    -    requireNonNull(keytab, "Keytab was null");
    +    this.principal = requireNonNull(principal, "Principal was null");
    +    this.keytab = requireNonNull(keytab, "Keytab was null");
         checkArgument(keytab.exists() && keytab.isFile(), "Keytab was not a normal file");
    -    UserGroupInformation ugi;
         if (replaceCurrentUser) {
           UserGroupInformation.loginUserFromKeytab(principal, keytab.getAbsolutePath());
    -      ugi = UserGroupInformation.getCurrentUser();
    --- End diff --
   
    I don't understand UserGroupInformation.  Is it easy to explain why is this the wrong thing to do?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #273: ACCUMULO-4665 Use UGI with real Kerberos credentials

ctubbsii
In reply to this post by ctubbsii
Github user keith-turner commented on the issue:

    https://github.com/apache/accumulo/pull/273
 
    I notice this change was targeted to 1.7.  Is there any impact from this change on using 1.7.3 server with 1.7.4 client and visa versa?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #273: ACCUMULO-4665 Use UGI with real Kerberos credent...

ctubbsii
In reply to this post by ctubbsii
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/273#discussion_r124113747
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java ---
    @@ -282,21 +283,39 @@ public static TTransport createClientTransport(HostAndPort address, int timeout,
             try {
               // Log in via UGI, ensures we have logged in with our KRB credentials
               final UserGroupInformation currentUser = UserGroupInformation.getCurrentUser();
    +          final UserGroupInformation userForRpc;
    +          if (AuthenticationMethod.PROXY == currentUser.getAuthenticationMethod()) {
    +            // A "proxy" user is when the real (Kerberos) credentials are for a user
    +            // other than the one we're acting as. When we make an RPC though, we need to make sure
    +            // that the current user is the user that has some credentials.
    +            if (currentUser.getRealUser() != null) {
    +              userForRpc = currentUser.getRealUser();
    +              log.trace("{} is a proxy user, using real user instead {}", currentUser, userForRpc);
    +            } else {
    +              // The current user has no credentials, let it fail naturally at the RPC layer (no ticket)
    +              // We know this won't work, but we can't do anything else
    +              log.warn("The current user is a proxy user but there is no underlying real user (RPCs will fail): {}", currentUser);
    --- End diff --
   
    Mostly, because it is a "best guess". This code is essentially reverting strict assertions because I thought I knew how things were going to work in practice.
   
    I should update the comment to be a little less strongly worded.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #273: ACCUMULO-4665 Use UGI with real Kerberos credent...

ctubbsii
In reply to this post by ctubbsii
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/273#discussion_r124114509
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java ---
    @@ -70,18 +71,12 @@ public KerberosToken(String principal) throws IOException {
        *          Should the current Hadoop user be replaced with this user
        */
       public KerberosToken(String principal, File keytab, boolean replaceCurrentUser) throws IOException {
    -    requireNonNull(principal, "Principal was null");
    -    requireNonNull(keytab, "Keytab was null");
    +    this.principal = requireNonNull(principal, "Principal was null");
    +    this.keytab = requireNonNull(keytab, "Keytab was null");
         checkArgument(keytab.exists() && keytab.isFile(), "Keytab was not a normal file");
    -    UserGroupInformation ugi;
         if (replaceCurrentUser) {
           UserGroupInformation.loginUserFromKeytab(principal, keytab.getAbsolutePath());
    -      ugi = UserGroupInformation.getCurrentUser();
    --- End diff --
   
    Essentially, Accumulo's Kerberos support was written to support full principals as "usernames". Hadoop, however, has rules that define how principals are converted into "short names" and uses though.
   
    e.g. the Kerberos principal {{[hidden email]}} would be shortened to {{accumulo}} by the default Hadoop rules (in core-site.xml).
   
    The problem is that Accumulo would treat {{accumulo}} and {{[hidden email]}} differently. The check above that was removed was to remove this client-side check and let it happen server-side.
   
    The flaw was that when a user has their own Kerberos ticket and talking to Accumulo, this is likely programmer error. But in the case where I am a server making a request to Accumulo on behalf of a user, it prevents the server from writing "logical" code (the test case hopefully makes this clear).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #273: ACCUMULO-4665 Use UGI with real Kerberos credent...

ctubbsii
In reply to this post by ctubbsii
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/273#discussion_r124117442
 
    --- Diff: test/src/test/java/org/apache/accumulo/test/functional/KerberosProxyIT.java ---
    @@ -463,6 +477,56 @@ public void testMismatchPrincipals() throws Exception {
         }
       }
     
    +  @Test
    +  public void proxiedUserAccessWithoutAccumuloProxy() throws Exception {
    +    final String tableName = getUniqueNames(1)[0];
    +    ClusterUser rootUser = kdc.getRootUser();
    +    final UserGroupInformation rootUgi = UserGroupInformation.loginUserFromKeytabAndReturnUGI(rootUser.getPrincipal(), rootUser.getKeytab().getAbsolutePath());
    +    final UserGroupInformation realUgi = UserGroupInformation.loginUserFromKeytabAndReturnUGI(proxyPrincipal, proxyKeytab.getAbsolutePath());
    +    final String userWithoutCredentials = kdc.qualifyUser(PROXIED_USER);
    +    final UserGroupInformation proxyUser = UserGroupInformation.createProxyUser(userWithoutCredentials, realUgi);
    +
    +    // Create a table and user, grant permission to our user to read that table.
    +    rootUgi.doAs(new PrivilegedExceptionAction<Void>() {
    +      @Override
    +      public Void run() throws Exception {
    +        ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig());
    +        Connector conn = inst.getConnector(rootUgi.getUserName(), new KerberosToken());
    +        conn.tableOperations().create(tableName);
    +        conn.securityOperations().createLocalUser(userWithoutCredentials, new PasswordToken("ignored"));
    +        conn.securityOperations().grantTablePermission(userWithoutCredentials, tableName, TablePermission.READ);
    +        return null;
    +      }
    +    });
    +    realUgi.doAs(new PrivilegedExceptionAction<Void>() {
    +      @Override
    +      public Void run() throws Exception {
    +        ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig());
    +        Connector conn = inst.getConnector(proxyPrincipal, new KerberosToken());
    +        try {
    +          Scanner s = conn.createScanner(tableName, Authorizations.EMPTY);
    +          s.iterator().hasNext();
    +          Assert.fail("Expected to see an exception");
    +        } catch (RuntimeException e) {
    +          int numSecurityExceptionsSeen = Iterables.size(Iterables.filter(Throwables.getCausalChain(e),
    +              org.apache.accumulo.core.client.AccumuloSecurityException.class));
    +          assertTrue("Expected to see at least one AccumuloSecurityException, but saw: " + Throwables.getStackTraceAsString(e), numSecurityExceptionsSeen > 0);
    +        }
    +        return null;
    +      }
    +    });
    +    proxyUser.doAs(new PrivilegedExceptionAction<Void>() {
    +      @Override
    +      public Void run() throws Exception {
    +        ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig());
    +        Connector conn = inst.getConnector(userWithoutCredentials, new KerberosToken(userWithoutCredentials));
    +        Scanner s = conn.createScanner(tableName, Authorizations.EMPTY);
    +        assertFalse(s.iterator().hasNext());
    +        return null;
    --- End diff --
   
    seems like `return null` could be omitted


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #273: ACCUMULO-4665 Use UGI with real Kerberos credent...

ctubbsii
In reply to this post by ctubbsii
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/273#discussion_r124118094
 
    --- Diff: test/src/test/java/org/apache/accumulo/test/functional/KerberosProxyIT.java ---
    @@ -463,6 +477,56 @@ public void testMismatchPrincipals() throws Exception {
         }
       }
     
    +  @Test
    +  public void proxiedUserAccessWithoutAccumuloProxy() throws Exception {
    +    final String tableName = getUniqueNames(1)[0];
    +    ClusterUser rootUser = kdc.getRootUser();
    +    final UserGroupInformation rootUgi = UserGroupInformation.loginUserFromKeytabAndReturnUGI(rootUser.getPrincipal(), rootUser.getKeytab().getAbsolutePath());
    +    final UserGroupInformation realUgi = UserGroupInformation.loginUserFromKeytabAndReturnUGI(proxyPrincipal, proxyKeytab.getAbsolutePath());
    +    final String userWithoutCredentials = kdc.qualifyUser(PROXIED_USER);
    +    final UserGroupInformation proxyUser = UserGroupInformation.createProxyUser(userWithoutCredentials, realUgi);
    +
    +    // Create a table and user, grant permission to our user to read that table.
    +    rootUgi.doAs(new PrivilegedExceptionAction<Void>() {
    +      @Override
    +      public Void run() throws Exception {
    +        ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig());
    +        Connector conn = inst.getConnector(rootUgi.getUserName(), new KerberosToken());
    +        conn.tableOperations().create(tableName);
    +        conn.securityOperations().createLocalUser(userWithoutCredentials, new PasswordToken("ignored"));
    +        conn.securityOperations().grantTablePermission(userWithoutCredentials, tableName, TablePermission.READ);
    +        return null;
    +      }
    +    });
    +    realUgi.doAs(new PrivilegedExceptionAction<Void>() {
    +      @Override
    +      public Void run() throws Exception {
    +        ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig());
    +        Connector conn = inst.getConnector(proxyPrincipal, new KerberosToken());
    +        try {
    +          Scanner s = conn.createScanner(tableName, Authorizations.EMPTY);
    +          s.iterator().hasNext();
    +          Assert.fail("Expected to see an exception");
    +        } catch (RuntimeException e) {
    +          int numSecurityExceptionsSeen = Iterables.size(Iterables.filter(Throwables.getCausalChain(e),
    +              org.apache.accumulo.core.client.AccumuloSecurityException.class));
    +          assertTrue("Expected to see at least one AccumuloSecurityException, but saw: " + Throwables.getStackTraceAsString(e), numSecurityExceptionsSeen > 0);
    +        }
    +        return null;
    +      }
    +    });
    +    proxyUser.doAs(new PrivilegedExceptionAction<Void>() {
    +      @Override
    +      public Void run() throws Exception {
    +        ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig());
    +        Connector conn = inst.getConnector(userWithoutCredentials, new KerberosToken(userWithoutCredentials));
    +        Scanner s = conn.createScanner(tableName, Authorizations.EMPTY);
    +        assertFalse(s.iterator().hasNext());
    +        return null;
    +      }
    +    });
    +  }
    --- End diff --
   
    Would it make sense to have a 3rd test with a `proxied_user2` who does not have read access to the table?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #273: ACCUMULO-4665 Use UGI with real Kerberos credent...

ctubbsii
In reply to this post by ctubbsii
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/273#discussion_r124118903
 
    --- Diff: test/src/test/java/org/apache/accumulo/test/functional/KerberosProxyIT.java ---
    @@ -463,6 +477,56 @@ public void testMismatchPrincipals() throws Exception {
         }
       }
     
    +  @Test
    +  public void proxiedUserAccessWithoutAccumuloProxy() throws Exception {
    +    final String tableName = getUniqueNames(1)[0];
    +    ClusterUser rootUser = kdc.getRootUser();
    +    final UserGroupInformation rootUgi = UserGroupInformation.loginUserFromKeytabAndReturnUGI(rootUser.getPrincipal(), rootUser.getKeytab().getAbsolutePath());
    +    final UserGroupInformation realUgi = UserGroupInformation.loginUserFromKeytabAndReturnUGI(proxyPrincipal, proxyKeytab.getAbsolutePath());
    +    final String userWithoutCredentials = kdc.qualifyUser(PROXIED_USER);
    +    final UserGroupInformation proxyUser = UserGroupInformation.createProxyUser(userWithoutCredentials, realUgi);
    +
    +    // Create a table and user, grant permission to our user to read that table.
    +    rootUgi.doAs(new PrivilegedExceptionAction<Void>() {
    +      @Override
    +      public Void run() throws Exception {
    +        ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig());
    +        Connector conn = inst.getConnector(rootUgi.getUserName(), new KerberosToken());
    +        conn.tableOperations().create(tableName);
    +        conn.securityOperations().createLocalUser(userWithoutCredentials, new PasswordToken("ignored"));
    +        conn.securityOperations().grantTablePermission(userWithoutCredentials, tableName, TablePermission.READ);
    +        return null;
    +      }
    +    });
    +    realUgi.doAs(new PrivilegedExceptionAction<Void>() {
    +      @Override
    +      public Void run() throws Exception {
    +        ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig());
    +        Connector conn = inst.getConnector(proxyPrincipal, new KerberosToken());
    +        try {
    +          Scanner s = conn.createScanner(tableName, Authorizations.EMPTY);
    +          s.iterator().hasNext();
    +          Assert.fail("Expected to see an exception");
    +        } catch (RuntimeException e) {
    +          int numSecurityExceptionsSeen = Iterables.size(Iterables.filter(Throwables.getCausalChain(e),
    +              org.apache.accumulo.core.client.AccumuloSecurityException.class));
    +          assertTrue("Expected to see at least one AccumuloSecurityException, but saw: " + Throwables.getStackTraceAsString(e), numSecurityExceptionsSeen > 0);
    +        }
    +        return null;
    +      }
    +    });
    +    proxyUser.doAs(new PrivilegedExceptionAction<Void>() {
    +      @Override
    +      public Void run() throws Exception {
    +        ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig());
    +        Connector conn = inst.getConnector(userWithoutCredentials, new KerberosToken(userWithoutCredentials));
    +        Scanner s = conn.createScanner(tableName, Authorizations.EMPTY);
    +        assertFalse(s.iterator().hasNext());
    +        return null;
    --- End diff --
   
    Sadly, it cannot. It would be a compilation error. We cannot do `PrivilegedExceptionAction<void>`, only `Void`. As such, we need to have an explicit return value (it's annoying :D)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #273: ACCUMULO-4665 Use UGI with real Kerberos credent...

ctubbsii
In reply to this post by ctubbsii
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/273#discussion_r124119086
 
    --- Diff: test/src/test/java/org/apache/accumulo/test/functional/KerberosProxyIT.java ---
    @@ -463,6 +477,56 @@ public void testMismatchPrincipals() throws Exception {
         }
       }
     
    +  @Test
    +  public void proxiedUserAccessWithoutAccumuloProxy() throws Exception {
    +    final String tableName = getUniqueNames(1)[0];
    +    ClusterUser rootUser = kdc.getRootUser();
    +    final UserGroupInformation rootUgi = UserGroupInformation.loginUserFromKeytabAndReturnUGI(rootUser.getPrincipal(), rootUser.getKeytab().getAbsolutePath());
    +    final UserGroupInformation realUgi = UserGroupInformation.loginUserFromKeytabAndReturnUGI(proxyPrincipal, proxyKeytab.getAbsolutePath());
    +    final String userWithoutCredentials = kdc.qualifyUser(PROXIED_USER);
    +    final UserGroupInformation proxyUser = UserGroupInformation.createProxyUser(userWithoutCredentials, realUgi);
    +
    +    // Create a table and user, grant permission to our user to read that table.
    +    rootUgi.doAs(new PrivilegedExceptionAction<Void>() {
    +      @Override
    +      public Void run() throws Exception {
    +        ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig());
    +        Connector conn = inst.getConnector(rootUgi.getUserName(), new KerberosToken());
    +        conn.tableOperations().create(tableName);
    +        conn.securityOperations().createLocalUser(userWithoutCredentials, new PasswordToken("ignored"));
    +        conn.securityOperations().grantTablePermission(userWithoutCredentials, tableName, TablePermission.READ);
    +        return null;
    +      }
    +    });
    +    realUgi.doAs(new PrivilegedExceptionAction<Void>() {
    +      @Override
    +      public Void run() throws Exception {
    +        ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig());
    +        Connector conn = inst.getConnector(proxyPrincipal, new KerberosToken());
    +        try {
    +          Scanner s = conn.createScanner(tableName, Authorizations.EMPTY);
    +          s.iterator().hasNext();
    +          Assert.fail("Expected to see an exception");
    +        } catch (RuntimeException e) {
    +          int numSecurityExceptionsSeen = Iterables.size(Iterables.filter(Throwables.getCausalChain(e),
    +              org.apache.accumulo.core.client.AccumuloSecurityException.class));
    +          assertTrue("Expected to see at least one AccumuloSecurityException, but saw: " + Throwables.getStackTraceAsString(e), numSecurityExceptionsSeen > 0);
    +        }
    +        return null;
    +      }
    +    });
    +    proxyUser.doAs(new PrivilegedExceptionAction<Void>() {
    +      @Override
    +      public Void run() throws Exception {
    +        ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig());
    +        Connector conn = inst.getConnector(userWithoutCredentials, new KerberosToken(userWithoutCredentials));
    +        Scanner s = conn.createScanner(tableName, Authorizations.EMPTY);
    +        assertFalse(s.iterator().hasNext());
    +        return null;
    +      }
    +    });
    +  }
    --- End diff --
   
    Certainly wouldn't hurt! Could also do a test for a user that has read permission but that is not allowed to be "proxied" on the "real" user.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #273: ACCUMULO-4665 Use UGI with real Kerberos credentials

ctubbsii
In reply to this post by ctubbsii
Github user joshelser commented on the issue:

    https://github.com/apache/accumulo/pull/273
 
    > I notice this change was targeted to 1.7. Is there any impact from this change on using 1.7.3 server with 1.7.4 client and visa versa?
   
    There is obviously the change in implementation that this introduces. However, I believe it is purely in the "bugfix" realm. If you ran my test without the KerberosToken patch, you'd get an error in KerberosToken about mis-matched principals -- if you squashed that, you would get a `GSS initiate failed` error when you try to make the Accumulo RPC. All of this comes from our poor handling of this scenario.
   
    To the best of my knowledge, there wouldn't be any negative aspect to this change WRT cross-version support aside from the implicit change in how the clients operate. As long as the RPC was set up in such a way that it _could_ theoretically work (at the SASL/Kerberos level) it should continue to work.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #273: ACCUMULO-4665 Use UGI with real Kerberos credent...

ctubbsii
In reply to this post by ctubbsii
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/273#discussion_r124122964
 
    --- Diff: test/src/test/java/org/apache/accumulo/test/functional/KerberosProxyIT.java ---
    @@ -463,6 +477,56 @@ public void testMismatchPrincipals() throws Exception {
         }
       }
     
    +  @Test
    +  public void proxiedUserAccessWithoutAccumuloProxy() throws Exception {
    +    final String tableName = getUniqueNames(1)[0];
    +    ClusterUser rootUser = kdc.getRootUser();
    +    final UserGroupInformation rootUgi = UserGroupInformation.loginUserFromKeytabAndReturnUGI(rootUser.getPrincipal(), rootUser.getKeytab().getAbsolutePath());
    +    final UserGroupInformation realUgi = UserGroupInformation.loginUserFromKeytabAndReturnUGI(proxyPrincipal, proxyKeytab.getAbsolutePath());
    +    final String userWithoutCredentials = kdc.qualifyUser(PROXIED_USER);
    +    final UserGroupInformation proxyUser = UserGroupInformation.createProxyUser(userWithoutCredentials, realUgi);
    +
    +    // Create a table and user, grant permission to our user to read that table.
    +    rootUgi.doAs(new PrivilegedExceptionAction<Void>() {
    +      @Override
    +      public Void run() throws Exception {
    +        ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig());
    +        Connector conn = inst.getConnector(rootUgi.getUserName(), new KerberosToken());
    +        conn.tableOperations().create(tableName);
    +        conn.securityOperations().createLocalUser(userWithoutCredentials, new PasswordToken("ignored"));
    +        conn.securityOperations().grantTablePermission(userWithoutCredentials, tableName, TablePermission.READ);
    +        return null;
    +      }
    +    });
    +    realUgi.doAs(new PrivilegedExceptionAction<Void>() {
    +      @Override
    +      public Void run() throws Exception {
    +        ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig());
    +        Connector conn = inst.getConnector(proxyPrincipal, new KerberosToken());
    +        try {
    +          Scanner s = conn.createScanner(tableName, Authorizations.EMPTY);
    +          s.iterator().hasNext();
    +          Assert.fail("Expected to see an exception");
    +        } catch (RuntimeException e) {
    +          int numSecurityExceptionsSeen = Iterables.size(Iterables.filter(Throwables.getCausalChain(e),
    +              org.apache.accumulo.core.client.AccumuloSecurityException.class));
    +          assertTrue("Expected to see at least one AccumuloSecurityException, but saw: " + Throwables.getStackTraceAsString(e), numSecurityExceptionsSeen > 0);
    +        }
    +        return null;
    +      }
    +    });
    +    proxyUser.doAs(new PrivilegedExceptionAction<Void>() {
    +      @Override
    +      public Void run() throws Exception {
    +        ZooKeeperInstance inst = new ZooKeeperInstance(mac.getClientConfig());
    +        Connector conn = inst.getConnector(userWithoutCredentials, new KerberosToken(userWithoutCredentials));
    +        Scanner s = conn.createScanner(tableName, Authorizations.EMPTY);
    +        assertFalse(s.iterator().hasNext());
    +        return null;
    --- End diff --
   
    oh right.  I assumed this was a Runnable w/o looking closely at the types.  But returning null in that case would probably be a compile error.  Sorry for the noise.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #273: ACCUMULO-4665 Use UGI with real Kerberos credentials

ctubbsii
In reply to this post by ctubbsii
Github user joshelser commented on the issue:

    https://github.com/apache/accumulo/pull/273
 
    Will wait on travis as well as my local `mvn clean verify -Dit.test='KerberosIT,KerberosProxyIT'` to come back.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #273: ACCUMULO-4665 Use UGI with real Kerberos credentials

ctubbsii
In reply to this post by ctubbsii
Github user keith-turner commented on the issue:

    https://github.com/apache/accumulo/pull/273
 
    The two new test are nice.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #273: ACCUMULO-4665 Use UGI with real Kerberos credent...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/273#discussion_r124141590
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java ---
    @@ -51,11 +52,11 @@
        *          The user that is logged in
        */
       public KerberosToken(String principal) throws IOException {
    -    requireNonNull(principal);
    +    this.principal = requireNonNull(principal);
         final UserGroupInformation ugi = UserGroupInformation.getCurrentUser();
    -    checkArgument(ugi.hasKerberosCredentials(), "Subject is not logged in via Kerberos");
    -    checkArgument(principal.equals(ugi.getUserName()), "Provided principal does not match currently logged-in user");
    -    this.principal = ugi.getUserName();
    +    if (AuthenticationMethod.KERBEROS == ugi.getAuthenticationMethod()) {
    +      checkArgument(ugi.hasKerberosCredentials(), "Subject is not logged in via Kerberos");
    +    }
    --- End diff --
   
    I think this check is backwards. We should exclude the check when it doesn't apply (`AuthenticationMethod.PROXY`?), rather than include the check when it does. Right now, this allows even `AuthenticationMethod.SIMPLE` to also construct a valid `KerberosToken`, and that's clearly incorrect.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #273: ACCUMULO-4665 Use UGI with real Kerberos credent...

ctubbsii
In reply to this post by ctubbsii
Github user asfgit closed the pull request at:

    https://github.com/apache/accumulo/pull/273


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #273: ACCUMULO-4665 Use UGI with real Kerberos credent...

ctubbsii
In reply to this post by ctubbsii
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/273#discussion_r124146716
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java ---
    @@ -51,11 +52,11 @@
        *          The user that is logged in
        */
       public KerberosToken(String principal) throws IOException {
    -    requireNonNull(principal);
    +    this.principal = requireNonNull(principal);
         final UserGroupInformation ugi = UserGroupInformation.getCurrentUser();
    -    checkArgument(ugi.hasKerberosCredentials(), "Subject is not logged in via Kerberos");
    -    checkArgument(principal.equals(ugi.getUserName()), "Provided principal does not match currently logged-in user");
    -    this.principal = ugi.getUserName();
    +    if (AuthenticationMethod.KERBEROS == ugi.getAuthenticationMethod()) {
    +      checkArgument(ugi.hasKerberosCredentials(), "Subject is not logged in via Kerberos");
    +    }
    --- End diff --
   
    Drat, I pushed without re-checking my email.
   
    I mulled around whether or not we should even have this check (I haven't traced the hadoop code to understand whether or not this is even possible as-written). I probably would have just suggested to remove it entirely.
   
    Having it check for PROXY or KERBEROS might be a good middle-ground (failing on SIMPLE, TOKEN, etc). I can spin out a follow-on issue since I already pushed this and it was only tweaking the original, already sub-par logic as you said.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #273: ACCUMULO-4665 Use UGI with real Kerberos credent...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/273#discussion_r124146860
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java ---
    @@ -51,11 +52,11 @@
        *          The user that is logged in
        */
       public KerberosToken(String principal) throws IOException {
    -    requireNonNull(principal);
    +    this.principal = requireNonNull(principal);
         final UserGroupInformation ugi = UserGroupInformation.getCurrentUser();
    -    checkArgument(ugi.hasKerberosCredentials(), "Subject is not logged in via Kerberos");
    -    checkArgument(principal.equals(ugi.getUserName()), "Provided principal does not match currently logged-in user");
    -    this.principal = ugi.getUserName();
    +    if (AuthenticationMethod.KERBEROS == ugi.getAuthenticationMethod()) {
    +      checkArgument(ugi.hasKerberosCredentials(), "Subject is not logged in via Kerberos");
    +    }
    --- End diff --
   
    No problem. A follow-on issue to limit to PROXY or KERBEROS would suffice. :)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #273: ACCUMULO-4665 Use UGI with real Kerberos credent...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/273#discussion_r124147278
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java ---
    @@ -51,11 +52,11 @@
        *          The user that is logged in
        */
       public KerberosToken(String principal) throws IOException {
    -    requireNonNull(principal);
    +    this.principal = requireNonNull(principal);
         final UserGroupInformation ugi = UserGroupInformation.getCurrentUser();
    -    checkArgument(ugi.hasKerberosCredentials(), "Subject is not logged in via Kerberos");
    -    checkArgument(principal.equals(ugi.getUserName()), "Provided principal does not match currently logged-in user");
    -    this.principal = ugi.getUserName();
    +    if (AuthenticationMethod.KERBEROS == ugi.getAuthenticationMethod()) {
    +      checkArgument(ugi.hasKerberosCredentials(), "Subject is not logged in via Kerberos");
    +    }
    --- End diff --
   
    FWIW, the current code would always fail without Kerberos credentials, so it was already somewhat comprehensive, covering the other cases (SIMPLE, TOKEN, etc.), so I wouldn't go so far as to say your previous logic was sub-par (the issue in this PR aside). But, after this change, the extra check would be nice to restore the previous restrictions to the Kerberos and now Proxy case.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
12
Loading...