Return-Path: X-Original-To: apmail-accumulo-dev-archive@www.apache.org Delivered-To: apmail-accumulo-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 6D8F31007D for ; Wed, 11 Feb 2015 00:42:34 +0000 (UTC) Received: (qmail 62566 invoked by uid 500); 11 Feb 2015 00:42:34 -0000 Delivered-To: apmail-accumulo-dev-archive@accumulo.apache.org Received: (qmail 62523 invoked by uid 500); 11 Feb 2015 00:42:34 -0000 Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@accumulo.apache.org Delivered-To: mailing list dev@accumulo.apache.org Received: (qmail 62501 invoked by uid 99); 11 Feb 2015 00:42:34 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 11 Feb 2015 00:42:34 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 612191CF879; Wed, 11 Feb 2015 00:42:33 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============0761167261717527622==" MIME-Version: 1.0 Subject: Re: Review Request 30817: ACCUMULO-3513 Delegation token support From: "Josh Elser" To: "Christopher Tubbs" Cc: "Sean Busbey" , "Josh Elser" , "accumulo" Date: Wed, 11 Feb 2015 00:42:33 -0000 Message-ID: <20150211004233.11124.88857@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: "Josh Elser" X-ReviewGroup: accumulo X-ReviewRequest-URL: https://reviews.apache.org/r/30817/ X-Sender: "Josh Elser" References: <20150210213845.11124.19322@reviews.apache.org> In-Reply-To: <20150210213845.11124.19322@reviews.apache.org> Reply-To: "Josh Elser" X-ReviewRequest-Repository: accumulo --===============0761167261717527622== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > On Feb. 10, 2015, 9:38 p.m., Sean Busbey wrote: > > core/src/main/java/org/apache/accumulo/core/cli/MapReduceClientOpts.java, lines 65-66 > > > > > > it would help if this error message included what the permission an operator has to grant to recover. Good point. I think I wrote this before I added the system permission. Will add. > On Feb. 10, 2015, 9:38 p.m., Sean Busbey wrote: > > core/src/main/java/org/apache/accumulo/core/cli/MapReduceClientOpts.java, line 47 > > > > > > Could we add a method to AuthenticationToken that signals if delegation tokens are needed? > > > > Or make an intermediate between AuthenticationToken and KerberosToken that is something like NeedsADelegationToken? > > > > just something so token providers who also need a delegation token can piggy back on this without extending KerberosToken. It seems like a decent addition, but what's a use case that we'd want to enable users to actually do this? > On Feb. 10, 2015, 9:38 p.m., Sean Busbey wrote: > > docs/src/main/asciidoc/chapters/kerberos.txt, lines 80-82 > > > > > > link to Hadoop docs on delgation tokens? If you know where some documentation can be found, I'm all for adding it. The only good writeup I've found is vendor-hosted and not on Apache. Not sure if that's an issue or not (personally, I'd prefer some on apache, but that might mean I have to write it) > On Feb. 10, 2015, 9:38 p.m., Sean Busbey wrote: > > docs/src/main/asciidoc/chapters/kerberos.txt, lines 264-268 > > > > > > an example of checking and granting the system permission would help round this out. good point, will expand. > On Feb. 10, 2015, 9:38 p.m., Sean Busbey wrote: > > server/base/src/main/java/org/apache/accumulo/server/security/delegation/AuthenticationKey.java, line 39 > > > > > > can we use something other than Writable (e.g. protobuf, avro, etc) for this serialization? See TAuthenticationKey which implements the Writable. > On Feb. 10, 2015, 9:38 p.m., Sean Busbey wrote: > > server/base/src/main/java/org/apache/accumulo/server/security/delegation/AuthenticationTokenSecretManager.java, lines 47-49 > > > > > > Since we expect a subset of calls to only happen from the master, is it worth breaking this into two classes so we can enforce that? I don't think so, especially this was built in such a way that would allow any tserver to take over the role currently the master only serves. The KeyManager, KeyDistributor, and KeyWatcher do a good job at breaking down the functionality into "leader" and "followers", IMO. > On Feb. 10, 2015, 9:38 p.m., Sean Busbey wrote: > > server/base/src/main/java/org/apache/accumulo/server/security/delegation/AuthenticationTokenSecretManager.java, lines 86-97 > > > > > > can we add a correctness test that this server doesnt' think _now_ is before the issueDate? Sure -- what do you think it should do in this case? Throw an InvalidToken exception? I haven't really thought about time-skew/ntp issues; hopefully we can continue to avoid with the secret key expiration on the order of days... > On Feb. 10, 2015, 9:38 p.m., Sean Busbey wrote: > > server/base/src/main/java/org/apache/accumulo/server/security/delegation/AuthenticationTokenSecretManager.java, lines 173-200 > > > > > > our audit logs are per server, right? could we add in audit log events for key added, removed, and expired? > > > > I guess with the "user" from the audit events as the one in the delegation? Or too much? We'll get an audit when a user requests a delegation token. Auditing "server operations" would be a new addition to the audit log and might be confusing? Not sure. > On Feb. 10, 2015, 9:38 p.m., Sean Busbey wrote: > > server/base/src/main/java/org/apache/accumulo/server/security/delegation/ZooAuthenticationKeyDistributor.java, line 68 > > > > > > follow on ticket number? could we just always fail for now? Ooo, I'm glad I left that for myself (and you brought it up). Forgot about it :). I think I can nab that on the next revision. > On Feb. 10, 2015, 9:38 p.m., Sean Busbey wrote: > > server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthenticator.java, lines 120-134 > > > > > > probably a follow on and not for this issue, but do these events all make it to the audit log? Yeah, I forgot I added this -- out of scope for these changes. Can address impersonation auditing separately as well. > On Feb. 10, 2015, 9:38 p.m., Sean Busbey wrote: > > server/base/src/test/java/org/apache/accumulo/server/security/delegation/AuthenticationTokenSecretManagerTest.java, line 51 > > > > > > can we add a test that when presented with a token secret with an issue date in the future we reject? > > > > right now we have a test that the generated issuedate isn't super far in the past or in the future, but that won't cover if e.g. one tablet server ends up with a lagging system clock. I'm not sure if we need to be making such assertions on the issueDate. The only reason we even have the issueDate is so that the leader can appropriately "reinitialize" itself by using the last secret key that was generated if it satisfies the current configuration values (the interval hasn't passed). At first glance, this would make use of DelegationTokens enforce that server-side NTP is properly configured with no authentication purpose. Am I missing something there e.g. reducing plane of attacks? > On Feb. 10, 2015, 9:38 p.m., Sean Busbey wrote: > > server/master/src/main/java/org/apache/accumulo/master/Master.java, lines 612-614 > > > > > > please log the negative case at INFO as well. Agreed. > On Feb. 10, 2015, 9:38 p.m., Sean Busbey wrote: > > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java, lines 2444-2446 > > > > > > I'd say the error messge should atleast mention that delegation tokens will fail. That'll help someone lookign at logs to figure out why their MapReduce job won't run. Good point. Will expand. - Josh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30817/#review71849 ----------------------------------------------------------- On Feb. 10, 2015, 7:15 p.m., Josh Elser wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30817/ > ----------------------------------------------------------- > > (Updated Feb. 10, 2015, 7:15 p.m.) > > > Review request for accumulo and Christopher Tubbs. > > > Bugs: ACCUMULO-3513 > https://issues.apache.org/jira/browse/ACCUMULO-3513 > > > Repository: accumulo > > > Description > ------- > > DelegationTokens are a way to enable MapReduce jobs (or any distributed task which does not have direct access to the user's kerberos credentials) to identify themself and communicate with Accumulo. This is implemented in the same style of Hadoop which does this to provide HDFS and YARN access in YARN jobs, as well as HBase (HBase also served as the base when I started coding). > > A DelegationToken (nothing more than a short-lived, on-demand password generated by Accumulo) is provided to users who request one through the SecurityOperations API. Secret keys are created, distributed and expired within Accumulo itself (Master creates and expires keys, distributes via ZK to tservers), which allow the master to create delegation tokens upon request and tabletservers to validate delegation tokens provided as the authentication mechanism by users. Tokens are valid for 7 days by default and new secret keys are rolled every day (configurable). > > RPC is implemented SASL's DIGEST-MD5 mechanism (in addition to the GSSAPI support we already have for "normal" kerberos -- thrift makes this really easy). YARN provides a secure way to pass delegation tokens from the client JVM to each YARN task (mapper/reducer) to prevent unwanted parties from stealing the token (password) and using it for their own (nefarious) purposes. The feature is transparent for users as long as they as using our MapReduce JCommander options classes. > > User documentation has been added to the kerberos chapter. > > > Diffs > ----- > > core/src/main/java/org/apache/accumulo/core/Constants.java 0229d4e > core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java 216f32d > core/src/main/java/org/apache/accumulo/core/cli/MapReduceClientOnDefaultTable.java 0cf081f > core/src/main/java/org/apache/accumulo/core/cli/MapReduceClientOnRequiredTable.java 7719e92 > core/src/main/java/org/apache/accumulo/core/cli/MapReduceClientOpts.java 4b3b7ed > core/src/main/java/org/apache/accumulo/core/client/admin/SecurityOperations.java efeafc0 > core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java 8470da4 > core/src/main/java/org/apache/accumulo/core/client/impl/SecurityOperationsImpl.java feb1ee7 > core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportKey.java 072724b > core/src/main/java/org/apache/accumulo/core/client/mapred/AbstractInputFormat.java b83a024 > core/src/main/java/org/apache/accumulo/core/client/mapred/AccumuloOutputFormat.java f877ec6 > core/src/main/java/org/apache/accumulo/core/client/mapreduce/AbstractInputFormat.java 5c7b780 > core/src/main/java/org/apache/accumulo/core/client/mapreduce/AccumuloOutputFormat.java 5e0aa73 > core/src/main/java/org/apache/accumulo/core/client/mapreduce/impl/DelegationTokenStub.java PRE-CREATION > core/src/main/java/org/apache/accumulo/core/client/mapreduce/lib/impl/ConfiguratorBase.java b2b5150 > core/src/main/java/org/apache/accumulo/core/client/mapreduce/lib/impl/InputConfigurator.java 5405ac0 > core/src/main/java/org/apache/accumulo/core/client/mock/MockSecurityOperations.java db88cfb > core/src/main/java/org/apache/accumulo/core/client/security/tokens/DelegationToken.java PRE-CREATION > core/src/main/java/org/apache/accumulo/core/conf/Property.java 68fac73 > core/src/main/java/org/apache/accumulo/core/rpc/SaslClientDigestCallbackHandler.java PRE-CREATION > core/src/main/java/org/apache/accumulo/core/rpc/SaslConnectionParams.java e067e23 > core/src/main/java/org/apache/accumulo/core/rpc/SaslDigestCallbackHandler.java PRE-CREATION > core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java d880fb3 > core/src/main/java/org/apache/accumulo/core/security/AuthenticationTokenIdentifier.java PRE-CREATION > core/src/main/java/org/apache/accumulo/core/security/SystemPermission.java b998179 > core/src/main/java/org/apache/accumulo/core/util/ThriftMessageUtil.java PRE-CREATION > core/src/main/thrift/master.thrift d89e381 > core/src/main/thrift/security.thrift 66235a8 > core/src/test/java/org/apache/accumulo/core/client/impl/ThriftTransportKeyTest.java 2723273 > core/src/test/java/org/apache/accumulo/core/client/security/tokens/DelegationTokenTest.java PRE-CREATION > core/src/test/java/org/apache/accumulo/core/rpc/SaslClientDigestCallbackHandlerTest.java PRE-CREATION > core/src/test/java/org/apache/accumulo/core/rpc/SaslConnectionParamsTest.java 3910f34 > core/src/test/java/org/apache/accumulo/core/security/AuthenticationTokenIdentifierTest.java PRE-CREATION > core/src/test/java/org/apache/accumulo/core/util/ThriftMessageUtilTest.java PRE-CREATION > docs/src/main/asciidoc/chapters/kerberos.txt 05d7384 > fate/src/main/java/org/apache/accumulo/fate/zookeeper/IZooReader.java 610b1bd > fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReader.java 5706cf3 > proxy/src/main/java/org/apache/accumulo/proxy/Proxy.java f9039be > server/base/src/main/java/org/apache/accumulo/server/AccumuloServerContext.java 84c3853 > server/base/src/main/java/org/apache/accumulo/server/master/state/MetaDataStateStore.java bf56a7a > server/base/src/main/java/org/apache/accumulo/server/rpc/SaslServerConnectionParams.java PRE-CREATION > server/base/src/main/java/org/apache/accumulo/server/rpc/SaslServerDigestCallbackHandler.java PRE-CREATION > server/base/src/main/java/org/apache/accumulo/server/rpc/TCredentialsUpdatingInvocationHandler.java f85505d > server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java f1f8963 > server/base/src/main/java/org/apache/accumulo/server/rpc/UGIAssumingProcessor.java ab106a6 > server/base/src/main/java/org/apache/accumulo/server/security/AuditedSecurityOperation.java cc7a7cd > server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java 7adb46e > server/base/src/main/java/org/apache/accumulo/server/security/SystemCredentials.java 51d50a1 > server/base/src/main/java/org/apache/accumulo/server/security/delegation/AuthenticationKey.java PRE-CREATION > server/base/src/main/java/org/apache/accumulo/server/security/delegation/AuthenticationTokenKeyManager.java PRE-CREATION > server/base/src/main/java/org/apache/accumulo/server/security/delegation/AuthenticationTokenSecretManager.java PRE-CREATION > server/base/src/main/java/org/apache/accumulo/server/security/delegation/ZooAuthenticationKeyDistributor.java PRE-CREATION > server/base/src/main/java/org/apache/accumulo/server/security/delegation/ZooAuthenticationKeyWatcher.java PRE-CREATION > server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthenticator.java 08fa55b > server/base/src/test/java/org/apache/accumulo/server/AccumuloServerContextTest.java 49a60a6 > server/base/src/test/java/org/apache/accumulo/server/rpc/SaslDigestCallbackHandlerTest.java PRE-CREATION > server/base/src/test/java/org/apache/accumulo/server/rpc/SaslServerConnectionParamsTest.java PRE-CREATION > server/base/src/test/java/org/apache/accumulo/server/security/delegation/AuthenticationKeyTest.java PRE-CREATION > server/base/src/test/java/org/apache/accumulo/server/security/delegation/AuthenticationTokenKeyManagerTest.java PRE-CREATION > server/base/src/test/java/org/apache/accumulo/server/security/delegation/AuthenticationTokenSecretManagerTest.java PRE-CREATION > server/base/src/test/java/org/apache/accumulo/server/security/delegation/ZooAuthenticationKeyDistributorTest.java PRE-CREATION > server/base/src/test/java/org/apache/accumulo/server/security/delegation/ZooAuthenticationKeyWatcherTest.java PRE-CREATION > server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java da0b07c > server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogsTest.java 1d7f90f > server/gc/src/test/java/org/apache/accumulo/gc/SimpleGarbageCollectorTest.java 6fcdd37 > server/gc/src/test/java/org/apache/accumulo/gc/replication/CloseWriteAheadLogReferencesTest.java 120692a > server/master/src/main/java/org/apache/accumulo/master/Master.java be476de > server/master/src/main/java/org/apache/accumulo/master/MasterClientServiceHandler.java 72cba26 > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java a5675dc > test/src/main/java/org/apache/accumulo/test/continuous/ContinuousBatchWalker.java a2687bb > test/src/main/java/org/apache/accumulo/test/continuous/ContinuousIngest.java dba6ac9 > test/src/main/java/org/apache/accumulo/test/continuous/ContinuousMoru.java 4b5c3e7 > test/src/main/java/org/apache/accumulo/test/continuous/ContinuousOpts.java PRE-CREATION > test/src/main/java/org/apache/accumulo/test/continuous/ContinuousQuery.java 73048f6 > test/src/main/java/org/apache/accumulo/test/continuous/ContinuousScanner.java f68377a > test/src/main/java/org/apache/accumulo/test/continuous/ContinuousWalk.java 60f8ec2 > test/src/test/java/org/apache/accumulo/harness/MiniClusterHarness.java 06b4303 > test/src/test/java/org/apache/accumulo/test/functional/KerberosIT.java 3d48657 > > Diff: https://reviews.apache.org/r/30817/diff/ > > > Testing > ------- > > Many new unit tests. Additional integration tests. Standalone mapreduce jobs (continuous verify and terasort ingest) on a fully secure cluster. > > > Thanks, > > Josh Elser > > --===============0761167261717527622==--