incubator-blur-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tim Williams <william...@gmail.com>
Subject Re: git commit: Changing the server security feature to be driven by a factory pattern. It also supports a filter pattern for server security.
Date Tue, 03 Feb 2015 13:22:54 GMT
On Tue, Feb 3, 2015 at 6:30 AM, Aaron McCurry <amccurry@gmail.com> wrote:
> On Monday, February 2, 2015, Tim Williams <williamstw@gmail.com> wrote:
>
>> On Mon, Feb 2, 2015 at 8:36 PM,  <amccurry@apache.org <javascript:;>>
>> wrote:
>> > Repository: incubator-blur
>> > Updated Branches:
>> >   refs/heads/master 4468f6cc5 -> 9e9e8ed1b
>> >
>> >
>> > Changing the server security feature to be driven by a factory pattern.
>> It also supports a filter pattern for server security.
>> >
>> >
>> > Project: http://git-wip-us.apache.org/repos/asf/incubator-blur/repo
>> > Commit:
>> http://git-wip-us.apache.org/repos/asf/incubator-blur/commit/9e9e8ed1
>> > Tree:
>> http://git-wip-us.apache.org/repos/asf/incubator-blur/tree/9e9e8ed1
>> > Diff:
>> http://git-wip-us.apache.org/repos/asf/incubator-blur/diff/9e9e8ed1
>> >
>> > Branch: refs/heads/master
>> > Commit: 9e9e8ed1b21e4537f3a5bcffe4573f04a8a644f4
>> > Parents: 4468f6c
>> > Author: Aaron McCurry <amccurry@gmail.com <javascript:;>>
>> > Authored: Mon Feb 2 20:36:03 2015 -0500
>> > Committer: Aaron McCurry <amccurry@gmail.com <javascript:;>>
>> > Committed: Mon Feb 2 20:36:03 2015 -0500
>> >
>> > ----------------------------------------------------------------------
>> >  .../org/apache/blur/server/ServerSecurity.java  |  8 +---
>> >  .../blur/server/ServerSecurityFactory.java      | 33 +++++++++++++
>> >  .../apache/blur/server/ServerSecurityUtil.java  | 24 ++++++----
>> >  .../example/SimpleExampleServerSecurity.java    | 24 +++++-----
>> >  .../blur/thrift/ThriftBlurControllerServer.java |  4 +-
>> >  .../blur/thrift/ThriftBlurShardServer.java      |  4 +-
>> >  .../org/apache/blur/thrift/ThriftServer.java    | 50
>> +++++++++++++-------
>> >  .../org/apache/blur/utils/BlurConstants.java    |  3 +-
>> >  8 files changed, 104 insertions(+), 46 deletions(-)
>> > ----------------------------------------------------------------------
>> >
>> >
>> >
>> http://git-wip-us.apache.org/repos/asf/incubator-blur/blob/9e9e8ed1/blur-core/src/main/java/org/apache/blur/server/ServerSecurity.java
>> > ----------------------------------------------------------------------
>> > diff --git
>> a/blur-core/src/main/java/org/apache/blur/server/ServerSecurity.java
>> b/blur-core/src/main/java/org/apache/blur/server/ServerSecurity.java
>> > index 3ebf079..ee74559 100644
>> > --- a/blur-core/src/main/java/org/apache/blur/server/ServerSecurity.java
>> > +++ b/blur-core/src/main/java/org/apache/blur/server/ServerSecurity.java
>> > @@ -19,16 +19,12 @@ package org.apache.blur.server;
>> >  import java.lang.reflect.Method;
>> >  import java.net.InetAddress;
>> >
>> > -import org.apache.blur.BlurConfiguration;
>> >  import org.apache.blur.thrift.generated.BlurException;
>> >  import org.apache.blur.user.User;
>> >
>> >  public abstract class ServerSecurity {
>>
>> I didn't mention earlier - this should be named ServerAccessFilter?
>>
>> > -
>> > -  public ServerSecurity(BlurConfiguration configuration) {
>> > -
>> > -  }
>> >
>> > -  public abstract boolean canAccess(Method method, Object[] args, User
>> user, InetAddress address, int port) throws BlurException;
>> > +  public abstract boolean canAccess(Method method, Object[] args, User
>> user, InetAddress address, int port)
>> > +      throws BlurException;
>> >
>> >  }
>> >
>> >
>> http://git-wip-us.apache.org/repos/asf/incubator-blur/blob/9e9e8ed1/blur-core/src/main/java/org/apache/blur/server/ServerSecurityFactory.java
>> > ----------------------------------------------------------------------
>> > diff --git
>> a/blur-core/src/main/java/org/apache/blur/server/ServerSecurityFactory.java
>> b/blur-core/src/main/java/org/apache/blur/server/ServerSecurityFactory.java
>> > new file mode 100644
>> > index 0000000..759112e
>> > --- /dev/null
>> > +++
>> b/blur-core/src/main/java/org/apache/blur/server/ServerSecurityFactory.java
>> > @@ -0,0 +1,33 @@
>> > +/**
>> > + * Licensed to the Apache Software Foundation (ASF) under one or more
>> > + * contributor license agreements.  See the NOTICE file distributed with
>> > + * this work for additional information regarding copyright ownership.
>> > + * The ASF licenses this file to You under the Apache License, Version
>> 2.0
>> > + * (the "License"); you may not use this file except in compliance with
>> > + * the License.  You may obtain a copy of the License at
>> > + *
>> > + *     http://www.apache.org/licenses/LICENSE-2.0
>> > + *
>> > + * Unless required by applicable law or agreed to in writing, software
>> > + * distributed under the License is distributed on an "AS IS" BASIS,
>> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>> implied.
>> > + * See the License for the specific language governing permissions and
>> > + * limitations under the License.
>> > + */
>> > +package org.apache.blur.server;
>> > +
>> > +import org.apache.blur.BlurConfiguration;
>> > +
>> > +public abstract class ServerSecurityFactory {
>>
>> Again with naming, ServerAccessFilterFactory?
>>
>> > +
>> > +  public enum ServerType {
>> > +    CONTROLLER, SHARD
>> > +  }
>> > +
>> > +  public ServerSecurityFactory() {
>> > +
>> > +  }
>> > +
>> > +  public abstract ServerSecurity getServerSecurity(ServerType server,
>> BlurConfiguration configuration);
>> > +
>> > +}
>> >
>> >
>> http://git-wip-us.apache.org/repos/asf/incubator-blur/blob/9e9e8ed1/blur-core/src/main/java/org/apache/blur/server/ServerSecurityUtil.java
>> > ----------------------------------------------------------------------
>> > diff --git
>> a/blur-core/src/main/java/org/apache/blur/server/ServerSecurityUtil.java
>> b/blur-core/src/main/java/org/apache/blur/server/ServerSecurityUtil.java
>> > index 162fe35..cd7deca 100644
>> > ---
>> a/blur-core/src/main/java/org/apache/blur/server/ServerSecurityUtil.java
>> > +++
>> b/blur-core/src/main/java/org/apache/blur/server/ServerSecurityUtil.java
>> > @@ -22,6 +22,7 @@ import java.lang.reflect.Method;
>> >  import java.lang.reflect.Proxy;
>> >  import java.net.InetAddress;
>> >  import java.net.InetSocketAddress;
>> > +import java.util.List;
>> >
>> >  import org.apache.blur.log.Log;
>> >  import org.apache.blur.log.LogFactory;
>> > @@ -34,12 +35,15 @@ public class ServerSecurityUtil {
>> >
>> >    private static final Log LOG =
>> LogFactory.getLog(ServerSecurityUtil.class);
>> >
>> > -  public static Iface applySecurity(final Iface iface, final
>> ServerSecurity serverSecurity, final boolean shardServer) {
>> > -    if (serverSecurity == null) {
>> > +  public static Iface applySecurity(final Iface iface, final
>> List<ServerSecurity> serverSecurityList,
>> > +      final boolean shardServer) {
>> > +    if (serverSecurityList == null || serverSecurityList.isEmpty()) {
>> >        LOG.info("No server security configured.");
>> >        return iface;
>> >      }
>> > -    LOG.info("Server security configured with [{0}] class [{1}].",
>> serverSecurity, serverSecurity.getClass());
>> > +    for (ServerSecurity serverSecurity : serverSecurityList) {
>> > +      LOG.info("Server security configured with [{0}] class [{1}].",
>> serverSecurity, serverSecurity.getClass());
>> > +    }
>> >      InvocationHandler handler = new InvocationHandler() {
>> >        @Override
>> >        public Object invoke(Object proxy, Method method, Object[] args)
>> throws Throwable {
>> > @@ -53,14 +57,16 @@ public class ServerSecurityUtil {
>> >          InetAddress address = remoteSocketAddress.getAddress();
>> >          int port = remoteSocketAddress.getPort();
>> >          User user = UserContext.getUser();
>> > -        if (serverSecurity.canAccess(method, args, user, address,
>> port)) {
>> > -          try {
>> > -            return method.invoke(iface, args);
>> > -          } catch (InvocationTargetException e) {
>> > -            throw e.getTargetException();
>> > +        for (ServerSecurity serverSecurity : serverSecurityList) {
>> > +          if (!serverSecurity.canAccess(method, args, user, address,
>> port)) {
>> > +            throw new BException("ACCESS DENIED for User [{0}] method
>> [{1}].", user, method.getName());
>> >            }
>> >          }
>> > -        throw new BException("ACCESS DENIED for User [{0}] method
>> [{1}].", user, method.getName());
>> > +        try {
>> > +          return method.invoke(iface, args);
>> > +        } catch (InvocationTargetException e) {
>> > +          throw e.getTargetException();
>> > +        }
>> >        }
>> >      };
>> >      return (Iface) Proxy.newProxyInstance(Iface.class.getClassLoader(),
>> new Class[] { Iface.class }, handler);
>> >
>> >
>> http://git-wip-us.apache.org/repos/asf/incubator-blur/blob/9e9e8ed1/blur-core/src/main/java/org/apache/blur/server/example/SimpleExampleServerSecurity.java
>> > ----------------------------------------------------------------------
>> > diff --git
>> a/blur-core/src/main/java/org/apache/blur/server/example/SimpleExampleServerSecurity.java
>> b/blur-core/src/main/java/org/apache/blur/server/example/SimpleExampleServerSecurity.java
>> > index 206c054..e6628ae 100644
>> > ---
>> a/blur-core/src/main/java/org/apache/blur/server/example/SimpleExampleServerSecurity.java
>> > +++
>> b/blur-core/src/main/java/org/apache/blur/server/example/SimpleExampleServerSecurity.java
>> > @@ -21,24 +21,26 @@ import java.net.InetAddress;
>> >
>> >  import org.apache.blur.BlurConfiguration;
>> >  import org.apache.blur.server.ServerSecurity;
>> > +import org.apache.blur.server.ServerSecurityFactory;
>> >  import org.apache.blur.thrift.generated.BlurException;
>> >  import org.apache.blur.user.User;
>> >
>> > -public class SimpleExampleServerSecurity extends ServerSecurity {
>> > -
>> > -  public SimpleExampleServerSecurity(BlurConfiguration configuration) {
>> > -    super(configuration);
>> > -  }
>> > +public class SimpleExampleServerSecurity extends ServerSecurityFactory {
>> >
>> >    @Override
>> > -  public boolean canAccess(Method method, Object[] args, User user,
>> InetAddress address, int port) throws BlurException {
>> > -    if (method.getName().equals("createTable")) {
>> > -      if (user != null && user.getUsername().equals("admin")) {
>> > +  public ServerSecurity getServerSecurity(ServerType server,
>> BlurConfiguration configuration) {
>> > +    return new ServerSecurity() {
>> > +      @Override
>> > +      public boolean canAccess(Method method, Object[] args, User user,
>> InetAddress address, int port) throws BlurException {
>> > +        if (method.getName().equals("createTable")) {
>> > +          if (user != null && user.getUsername().equals("admin")) {
>> > +            return true;
>> > +          }
>> > +          return false;
>> > +        }
>> >          return true;
>> >        }
>> > -      return false;
>> > -    }
>> > -    return true;
>> > +    };
>> >    }
>> >
>> >  }
>> >
>> >
>> http://git-wip-us.apache.org/repos/asf/incubator-blur/blob/9e9e8ed1/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurControllerServer.java
>> > ----------------------------------------------------------------------
>> > diff --git
>> a/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurControllerServer.java
>> b/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurControllerServer.java
>> > index 5632b88..2e5bb20 100644
>> > ---
>> a/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurControllerServer.java
>> > +++
>> b/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurControllerServer.java
>> > @@ -50,6 +50,7 @@ import static
>> org.apache.blur.utils.BlurConstants.BLUR_ZOOKEEPER_TIMEOUT_DEFAULT
>> >  import static org.apache.blur.utils.BlurUtil.quietClose;
>> >
>> >  import java.io.File;
>> > +import java.util.List;
>> >
>> >  import org.apache.blur.BlurConfiguration;
>> >  import org.apache.blur.command.ControllerCommandManager;
>> > @@ -65,6 +66,7 @@ import
>> org.apache.blur.manager.indexserver.BlurServerShutDown.BlurShutdown;
>> >  import org.apache.blur.metrics.ReporterSetup;
>> >  import org.apache.blur.server.ControllerServerEventHandler;
>> >  import org.apache.blur.server.ServerSecurity;
>> > +import org.apache.blur.server.ServerSecurityFactory;
>> >  import org.apache.blur.server.ServerSecurityUtil;
>> >  import org.apache.blur.thirdparty.thrift_0_9_0.protocol.TJSONProtocol;
>> >  import org.apache.blur.thirdparty.thrift_0_9_0.server.TServlet;
>> > @@ -184,7 +186,7 @@ public class ThriftBlurControllerServer extends
>> ThriftServer {
>> >      Trace.setStorage(traceStorage);
>> >      Trace.setNodeName(nodeName);
>> >
>> > -    ServerSecurity serverSecurity = getServerSecurity(configuration,
>> false);
>> > +    List<ServerSecurity> serverSecurity =
>> getServerSecurityList(configuration,
>> ServerSecurityFactory.ServerType.CONTROLLER);
>> >
>> >      Iface iface = BlurUtil.wrapFilteredBlurServer(configuration,
>> controllerServer, false);
>> >      iface = ServerSecurityUtil.applySecurity(iface, serverSecurity,
>> false);
>> >
>> >
>> http://git-wip-us.apache.org/repos/asf/incubator-blur/blob/9e9e8ed1/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurShardServer.java
>> > ----------------------------------------------------------------------
>> > diff --git
>> a/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurShardServer.java
>> b/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurShardServer.java
>> > index 0d91bec..5a0549a 100644
>> > ---
>> a/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurShardServer.java
>> > +++
>> b/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurShardServer.java
>> > @@ -59,6 +59,7 @@ import static
>> org.apache.blur.utils.BlurUtil.quietClose;
>> >  import java.io.Closeable;
>> >  import java.io.File;
>> >  import java.lang.reflect.Constructor;
>> > +import java.util.List;
>> >  import java.util.Map.Entry;
>> >  import java.util.Set;
>> >  import java.util.Timer;
>> > @@ -85,6 +86,7 @@ import
>> org.apache.blur.manager.indexserver.DistributedLayoutFactoryImpl;
>> >  import org.apache.blur.metrics.JSONReporter;
>> >  import org.apache.blur.metrics.ReporterSetup;
>> >  import org.apache.blur.server.ServerSecurity;
>> > +import org.apache.blur.server.ServerSecurityFactory;
>> >  import org.apache.blur.server.ServerSecurityUtil;
>> >  import org.apache.blur.server.ShardServerEventHandler;
>> >  import org.apache.blur.server.TableContext;
>> > @@ -264,7 +266,7 @@ public class ThriftBlurShardServer extends
>> ThriftServer {
>> >      Trace.setStorage(traceStorage);
>> >      Trace.setNodeName(nodeName);
>> >
>> > -    ServerSecurity serverSecurity = getServerSecurity(configuration,
>> true);
>> > +    List<ServerSecurity> serverSecurity =
>> getServerSecurityList(configuration,
>> ServerSecurityFactory.ServerType.SHARD);
>> >
>> >      Iface iface = BlurUtil.wrapFilteredBlurServer(configuration,
>> shardServer, true);
>> >      iface = ServerSecurityUtil.applySecurity(iface, serverSecurity,
>> true);
>> >
>> >
>> http://git-wip-us.apache.org/repos/asf/incubator-blur/blob/9e9e8ed1/blur-core/src/main/java/org/apache/blur/thrift/ThriftServer.java
>> > ----------------------------------------------------------------------
>> > diff --git
>> a/blur-core/src/main/java/org/apache/blur/thrift/ThriftServer.java
>> b/blur-core/src/main/java/org/apache/blur/thrift/ThriftServer.java
>> > index c7310ba..f1c3e94 100644
>> > --- a/blur-core/src/main/java/org/apache/blur/thrift/ThriftServer.java
>> > +++ b/blur-core/src/main/java/org/apache/blur/thrift/ThriftServer.java
>> > @@ -22,10 +22,9 @@ import static
>> org.apache.blur.metrics.MetricsConstants.JVM;
>> >  import static org.apache.blur.metrics.MetricsConstants.LOAD_AVERAGE;
>> >  import static org.apache.blur.metrics.MetricsConstants.ORG_APACHE_BLUR;
>> >  import static org.apache.blur.metrics.MetricsConstants.SYSTEM;
>> > -import static
>> org.apache.blur.utils.BlurConstants.BLUR_CONTROLLER_SERVER_SECURITY_CLASS;
>> >  import static org.apache.blur.utils.BlurConstants.BLUR_HDFS_TRACE_PATH;
>> >  import static org.apache.blur.utils.BlurConstants.BLUR_HOME;
>> > -import static
>> org.apache.blur.utils.BlurConstants.BLUR_SHARD_SERVER_SECURITY_CLASS;
>> > +import static
>> org.apache.blur.utils.BlurConstants.BLUR_SERVER_SECURITY_CLASS;
>> >  import static
>> org.apache.blur.utils.BlurConstants.BLUR_ZOOKEEPER_TRACE_PATH;
>> >
>> >  import java.io.BufferedReader;
>> > @@ -37,11 +36,15 @@ import java.lang.management.ManagementFactory;
>> >  import java.lang.management.MemoryMXBean;
>> >  import java.lang.management.MemoryUsage;
>> >  import java.lang.management.OperatingSystemMXBean;
>> > -import java.lang.reflect.Constructor;
>> >  import java.lang.reflect.Method;
>> >  import java.net.InetAddress;
>> >  import java.net.InetSocketAddress;
>> >  import java.net.UnknownHostException;
>> > +import java.util.ArrayList;
>> > +import java.util.List;
>> > +import java.util.Map;
>> > +import java.util.Map.Entry;
>> > +import java.util.TreeMap;
>> >  import java.util.UUID;
>> >  import java.util.concurrent.ExecutorService;
>> >
>> > @@ -51,6 +54,7 @@ import org.apache.blur.log.Log;
>> >  import org.apache.blur.log.LogFactory;
>> >  import
>> org.apache.blur.manager.indexserver.BlurServerShutDown.BlurShutdown;
>> >  import org.apache.blur.server.ServerSecurity;
>> > +import org.apache.blur.server.ServerSecurityFactory;
>> >  import org.apache.blur.thirdparty.thrift_0_9_0.protocol.TBinaryProtocol;
>> >  import
>> org.apache.blur.thirdparty.thrift_0_9_0.protocol.TCompactProtocol;
>> >  import org.apache.blur.thirdparty.thrift_0_9_0.server.TServer;
>> > @@ -439,22 +443,36 @@ public class ThriftServer {
>> >    }
>> >
>> >    @SuppressWarnings("unchecked")
>> > -  public static ServerSecurity getServerSecurity(BlurConfiguration
>> configuration, boolean shardServer) {
>> > -    String className;
>> > -    if (shardServer) {
>> > -      className = configuration.get(BLUR_SHARD_SERVER_SECURITY_CLASS);
>> > -    } else {
>> > -      className =
>> configuration.get(BLUR_CONTROLLER_SERVER_SECURITY_CLASS);
>> > +  public static List<ServerSecurity>
>> getServerSecurityList(BlurConfiguration configuration,
>> > +      ServerSecurityFactory.ServerType type) {
>> > +    Map<String, String> properties = configuration.getProperties();
>> > +    Map<String, String> classMap = new TreeMap<String, String>();
>> > +    for (Entry<String, String> e : properties.entrySet()) {
>> > +      String property = e.getKey();
>> > +      String value = e.getValue();
>> > +      if (value == null || value.isEmpty()) {
>> > +        continue;
>> > +      }
>> > +      if (property.startsWith(BLUR_SERVER_SECURITY_CLASS)) {
>> > +        classMap.put(property, value);
>> > +      }
>> >      }
>> > -    if (className == null) {
>> > +    if (classMap.isEmpty()) {
>> >        return null;
>> >      }
>> > -    try {
>> > -      Class<? extends ServerSecurity> clazz = (Class<? extends
>> ServerSecurity>) Class.forName(className);
>> > -      Constructor<? extends ServerSecurity> constructor =
>> clazz.getConstructor(new Class[] { BlurConfiguration.class });
>> > -      return constructor.newInstance(configuration);
>> > -    } catch (Exception e) {
>> > -      throw new RuntimeException(e);
>> > +    List<ServerSecurity> result = new ArrayList<ServerSecurity>();
>> > +    for (Entry<String, String> entry : classMap.entrySet()) {
>> > +      String className = entry.getValue();
>> > +      try {
>> > +        LOG.info("Loading factory class [{0}]", className);
>> > +        Class<? extends ServerSecurityFactory> clazz = (Class<? extends
>> ServerSecurityFactory>) Class
>> > +            .forName(className);
>> > +        ServerSecurityFactory serverSecurityFactory =
>> clazz.newInstance();
>> > +        result.add(serverSecurityFactory.getServerSecurity(type,
>> configuration));
>>
>> I was concerned that this would feel awkward and I think it does.  We
>> want the flexibility of chained filters which feels weird with a
>> factory pattern because we're really chaining factories here; but then
>> we want to vary by server type, so I dunno, maybe this is all that we
>> can do - just thinking aloud at this point...
>>
>> --tim
>>
>
> What do you think?  Drop the factory pattern? Or no?

Well, as I said, I'm just thinking aloud at this point;)  Maybe go
back to a regular interface with lifecycle methods? E.g.

public interface ServerAccessFilter {
  void init(ServerType type, BlurConfiguration conf);
  void filter(...)
  void close/exit/whatever
}

That'd also give a chance to close any connections/handles down as well?

--tim

Mime
View raw message