You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2022/04/21 03:18:26 UTC

[GitHub] [skywalking] wankai123 opened a new pull request, #8917: HttpServer support configure the allowed HTTP methods.

wankai123 opened a new pull request, #8917:
URL: https://github.com/apache/skywalking/pull/8917

   - [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
   - [X] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/changelog/docs/en/changes/changes.md).
   


-- 
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: notifications-unsubscribe@skywalking.apache.org

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

[GitHub] [skywalking] wankai123 commented on pull request #8917: HttpServer support configure the allowed HTTP methods.

Posted by GitBox <gi...@apache.org>.
wankai123 commented on PR #8917:
URL: https://github.com/apache/skywalking/pull/8917#issuecomment-1104680789

   > I mean, if you add other methods than POST you need to enable this otherwise the added methods won't take effect. 
   
   Make sense. So could we just let this configure in the specific module `configure.java` code, like `ZipkinReceiverConfig.java`
   not expose it to the config file(yaml) or env. variables.


-- 
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: notifications-unsubscribe@skywalking.apache.org

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

[GitHub] [skywalking] wankai123 commented on pull request #8917: HttpServer support configure the allowed HTTP methods.

Posted by GitBox <gi...@apache.org>.
wankai123 commented on PR #8917:
URL: https://github.com/apache/skywalking/pull/8917#issuecomment-1104672477

   > Do we need this to be configurable? I remember we disable other methods because of Jetty server CVE. Can you take a 
   
   I don't want to set the other allowed method as global, since mostly are `POST` for now.
   This configuration needs to be explicitly turned on by configuration, I  think it's no different to configure it in java code.


-- 
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: notifications-unsubscribe@skywalking.apache.org

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

[GitHub] [skywalking] kezhenxu94 commented on pull request #8917: HttpServer support configure the allowed HTTP methods.

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #8917:
URL: https://github.com/apache/skywalking/pull/8917#issuecomment-1104680754

   Let trace back to when this was added https://github.com/apache/skywalking/pull/7052 I'd just remove the method check as this was to fix Jetty CVE  and now we don't use Jetty we could just allow all methods. 


-- 
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: notifications-unsubscribe@skywalking.apache.org

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

[GitHub] [skywalking] wankai123 commented on a diff in pull request #8917: HTTPServer support the handler register with allowed HTTP methods

Posted by GitBox <gi...@apache.org>.
wankai123 commented on code in PR #8917:
URL: https://github.com/apache/skywalking/pull/8917#discussion_r854865818


##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/server/HTTPHandlerRegister.java:
##########
@@ -18,8 +18,10 @@
 
 package org.apache.skywalking.oap.server.core.server;
 
+import com.linecorp.armeria.common.HttpMethod;
+import java.util.List;
 import org.apache.skywalking.oap.server.library.module.Service;
 
 public interface HTTPHandlerRegister extends Service {
-    void addHandler(Object httpService);
+    void addHandler(Object httpService, List<HttpMethod> allowedMethods);

Review Comment:
   `allowedMethods` of course the server level, but when the handlers share the server, only themselves know provide which methods. So I think it's should send this info to the server when registering.



-- 
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: notifications-unsubscribe@skywalking.apache.org

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

[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #8917: HTTPServer support the handler register with allowed HTTP methods

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #8917:
URL: https://github.com/apache/skywalking/pull/8917#discussion_r854857902


##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/server/HTTPHandlerRegister.java:
##########
@@ -18,8 +18,10 @@
 
 package org.apache.skywalking.oap.server.core.server;
 
+import com.linecorp.armeria.common.HttpMethod;
+import java.util.List;
 import org.apache.skywalking.oap.server.library.module.Service;
 
 public interface HTTPHandlerRegister extends Service {
-    void addHandler(Object httpService);
+    void addHandler(Object httpService, List<HttpMethod> allowedMethods);

Review Comment:
   `allowedMethods` should not be handler level, it should be server level, so I don't think it is a good idea to set in `addHandler` method, it should be set in constructing an `HTTPServer`



##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/server/HTTPHandlerRegister.java:
##########
@@ -18,8 +18,10 @@
 
 package org.apache.skywalking.oap.server.core.server;
 
+import com.linecorp.armeria.common.HttpMethod;
+import java.util.List;
 import org.apache.skywalking.oap.server.library.module.Service;
 
 public interface HTTPHandlerRegister extends Service {
-    void addHandler(Object httpService);
+    void addHandler(Object httpService, List<HttpMethod> allowedMethods);

Review Comment:
   `allowedMethods` can't be handler level, it should be server level, so I don't think it is a good idea to set in `addHandler` method, it should be set in constructing an `HTTPServer`



-- 
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: notifications-unsubscribe@skywalking.apache.org

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

[GitHub] [skywalking] wu-sheng commented on pull request #8917: HttpServer support configure the allowed HTTP methods.

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #8917:
URL: https://github.com/apache/skywalking/pull/8917#issuecomment-1104679450

   From HTTP server perspective, these configurations should only affect to codes interaction, which is fine. We don't expose these to end users.
   But for different HTTP server, it could be different, SkyWalking only exposes POST for native protocol, but Zipkin expose Get for query.


-- 
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: notifications-unsubscribe@skywalking.apache.org

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

[GitHub] [skywalking] wu-sheng merged pull request #8917: HTTPServer support the handler register with allowed HTTP methods

Posted by GitBox <gi...@apache.org>.
wu-sheng merged PR #8917:
URL: https://github.com/apache/skywalking/pull/8917


-- 
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: notifications-unsubscribe@skywalking.apache.org

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

[GitHub] [skywalking] wu-sheng commented on a diff in pull request #8917: HTTPServer support the handler register with allowed HTTP methods

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #8917:
URL: https://github.com/apache/skywalking/pull/8917#discussion_r854951815


##########
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServer.java:
##########
@@ -27,14 +27,19 @@
 import com.linecorp.armeria.server.logging.LoggingService;
 import java.net.InetSocketAddress;
 import java.time.Duration;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
 import lombok.extern.slf4j.Slf4j;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.skywalking.oap.server.library.server.Server;
+import static java.util.Objects.requireNonNull;
 
 @Slf4j
 public class HTTPServer implements Server {
     private final HTTPServerConfig config;
     private ServerBuilder sb;
+    private final Set<HttpMethod> allowedMethods = new HashSet<>();

Review Comment:
   Let's rename `allowedMethods` to `httpMethods`, which are more clear.



-- 
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: notifications-unsubscribe@skywalking.apache.org

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

[GitHub] [skywalking] wu-sheng commented on a diff in pull request #8917: HTTPServer support the handler register with allowed HTTP methods

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #8917:
URL: https://github.com/apache/skywalking/pull/8917#discussion_r854952016


##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/server/HTTPHandlerRegisterImpl.java:
##########
@@ -29,7 +31,7 @@ public HTTPHandlerRegisterImpl(HTTPServer server) {
     }
 
     @Override
-    public void addHandler(Object httpService) {
-        server.addHandler(httpService);
+    public void addHandler(final Object httpService, final List<HttpMethod> allowedMethods) {

Review Comment:
   ```suggestion
       public void addHandler(final Object httpService, final List<HttpMethod> httpMethods) {
   ```



-- 
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: notifications-unsubscribe@skywalking.apache.org

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

[GitHub] [skywalking] wankai123 commented on a diff in pull request #8917: HTTPServer support the handler register with allowed HTTP methods

Posted by GitBox <gi...@apache.org>.
wankai123 commented on code in PR #8917:
URL: https://github.com/apache/skywalking/pull/8917#discussion_r855053020


##########
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServer.java:
##########
@@ -27,14 +27,19 @@
 import com.linecorp.armeria.server.logging.LoggingService;
 import java.net.InetSocketAddress;
 import java.time.Duration;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
 import lombok.extern.slf4j.Slf4j;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.skywalking.oap.server.library.server.Server;
+import static java.util.Objects.requireNonNull;
 
 @Slf4j
 public class HTTPServer implements Server {
     private final HTTPServerConfig config;
     private ServerBuilder sb;
+    private final Set<HttpMethod> allowedMethods = new HashSet<>();

Review Comment:
   That's in HTTP server exactly what it means, and have changed others. 



-- 
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: notifications-unsubscribe@skywalking.apache.org

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

[GitHub] [skywalking] kezhenxu94 commented on pull request #8917: HttpServer support configure the allowed HTTP methods.

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #8917:
URL: https://github.com/apache/skywalking/pull/8917#issuecomment-1104676693

   I mean, if you add other methods than POST you need to enable this otherwise the added methods won't take effect. What's the point to let users set it themself if this is going to be always "POST,GET"? Do you think users will change the configuration to something like "POST,GET,DELETE" if they don't modify the codes (add a DELETE method)? I don't think we want to add random configuration that users might never set themselves.


-- 
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: notifications-unsubscribe@skywalking.apache.org

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

[GitHub] [skywalking] wu-sheng commented on pull request #8917: HttpServer support configure the allowed HTTP methods.

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #8917:
URL: https://github.com/apache/skywalking/pull/8917#issuecomment-1104686669

   In my mind, I prefer to generate this setting from all declaration of handlers, and set this when server start. This is not a setting, this is a expose limitation, which is good. 
   I know this was introduced by jetty, but this kind of CVE happenned from time to time for any HTTP server.
   @wankai123 Let's not check it, and accept this as a part of handler registration, and lock it just before server starts.
   


-- 
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: notifications-unsubscribe@skywalking.apache.org

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