Re: Review Request 37209: GEODE-17 : Integrated Security Code Merge
On Aug. 14, 2015, 10:22 p.m., Darrel Schneider wrote: gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java, line 592 https://reviews.apache.org/r/37209/diff/1/?file=1034140#file1034140line592 I'm not sure what you are saying in this that requires to manage tokens but I think this might be better that manage tokens Tushar Khairnar wrote: will confirm with Neelkanth. This part was added by me while I was involved with the project initially (before Geode). The javadoc template was copied from that of an exising property (see security-client-auth-init above). Yes, it should be changed to 'that is required to manage tokens' or 'that manage tokens'. On Aug. 14, 2015, 10:22 p.m., Darrel Schneider wrote: gemfire-core/src/main/java/com/gemstone/gemfire/internal/i18n/LocalizedStrings.java, line 1288 https://reviews.apache.org/r/37209/diff/1/?file=1034144#file1034144line1288 Something seems wrong here. It says it is a method name but a method can not implement the TokenService interface. Is this a class name instead? The last sentence also talks about method name and static method so maybe I just don't understand. Put some of this info into the javadocs on the property (like for REST client verification) in DistributedSystem. Also add the before TokenService. Tushar Khairnar wrote: I think Neelkanth meant either its fully classified name of class implementing TokenService or static method returning an instance of TokenService. Again, the format of this statement was copied from another existing StringId constants (e.g. AbstractDistributionConfig_SECURITY_CLIENT_AUTH_INIT_NAME_0) for consistency. But it should not have been ambiguous. Yes, it should really be ... fully qualified method name of a class implementing the TokenService ... Legal values can be any fully qualified \name\ of a static method that returns an instance of the class, present in the classpath. - Amogh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37209/#review95439 --- On Aug. 14, 2015, 4:31 p.m., Tushar Khairnar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37209/ --- (Updated Aug. 14, 2015, 4:31 p.m.) Review request for geode, Amogh Shetkar, Jens Deppe, and Nilkanth Patel. Repository: geode Description --- GEODE-77 : Integrated Security Code Merge This is manual merge of code from int_security branch. Testing done : JMX RMI-connector testing done from JConsole, Gfsh interactive testing with different roles. DUnits are not yet integrated into open. Adding description about changes done JMX - Key Changes ManagementAgent.java Hooks managementInterceptor when security plugins are configured ManagementInterceptor.java Central interceptor for JMX RMI connector. Each JMX call go through interceptor via MBeanServerWrapper in following fashion jmx(mxbean.op()) - mbeanServerWrapper - interceptor - security plugin - back to wrapper - mxbean.op() ResourceOperationContext OperationContext for all mm resource operations. This returns operation code as RESOURCE (except for data commands) and has additional code called resourceOperationCode which return exact operation requested ResourceOperation This annotation is used to mark mxbean interfaces and commands to corresponding mm action JMXOperationContext describes mbean operation(getAttr,SetAttr,Op) in terms of ResourceOperationContext. Parses all MXBean annotation and build map used for mapping jmx calls to resource codes CLIOperationContext describes gfsh command(name, params) in terms of ResourceOperationContext Parses all Command annotation and build map used for mapping gfsh command calls to resource codes *MXBean and *Commands Changes Added ResourceOperation annotation REST ADMIN - Key Changes AuthManager gateway to authorize and authenticate REST ADMIN internal/web/controllers/AbstractCommandsController.java Changes for ADMIN REST to add authentication and authorization callbacks Pulse - Key Changes from gemfire side AccessControlMXBean/AccessControlContext This is hidden mbean which opens up authorization end-point for Pulse Pulse will access this mbean to know its
Re: Review Request 37209: GEODE-17 : Integrated Security Code Merge
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37209/ --- (Updated Aug. 14, 2015, 11:01 a.m.) Review request for geode, Amogh Shetkar, Jens Deppe, and Nilkanth Patel. Repository: geode Description (updated) --- GEODE-77 : Integrated Security Code Merge This is manual merge of code from int_security branch. Testing done : JMX RMI-connector testing done from JConsole, Gfsh interactive testing with different roles. DUnits are not yet integrated into open. Adding description about changes done JMX - Key Changes ManagementAgent.java Hooks managementInterceptor when security plugins are configured ManagementInterceptor.java Central interceptor for JMX RMI connector. Each JMX call go through interceptor via MBeanServerWrapper in following fashion jmx(mxbean.op()) - mbeanServerWrapper - interceptor - security plugin - back to wrapper - mxbean.op() ResourceOperationContext OperationContext for all mm resource operations. This returns operation code as RESOURCE (except for data commands) and has additional code called resourceOperationCode which return exact operation requested ResourceOperation This annotation is used to mark mxbean interfaces and commands to corresponding mm action JMXOperationContext describes mbean operation(getAttr,SetAttr,Op) in terms of ResourceOperationContext. Parses all MXBean annotation and build map used for mapping jmx calls to resource codes CLIOperationContext describes gfsh command(name, params) in terms of ResourceOperationContext Parses all Command annotation and build map used for mapping gfsh command calls to resource codes *MXBean and *Commands Changes Added ResourceOperation annotation REST ADMIN - Key Changes AuthManager gateway to authorize and authenticate REST ADMIN internal/web/controllers/AbstractCommandsController.java Changes for ADMIN REST to add authentication and authorization callbacks Pulse - Key Changes from gemfire side AccessControlMXBean/AccessControlContext This is hidden mbean which opens up authorization end-point for Pulse Pulse will access this mbean to know its authorization levels after connecting with given credentials Any JMX Client can use this mbean to know its (currrent jmx connection) authorization levels REST - Key changes gemfire-web-api - AbstractBaseController.java and other controller classes REST API changes for At Az DistributionConfig (its impl) New system properties token-service for REST TokenService New interface for REST endpoint which is supposed to give secured token when given Princial RestAPIsOperationContext OperationContext for REST API Diffs - gemfire-core/src/main/java/com/gemstone/gemfire/cache/operations/OperationContext.java d25063c gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java b7b2cd8 gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/AbstractDistributionConfig.java 472959d gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/DistributionConfig.java 10094a9 gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/DistributionConfigImpl.java b8dfeb3 gemfire-core/src/main/java/com/gemstone/gemfire/internal/i18n/LocalizedStrings.java f5ae3e5 gemfire-core/src/main/java/com/gemstone/gemfire/internal/security/AuthorizeRequest.java 8ba07a2 gemfire-core/src/main/java/com/gemstone/gemfire/management/CacheServerMXBean.java 59f6537 gemfire-core/src/main/java/com/gemstone/gemfire/management/DiskStoreMXBean.java f14d16c gemfire-core/src/main/java/com/gemstone/gemfire/management/DistributedSystemMXBean.java f0a0a79 gemfire-core/src/main/java/com/gemstone/gemfire/management/GatewayReceiverMXBean.java 3e5ba1a gemfire-core/src/main/java/com/gemstone/gemfire/management/GatewaySenderMXBean.java b6c5219 gemfire-core/src/main/java/com/gemstone/gemfire/management/LockServiceMXBean.java e53d50a gemfire-core/src/main/java/com/gemstone/gemfire/management/ManagerMXBean.java 04fda7e gemfire-core/src/main/java/com/gemstone/gemfire/management/MemberMXBean.java e935fcd gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/ManagementAgent.java 43bfe73
Re: Review Request 37209: GEODE-17 : Integrated Security Code Merge
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37209/#review95439 --- gemfire-core/src/main/java/com/gemstone/gemfire/cache/operations/OperationContext.java (line 82) https://reviews.apache.org/r/37209/#comment150374 whe was 22 skipped? gemfire-core/src/main/java/com/gemstone/gemfire/cache/operations/OperationContext.java (line 88) https://reviews.apache.org/r/37209/#comment150375 why 28? seems like it should be 26 (last ordinal + 1). gemfire-core/src/main/java/com/gemstone/gemfire/cache/operations/OperationContext.java (line 101) https://reviews.apache.org/r/37209/#comment150376 for new external APIs add @since Geode 1.0 gemfire-core/src/main/java/com/gemstone/gemfire/cache/operations/OperationContext.java (line 307) https://reviews.apache.org/r/37209/#comment150373 Destory should be Destroy. Also it is called delete in the javadocs and OP_DELETE_QUERY. They should be consistent. gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java (line 592) https://reviews.apache.org/r/37209/#comment150377 I'm not sure what you are saying in this that requires to manage tokens but I think this might be better that manage tokens gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java (line 594) https://reviews.apache.org/r/37209/#comment150380 I couldn't find the TokenService interface in this code review even though it is mentioned on the Description. gemfire-core/src/main/java/com/gemstone/gemfire/internal/i18n/LocalizedStrings.java (line 1288) https://reviews.apache.org/r/37209/#comment150378 Something seems wrong here. It says it is a method name but a method can not implement the TokenService interface. Is this a class name instead? The last sentence also talks about method name and static method so maybe I just don't understand. Put some of this info into the javadocs on the property (like for REST client verification) in DistributedSystem. Also add the before TokenService. - Darrel Schneider On Aug. 14, 2015, 4:01 a.m., Tushar Khairnar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37209/ --- (Updated Aug. 14, 2015, 4:01 a.m.) Review request for geode, Amogh Shetkar, Jens Deppe, and Nilkanth Patel. Repository: geode Description --- GEODE-77 : Integrated Security Code Merge This is manual merge of code from int_security branch. Testing done : JMX RMI-connector testing done from JConsole, Gfsh interactive testing with different roles. DUnits are not yet integrated into open. Adding description about changes done JMX - Key Changes ManagementAgent.java Hooks managementInterceptor when security plugins are configured ManagementInterceptor.java Central interceptor for JMX RMI connector. Each JMX call go through interceptor via MBeanServerWrapper in following fashion jmx(mxbean.op()) - mbeanServerWrapper - interceptor - security plugin - back to wrapper - mxbean.op() ResourceOperationContext OperationContext for all mm resource operations. This returns operation code as RESOURCE (except for data commands) and has additional code called resourceOperationCode which return exact operation requested ResourceOperation This annotation is used to mark mxbean interfaces and commands to corresponding mm action JMXOperationContext describes mbean operation(getAttr,SetAttr,Op) in terms of ResourceOperationContext. Parses all MXBean annotation and build map used for mapping jmx calls to resource codes CLIOperationContext describes gfsh command(name, params) in terms of ResourceOperationContext Parses all Command annotation and build map used for mapping gfsh command calls to resource codes *MXBean and *Commands Changes Added ResourceOperation annotation REST ADMIN - Key Changes AuthManager gateway to authorize and authenticate REST ADMIN internal/web/controllers/AbstractCommandsController.java Changes for ADMIN REST to add authentication and authorization callbacks Pulse - Key Changes from gemfire side AccessControlMXBean/AccessControlContext This is hidden mbean which opens up authorization end-point for Pulse Pulse will access this mbean to know its authorization levels after connecting with
Re: Review Request 37209: GEODE-17 : Integrated Security Code Merge
Performance vs. security should never be considered. Security trumps everything. We should adopt standards where available. We should use other open source libraries where applicable. As part of the Apache ecosystem now we need to look at Apache projects that may provide these capabilities. We rarely want to reinvent something, especially in security. -Jake — Jacob Barrett Manager GemFire Advanced Customer Engineering (ACE) Pivotal jbarr...@pivotal.io 503-533-3763 For immediate support please contact Pivotal Support at http://support.pivotal.io/ On Fri, Aug 7, 2015 at 9:28 AM, Anthony Baker aba...@pivotal.io wrote: Am I missing something? Not verifying the integrity of a security token creates a vulnerability, right? Have you quantified the performance impact of Spring Security? Anthony Agreed. Initially I had spec'd it out based on Spring Security. But Neelkanth felt token based approach is better for performance where we check only for presence of Token but not its Integrity - Tushar
Re: Review Request 37209: GEODE-17 : Integrated Security Code Merge
Am I missing something? Not verifying the integrity of a security token creates a vulnerability, right? Have you quantified the performance impact of Spring Security? Anthony Agreed. Initially I had spec'd it out based on Spring Security. But Neelkanth felt token based approach is better for performance where we check only for presence of Token but not its Integrity - Tushar
