Eek/fix/windows login module

[eekodeerder]

Trying to understand the actual user and roles in the test environments.

[eekodeerder]

The Principals on the Subject interface is supposed to provide the principals of the Subject (the person logged in). "Principals simply bind names to a Subject" (as stated by the JAAS specification of Subject). This should be a Set containing the User identification (by name), including roles (by name) or groups (by name) in which the user can operate or belongs to.
For Waffle, these roles are supposed to be (according to documentation) the Windows groups to which the subject (user or computer) belongs. In this case the "name" used is either the FQN or the SID of the user and/or Windows security group. The Principals returned by the WindowsLoginModule should therefore be of type (interface) Principal, for which getName should return a name of the user or a name of a Windows group to which the user belongs.

The line of code which was deleted by the breaking change and which I have put back, is the line that places the Windows Groups (names) as "roles" in the Subject's Principals list. JAAS only knows UserPrincipals and RolePrincipals.

The existing code for WindowsLoginModule (since October 2016) does not bind the Windows (or Active DIrectory) group (names) to the Subject. It binds an object named "Roles" to the Subject. It therefore does not fulfill the basic function required of a JAAS WindowsLoginModule.

The packaging into a GroupPrincipal (which did not exist before the breaking change), was made to get it to work with WildFly (earlier versions as it does not work with latest versions in any case). Every Windows Group to which the User belongs, was placed into a new list of Principals (named "Roles") which was then packaged as a Principal (the GroupPrincipal) and added to the Subject.Principals list. This is just plain wrong, as it assumes that any user of the subject will know about the custom "Roles", and be able to extract the Principals from this list, This assumption is obviously invalid. The JAAS specified way is to obtain it from the Subject.Principles.getName(). The code modification also removed the ability to obtain the Windows Group (or JAAS roles) from the Subject.Principles.getName() as specified by JAAS.

The only reason I otiginally left the "duplication" code in there (including the definition of a GroupPrincipal), is because I do not know who is dependent on this incorrect implementation. Removing it is the correct action, but may break something somebody else implemented somewhere relying on this incorrect and JAAS non-compliant implementation. I have now also removed that

I am new to gitHub and the Waffle project and I am still trying to find the ropes. The unit tests failed, returning more Principals than expected by the test routines, even for the Guest. I need some time to work through the test environment setup to determine what should be expected (which Users and more importantly Windows Group memberships) should be expected from the test environment in order for me to fix the tests.

I have therefore temporarily closed the pull request, until I can figure out what Windows users and Windows security Groups are being returned by the test environments, in order for me to modify the tests to pass. This will take some time, as I am not familiar with the test environments. The call to authenticate the user returns the information (both FQN and SID) from some directory, which contains the user and group information. I need to know how this directory is populated.

The difference between the RolePrinciple and UserPrinciple from what I could gather is just for classification in the realm. For example: in the Tomcat JAASRealm, you specify which classes (by name) implementing Principal, to classify as Users and which as Roles. According to the Tomcat documentation, you specify:

<Context>
  <Realm className="org.apache.catalina.realm.JAASRealm"
         appName="Jaas"
         userClassNames="waffle.jaas.UserPrincipal"
         roleClassNames="waffle.jaas.RolePrincipal"
         useContextClassLoader="false"
         debug="true" />
</Context>

The earlier modification created a seperate LIST which it then added as a new "GroupPrincipal". It therefore removed the RolePrinciples from the Principles and placed it into a new List "Group"Principle, which is not understood by Tomcat (and as a matter of fact by any JAAS compliant realm). The correct "fix" would be to revert to the 1.7.x version, but as I said, I do not know who is dependent on the incorrect implementation. I do know though that the breaking change was made to get it to work with WildFly somehow, but this does not even work with WildFly anymore.

Just to elaborate: the Subject.Principals with the current code returns a Principal named "Roles", which in effect means the subject has an identity named "Roles" (either the user's name is "Roles", or the subject can operate in the role named "Roles", or the subject belongs to a group named "Roles", or the subject has a privilege named "Roles").

In essence what is fixed by the modification, is the link between the Windows Group FQN or SID and the JAAS Principal Name.
The previous modification removed this link by hiding the Windows Group FQN and SID inside a list with the name "Roles". The effect of the is that calling getPrinciples on the Subject interface, fails to return the Windows Groups as Principles, which is the core advertised feature of Waffle JAAS. Instead, it returns the "Roles" object as one of its Principles.

Here is the Tomcat JAASRealm logs when receiving the Subject on the 1.8.x and later versions of the WindowsLoginModule :

12-Dec-2020 10:15:12.959 FINE [http-nio-8080-exec-26] org.apache.catalina.realm.JAASRealm.authenticate JAAS LoginContext created for username [PARSEC\freek]
12-Dec-2020 10:15:12.959 FINE [http-nio-8080-exec-26] org.apache.catalina.realm.JAASRealm.createPrincipal Checking Principal [waffle.jaas.UserPrincipal@20e74b3] [waffle.jaas.UserPrincipal]
12-Dec-2020 10:15:12.959 FINE [http-nio-8080-exec-26] org.apache.catalina.realm.JAASRealm.createPrincipal Principal [PARSEC\freek] is a valid user class. We will use this as the user Principal.
12-Dec-2020 10:15:12.959 FINE [http-nio-8080-exec-26] org.apache.catalina.realm.JAASRealm.createPrincipal Checking Principal [waffle.jaas.UserPrincipal@d5306624] [waffle.jaas.UserPrincipal]
12-Dec-2020 10:15:12.959 FINE [http-nio-8080-exec-26] org.apache.catalina.realm.JAASRealm.createPrincipal Checking Principal [Roles(members:waffle.jaas.RolePrincipal@ab1ba21b,waffle.jaas.RolePrincipal@4407ce47,waffle.jaas.RolePrincipal@4a7052f4,waffle.jaas.RolePrincipal@729c2796,waffle.jaas.RolePrincipal@874cee86,waffle.jaas.RolePrincipal@1ed843d3,waffle.jaas.RolePrincipal@9ee34828,waffle.jaas.RolePrincipal@7f4d8ec3,waffle.jaas.RolePrincipal@e0f037be,waffle.jaas.RolePrincipal@76046155,waffle.jaas.RolePrincipal@be714aa0,waffle.jaas.RolePrincipal@43849950,waffle.jaas.RolePrincipal@d7973642,waffle.jaas.RolePrincipal@2c6cf27b,waffle.jaas.RolePrincipal@66d0628a,waffle.jaas.RolePrincipal@73061cdd,waffle.jaas.RolePrincipal@1359d682,waffle.jaas.RolePrincipal@88cbcad5,waffle.jaas.RolePrincipal@2e65c945,waffle.jaas.RolePrincipal@504fdeae,waffle.jaas.RolePrincipal@14a9058b,waffle.jaas.RolePrincipal@f768724a,waffle.jaas.RolePrincipal@d5a6df84,waffle.jaas.RolePrincipal@be18043a,waffle.jaas.RolePrincipal@908c5ae8,waffle.jaas.RolePrincipal@8db8745f,waffle.jaas.RolePrincipal@60e37cca,waffle.jaas.RolePrincipal@dc67a34c,waffle.jaas.RolePrincipal@a64190ac,waffle.jaas.RolePrincipal@9096f7f8,waffle.jaas.RolePrincipal@5eb4309a,waffle.jaas.RolePrincipal@194c117c,waffle.jaas.RolePrincipal@e3ed649e,waffle.jaas.RolePrincipal@8e915ba2,waffle.jaas.RolePrincipal@6a7dc2ce,waffle.jaas.RolePrincipal@d5db2e6f,waffle.jaas.RolePrincipal@205276a7,waffle.jaas.RolePrincipal@c79e0cf,waffle.jaas.RolePrincipal@74e3d69c,waffle.jaas.RolePrincipal@faba74c5)] [waffle.jaas.GroupPrincipal]
12-Dec-2020 10:15:12.959 FINE [http-nio-8080-exec-26] org.apache.catalina.realm.JAASRealm.createPrincipal No valid role Principals found.

I hope this better explains what is wrong and why the code fails dismally even in Tomcat.

I do need some help with a coverage error I am getting.

[dblock]

I think I understand :) @hazendaz should take a look and merge/comment

[eekodeerder]

I think I understand :) @hazendaz should take a look and merge/comment

I believe the same problem exists in the waffle-jna-jakarta implementation as well, but I have not modified that implementation. I know the current impementation does not work in Tomcat, whilst versions before October 2016 did work as advertised. I traced the problem to the JAAS incompatible modification that was made. I believe if such a change was required for WildFly, it should have been made into a different component and the existing working implementation should not have been broken.

[hazendaz]

The jakarta module is a copy of the jna for usage on jakarta ee9 package renames. Likely not used yet and there are some other recent changes that need applied there still so I can address that portion before I push a release as I already have to compare anyways. I doubt anyone is actually using the jakarta package renamed module at this time since ee9 just released and I think only tomcat 10 supports it right now.

[hazendaz]

coveralls issue can be safely ignored. Unrelated problem still being researched.

[hazendaz]

@FreekEek Sorry I was only following behind the scenes here. I know you mentioned somewhere the original PR that brought in the issue, can you point that out again here? The change here looks good, in fact the casting otherwise here seems to not pass the smell test. Before I merge, I just want to take a more in depth look at what was originally being done.

[eekodeerder]

The commit that broke the code was b421035.
The PR that broke it was #397.
The PR that "broke" the tests was #418.

[hazendaz]

See #232 that touches on this subject.

[hazendaz]

I guess you were already looking there. Would it make sense to keep the wildfly 10 implementation and just add that line back for at least one release here so we can properly mark it deprecated and then remove it after?

[hazendaz]

To add to that, what I would mean is keep this PR open but create a new branch and just fix so co-support is there and at same time deprecate the internal class that is intended to be removed. Adjust the test cases as would have been anways to check both. Then I can do a release with that being clearly called out in case anyone is still using the not so old wildfly. Then in the next release, this PR would basically come in removing all that (after a rebase of course).

[eekodeerder]

I have created a new branch and modified the code to support both the GroupPrincipals as well as the correct RolePrincipal handling, marking GroupPrincipal as deprecated and modifying the tests to pass if the correct UserPrincipals and RolePrincipals are present, regardless of whether or not a GroupPrincipal is also present.

However, I do not know how to create a new pull request off this branch. When I try to create it, it shows me the modifications in the other branch.

[hazendaz]

@eekodeerder Now with the other patch on master, can you run a new build and confirm everything is working for you? Provided all is working, I plan to release in next couple days. I'll follow up right after with the removal of deprecated logic.

[eekodeerder]

I finally managed to get a merge done. Had to make changes due to isSerializable tests using dangerous Java code and generating errors.

[hazendaz]

@eekodeerder Thanks for figuring out the error prone issue. I hadn't looked and thought it was due to regression resulting in them releasing a cut the next day. I suspect longer term those tests could be moved out to use javabean-tester now that I'm seeing them which would remove them from this code base directly.

[hazendaz]

@eekodeerder I'm going to squash only due to merges in this. That will cause you a bit of hassle when you pull code back so I just wanted to make you aware.

Thanks for following up on this change. Appreciate the good work you are putting in here.

[hazendaz]

To add to that, when you pull code again, best to run as rebase - git pull --rebase upstream master. That will cause merge conflicts due to the squash which you can then just skip (git rebase --skip). Then your copy should be all set. Again appreciate it and extra thanks for figuring out error prone situation.