Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JBPM-8177] Security exceptions in localhost log when running kie-server on JWS #1704

Merged
merged 1 commit into from Feb 6, 2019

Conversation

elguardian
Copy link
Member

setting servlet 3.1 compliant servlet http constraints.

This will remove the severe log. we will have a default deny by the entries not specified

@MarianMacik
Copy link
Member

Just FYI. This will be probably break this:
https://issues.jboss.org/browse/DROOLS-1350
Commit: da2a32b

@elguardian
Copy link
Member Author

@MarianMacik
Hi, thx for noticing that.... If I understood the jira, this means that options should be without auth and the rest needing auth.
Added the htttp omision to the OPTIONS

@elguardian
Copy link
Member Author

@MarianMacik you will need to check whether the OPTIONS is accesible without auth and the rest of the methods are still needing auth.

@elguardian elguardian force-pushed the JBPM-8177 branch 2 times, most recently from 930b89a to 5da5bc9 Compare January 29, 2019 14:24
…ver on JWS

setting servlet 3.1 compliant servlet http constraints
@MarianMacik
Copy link
Member

@elguardian I played with this a little bit and it seems that this configuration is not what we want. At first, by the specification:

http-method or http-method-omission is used to specify which methods should be protected or which methods should be omitted from protection. An HTTP method is protected by a web-resource-collection under any of the following circumstances:

  • If no HTTP methods are named in the collection (which means that all are protected)
  • If the collection specifically names the HTTP method in an http-method subelement
  • If the collection contains one or more http-method-omission elements, none of which names the HTTP method

By this definition, we currently (with the current state of PR) protect every method except for OPTION. But it seems that the new feature <deny-uncovered-http-methods /> means actually deny every unprotected http method, i.e. uncovered/unprotected are synonyms or to be more precise it seems that in Servlet 3.1 specifications wording has been changed a little bit. I found it by trying the web.xml from this PR, but it was instantly replying with 403 when using OPTION method. I have also tried to unprotect HEAD method to be sure there is no issue with the endpoint itself and it was forbidden too, although with former configuration it returned OK with/without authentication.

So my final proposal, which works, is:

<web-app xmlns="http://java.sun.com/xml/ns/javaee"
         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         version="3.1"
         xsi:schemaLocation="http://java.sun.com/xml/ns/javaee http://java.sun.com/xml/ns/javaee/web-app_3_1.xsd">
  <display-name>KieServer</display-name>
  <listener>
    <listener-class>org.jboss.narayana.tomcat.jta.NarayanaJtaServletContextListener</listener-class>
  </listener>

  <listener>
    <listener-class>org.jboss.resteasy.plugins.server.servlet.ResteasyBootstrap</listener-class>
  </listener>

  <filter>
    <filter-name>capture-request-filter</filter-name>
    <filter-class>org.kie.server.services.impl.security.web.CaptureHttpRequestFilter</filter-class>
  </filter>
  <filter-mapping>
    <filter-name>capture-request-filter</filter-name>
    <url-pattern>/*</url-pattern>
  </filter-mapping>
  <servlet>
    <servlet-name>Resteasy</servlet-name>
    <servlet-class>org.jboss.resteasy.plugins.server.servlet.HttpServletDispatcher</servlet-class>
    <init-param>
      <param-name>javax.ws.rs.Application</param-name>
      <param-value>org.kie.server.remote.rest.common.KieServerApplication</param-value>
    </init-param>
  </servlet>
  <servlet-mapping>
    <servlet-name>Resteasy</servlet-name>
    <url-pattern>/services/rest/*</url-pattern>
  </servlet-mapping>
  <context-param>
    <param-name>resteasy.servlet.mapping.prefix</param-name>
    <param-value>/services/rest</param-value>
  </context-param>

  <security-constraint>
    <web-resource-collection>
      <web-resource-name>Swagger web resources</web-resource-name>
      <url-pattern>/services/rest/server/swagger.json</url-pattern>
      <url-pattern>/services/rest/server/files/*</url-pattern> <!-- Every method is protected, although there are no restrictions who can has access -->
    </web-resource-collection>
    <!-- No authentication. Swagger resources should be accessible without authentication and files (form rendering). -->
  </security-constraint>
  <security-constraint>
    <web-resource-collection>
      <web-resource-name>REST web resources</web-resource-name>
      <url-pattern>/services/rest/*</url-pattern>
      <http-method-omission>OPTIONS</http-method-omission> <!-- Only OPTION is not protected -->
    </web-resource-collection>
    <auth-constraint>
      <role-name>kie-server</role-name>
      <role-name>user</role-name>
    </auth-constraint>
  </security-constraint>
  <login-config>
    <auth-method>BASIC</auth-method>
    <realm-name>KIE Server</realm-name>
  </login-config>
  <security-role>
    <role-name>kie-server</role-name>
  </security-role>
  <security-role>
    <role-name>user</role-name>
  </security-role>

</web-app>

With this web.xml we get only:
SEVERE: For security constraints with URL pattern [/services/rest/*] the HTTP methods [OPTIONS] are uncovered.

So it means that we don't protect only OPTIONS method now. For Swagger, everything is protected as per specification, although you don't need any roles to access it, so it is available as before without authentication.

I have briefly tested this with both Wildfly and Tomcat.

@elguardian
Copy link
Member Author

elguardian commented Jan 30, 2019

Hi @MarianMacik the idea of the jira is to remove the severe INFO.

  <security-constraint>
    <web-resource-collection>
      <web-resource-name>REST web resources</web-resource-name>
      <url-pattern>/services/rest/*</url-pattern>
      <http-method>GET</http-method>
      <http-method>PUT</http-method>
      <http-method>POST</http-method>
      <http-method>DELETE</http-method>
    </web-resource-collection>
    <auth-constraint>
      <role-name>kie-server</role-name>
      <role-name>user</role-name>
    </auth-constraint>
  </security-constraint>
  <security-constraint>
    <web-resource-collection>
      <web-resource-name>REST web resources unprotected</web-resource-name>
      <url-pattern>/services/rest/*</url-pattern>
      <http-method>OPTIONS</http-method>
    </web-resource-collection>
    <!-- No authentication-->
  </security-constraint>

plus the deny tag (it is already in the commit)

@MarianMacik
Copy link
Member

To be honest, this SEVERE info should be warning from the tomcat side. But yes, if we want to completely get rid of these log entries, your solution is the way to go, although it looks to me like a workaround, since we are duplicating one security constraint basically just for sake of having one log entry dismissed :)

But I am fine with any of solutions.

@MarianMacik
Copy link
Member

Tested it with Tomcat and Wildfly.

@elguardian
Copy link
Member Author

ok to test

@elguardian
Copy link
Member Author

@mswiderski not really sure who should be cheking this one.

@mswiderski
Copy link
Contributor

@MarianMacik is already involved so I believe that is already covered. Do we have any outstanding issues or everything is done as discussed in comments?

@MarianMacik
Copy link
Member

I think it works now as expected. OPTIONS is the only method which is not authenticated and all other unlisted methods are denied with 403. It should also work for Swagger since we cover there services/rest/* endpoints. So I am fine with that :)

@MarianMacik
Copy link
Member

jenkins retest this

@MarianMacik
Copy link
Member

This currently failed because of https://issues.jboss.org/browse/JBPM-8191
So these 3 failed tests are not related.

@mswiderski
Copy link
Contributor

Jenkins retest this

1 similar comment
@mswiderski
Copy link
Contributor

Jenkins retest this

@mswiderski mswiderski merged commit 5ea28ce into kiegroup:master Feb 6, 2019
@elguardian elguardian deleted the JBPM-8177 branch February 6, 2019 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants