svn commit: r1823325 - in /ofbiz/ofbiz-framework/branches/release17.12: ./ framework/common/groovyScripts/ framework/security/config/ framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/

Author: jleroux
Date: Tue Feb  6 13:21:34 2018
New Revision: 1823325

URL: http://svn.apache.org/viewvc?rev=1823325&view=rev
Log:
"Applied fix from trunk for revision: 1823324  "
------------------------------------------------------------------------
r1823324 | jleroux | 2018-02-06 14:17:57 +0100 (mar., 06 févr. 2018) | 26 lines

Fixed: Security issue in Token Based Authentication
(OFBIZ-10206)

The version I commitetd so far in OFBIZ-9833 has a small security issue.
See the Jira description for all details

To test I have attached a OFBIZ-10206-external-server-test-example.patch to
the Jira

This removes the external-server-query property now useless

In  ContextFilter the getHeader (wrapper) now uses an autoLoginCookie to get
the userLoginId passed in the JWT instead of externalServerUserLogin parameter.
A sourceServerWebappName parameter must be passed from the client request to
allow reading the autoLoginCookie.

This userLoginId is then retrieved on the target server from the JWT in the
externalServerLoginCheck which is simplified.

In LoginWorker
  getAutoLoginCookieName() has now 2 versions to allow to pass a webappname

  A new autoLogoutFromAllBackendSessions() method has been added but for now
  commented out. Decommenting it out will be submitted as a patch in OFBIZ-4959.

Thanks: Leila Mekika for reporting the security issue directly to me
------------------------------------------------------------------------

Removed:
    ofbiz/ofbiz-framework/branches/release17.12/framework/common/groovyScripts/ExternalServerName.groovy
Modified:
    ofbiz/ofbiz-framework/branches/release17.12/   (props changed)
    ofbiz/ofbiz-framework/branches/release17.12/framework/security/config/security.properties
    ofbiz/ofbiz-framework/branches/release17.12/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ContextFilter.java
    ofbiz/ofbiz-framework/branches/release17.12/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ExternalLoginKeysManager.java
    ofbiz/ofbiz-framework/branches/release17.12/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java

Propchange: ofbiz/ofbiz-framework/branches/release17.12/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Tue Feb  6 13:21:34 2018
@@ -10,4 +10,4 @@
 /ofbiz/branches/json-integration-refactoring:1634077-1635900
 /ofbiz/branches/multitenant20100310:921280-927264
 /ofbiz/branches/release13.07:1547657
-/ofbiz/ofbiz-framework/trunk:1819499,1819598,1819800,1819805,1819811,1820038,1820262,1820374-1820375,1820441,1820457,1820644,1820658,1820790,1820823,1820949,1820966,1821012,1821036,1821112,1821115,1821144,1821186,1821219,1821226,1821230,1821386,1821600,1821613,1821628,1821965,1822125,1822310,1822377,1822383,1822393,1822882
+/ofbiz/ofbiz-framework/trunk:1819499,1819598,1819800,1819805,1819811,1820038,1820262,1820374-1820375,1820441,1820457,1820644,1820658,1820790,1820823,1820949,1820966,1821012,1821036,1821112,1821115,1821144,1821186,1821219,1821226,1821230,1821386,1821600,1821613,1821628,1821965,1822125,1822310,1822377,1822383,1822393,1822882,1823324

Modified: ofbiz/ofbiz-framework/branches/release17.12/framework/security/config/security.properties
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/branches/release17.12/framework/security/config/security.properties?rev=1823325&r1=1823324&r2=1823325&view=diff
==============================================================================
--- ofbiz/ofbiz-framework/branches/release17.12/framework/security/config/security.properties (original)
+++ ofbiz/ofbiz-framework/branches/release17.12/framework/security/config/security.properties Tue Feb  6 13:21:34 2018
@@ -137,9 +137,7 @@ security.login.externalLoginKey.enabled=
 # -- If true, then it's possible to connect to another webapp on another server w/o signing in
 # -- This needs to be changed on both the source server and the target server
 use-external-server=N
-# -- Name of the external server (DNS)  
-external-server-name=demo-trunk.ofbiz.apache.org
-# -- Query part of the URL to use
-external-server-query=/catalog/control/
+# -- Name of the external server (DNS) ex: demo-trunk.ofbiz.apache.org where the port is not needed, or localhost:8443 (default) for local tests (not using the same webapp)
+external-server-name=localhost:8443
 # -- Time To Live of the token send to the external server in seconds
 external-server-token-duration=30

Modified: ofbiz/ofbiz-framework/branches/release17.12/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ContextFilter.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/branches/release17.12/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ContextFilter.java?rev=1823325&r1=1823324&r2=1823325&view=diff
==============================================================================
--- ofbiz/ofbiz-framework/branches/release17.12/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ContextFilter.java (original)
+++ ofbiz/ofbiz-framework/branches/release17.12/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ContextFilter.java Tue Feb  6 13:21:34 2018
@@ -191,19 +191,23 @@ public class ContextFilter implements Fi
         HttpServletRequestWrapper wrapper = new HttpServletRequestWrapper(httpRequest) {
             @Override
             public String getHeader(String name) {
-                String externalServerUserLoginId = request.getParameter(ExternalLoginKeysManager.EXTERNAL_SERVER_LOGIN_KEY);
+                String sourceWebappName = request.getParameter(ExternalLoginKeysManager.SOURCE_SERVER_WEBAPP_NAME);
                 String value = null;
-                if (externalServerUserLoginId != null) {
-                    // ExternalLoginKeysManager .createJwt() arguments in order:
-                    // id an Id, I suggest userLoginId
-                    // issuer is who/what issued the token. I suggest the server DNS
-                    // subject is the subject of the token. I suggest the destination webapp
-                    // timeToLive is the token maximum duration
-                    String webAppName = UtilHttp.getApplicationName(httpRequest);
-                    String dnsName = ExternalLoginKeysManager.getExternalServerName(httpRequest);
-                    long timeToLive = ExternalLoginKeysManager.getJwtTokenTimeToLive(httpRequest);
-                    // We would need a Bearer token (in Authorization request header) if we were using Oauth2, here we don't, so no Bearer
-                    value = ExternalLoginKeysManager.createJwt(externalServerUserLoginId, dnsName, webAppName , timeToLive);
+                if (sourceWebappName != null) {
+                    HttpServletRequest httpRequest = (HttpServletRequest) request;
+                    String userLoginId = LoginWorker.getAutoUserLoginId(httpRequest, sourceWebappName);
+                    if (userLoginId != null) { // At this stage the user must be logged in. But safer to check because we can't grab it from the session here.
+                            // ExternalLoginKeysManager.createJwt() arguments in order:
+                            // id an Id, here userLoginId
+                            // issuer is who/what issued the token, here the server URL
+                            // subject is the subject of the token, here the target webapp
+                            // timeToLive is the token maximum duration, default 30 seconds
+                            String targetWebAppName = UtilHttp.getApplicationName(httpRequest);
+                            String targetServerUrl = ExternalLoginKeysManager.getTargetServerUrl(httpRequest);
+                            long timeToLive = ExternalLoginKeysManager.getJwtTokenTimeToLive(httpRequest);
+                            // We would need a Bearer token (in Authorization request header) if we were using Oauth2, here we don't, so no Bearer
+                            value = ExternalLoginKeysManager.createJwt(userLoginId, targetServerUrl, targetWebAppName , timeToLive);
+                    }
                 }
                 if (value != null) return value;
                 return super.getHeader(name);

Modified: ofbiz/ofbiz-framework/branches/release17.12/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ExternalLoginKeysManager.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/branches/release17.12/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ExternalLoginKeysManager.java?rev=1823325&r1=1823324&r2=1823325&view=diff
==============================================================================
--- ofbiz/ofbiz-framework/branches/release17.12/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ExternalLoginKeysManager.java (original)
+++ ofbiz/ofbiz-framework/branches/release17.12/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ExternalLoginKeysManager.java Tue Feb  6 13:21:34 2018
@@ -43,9 +43,13 @@ import org.apache.ofbiz.service.LocalDis
 import org.apache.ofbiz.webapp.WebAppUtil;
 
 import io.jsonwebtoken.Claims;
+import io.jsonwebtoken.ExpiredJwtException;
 import io.jsonwebtoken.JwtBuilder;
 import io.jsonwebtoken.Jwts;
+import io.jsonwebtoken.MalformedJwtException;
 import io.jsonwebtoken.SignatureAlgorithm;
+import io.jsonwebtoken.SignatureException;
+import io.jsonwebtoken.UnsupportedJwtException;
 
 /**
  * This class manages the authentication tokens that provide single sign-on authentication to the OFBiz applications.
@@ -55,7 +59,7 @@ public class ExternalLoginKeysManager {
     private static final String EXTERNAL_LOGIN_KEY_ATTR = "externalLoginKey";
     // This Map is keyed by the randomly generated externalLoginKey and the value is a UserLogin GenericValue object
     private static final Map<String, GenericValue> externalLoginKeys = new ConcurrentHashMap<>();
-    public static final String EXTERNAL_SERVER_LOGIN_KEY = "externalServerLoginKey";
+    public static final String SOURCE_SERVER_WEBAPP_NAME = "sourceServerWebappName";
     // This works the same way than externalLoginKey but between 2 servers, not 2 webapps on the same server.
     // The Single Sign On (SSO) is ensured by a JWT token, then all is handled as normal by a session on the reached server.
     // The servers may or may not share a database but the 2 loginUserIds must be the same.
@@ -175,23 +179,37 @@ public class ExternalLoginKeysManager {
     }
     
     public static String externalServerLoginCheck(HttpServletRequest request, HttpServletResponse response) {
-
         Delegator delegator = (Delegator) request.getAttribute("delegator");
         HttpSession session = request.getSession();
 
-        String externalServerUserLoginId = request.getParameter(EXTERNAL_SERVER_LOGIN_KEY);
-        if (externalServerUserLoginId == null) return "success"; // Nothing to do here
-        if (!"Y".equals(EntityUtilProperties.getPropertyValue("security", "use-external-server", "N", delegator))) return "success"; // The target server does not allow external login by default
-
-        GenericValue currentUserLogin = (GenericValue) session.getAttribute("userLogin");
+        // The target server does not allow external login by default
+        boolean useExternalServer = "Y".equals(EntityUtilProperties.getPropertyValue("security", "use-external-server", "N", delegator));
+        String sourceWebappName = request.getParameter(SOURCE_SERVER_WEBAPP_NAME);
+        if (!useExternalServer || sourceWebappName == null) return "success"; // Nothing to do here
 
         try {
-            GenericValue userLogin = EntityQuery.use(delegator).from("UserLogin").where("userLoginId", externalServerUserLoginId).queryOne();
+            String userLoginId = null;
+            String authorizationHeader = request.getHeader("Authorization");
+            if (authorizationHeader != null) {
+                Claims claims = returnsClaims(authorizationHeader);
+                userLoginId = getSourceUserLoginId(claims );
+                boolean jwtOK = checkJwt(authorizationHeader, userLoginId, getTargetServerUrl(request), UtilHttp.getApplicationName(request));
+                if (!jwtOK) {
+                    // Something unexpected happened here
+                    Debug.logWarning("*** There was a problem with the JWT token, not signin in the user login " + userLoginId, module);
+                    return "success";
+                }
+            } else {
+                // Nothing to do here
+                return "success";
+            }
+
+            
+            GenericValue userLogin = EntityQuery.use(delegator).from("UserLogin").where("userLoginId", userLoginId).queryOne();
             if (userLogin != null) {
-                //to check it's the right tenant
-                //in case username and password are the same in different tenants
+                // Check it's the right tenant in case username and password are the same in different tenants
+                // TODO : not sure this is really useful in the case of external server, should not hurt anyway
                 LocalDispatcher dispatcher = (LocalDispatcher) request.getAttribute("dispatcher");
-                delegator = (Delegator) request.getAttribute("delegator");
                 String oldDelegatorName = delegator.getDelegatorName();
                 ServletContext servletContext = session.getServletContext();
                 if (!oldDelegatorName.equals(userLogin.getDelegator().getDelegatorName())) {
@@ -199,44 +217,15 @@ public class ExternalLoginKeysManager {
                     dispatcher = WebAppUtil.makeWebappDispatcher(servletContext, delegator);
                     LoginWorker.setWebContextObjects(request, response, delegator, dispatcher);
                 }
-
-                String authorizationHeader = request.getHeader("Authorization");
-                if (authorizationHeader != null) {
-                    boolean jwtOK = checkJwt(authorizationHeader, userLogin.getString("userLoginId"), getExternalServerName(request), UtilHttp.getApplicationName(request));
-                    if (!jwtOK) {
-                        Debug.logWarning("*** There was a problem with the JWT token, loging out the current user: " + externalServerUserLoginId, module);
-                        LoginWorker.logout(request, response);
-                        return "success";
-                    }
-                } else {
-                    // Something weird happened here => logout current user
-                    Debug.logWarning("*** There was a problem with the JWT token, loging out the current user: " + externalServerUserLoginId, module);
-                    LoginWorker.logout(request, response);
-                    return "success";
-                }
-
-                // if the user is already logged in and the login is different, logout the other user
-                if (currentUserLogin != null) {
-                    if (currentUserLogin.getString("userLoginId").equals(userLogin.getString("userLoginId"))) {
-                        // is the same user, just carry on...
-                        return "success";
-                    }
-
-                    // logout the current user and login the new user...
-                    LoginWorker.logout(request, response);
-                    // ignore the return value; even if the operation failed we want to set the new UserLogin
-                }
-
-                //connect
                 String enabled = userLogin.getString("enabled");
                 if (enabled == null || "Y".equals(enabled)) {
                     userLogin.set("hasLoggedOut", "N");
                     userLogin.store();
                 }
-                LoginWorker.doBasicLogin(userLogin, request);
             } else {
-                Debug.logWarning("Could not find userLogin for external login key: " + externalServerUserLoginId, module);
+                Debug.logWarning("*** There was a problem with the JWT token. Could not find userLogin " + userLoginId, module);
             }
+            LoginWorker.doBasicLogin(userLogin, request);
         } catch (GenericEntityException e) {
             Debug.logError(e, "Cannot get autoUserLogin information: " + e.getMessage(), module);
         }
@@ -264,11 +253,11 @@ public class ExternalLoginKeysManager {
         Key signingKey = new SecretKeySpec(apiKeySecretBytes, signatureAlgorithm.getJcaName());
         //Let's set the JWT Claims
         JwtBuilder builder = Jwts.builder().setId(id)
-                                    .setIssuedAt(now)
-                                    .setSubject(subject)
-                                    .setIssuer(issuer)
-                                    .setIssuedAt(now)
-                                    .signWith(signatureAlgorithm, signingKey);
+                .setIssuedAt(now)
+                .setSubject(subject)
+                .setIssuer(issuer)
+                .setIssuedAt(now)
+                .signWith(signatureAlgorithm, signingKey);
 
         //if it has been specified, let's add the expiration date, this should always be true
         if (ttlMillis >= 0) {
@@ -292,6 +281,28 @@ public class ExternalLoginKeysManager {
      */
     private static boolean checkJwt(String jwt, String id, String issuer, String subject) {
         //The JWT signature algorithm is using this to sign the token
+        Claims claims = returnsClaims(jwt);
+
+        long nowMillis = System.currentTimeMillis();
+        Date now = new Date(nowMillis);
+
+        return claims.getId().equals(id)
+                && claims.getIssuer().equals(issuer)
+                && claims.getSubject().equals(subject)
+                && claims.getExpiration().after(now);
+    }
+
+    /**
+     * @param jwt a JWT token
+     * @return claims the claims
+     * @throws ExpiredJwtException
+     * @throws UnsupportedJwtException
+     * @throws MalformedJwtException
+     * @throws SignatureException
+     * @throws IllegalArgumentException
+     */
+    private static Claims returnsClaims(String jwt) throws ExpiredJwtException, UnsupportedJwtException,
+            MalformedJwtException, SignatureException, IllegalArgumentException {
         SignatureAlgorithm signatureAlgorithm = SignatureAlgorithm.HS512;
 
         byte[] apiKeySecretBytes = DatatypeConverter.parseBase64Binary(ExternalServerJwtMasterSecretKey);
@@ -301,25 +312,21 @@ public class ExternalLoginKeysManager {
         Claims claims = Jwts.parser()
            .setSigningKey(signingKey)
            .parseClaimsJws(jwt).getBody();
-
-        long nowMillis = System.currentTimeMillis();
-        Date now = new Date(nowMillis);
-
-        return claims.getId().equals(id)
-                && claims.getIssuer().equals(issuer)
-                && claims.getSubject().equals(subject)
-                && claims.getExpiration().after(now);
+        return claims;
     }
 
-    public static String getExternalServerName(HttpServletRequest request) {
-        String reportingServerName = "";
+    private static String getSourceUserLoginId(Claims claims) {
+        return claims.getId();
+    }
+    
+    public static String getTargetServerUrl(HttpServletRequest request) {
+        String targetServerUrl = "";
         Delegator delegator = (Delegator) request.getAttribute("delegator");
         if (delegator != null && "Y".equals(EntityUtilProperties.getPropertyValue("security", "use-external-server", "N", delegator))) {
-            reportingServerName = EntityUtilProperties.getPropertyValue("security", "external-server-name", "localhost:8443", delegator);
-            String reportingServerQuery = EntityUtilProperties.getPropertyValue("security", "external-server-query", "/catalog/control/", delegator);
-            reportingServerName = "https://" + reportingServerName + reportingServerQuery;
+            targetServerUrl = EntityUtilProperties.getPropertyValue("security", "external-server-name", "localhost:8443", delegator);
+            targetServerUrl = "https://" + targetServerUrl;
         }
-        return reportingServerName;
+        return targetServerUrl;
     }
     
     public static long getJwtTokenTimeToLive(HttpServletRequest request) {

Modified: ofbiz/ofbiz-framework/branches/release17.12/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/branches/release17.12/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java?rev=1823325&r1=1823324&r2=1823325&view=diff
==============================================================================
--- ofbiz/ofbiz-framework/branches/release17.12/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java (original)
+++ ofbiz/ofbiz-framework/branches/release17.12/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java Tue Feb  6 13:21:34 2018
@@ -622,12 +622,13 @@ public class LoginWorker {
         RequestHandler rh = RequestHandler.getRequestHandler(request.getSession().getServletContext());
         rh.runBeforeLogoutEvents(request, response);
 
-
         // invalidate the security group list cache
         GenericValue userLogin = (GenericValue) request.getSession().getAttribute("userLogin");
 
         doBasicLogout(userLogin, request, response);
-
+        
+        //autoLogoutFromAllBackendSessions(userLogin, request, response);
+        // TODO check why, seems no sense
         if (request.getAttribute("_AUTO_LOGIN_LOGOUT_") == null) {
             return autoLoginCheck(request, response);
         }
@@ -718,6 +719,14 @@ public class LoginWorker {
         return UtilHttp.getApplicationName(request) + ".autoUserLoginId";
     }
 
+    protected static String getAutoLoginCookieName(String webappName) {
+        return webappName + ".autoUserLoginId";
+    }
+    
+    /**
+    * @deprecated Moved to {@link org.apache.ofbiz.webapp.control.LoginWorker#getAutoUserLoginId(HttpServletRequest request, String webappName) String}
+    */
+   @Deprecated
     public static String getAutoUserLoginId(HttpServletRequest request) {
         String autoUserLoginId = null;
         Cookie[] cookies = request.getCookies();
@@ -734,12 +743,31 @@ public class LoginWorker {
         }
         return autoUserLoginId;
     }
+    
+    public static String getAutoUserLoginId(HttpServletRequest request, String webappName) {
+        String autoUserLoginId = null;
+        Cookie[] cookies = request.getCookies();
+        if (Debug.verboseOn()) {
+            Debug.logVerbose("Cookies: " + Arrays.toString(cookies), module);
+        }
+        if (cookies != null) {
+            for (Cookie cookie: cookies) {
+                String cookieName = (webappName != null) ? getAutoLoginCookieName(webappName) : getAutoLoginCookieName(request);
+                if (cookie.getName().equals(cookieName)) {
+                    autoUserLoginId = cookie.getValue();
+                    break;
+                }
+            }
+        }
+        return autoUserLoginId;
+    }
+
 
     public static String autoLoginCheck(HttpServletRequest request, HttpServletResponse response) {
         Delegator delegator = (Delegator) request.getAttribute("delegator");
         HttpSession session = request.getSession();
 
-        return autoLoginCheck(delegator, session, getAutoUserLoginId(request));
+        return autoLoginCheck(delegator, session, getAutoUserLoginId(request, null));
     }
 
     private static String autoLoginCheck(Delegator delegator, HttpSession session, String autoUserLoginId) {
@@ -794,6 +822,34 @@ public class LoginWorker {
         return "success";
     }
 
+    public static String autoLogoutFromAllBackendSessions(GenericValue userLogin, HttpServletRequest request, HttpServletResponse response) {
+        HttpSession session = request.getSession();
+
+        // remove all the autoLoginCookies but if in ecommerce/ecomseo and webpos (it's done manually there, not sure for webpos TODO: check)
+        Cookie[] cookies = request.getCookies();
+        if (Debug.verboseOn()) {
+            Debug.logVerbose("Cookies: " + Arrays.toString(cookies), module);
+        }
+        if (cookies != null && userLogin != null) {
+            for (Cookie autoLoginCookie: cookies) {
+                if (autoLoginCookie.getName().contains("autoUserLoginId")
+                        && !(autoLoginCookie.getName().contains("ecommerce")
+                        || autoLoginCookie.getName().contains("ecomseo")
+                        || autoLoginCookie.getName().contains("webpos")))
+                autoLoginCookie.setMaxAge(0);
+                autoLoginCookie.setPath("/");
+                response.addCookie(autoLoginCookie);
+            }
+        }
+        
+        // remove the session attributes
+        session.removeAttribute("autoUserLogin");
+        session.removeAttribute("autoName");
+
+        request.setAttribute("_AUTO_LOGIN_LOGOUT_", Boolean.TRUE); // TODO check it's useful
+        return "success";
+    }
+
     public static boolean isUserLoggedIn(HttpServletRequest request) {
         HttpSession session = request.getSession();
         GenericValue currentUserLogin = (GenericValue) session.getAttribute("userLogin");