[GitHub] [incubator-eventmesh] xwm1992 commented on a diff in pull request #3322: [ISSUE #3309] add subscriber acl authentication

["xwm1992 (via GitHub)" <gi...@apache.org>]

xwm1992 commented on code in PR #3322:
URL: https://github.com/apache/incubator-eventmesh/pull/3322#discussion_r1122666820


##########
settings.gradle:
##########
@@ -66,4 +66,9 @@ include 'eventmesh-trace-plugin:eventmesh-trace-jaeger'
 include 'eventmesh-webhook'
 include 'eventmesh-webhook:eventmesh-webhook-api'
 include 'eventmesh-webhook:eventmesh-webhook-admin'
-include 'eventmesh-webhook:eventmesh-webhook-receive'
\ No newline at end of file
+include 'eventmesh-webhook:eventmesh-webhook-receive'
+include 'eventmesh-security-auth-token'
+include 'eventmesh-security-plugin:eventmesh-security-auth-token'
+findProject(':eventmesh-security-plugin:eventmesh-security-auth-token')?.name = 'eventmesh-security-auth-token'
+include 'eventmesh-security-plugin:eventmesh-security-acl-token'
+findProject(':eventmesh-security-plugin:eventmesh-security-acl-token')?.name = 'eventmesh-security-acl-token'

Review Comment:
   these lines can changed to :
   ```
   include 'eventmesh-security-plugin'
   include 'eventmesh-security-plugin:eventmesh-security-api'
   include 'eventmesh-security-plugin:eventmesh-security-auth-token'
   ```



##########
eventmesh-common/src/main/java/org/apache/eventmesh/common/config/CommonConfiguration.java:
##########
@@ -72,6 +72,8 @@ public class CommonConfiguration {
     @ConfigFiled(field = "connector.plugin.type", notEmpty = true)
     private String eventMeshConnectorPluginType = "rocketmq";
 
+    @ConfigFiled(field = "security.validation.type.token", notEmpty = true)
+    private boolean eventMeshSecurityValidateTypeToken = false;

Review Comment:
   this configuration need add to the eventmesh.properties file, since it is marked with @ConfigFiled, you can refer to `eventMeshConnectorPluginType`



##########
eventmesh-common/src/main/java/org/apache/eventmesh/common/config/CommonConfiguration.java:
##########
@@ -95,6 +97,8 @@ public class CommonConfiguration {
     @ConfigFiled(field = "server.registry.enabled")
     private boolean eventMeshServerRegistryEnable = false;
 
+    @ConfigFiled(field = "security.publickey")
+    private String eventMeshSecurityPublickey = "";

Review Comment:
   same as above



##########
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/acl/Acl.java:
##########
@@ -138,6 +141,15 @@ public void doAclCheckInHttpReceive(String remoteAddr, String user, String pass,
         aclService.doAclCheckInReceive(buildHttpAclProperties(remoteAddr, user, pass, subsystem, topic, requestURI));
     }
 
+    public void doAclCheckInTcpReceive(String remoteAddr, String token, String subsystem, String topic,
+        String requestURI, Object obj) throws AclException {
+        aclService.doAclCheckInReceive(buildHttpAclProperties(remoteAddr, token, subsystem, topic, requestURI, obj));

Review Comment:
   same as above



##########
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/core/protocol/tcp/client/task/SubscribeTask.java:
##########
@@ -59,10 +61,19 @@ public void run() {
             final List<SubscriptionItem> subscriptionItems = new ArrayList<>();
             final boolean eventMeshServerSecurityEnable = eventMeshTCPServer.getEventMeshTCPConfiguration().isEventMeshServerSecurityEnable();
             final String remoteAddr = RemotingHelper.parseChannelRemoteAddr(ctx.channel());
+
+            String group = session.getClient().getGroup();
+            String token = session.getClient().getToken();
+            String subsystem = session.getClient().getSubsystem();
+
+            EventMeshAppSubTopicInfo eventMeshAppSubTopicInfo = eventMeshTCPServer.getRegistry().findEventMeshAppSubTopicInfo(group);
             subscriptionInfo.getTopicList().forEach(item -> {
-                //do acl check for receive msg
                 if (eventMeshServerSecurityEnable) {

Review Comment:
   `findEventMeshAppSubTopicInfo` method move into `if(eventMeshServerSecurityEnable)` is more convenient.



##########
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/core/protocol/tcp/client/task/HelloTask.java:
##########
@@ -65,10 +65,17 @@ public void run() {
         Session session = null;
         UserAgent user = (UserAgent) pkg.getBody();
         try {
+
             //do acl check in connect
+            String remoteAddr = RemotingHelper.parseChannelRemoteAddr(ctx.channel());
+            String group = user.getGroup();
+            String token = user.getToken();
+            String subsystem = user.getSubsystem();
+            String topic = "ddd/dnamespace/dtopic";

Review Comment:
   What is the topic here for？



##########
eventmesh-registry-plugin/eventmesh-registry-etcd/src/main/java/org/apache/eventmesh/registry/etcd/service/EtcdRegistryService.java:
##########
@@ -231,6 +232,11 @@ public boolean unRegister(EventMeshUnRegisterInfo eventMeshUnRegisterInfo) throw
         }
     }
 
+    @Override
+    public EventMeshAppSubTopicInfo findEventMeshAppSubTopicInfoByGroup(String group) throws RegistryException {
+        return null;

Review Comment:
   the return value here is null, will it be implemented in other pr



##########
eventmesh-connector-plugin/eventmesh-connector-pulsar/src/main/java/org/apache/eventmesh/connector/pulsar/config/ClientConfiguration.java:
##########
@@ -25,7 +25,7 @@
 
 @Getter
 @Setter
-@Config(prefix = "eventMesh.server.pulsar", path = "classPath://pulsar-client.properties")
+@Config(prefix = "eventMesh.server.pulsar")

Review Comment:
   why you remove this config: path = "classPath://pulsar-client.properties"



##########
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/acl/Acl.java:
##########
@@ -92,6 +94,11 @@ public void doAclCheckInTcpConnect(String remoteAddr, UserAgent userAgent, int r
         aclService.doAclCheckInConnect(buildTcpAclProperties(remoteAddr, userAgent, null, requestCode));
     }
 
+    public void doAclCheckInTcpConnect(String remoteAddr, String token, String subsystem, String topic,
+        String requestURI, Object obj) throws AclException {
+        aclService.doAclCheckInReceive(buildHttpAclProperties(remoteAddr, token, subsystem, topic, requestURI, obj));

Review Comment:
   this method will be triggered in tcp mode, but your's method is `buildHttpAclProperties`, it's not a good place to put it here



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: dev-help@eventmesh.apache.org