Fixed JmacHttpsTest - created XmlPrincipalName with more tolerant equals

- Note that the equals method is not symmetric here - Exousia uses Set.contains -> equals to check the authorization. - I had to improve logs and code on several places to find out what is happening Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
eclipse-ee4j · Mar 26, 2024 · 72d61f4 · 72d61f4
1 parent 1763422
commit 72d61f4
Show file tree

Hide file tree

Showing 11 changed files with 248 additions and 254 deletions.
diff --git a/...dol/src/main/java/com/sun/enterprise/deployment/node/runtime/common/XmlPrincipalName.java b/...dol/src/main/java/com/sun/enterprise/deployment/node/runtime/common/XmlPrincipalName.java
@@ -0,0 +1,75 @@
+/*
+ * Copyright (c) 2024 Contributors to the Eclipse Foundation.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v. 2.0, which is available at
+ * http://www.eclipse.org/legal/epl-2.0.
+ *
+ * This Source Code may also be made available under the following Secondary
+ * Licenses when the conditions for such availability set forth in the
+ * Eclipse Public License v. 2.0 are satisfied: GNU General Public License,
+ * version 2 with the GNU Classpath Exception, which is available at
+ * https://www.gnu.org/software/classpath/license.html.
+ *
+ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
+ */
+
+package com.sun.enterprise.deployment.node.runtime.common;
+
+import java.io.Serializable;
+import java.security.Principal;
+import java.util.Objects;
+
+import org.glassfish.security.common.UserPrincipal;
+
+/**
+ * {@link Principal} loaded from XML descriptor.
+ * When the equals method is used, it compares just principal names and that the other object
+ * is an {@link Principal} instance too.
+ */
+// Must be UserPrincipal, because RoleMapper.internalAssignRole knows just that and Group.
+public class XmlPrincipalName implements UserPrincipal, Serializable {
+
+    private static final long serialVersionUID = -640182254691955451L;
+
+    private final String name;
+
+    /**
+     * @param name must not be null.
+     */
+    public XmlPrincipalName(String name) {
+        this.name = Objects.requireNonNull(name, "XML principal-name element must not be null.");
+    }
+
+
+    @Override
+    public String getName() {
+        return name;
+    }
+
+
+    @Override
+    public int hashCode() {
+        return name.hashCode();
+    }
+
+
+    /**
+     * We match user principals just by name.
+     * This is used in Jakarta Security to resolve authorisation.
+     */
+    @Override
+    public boolean equals(Object o) {
+        if (o instanceof Principal) {
+            Principal other = (Principal) o;
+            return getName().equals(other.getName());
+        }
+        return false;
+    }
+
+
+    @Override
+    public String toString() {
+        return getClass().getSimpleName() + "[" + getName() + "]";
+    }
+}
diff --git a/...l/src/main/java/com/sun/enterprise/deployment/runtime/common/PrincipalNameDescriptor.java b/...l/src/main/java/com/sun/enterprise/deployment/runtime/common/PrincipalNameDescriptor.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022 Contributors to the Eclipse Foundation
+ * Copyright (c) 2022, 2024 Contributors to the Eclipse Foundation
  * Copyright (c) 1997, 2018 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
@@ -17,11 +17,12 @@
 
 package com.sun.enterprise.deployment.runtime.common;
 
+import com.sun.enterprise.deployment.node.runtime.common.XmlPrincipalName;
+
 import java.lang.reflect.Constructor;
 import java.security.Principal;
 
 import org.glassfish.deployment.common.Descriptor;
-import org.glassfish.security.common.UserNameAndPassword;
 
 /**
  * This is an in memory representation of the principal-name with its name of
@@ -53,7 +54,7 @@ public String getName() {
      */
     public String getClassName() {
         if (className == null) {
-            return UserNameAndPassword.class.getName();
+            return XmlPrincipalName.class.getName();
         }
         return className;
     }

diff --git a/appserver/security/core-ee/src/main/java/com/sun/enterprise/security/ee/acl/RoleMapper.java b/appserver/security/core-ee/src/main/java/com/sun/enterprise/security/ee/acl/RoleMapper.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022 Contributors to the Eclipse Foundation
+ * Copyright (c) 2022, 2024 Contributors to the Eclipse Foundation
  * Copyright (c) 1997, 2018 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
@@ -46,6 +46,9 @@
 import org.glassfish.security.common.UserNameAndPassword;
 import org.glassfish.security.common.UserPrincipal;
 
+import static java.util.logging.Level.FINE;
+import static java.util.logging.Level.WARNING;
+
 /**
  * This Object maintains a mapping of users and groups to application specific Roles. Using this object this mapping
  * information could be maintained and queried at a later time. This is a complete rewrite of the previous RoleMapper
@@ -195,9 +198,7 @@ public Map<String, Subject> getRoleToSubjectMapping() {
     // The method that does the work for assignRole().
     private void internalAssignRole(Principal p, Role r) {
         String role = r.getName();
-        if (LOG.isLoggable(Level.FINE)) {
-            LOG.log(Level.FINE, "SECURITY:RoleMapper Assigning Role {0} to {1}", new Object[] {role, p});
-        }
+        LOG.log(FINE, "Assigning Role {0} to {1}", new Object[] {role, p});
         addRoleToPrincipal(p, role);
         if (p instanceof Group) {
             Set<Group> groups = roleToGroup.get(role);
@@ -307,9 +308,6 @@ public String toString() {
             }
             s.append(")");
         }
-        if (LOG.isLoggable(Level.FINER)) {
-            LOG.log(Level.FINER, s.toString());
-        }
         return s.toString();
     }
 
@@ -365,7 +363,7 @@ private String getDefaultP2RMappingClassName() {
             c.newInstance("anystring");
             return className;
         } catch (Exception e) {
-            LOG.log(Level.SEVERE, "pc.getDefaultP2RMappingClass: " + e);
+            LOG.log(Level.SEVERE, "pc.getDefaultP2RMappingClass: " + className, e);
             return null;
         }
     }
@@ -414,23 +412,17 @@ private void checkAndAddMappings() {
 
             if (topLevelRoles != null && topLevelRoles.contains(r)) {
                 logConflictWarning();
-                if (LOG.isLoggable(Level.FINE)) {
-                    LOG.log(Level.FINE,
-                            "Role " + r + " from module " + currentMapping.owner + " is being overridden by top-level mapping.");
-                }
-
+                LOG.log(FINE, "Role {0} from module {1} is being overridden by top-level mapping.",
+                    new Object[] {r, currentMapping.owner});
                 continue;
             }
 
             if (currentMapping.owner.equals(TOP_LEVEL)) {
                 topLevelRoles.add(r);
                 if (roleToSubject.keySet().contains(r.getName())) {
                     logConflictWarning();
-                    if (LOG.isLoggable(Level.FINE)) {
-                        LOG.log(Level.FINE,
-                                "Role " + r + " from top-level mapping descriptor is " + "overriding existing role in sub module.");
-                    }
-
+                    LOG.log(FINE,
+                        "Role {0} from top-level mapping descriptor is overriding existing role in sub module.", r);
                     unassignRole(r);
                 }
 
@@ -456,11 +448,8 @@ private boolean roleConflicts(Role r, Set<Principal> ps) {
 
         // check to see if there has been a previous conflict
         if (conflictedRoles != null && conflictedRoles.contains(r)) {
-            if (LOG.isLoggable(Level.FINE)) {
-                LOG.log(Level.FINE,
-                        "Role " + r + " from module " + currentMapping.owner + " has already had a conflict with other modules.");
-            }
-
+            LOG.log(FINE, "Role {0} from module {1} has already had a conflict with other modules.",
+                new Object[] {r, currentMapping.owner});
             return true;
         }
 
@@ -477,10 +466,8 @@ private boolean roleConflicts(Role r, Set<Principal> ps) {
         actualNum += pSet == null ? 0 : pSet.size();
         actualNum += gSet == null ? 0 : gSet.size();
         if (targetNumPrin != actualNum) {
-            if (LOG.isLoggable(Level.FINE)) {
-                LOG.log(Level.FINE, "Module " + currentMapping.owner + " has different number of mappings for role " + r.getName()
-                        + " than other mapping files");
-            }
+            LOG.log(FINE, "Module {0} has different number of mappings for role {1} than other mapping files",
+                new Object[] {currentMapping.owner, r.getName()});
 
             if (conflictedRoles == null) {
                 conflictedRoles = new HashSet<>();
@@ -503,9 +490,8 @@ private boolean roleConflicts(Role r, Set<Principal> ps) {
             }
 
             if (fail) {
-                if (LOG.isLoggable(Level.FINE)) {
-                    LOG.log(Level.FINE, "Role " + r + " in module " + currentMapping.owner + " is not included in other modules.");
-                }
+                LOG.log(FINE, "Role {0} in module {1} is not included in other modules.",
+                    new Object[] {r, currentMapping.owner});
 
                 if (conflictedRoles == null) {
                     conflictedRoles = new HashSet<>();
@@ -523,7 +509,8 @@ private boolean roleConflicts(Role r, Set<Principal> ps) {
 
     private void logConflictWarning() {
         if (!conflictLogged) {
-            LOG.log(Level.WARNING, "Role mapping conflicts found in application {0}. Some roles may not be mapped.", getName());
+            LOG.log(WARNING, "Role mapping conflicts found in application {0}. Some roles may not be mapped.",
+                getName());
             conflictLogged = true;
         }
     }
@@ -576,8 +563,7 @@ Principal getSameNamedPrincipal(String roleName) {
                 Principal principal = (Principal) c.newInstance(arg);
                 return principal;
             } catch (Exception e) {
-                LOG.log(Level.SEVERE, "rm.getSameNamedPrincipal", new Object[] { roleName, e });
-                throw new RuntimeException("Unable to get principal by default p2r mapping");
+                throw new RuntimeException("Unable to get principal by default p2r mapping", e);
             }
         }
 

diff --git a/appserver/security/webintegration/src/main/java/com/sun/web/security/RealmAdapter.java b/appserver/security/webintegration/src/main/java/com/sun/web/security/RealmAdapter.java
@@ -832,7 +832,6 @@ private void doLogout(HttpRequest request, boolean extensionEnabled) {
         logout();
     }
 
-    @SuppressWarnings({ "removal", "deprecation" })
     @Override
     public void logout() {
         setSecurityContext(null);
@@ -851,15 +850,14 @@ public Void run() {
         }
     }
 
-    /*
+    /**
      * IASRI 4688449 This method was only used by EEInstanceListener to set the security context prior to invocations by
      * re-authenticating a previously set WebPrincipal. This is now cached so no need.
      */
     public boolean authenticate(HttpServletRequest request, WebPrincipal principal) {
         if (principal.isUsingCertificate()) {
             return authenticate(request, null, null, null, principal.getCertificates());
         }
-
         return authenticate(request, principal.getName(), principal.getPassword(), null, null);
     }
 
@@ -871,7 +869,6 @@ public boolean authenticate(HttpServletRequest request, WebPrincipal principal)
      * @param the authentication method.
      * @param the authentication data.
      */
-    @SuppressWarnings({ "removal", "deprecation" })
     private boolean authenticate(HttpServletRequest request, String username, char[] password, DigestCredentials digestCredentials, X509Certificate[] certificates) {
         try {
             if (certificates != null) {
@@ -881,18 +878,15 @@ private boolean authenticate(HttpServletRequest request, String username, char[]
             } else {
                 LoginContextDriver.login(username, password, realmName);
             }
-            _logger.log(FINE, () -> "Web login succeeded for: " + SecurityContext.getCurrent().getCallerPrincipal());
+            _logger.log(FINE, "Web login succeeded for: {0}", SecurityContext.getCurrent().getCallerPrincipal());
 
-            WebSecurityManager webSecurityManager = getWebSecurityManager(false);
+            WebSecurityManager manager = getWebSecurityManager(false);
 
             // Sets the security context for Jakarta Authorization
-            if (webSecurityManager != null) {
-                AccessController.doPrivileged(new PrivilegedAction<Void>() {
-                    @Override
-                    public Void run() {
-                        webSecurityManager.onLogin(request);
-                        return null;
-                    }
+            if (manager != null) {
+                AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
+                    manager.onLogin(request);
+                    return null;
                 });
             }
 

diff --git a/...tion/src/main/resources/org/glassfish/main/test/app/security/jmac/https/glassfish-web.xml b/...tion/src/main/resources/org/glassfish/main/test/app/security/jmac/https/glassfish-web.xml
@@ -2,7 +2,7 @@
 <!DOCTYPE glassfish-web-app PUBLIC "-//GlassFish.org//DTD GlassFish Application Server 3.1 Servlet 3.0//EN" "http://glassfish.org/dtds/glassfish-web-app_3_0-1.dtd">
 <!--
 
-    Copyright (c) 2023 Contributors to the Eclipse Foundation.
+    Copyright (c) 2023, 2024 Contributors to the Eclipse Foundation.
     Copyright (c) 2017, 2018 Oracle and/or its affiliates. All rights reserved.
 
     This program and the accompanying materials are made available under the
@@ -22,6 +22,6 @@
 <glassfish-web-app httpservlet-security-provider="httpsTestAuthModule">
     <security-role-mapping>
         <role-name>myrole</role-name>
-        <principal-name>CN=HTTPSTEST, OU=Eclipse GlassFish Tests, O=Eclipse Foundation, L=Brussels, ST=Belgium, C=Belgium</principal-name>
+        <principal-name>CN=HTTPSTEST,OU=Eclipse GlassFish Tests,O=Eclipse Foundation,L=Brussels,ST=Belgium,C=Belgium</principal-name>
     </security-role-mapping>
 </glassfish-web-app>
diff --git a/...lication/src/test/java/org/glassfish/main/test/app/security/jmac/https/JmacHttpsTest.java b/...lication/src/test/java/org/glassfish/main/test/app/security/jmac/https/JmacHttpsTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2023 Eclipse Foundation and/or its affiliates. All rights reserved.
+ * Copyright (c) 2023, 2024 Eclipse Foundation and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0, which is available at
@@ -44,7 +44,6 @@
 import org.jboss.shrinkwrap.api.spec.WebArchive;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
 
@@ -59,8 +58,8 @@
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.stringContainsInOrder;
+import static org.junit.jupiter.api.Assertions.assertAll;
 
-@Disabled("Fails with GF 7.0.11, the attribute jakarta.servlet.request.X509Certificate is not set for some reason.")
 public class JmacHttpsTest {
     private static final String MYKS_PASSWORD = "httpspassword";
 
@@ -88,10 +87,12 @@ public static void prepareDeployment() throws Exception {
             "-validity", "7", "-keypass", MYKS_PASSWORD, "-keystore", myKeyStore.getAbsolutePath(), "-storepass",
             MYKS_PASSWORD);
 
-        // TODO: Is it required? Or the client certificate should be completely checked just by the auth module?
         KEYTOOL.exec("-importkeystore", "-srckeystore", myKeyStore.getAbsolutePath(), "-srcstorepass", MYKS_PASSWORD,
             "-destkeystore", GlassFishTestEnvironment.getDomain1Directory().resolve(Paths.get("config", "cacerts.jks"))
                 .toFile().getAbsolutePath(), "-deststorepass", "changeit");
+
+        // Default is false, required to set the client certificate to the context.
+        ASADMIN.exec("set", "configs.config.server-config.network-config.protocols.protocol.http-listener-2.ssl.client-auth-enabled=true");
         ASADMIN.exec("restart-domain", "domain1");
 
         JavaArchive loginModule = ShrinkWrap.create(JavaArchive.class).addClass(HttpsTestAuthModule.class);
@@ -123,6 +124,8 @@ public static void prepareDeployment() throws Exception {
     public static void cleanup() {
         ASADMIN.exec("undeploy", APP_NAME);
         ASADMIN.exec("delete-message-security-provider", "--layer", "HttpServlet", AUTH_MODULE_NAME);
+        ASADMIN.exec("set", "configs.config.server-config.network-config.protocols.protocol.http-listener-2.ssl.client-auth-enabled=false");
+
         delete(warFile);
         delete(keyFile);
         delete(loginModuleFile);
@@ -132,16 +135,20 @@ public static void cleanup() {
     @Test
     void test() throws Exception {
         HttpsURLConnection connection = openConnection(true, 8181, "/" + APP_NAME + "/index.jsp");
-        SSLContext sslContext = SSLContext.getInstance("TLS");
+        SSLContext sslContext = SSLContext.getInstance("TLSv1.3");
         KeyManagerFactory keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
         keyManagerFactory.init(KeyTool.loadKeyStore(myKeyStore, MYKS_PASSWORD.toCharArray()), MYKS_PASSWORD.toCharArray());
         sslContext.init(keyManagerFactory.getKeyManagers(), new TrustManager[] {new TestTrustManager()}, null);
         SSLSocketFactory sslSocketFactory = sslContext.getSocketFactory();
         connection.setSSLSocketFactory(sslSocketFactory);
         connection.setHostnameVerifier(new NaiveHostnameVerifier());
         connection.setDoOutput(true);
+        connection.setDoInput(true);
         connection.setInstanceFollowRedirects(false);
-        assertThat(connection.getResponseCode(), equalTo(200));
+        assertAll(
+            () -> assertThat("HTTP response code", connection.getResponseCode(), equalTo(200)),
+            () -> assertThat("HTTP response content length", connection.getContentLength(), equalTo(-1))
+        );
         try (InputStream is = connection.getInputStream()) {
             String text = new String(is.readAllBytes(), UTF_8);
             assertThat(text, stringContainsInOrder("Hello World from 196 HttpServlet AuthModule Test!", //