[ELY-2254] Provide a LoginModule compatible security realm. #1623

Skyllarr · 2021-11-15T09:43:57Z

https://issues.redhat.com/browse/ELY-2254

Skyllarr · 2021-11-16T09:49:09Z

auth/realm/base/src/main/java/org/wildfly/security/auth/realm/JaasSecurityRealm.java

+            // map all subject's principals to attributes with the following rule:
+            // key of the attribute is principal's simple classname and the value is principal's name
+            for (Principal principal : subject.getPrincipals()) {
+                attributes.addLast(principal.getClass().getSimpleName(), principal.getName());


@darranl Just FYI, this is how I am currenly mapping subject's principal list into attributes of SecurityIdentity. It is like you mentioned on the meeting - attribute key is simple classname of Principal and attribute value is principal's name.

+1 Most importantly here they have a predictable way to view information about the principals in the resulting attributes.

Skyllarr · 2021-11-16T09:57:59Z

tests/base/src/test/java/org/wildfly/security/auth/TestLoginModule.java

+        this.subject.getPrincipals().add(new AnonymousPrincipal());
+        this.subject.getPrincipals().add(new Roles("Admin"));
+        this.subject.getPrincipals().add( new Roles("User"));
+        this.subject.getPrincipals().add(new Roles("Guest"));


@darranl Each role must then be placed into separate principal.

It might make sense to map principal with classname "Role" (without plural) into "Roles" attribute, but I though it might be confusing and would need to be documented.

+1 on this being documented, I think the use if plural / singular attribute names will just be a side effect of the mapping. Otherwise we could come up with a more complex mapping configuration but I don't think we would benefit from that really most importantly the attribute names are predictable.

Skyllarr · 2021-11-16T10:03:13Z

tests/base/src/test/java/org/wildfly/security/auth/TestLoginModule.java

-     * A {@code Group} implementation used in the tests to store the caller principal.
-     */
-    private class TestGroup implements Group {
+    private static class Roles implements Principal {


@darranl I do not expose this class anywhere, users can create their own principal classes that will be mapped to attribute keys.

+1 this is just for our tests.

Skyllarr · 2021-11-16T10:08:42Z

auth/realm/base/src/main/java/org/wildfly/security/auth/realm/JaasSecurityRealm.java

    }

    @Override
    public RealmIdentity getRealmIdentity(final Principal principal) {
-        return principal instanceof NamePrincipal ? new JaasRealmIdentity(principal) : RealmIdentity.NON_EXISTENT;
+        return new JaasRealmIdentity(principal);


@darranl I do not map any specific subject's principal to be the main "caller" principal. The main caller principal is the one that was passed into the realm for authentication.

+1 that sounds good so that is not a problem that needs to be resolved as we already have the Principal identified.

darranl

I have added some discussion points Diana.

It is great that this looks like it slots into the Elytron SPIs without too much special configuration or behaviour needed. One point I flagged up to discuss is just double checking the SPI changes.

Maybe once @fjuma has had a chance to review we could discuss these last points tomorrow?

darranl · 2021-11-16T11:31:32Z

auth/realm/base/src/main/java/org/wildfly/security/auth/realm/JaasSecurityRealm.java

     */
-    public JaasSecurityRealm(final String loginConfiguration) {
-        this(loginConfiguration, null);
+    public JaasSecurityRealm(final String jaasConfigFilePath, ClassLoader classLoader) {


One point on this class, I think we should double check if this was a part of public API or was always private API - if it was public we may need to fork it instead.

If this is not being picked up by JAPICMP it likely was private.

It looks like this has always been private.

I have put this constructor back to the class. I realized a person might not want to change the classloader and so classloader can be null.

darranl · 2021-11-16T11:32:05Z

auth/realm/base/src/main/java/org/wildfly/security/auth/realm/JaasSecurityRealm.java

    }

    @Override
    public RealmIdentity getRealmIdentity(final Principal principal) {
-        return principal instanceof NamePrincipal ? new JaasRealmIdentity(principal) : RealmIdentity.NON_EXISTENT;
+        return new JaasRealmIdentity(principal);


+1 that sounds good so that is not a problem that needs to be resolved as we already have the Principal identified.

darranl · 2021-11-16T11:33:44Z

auth/realm/base/src/main/java/org/wildfly/security/auth/realm/JaasSecurityRealm.java

+            for (Object credential : subject.getPrivateCredentials()) {
+                if (credential instanceof Credential) {
+                    privateCredentials.add((Credential) credential);
+                }


If we are dropping credentials that are not the correct type maybe we should log at TRACE level to identity the class names of the ones being dropped.

darranl · 2021-11-16T11:36:18Z

auth/realm/base/src/main/java/org/wildfly/security/auth/realm/JaasSecurityRealm.java

+            // map all subject's principals to attributes with the following rule:
+            // key of the attribute is principal's simple classname and the value is principal's name
+            for (Principal principal : subject.getPrincipals()) {
+                attributes.addLast(principal.getClass().getSimpleName(), principal.getName());


+1 Most importantly here they have a predictable way to view information about the principals in the resulting attributes.

darranl · 2021-11-16T11:36:36Z

auth/realm/base/src/main/java/org/wildfly/security/auth/realm/ObjectCallback.java

@@ -0,0 +1,40 @@
+package org.wildfly.security.auth.realm;


Copyright header ;-)

Fixed by removing of the ObjectCallback .

darranl · 2021-11-16T11:59:22Z

auth/server/base/src/main/java/org/wildfly/security/auth/server/RealmIdentity.java

@@ -268,6 +269,14 @@ default Attributes getAttributes() throws RealmUnavailableException {
        return null;
    }

+    default Set<Credential> getPublicCredentials() {


If we add to RealmIdentity it should contain javadoc to describe the purpose, within this we should also make sure the contract is clear as to if these methods should return null or an empty collection - the latter would allow callers to skip the null check.

I would suggest if an API change is needed here it is checked in relation to the token authentication work being undertaken, that may also be adding credentials to the identity based on interaction with the realm so probably worth a double check.

Just FYI, some of the draft token authentication work can be found here:

#1541

Looks like we were using SecurityIdentity.withPrivateCredential() there, there wasn't interaction with the realm.

@darranl @fjuma I added javadoc to these methods and made them return IdentityCredentials.NONE instead of null.

Btw I am not sure anymore whether the credentials from the subject are useful for us to propagate to security identity? I added it here just to preserve all the information I can from the subject. But I am not sure if these private and public credentials have its use case once they are put in the security identity.

UPDATE - we decided not to propagate the credentials from the subject to the security identity. Reason is that private / public credentials are tied with the mechanism in elytron, so we decided not to change Elytron API for this currently. If it will be needed in the future we can add it or use subject in some sort of attachment.

darranl · 2021-11-16T12:01:42Z

.../server/base/src/main/java/org/wildfly/security/auth/server/ServerAuthenticationContext.java

+            Set<Credential> privateCredentialsFromRealmIdentity = realmIdentity.getPrivateCredentials();
+            if (privateCredentialsFromRealmIdentity != null) {
+                for (Credential credential : privateCredentialsFromRealmIdentity) {
+                    authorizedIdentity = authorizedIdentity.withPrivateCredential(credential);


Each iteration of the for loop will result in a new SecurityIdentity being created and one more being left for GC. Could this be reduced by converting to an IdentityCredentials first?

darranl · 2021-11-16T12:05:06Z

tests/base/src/test/java/org/wildfly/security/auth/TestLoginModule.java

+        this.subject.getPrincipals().add(new AnonymousPrincipal());
+        this.subject.getPrincipals().add(new Roles("Admin"));
+        this.subject.getPrincipals().add( new Roles("User"));
+        this.subject.getPrincipals().add(new Roles("Guest"));


+1 on this being documented, I think the use if plural / singular attribute names will just be a side effect of the mapping. Otherwise we could come up with a more complex mapping configuration but I don't think we would benefit from that really most importantly the attribute names are predictable.

darranl · 2021-11-16T12:05:29Z

tests/base/src/test/java/org/wildfly/security/auth/TestLoginModule.java

-     * A {@code Group} implementation used in the tests to store the caller principal.
-     */
-    private class TestGroup implements Group {
+    private static class Roles implements Principal {


+1 this is just for our tests.

darranl · 2021-11-16T12:09:09Z

auth/realm/base/src/main/java/org/wildfly/security/auth/realm/ObjectCallback.java

+ * @author [email protected]
+ * @version $Revision$
+ */
+public class ObjectCallback implements Callback {


I think supporting this callback may be worth discussing more, as a new callback existing login modules will not be using this - maybe for now we would be better sticking with the PasswordCallback and CredentialCallback support - at least the former of the two should be a standard type?

fjuma · 2021-11-16T16:10:40Z

auth/realm/base/src/main/java/org/wildfly/security/auth/realm/JaasSecurityRealm.java

-     * @param handler the JAAS callback handler to use
+     * @param jaasConfigFilePath path to JAAS configuration file
+     * @param entry              login configuration file entry
+     * @param classLoader        classLoader to use with LoginContext, this class loader must contain LoginModule CallbackHandler classes


Would be good to mention in the javadoc for each param whether or not null is allowed (here and in the other constructors too).

wildfly-ci · 2021-11-25T08:19:44Z

Linux - JDK11 EA 28 Build 827 outcome was FAILURE using a merge of 8d0ce4b
Summary: Exit code 1 (Step: Maven) (new) Build time: 00:00:11

wildfly-ci · 2021-11-25T08:19:51Z

Windows Build 807 outcome was FAILURE using a merge of 8d0ce4b
Summary: Exit code 1 (Step: Maven) (new) Build time: 00:00:18

Skyllarr · 2021-11-25T10:59:29Z

Current failures in CI are unrelated to this PR and are addressed by ELY-2259 #1628

Skyllarr mentioned this pull request Nov 15, 2021

[WFCORE-5483] Provide a LoginModule compatible security realm. wildfly/wildfly-core#4867

Closed

Skyllarr commented Nov 16, 2021

View reviewed changes

Skyllarr force-pushed the eap7-1741-temp branch from b16506e to efbe1f4 Compare November 16, 2021 11:59

darranl reviewed Nov 16, 2021

View reviewed changes

fjuma reviewed Nov 16, 2021

View reviewed changes

Skyllarr force-pushed the eap7-1741-temp branch 2 times, most recently from eebf4fd to de170c8 Compare November 18, 2021 02:33

Skyllarr force-pushed the eap7-1741-temp branch from de170c8 to 8d0ce4b Compare November 25, 2021 08:16

[ELY-2254] Provide a LoginModule compatible security realm.

0b4aeb9

Skyllarr force-pushed the eap7-1741-temp branch from 8d0ce4b to 0b4aeb9 Compare November 25, 2021 15:02

fjuma approved these changes Nov 25, 2021

View reviewed changes

fjuma added the +1 FJ label Nov 25, 2021

darranl approved these changes Nov 25, 2021

View reviewed changes

darranl merged commit f5ed356 into wildfly-security:1.x Nov 25, 2021

fjuma mentioned this pull request Jul 26, 2023

[ELY-2544] [ELY-2254] Provide a LoginModule compatible security realm. #1931

Merged

ivassile mentioned this pull request Oct 5, 2023

[ELY-2254] Provide a LoginModule compatible security realm. #1992

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ELY-2254] Provide a LoginModule compatible security realm. #1623

[ELY-2254] Provide a LoginModule compatible security realm. #1623

Skyllarr commented Nov 15, 2021

Skyllarr Nov 16, 2021

darranl Nov 16, 2021

Skyllarr Nov 16, 2021

darranl Nov 16, 2021

Skyllarr Nov 16, 2021

darranl Nov 16, 2021

Skyllarr Nov 16, 2021

darranl Nov 16, 2021

darranl left a comment

darranl Nov 16, 2021

fjuma Nov 16, 2021

Skyllarr Nov 17, 2021

darranl Nov 16, 2021

darranl Nov 16, 2021

darranl Nov 16, 2021

darranl Nov 16, 2021

Skyllarr Nov 17, 2021

darranl Nov 16, 2021

fjuma Nov 16, 2021 •

edited

Loading

Skyllarr Nov 17, 2021 •

edited

Loading

darranl Nov 16, 2021

darranl Nov 16, 2021

darranl Nov 16, 2021

darranl Nov 16, 2021

fjuma Nov 16, 2021 •

edited

Loading

wildfly-ci commented Nov 25, 2021

wildfly-ci commented Nov 25, 2021

Skyllarr commented Nov 25, 2021 •

edited

Loading

[ELY-2254] Provide a LoginModule compatible security realm. #1623

[ELY-2254] Provide a LoginModule compatible security realm. #1623

Conversation

Skyllarr commented Nov 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darranl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

Skyllarr Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

wildfly-ci commented Nov 25, 2021

wildfly-ci commented Nov 25, 2021

Skyllarr commented Nov 25, 2021 • edited Loading

fjuma Nov 16, 2021 •

edited

Loading

Skyllarr Nov 17, 2021 •

edited

Loading

fjuma Nov 16, 2021 •

edited

Loading

Skyllarr commented Nov 25, 2021 •

edited

Loading