accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Christopher Tubbs" <ctubbsii...@apache.org>
Subject Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos
Date Tue, 06 Jan 2015 18:54:36 GMT


> On Dec. 30, 2014, 5:46 p.m., Christopher Tubbs wrote:
> > core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java,
lines 71-79
> > <https://reviews.apache.org/r/29386/diff/4/?file=803145#file803145line71>
> >
> >     Why is this unsupported?
> 
> Josh Elser wrote:
>     Again, see earlier discussion. I have no idea what this is actually supposed to destroy
(we have zero documentation on the matter that I found) and it's already been changed to be
a noop. I am just as confident that a noop is correct as an unsupported operation is correct
(I have no confidence).
>     
>     As far as destroying a UGI, I haven't found any parallel to the Destroyable interface.
I'm not sure if something exists that we should be calling.
> 
> Christopher Tubbs wrote:
>     It's a way for objects to remove their internal state (preferably by securely overwriting
memory) so any credentials can be removed from memory, to give the user control over their
credentials. It is expected to be called by the user.
>     
>     In the earlier implementation, setting the ugi to null would have been appropriate.
In the latest version, you can nullify the principal. It's not terribly important, since the
principal alone is not sensitive, but it would be helpful to have reasonable behavior when
the user calls these methods to avoid things like "I called destroy, but it won't destroy!"
or "My application verifies the token isn't destroyed before using it, but it seems to always
be destroyed!".
>     
>     Even if it was a truly no-op, we could add an internal "destroyed" flag to give users
reasonable behavior. Since we do have the principal, we can just make that null instead of
adding a new flag, and use that to check the state of its destruction.
>     
>     Also, I think the other methods which assume it is not destroyed should throw IllegalStateException
explicitly if it is, in fact, destroyed. But, I'm not even sure we do that in the other tokens,
so it's probably fine to leave that behavior undefined.
> 
> Josh Elser wrote:
>     WRT UserGroupInformation, I don't know how (or if I even can) destroy the current
login which is.. awkward to me. Generally speaking, this would be great to get written down
somewhere -- I'll try to fire up a ticket for that so we don't have this discussion again
in 6mos.

The semantics of destroy are not to destroy the current login. They are to destroy the current
token, removing any user credentials from it. These semantics can be accomplished with a flag,
or by setting the principal to null, without touching the Kerberos state that they represent.


> On Dec. 30, 2014, 5:46 p.m., Christopher Tubbs wrote:
> > core/src/main/java/org/apache/accumulo/core/conf/Property.java, lines 165-167
> > <https://reviews.apache.org/r/29386/diff/4/?file=803146#file803146line165>
> >
> >     I don't think we want to expose this. It's high risk. What is the reason and
how is it related to this change?
> 
> Josh Elser wrote:
>     Again, see the previous discussion on this subject. This is necessary to actually
perform an apples to apples comparison of the impact of Kerberos on Accumulo. Comparing KRB/SASL
with TThreadPoolServer against no KRB/SASL and THsHaServer is an invalid benchmark to determine
the overhead of KRB.
>     
>     Like I said earlier, `Experimental` is not really the appropriate annotation here
(Internal use would be more accurate), but it's what we have. Do you have a recommendation
on some other change in wording/annotation to convey the intent?
>     
>     I'd like to not have to change the codebase any time I want to verify no performance
regressions in future versions (having to change code would preclude any non-devs from running
their own benchmarks too).
> 
> Christopher Tubbs wrote:
>     My main concern is that I'm not entirely confident we're not exposing these to users
in some way, still: see ACCUMULO-2460 and ACCUMULO-2401.
> 
> Josh Elser wrote:
>     It sounds to me like we should introduce a new annotation then to cover things not
meant for user consumption. Experimental is for "maybe not stable or entirely working" user
facing things. This property is (likely) only relevant for developers. Any objections to addressing
this by introduction of a new annotation (not in this changeset)?

That's a simple enough solution. I still kind of object to the idea that "experimental" properties
have default values at all (it's experimental, so it should be okay to set to empty value,
and should be null/unused by default), which was the underlying problem between ACCUMULO-2460
and ACCUMULO-2401 (the default value was added and used as a proxy for what probably should
have been a constant). If that underlying problem was addressed, then using Experimental here
would be perfectly fine. However, it may still be useful to have separate, clear semantics,
with an additional annotation.


> On Dec. 30, 2014, 5:46 p.m., Christopher Tubbs wrote:
> > server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java, lines
285-293
> > <https://reviews.apache.org/r/29386/diff/4/?file=803157#file803157line285>
> >
> >     It was discussed in the context of ACCUMULO-259 that the root user would be
a built-in, password-based user, even if other auth mechanisms were available (like Linux
root user). Some proposals were made to make this easier to work with and more intuitive for
users, but the initialization of the root user should really just be the 
> >     
> >     See ACCUMULO-1300 and related issues, for more details.
> >     
> >     What are the risks of making the root user rely on Kerberos?
> 
> Josh Elser wrote:
>     Thanks for the background; I didn't remember this discussion.
>     
>     Hadoop does have a similar approach that we could adopt but I'm extremely hesitant
to recommend we do it (because I think it's some of the ugliest configuration in Hadoop).
Look up hadoop.security.auth_to_local: (first result from google for me http://hortonworks.com/blog/fine-tune-your-apache-hadoop-security-settings/).
Essentially, we could introduce this very convoluted user mapping into the "root" user. Personally,
I think this is much more confusing.
>     
>     I regret not being a part of the previous discussion, but I don't understand why
we should rely on always having a password based user. It seems like forcing us into edge-case-city.
Do you have more information on this discussion? I didn't find anything on ACCUMULO-259 on
it and I want to make sure I'm up to speed. Comparing our "root" user is the Linux root user
is a bit of a fallacy in my opinion because they're not equivalent. In a properly configured
system, root should never be used by anyone but an administrator to delegate permissions or
perform one-time tasks (to avoid permission delegation).
>     
>     In my opinion, avoid some "magical" user helps with the auditing and authentication
Accumulo uses as a whole. Each client that comes into Accumulo is a user. They have a Kerberos
identity and we treat them as the "Accumulo user" based on that Kerberos identity. The internal
"!SYSTEM" user is the only other user; however it is still identified by a specific Kerberos
identity (the ones we know the servers use to log in).
>     
>     Also, this may benefit from non-reviewboard discussion, please feel free to pop this
over to the ML.
> 
> Christopher Tubbs wrote:
>     As far as I know, all the conversation happened in the comments on ACCUMULO-259 and
it's linked issues, as well as on the mailing list, but I don't have direct references, other
than the issues I linked.
>     
>     The root user as compared to the Linux root user is analogous, not equivalent. I
agree that a root user should never be used except to delegate and perform one time tasks.
The problem is that, like the Linux root user, it is sometimes needed to do these tasks when
other components of the system (such as the pluggable authentication service) are unavailable.
The same permissions could be delegated to an admin in that service, so the built-in root
would be even less frequently used, but it still might be needed. This is the same reasoning
as setting a BIOS password, or Linux root user, or MySQL root user, or Hadoop SuperUser, or
ZK's "super" user.
>     
>     The point may be moot, though, if the system is completely unusable if Kerberos is
down (because the system token relies on Kerberos). Though... I can see a use case for restarting
the system with Kerberos disabled to do maintenance and to have root available for that.
>     
>     As for the system user being a Kerberos identity, see my other concerns regarding
that.
> 
> Josh Elser wrote:
>     bq. This is the same reasoning as setting a BIOS password, or Linux root user, or
MySQL root user, or Hadoop SuperUser, or ZK's "super" user.
>     
>     I kind of see what's done here as analogous to what Hadoop already provides -- a
single, well-defined user with `System.SYSTEM`.
>     
>     bq. The point may be moot, though, if the system is completely unusable if Kerberos
is down (because the system token relies on Kerberos). 
>     
>     Yes, that's accurate.
>     
>     bq. Though... I can see a use case for restarting the system with Kerberos disabled
to do maintenance and to have root available for that.
>     
>     I think this would be wrong. If the KDC is down, you're already going to be unable
to connect to HDFS or ZooKeeper. Allowing a "backdoor" into the instance without the (strong)
Kerberos identity seems like a security risk to me. 
>     
>     I still have to think about the "re-initialization" problem (both malicious and benign
as "I lost my credentials"). I'm also a bit worried about trying to tackle the "change authentication
handlers" problem in the first patch as this is already gettin really big (despite my best
efforts).

bq. Allowing a "backdoor" into the instance without the (strong) Kerberos identity seems like
a security risk to me.

It's a trade-off between maintenance and security. And, I question the idea that a "strong"
root password is any less secure than a keytab. It's also an acceptable risk, in my opinion,
just as it is in the analogous systems.

I agree that tackling the authentication handler problem in a first pass here risks derailing
this. It's unfortunate that ACCUMULO-1300 was not completed sooner.


> On Dec. 30, 2014, 5:46 p.m., Christopher Tubbs wrote:
> > server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthenticator.java,
line 41
> > <https://reviews.apache.org/r/29386/diff/4/?file=803164#file803164line41>
> >
> >     I don't think this should be so tightly coupled with the ZKAuthenticator. It
doesn't seem that this coupling is necessary to provide authentication functionality.
> >     
> >     Have a different implementation of an Authenticator that hijacks the data storage
of another implementation, could be quite confusing to administrators, especially if they
need to swap out the implementation.
> >     
> >     See ACCUMULO-1300 and related issues for other options.
> 
> Josh Elser wrote:
>     I understand the intentions for ACCUMULO-1300; however, that's not implemented. Being
able to leverage the existing ZKAuthorizor was wonderful so I really don't want to move away
from how this fundamentally works.
>     
>     Shouldn't the Authenticator and Authorizor be fixed per instance (allowing reset
via `accumulo init --reset-security` or w/e)? What do you actually want changed aside from
documentation to make it clear to administrators (I haven't written any documentation yet).
> 
> Christopher Tubbs wrote:
>     So, I pointed out in the conversation around ACCUMULO-259, that I would prefer a
pluggable security module that integrated all the security-related functions, because they
are so dependent on each other. However, they were implemented as 3 separately configurable
pluggable components. I would still like to see them merged (which doesn't preclude making
one from modular components themselves).
>     
>     For now, I'm not really sure what it means for somebody to "create a user" when the
authenticator already validates them. That's why it's confusing. Are we essentially making
ZKAuthenticator Kerberos-aware, or are we providing a separate Kerberos Authenticator? That's
really what it boils down to for me. It seems like we're doing a hybrid... but I think we
should solidify on one or the other.
> 
> Josh Elser wrote:
>     I view it as a Kerberos Authenticator. The implementation details of that authenticator
is that is happens to persist information into ZK to handle things like permissions and authorizations,
but a user doesn't have to know that to use it. We could, hypothetically, swap out ZK for
any other persistent store for that information and it would continue to work.
>     
>     What would help alleviate your confusion? More javadocs? A different class name?

Well, part is documentation. Part is behavior.

If it's a separate authenticator, we should make sure that it does not use the same storage
area in such a tightly coupled way. It should not, for instance, be setting things that look
like passwords, and "create local user", "list local users", and "delete local users" should
be unsupported client side operations, just as you've done with "change password" (since users
are managed within Kerberos). We already share storage between the ZKAuthenticator and the
corresponding permissions handler and authorizer, so it's probably okay to share the directory
identified with the principal. However, it can seamlessly create this directory as needed.

Mainly, I'm thinking the more we can separate the "local user" management functions from the
KerberosAuthenticator, the better, since users are managed in Kerberos. Some of my concerns
are with the current state of things, but I'm hoping not to exacerbate the problems I see
with the current implementations. I don't want to make it harder to make improvements later.
I'd rather move towards a better design, then simply add features to the existing ad-hoc design.


> On Dec. 30, 2014, 5:46 p.m., Christopher Tubbs wrote:
> > shell/src/main/java/org/apache/accumulo/shell/ShellOptionsJC.java, line 210
> > <https://reviews.apache.org/r/29386/diff/4/?file=803175#file803175line210>
> >
> >     Why is username the "short" user name? Is that unique in Kerberos? If not, the
long version should be used everywhere instead. Otherwise, one user can appear to be another
in logs, etc.
> >     
> >     If "getShortUserName" is not unique, it should avoided everywhere.
> 
> Josh Elser wrote:
>     Check out: http://web.mit.edu/kerberos/krb5-1.5/krb5-1.5.4/doc/krb5-user/What-is-a-Kerberos-Principal_003f.html
>     
>     Kerberos principals are of the form: primary/instance@realm. Kerberos principals
are typically categorized as users and services. A user is not qualified to a single instance
(a host) and represent authentication across the realm. For example, elserj@EXAMPLE.COM means
that I can "roam". Conversely, a service is typically "fixed" to a specific host. For example,
accumulo/node1.example.com@EXAMPLE.COM means that there is a process, logged in as 'accumulo'
on the host 'node1.example.com'. That service can't be run on any other host. Now, an important
note if someone actually creates a principal "accumulo@EXAMPLE.COM" this is unique with respect
to any other "accumulo/`host`@EXAMPLE.COM" principal. I'm not sure if we need to do anything
else other than convention of kerberos principals, or if we should be including the instance
in "our" username when present.
>     
>     This kind of ties back into the SystemCredentials discussion again.
> 
> Christopher Tubbs wrote:
>     Okay, so a smart configuration would make shortnames unique. However, UserGroupInformation
returns only the `primary` for the short name. This means that user names will have to be
unique across realms and instances. Right now, you are storing permissions using the short
name. So, any user with the same primary, will be able to masquerade as any other user with
the same primary from a different instance and/or realm, and be able to user their permissions
and authorizations. That's the problem with the shortname here. That's very unexpected.
> 
> Josh Elser wrote:
>     Bingo. If you look at how HDFS does their configuration, this is the same convention.
The lack of documentation from me leaves something to be desired here, and I apologize for
that.
>     
>     To save you looking at HDFS (if you care not to look), you'll see that an HDFS process
uses a given principal with a special replacement string `_HOST`. The common convention is
to use something like `dn/_HOST@EXAMPLE.COM` (the realm is unimportant for this example).
This ensures that the same configuration files can be used across all hosts in the HDFS instance,
and Hadoop dynamically replaces `_HOST` with the FQDN of the host. Thus, there's an implicit
link that all `dn/*@EXAMPLE.COM` can act as datanodes and this is protected by the fact that
access to the KDC is restricted (you can't make your own user). The circle of trust is two-fold:
having a keytab with the correct principal and that Hadoop is requires that specific configuration
(which restricts the principal).

My concerns here are more about the impact on users, than for the system credentials. I don't
know what HDFS is doing, but if they aren't (minimally) checking the realm when checking permissions/access
on an authenticated principal, then they are less secure than I think we should be. Referencing
HDFS also seems to imply that we're not so much doing Kerberos, as we are implementing HDFS-specific
Kerberos conventions (which are less secure, with respect to data authorizations/permissions
within Accumulo, than I'm comfortable with).


- Christopher


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29386/#review66382
-----------------------------------------------------------


On Dec. 31, 2014, 4:24 p.m., Josh Elser wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29386/
> -----------------------------------------------------------
> 
> (Updated Dec. 31, 2014, 4:24 p.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-2815
>     https://issues.apache.org/jira/browse/ACCUMULO-2815
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-2815 Initial support for Kerberos client authentication.
> 
> Leverage SASL transport provided by Thrift which can speak GSSAPI, which Kerberos implements.
Introduced...
> 
> * An Accumulo KerberosToken which is an AuthenticationToken to validate users.
> * Custom thrift processor and invocation handler to ensure server RPCs have a valid KRB
identity and Accumulo authentication.
> * A KerberosAuthenticator which extends ZKAuthenticator to support Kerberos identities
seamlessly.
> * New ClientConf variables to use SASL transport and pass Kerberos server principal
> * Updated ClientOpts and Shell opts to transparently use a KerberosToken when SASL is
enabled (no extra client work).
> 
> I believe this is the "bare minimum" for Kerberos support. They are also grossly lacking
in unit and integration tests. I believe that I might have somehow broken the client address
string in the server (I saw log messages with client: null, but I'm not sure if it's due to
these changes or not). A necessary limitation in the Thrift server used is that, like the
SSL transport, the SASL transport cannot presently be used with the TFramedTransport, which
means none of the [half]async thrift servers will function with this -- we're stuck with the
TThreadPoolServer.
> 
> Performed some contrived benchmarks on my laptop (while still using it myself) to get
at big-picture view of the performance impact against "normal" operation and Kerberos alone.
Each "run" was the duration to ingest 100M records using continuous-ingest, timed with `time`,
using 'real'.
> 
> THsHaServer (our default), 6 runs:
> 
> Avg: 10m7.273s (607.273s)
> Min: 9m43.395s
> Max: 10m52.715s
> 
> TThreadPoolServer (no SASL), 5 runs:
> 
> Avg: 11m16.254s (676.254s)
> Min: 10m30.987s
> Max: 12m24.192s
> 
> TThreadPoolServer+SASL/GSSAPI (these changes), 6 runs:
> 
> Avg: 13m17.187s (797.187s)
> Min: 10m52.997s
> Max: 16m0.975s
> 
> The general takeway is that there's about 15% performance degredation in its initial
state which is in the realm of what I expected (~10%).
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java f6ea934 
>   core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java 6fe61a5

>   core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java e75bec6

>   core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java f481cc3

>   core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportKey.java 6dc846f

>   core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportPool.java 5da803b

>   core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/conf/Property.java e054a5f 
>   core/src/main/java/org/apache/accumulo/core/rpc/FilterTransport.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/rpc/SaslConnectionParams.java PRE-CREATION

>   core/src/main/java/org/apache/accumulo/core/rpc/TTimeoutTransport.java 6eace77 
>   core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java 09bd6c4 
>   core/src/main/java/org/apache/accumulo/core/rpc/UGIAssumingTransport.java PRE-CREATION

>   core/src/main/java/org/apache/accumulo/core/rpc/UGIAssumingTransportFactory.java PRE-CREATION

>   core/src/main/java/org/apache/accumulo/core/security/Credentials.java 525a958 
>   core/src/test/java/org/apache/accumulo/core/cli/TestClientOpts.java ff49bc0 
>   core/src/test/java/org/apache/accumulo/core/client/ClientConfigurationTest.java PRE-CREATION

>   core/src/test/java/org/apache/accumulo/core/conf/ClientConfigurationTest.java 40be70f

>   core/src/test/java/org/apache/accumulo/core/rpc/SaslConnectionParamsTest.java PRE-CREATION

>   proxy/src/main/java/org/apache/accumulo/proxy/Proxy.java 4b048eb 
>   server/base/src/main/java/org/apache/accumulo/server/AccumuloServerContext.java 09ae4f4

>   server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java 046cfb5 
>   server/base/src/main/java/org/apache/accumulo/server/rpc/TCredentialsUpdatingInvocationHandler.java
PRE-CREATION 
>   server/base/src/main/java/org/apache/accumulo/server/rpc/TCredentialsUpdatingWrapper.java
PRE-CREATION 
>   server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java 641c0bf

>   server/base/src/main/java/org/apache/accumulo/server/rpc/ThriftServerType.java PRE-CREATION

>   server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java
5e81018 
>   server/base/src/main/java/org/apache/accumulo/server/security/SecurityUtil.java 29e4939

>   server/base/src/main/java/org/apache/accumulo/server/security/SystemCredentials.java
a59d57c 
>   server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthenticator.java
PRE-CREATION 
>   server/base/src/main/java/org/apache/accumulo/server/thrift/UGIAssumingProcessor.java
PRE-CREATION 
>   server/base/src/test/java/org/apache/accumulo/server/AccumuloServerContextTest.java
PRE-CREATION 
>   server/base/src/test/java/org/apache/accumulo/server/rpc/TCredentialsUpdatingInvocationHandlerTest.java
PRE-CREATION 
>   server/base/src/test/java/org/apache/accumulo/server/security/SystemCredentialsTest.java
4202a7e 
>   server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java 93a9a49

>   server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogsTest.java
f98721f 
>   server/gc/src/test/java/org/apache/accumulo/gc/SimpleGarbageCollectorTest.java 99558b8

>   server/gc/src/test/java/org/apache/accumulo/gc/replication/CloseWriteAheadLogReferencesTest.java
cad1e01 
>   server/master/src/main/java/org/apache/accumulo/master/Master.java 12195fa 
>   server/tracer/src/main/java/org/apache/accumulo/tracer/TraceServer.java 7e33300 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java d5c1d2f

>   shell/src/main/java/org/apache/accumulo/shell/Shell.java 58308ff 
>   shell/src/main/java/org/apache/accumulo/shell/ShellOptionsJC.java 8167ef8 
>   shell/src/test/java/org/apache/accumulo/shell/ShellConfigTest.java 0e72c8c 
>   shell/src/test/java/org/apache/accumulo/shell/ShellOptionsJCTest.java PRE-CREATION

>   test/src/main/java/org/apache/accumulo/test/functional/ZombieTServer.java eb84533 
>   test/src/main/java/org/apache/accumulo/test/performance/thrift/NullTserver.java 2ebc2e3

>   test/src/test/java/org/apache/accumulo/server/security/SystemCredentialsIT.java fb71f5f

> 
> Diff: https://reviews.apache.org/r/29386/diff/
> 
> 
> Testing
> -------
> 
> Ensure existing unit tests still function. Accumulo is functional and ran continuous
ingest multiple times using a client with only a Kerberos identity (no user/password provided).
Used MIT Kerberos with Apache Hadoop 2.6.0 and Apache ZooKeeper 3.4.5.
> 
> 
> Thanks,
> 
> Josh Elser
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message