Fix incorrect CasProfile caching between different OAuth 2.0 services

[gagarski]

This PR fixes incorrect CasProfile caching when doing two subsequent authentications into different services.

Let's consider two services:

• A that is allowed to release attributes a1, a2
• B that is allowed to release attributes a1, a2, a3

Let's consider that user is also logged into CAS (for simplicity), i. e. he has TGT.

Now let's consider this user initiating authorization code flow by going to /oauth2.0/authorize endpoint with service A credentials. User's browser goes through the following redirects:

• /oauth2.0/callbackAuthorize/
• /login/
• /oauth2.0/authorize
• supplied callback URL

So far so good. But the bad thing is that now a pac4j profile is attached to a user session and this profile contains only a1 and a2 attributes. I believe the profile is added by this interceptor. This profile is bound to CasOAuthClient client (and it is not aware of a specific OAuth 2.0 service).

Now the bad things happen.

Let's consider the same user initialing authorization code flow by going to /oauth2.0/authorize endpoint with service B credentials. Now he already have a pac4j session, so authorize endpoint immediately produces OC ticket with only a1 and a2 attributes (a3 is missing). Therefore ATs created from this OC also miss a3. If B relies on a3 attribute for authentication then he can't perform it.

If you do these steps vice-versa, this also happens but now A can release more attributes than it is allowed.

This PR tries to fix it.

[leleuj]

Yes, good point. As soon as the profile is in the session, it will be reused keeping its existing attributes.
There is a security concern here (the second scenario you mention) if you can get more attributes than the service is allowed to.
I remember creating a DirectCasClient which has the same behavior as the CasClient but without keeping the profile in session. It's used somewhere else and at first, I thought we should use it here also. Yet, I fear it may break things (especially this one: DelegatedAuthenticationSAML2ClientLogoutAction although I doubt it works well in a clustered environment).

Your solution looks to be a good one: attach the user profile to the client_id and reuse it only for the same client_id. It must be tested in depth though.

[leleuj]

I think it would make more sense to use the authentication attributes here instead of the user attributes.

[gagarski]

You're right. I've changed it.

[leleuj]

I think it would make more sense to use the authentication attributes here instead of the user attributes.

[gagarski]

I remember creating a DirectCasClient which has the same behavior as the CasClient but without keeping the profile in session.

I am not sure I understand how it is supposed to work.

Now during the redirect chain we go twice through /ouaht2.0/callback: first time being "unauthenticated" (in terms of oauth) and second time with stored session. When the user is not authenticated in CAS, the user also should type his credentials between these requests. This redirect chain relies on the stored session, I am not sure how is it possible to remove it (I had this idea too initially).

[leleuj]

@gagarski About the DirectCasClient, it will do the same thing as the CasClient: redirect the unauthenticated user for login and finish the login process on the callback URL. The only difference is that it won't store the profile into the web session, only in the HTTP request.

[leleuj]

We're good for the merge, except that you must sync with the 5.3.x branch and open the same PR for the 6.0.x and master branches

[mmoayyed]

We cannot merge this just yet.

• I was unable to duplicate the problem against master, 6.0.x or 5.3.x. So, before any merge is to happen, could you please upload a few sample projects that duplicate the issue?
• This pull request should also be updated with tests that show the failure and the fix, before and after so we can better examine and understand the root cause.

[mmoayyed]

PS Or perhaps, some sample configurations for both services that demonstrate different attribute release policies would be good, to run some independent checks.

[gagarski]

OK, I've made some simple zero-configuration example based on latest cas-overlay-template (with CAS 6.1.0-RC1).

I have two services here (see etc/cas/services):

• A that is allowed to release attrs a, b, c
• B that is allowed to release attrs a and b

The values for these attrs are set by stub attr repo (see etc/cas/cas.properties)

Here are the steps to reproduce:

0. ./gradlew build
   0.5. You do not have to copy anything to /etc or anywhere, just stay in cas-overlay-template directory.
1. Run java -Djavax.net.ssl.trustStore=/full/path/to/your/cas-overlay-template/etc/cas/thetruststore -Djavax.net.ssl.trustStorePassword=changeit -jar build/libs/cas.war (we have to add self-signed cert to CAS trust store so CAS trusts itself)
2. In your browser go to https://localhost:8443/cas/oauth2.0/authorize?client_id=aid&response_type=code&redirect_uri=http://localhost:8000/callback (start authn for A)
3. Then go to https://localhost:8443/cas/oauth2.0/accessToken?client_id=aid&client_secret=asecret&grant_type=authorization_code&redirect_uri=http://localhost:8000/callback&code={THE_OC_OBTAINED_IN_STEP1}. Save the acquired AT for later. (you have to act quickly here)
4. Go to https://localhost:8443/cas/oauth2.0/authorize?client_id=bid&response_type=code&redirect_uri=http://localhost:9000/callback (start authn for B)
5. Go to https://localhost:8443/cas/oauth2.0/accessToken?client_id=bid&client_secret=bsecret&grant_type=authorization_code&redirect_uri=http://localhost:9000/callback&code={THE_OC_OBTAINED_IN_STEP4}. Save the acquired AT for later. (you have to act quickly here)
6. Go to https://localhost:8443/cas/oauth2.0/profile?access_token={THE_AT_OBTAINED_IN_STEP3}. Look at the profile. It has a, b and c attrs. It's OK.
7. Go to https://localhost:8443/cas/oauth2.0/profile?access_token={THE_AT_OBTAINED_IN_STEP5}. Look at the profile. It has a, b and c attrs. It's not OK. c should not be here.

You may restart CAS and conduct another experiment but with permuting steps. Do 1, 4, 5, 2, 3. In 6 and 7 you will see a and b attrs, but in 6 there should also be c.

[mmoayyed]

Thanks. Would you mind attaching that project with the config files here?

[gagarski]

@mmoayyed OOps, sorry, I forgot to attach the link: https://github.com/gagarski/cas-overlay-template

GitHub

gagarski/cas-overlay-template

Apereo CAS WAR Overlay template. Contribute to gagarski/cas-overlay-template development by creating an account on GitHub.

[mmoayyed]

Thanks again. I spoke to @dima767 about this as well, and he should be able to duplicate and confirm the issue before we move forward. Stay tuned please.

[mmoayyed]

@dima767 do you think you might be able to confirm the issue/solution by this Friday? Or do you need more time to push back the release date for 5.3.8?

[dima767]

@mmoayyed Should be able to. Working on it this morning. Give me few hours, plz

[dima767]

OK, with the provided overlay and steps described, was able to re-produce. CAS Commit Id: dce9d59d916788a7f76fad1492ba36aefc71f9ee

[mmoayyed]

Great. Can you verify the patch fixes this issue in 5.3.8?

[dima767]

Will do

[dima767]

Currently having a hard time running the overlay build against 5.3.8-SNAPSHOT. @gagarski Were you able to run the overlay against 5.3.8-SNAPSHOT

I'm getting Caused by: java.lang.VerifyError: class org.apereo.cas.web.view.ChainingTemplateViewResolver overrides final method initialize.()V at start up

[dima767]

FWIW...

[gagarski]

I've created an example for 5.3.8-SNAPSHOT, same repo, 5.3 branch (note that in uses Maven here as does the previous commit there), so the step 0 would be mvn clean package and path to war in step 1 would be target/cas.war

[dima767]

@gagarski 👍

[gagarski]

@mmoayyed

I thought about adding JUnit test that at least follows the chain of redirects and obtains OC so we can compare expected attrs with actual. However it seems like we have to recreate quite big part of CAS inside the test. I didn't find any inspiration examples among existing tests.

[dima767]

Or perhaps some kind of a functional automated test against the running CAS server, etc. But manual verification would suffice for now

[dima767]

OK, @mmoayyed , @gagarski

after merging the PR into 5.3.x, building and installing the snapshot locally, then building the overlay against 5.3.8-SNAPSHOT, running the resulting cas.war, after sitting there for about a minute, produces "SOE":

	... 1024 more
Caused by: java.lang.StackOverflowError
	at java.util.concurrent.ConcurrentHashMap.putVal(ConcurrentHashMap.java:1012)
	at java.util.concurrent.ConcurrentHashMap.putIfAbsent(ConcurrentHashMap.java:1535)
	at java.lang.ClassLoader.getClassLoadingLock(ClassLoader.java:463)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:404)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:411)
	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:349)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
	... 1017 more

Building against published SNAPSHOT (CAS Commit Id: d14532310734f74d2f6a4864316c1b6e95538e15) runs fine.

[gagarski]

Looks strange. I'll take a look at it tomorrow.
@dima767 Could you please tell me what's the proper way to test my local CAS build against the overlay? Should I run some command to publish CAS build to ~/.m2?

[dima767]

@gagarski It's basically ./gradlew install -x test from the HEAD of the branch you want to test (based off of 5.3.x) of course - that should install the SNAPSHOT of that build into ~/.m2

[dima767]

Nope, still getting the SOE (could be my env. - I have total of 8 GB of OS RAM, but that's still not good). Here's the sequence of git and gradle steps to install the version into ~.m2 to test against:

git checkout 5.3.x
git checkout -b gagarski-oauth_profile_caching
git pull https://github.com/gagarski/cas.git oauth_profile_caching
./gradlew -x test -x javadoc -x check install

then built the overlay against it (5.3.8-SNAPSHOT in ~/.m2`) and tried to run it.

If someone else could try it, or we just merge it.

[gagarski]

or we just merge it

No, this is a bad idea. I've managed to reproduce it on my pC but now I have no idea what's going on. I am investigating.

[gagarski]

Now I have overlay working with snapshot from Sonatype repo but not working (with SOE) even with my local 5.3.x branch builds. Still digging into the problem.

[gagarski]

My current suggestions is that Gradle ignores my local hacks (and probably yours too) to temporary switch from JDK 11 to JDK 8. Checking this suggestion now.

[dima767]

Note - 5.3.x must run on Java 8, not 11

[gagarski]

That's the command that created working local builds in ~/.m2: ./gradlew clean assemble install -x test -x javadoc -x check -Dorg.gradle.java.home=/usr/lib/jvm/java-8-jdk/.

Probably the thing that do the trick is assemble, setting JDK home is probably unnecessary unless it is not properly set in your environment.

[gagarski]

I was looking for other usages of ProfileManager in CAS and found a few usages related to OAUth/OIDC. Most of them are in controllers which is fine: after interceptors we have properly authenticated request and only one profile (since we do not use multiProfile).

One usage bothers be a bit:

https://github.com/apereo/cas/blob/5.3.x/support/cas-server-support-oidc/src/main/java/org/apereo/cas/oidc/web/OidcSecurityInterceptor.java#L40

But it still looks OK for me: we only remove a profile here. However it is possible to use client id aware profile manager here and make remove also client_id aware.

[mmoayyed]

That's the command that created working ...

Good enough for me. Thanks again.

However it is possible to use client id aware profile manager here and make remove also client_id aware.

Also sounds fine. Let's do that in a separate PR, if you are interested?

[gagarski]

Also sounds fine. Let's do that in a separate PR, if you are interested?

If it is OK as it is, let's leave it. If it does not work properly then we probably should do it here. @leleuj any thoughts about this?

[gagarski]

@mmoayyed It seems like you already ported in onto 6.0 and 6.1. Thanks!