Add possibility to flexible match OIDC post_logout_redirect_uri against configured logout url

[Interessierter]

Hello,

we're using CAS that is configured as central IDP in authorisation systems like Keycloak which sometimes host authorization data for a multitude of applications. When users logout, they send the logout to e.g. Keycloak which sends the logout to CAS with a post_logout_redirect_uri specific to this application. As we just have one Service in CAS for the authorization system we currently need to configure a all application specific logout URLs which are really a lot, this is not maintainable.

With the change of this pull request I've added the possibility that the given logout URLs can be matched more flexible against the configured URLs - like I did for example in the test included as a configured regular expression.

If you agree I would also like to port this feature to the 6.3 branch.

best regards,
christian

[welcome]

Thank you so much for opening your first pull request here!

[CLAassistant]

All committers have signed the CLA.

[mmoayyed]

Thank you for the pull request. Please review, and always make sure your pull request passes CI checks on your own fork. There are quite few styling violations here that would not pass CI.

[Interessierter]

thanks for the review! intention was to leave the currently functionality as-is while adding the possibility to configure the matching more flexible. If you're fine with always doing regex-matching that is fine with me as well - makes it an even simpler change :)

Could you advise please how to activate CI on my fork? I've activated the actions in the forked project but still after pushing the latest commit nothing is run automatically. I also cannot find any information on this in the contributions documentation: https://apereo.github.io/cas/developer/Contributor-Guidelines.html

[mmoayyed]

I am fine with regex-matching be the default in fact. Can't see an issue with it, but I'll ping @hdeadman to be sure.

And for CI, I suspect what you have to do is to modify the GH workflow configuration to look at your branch for the CI builds. Things like this:
https://github.com/Interessierter/cas/blob/oidc-logout-url-flexible-matching/.github/workflows/build.yml#L15

Set the branch to be yours, for all workflows and do test. When you're ready and everything is passing, please post here and we'll activate CI here. (This is specifically done to ensure CI here for the repo only triggers for things we want to pursue, so as to not hog the CI backlog for when we actually do need something to go through) For the time being, I'll activate CI here once so you can take a first stab at it.

[mmoayyed]

CI is now activated for one build: https://github.com/apereo/cas/actions

[codecov]

Codecov Report

Merging #5116 (7ceb632) into master (109adbf) will decrease coverage by 61.92205%.
The diff coverage is 0.00000%.

@@                  Coverage Diff                   @@
##                master       #5116          +/-   ##
======================================================
- Coverage     79.41449%   17.49244%   -61.92205%     
+ Complexity       14366        3364       -11002     
======================================================
  Files             2517        2517                  
  Lines            58206       58208           +2     
  Branches          4699        4699                  
======================================================
- Hits             46224       10182       -36042     
- Misses            9390       47311       +37921     
+ Partials          2592         715        -1877     

Flag	Coverage Δ	Complexity Δ
actuatorendpoint	?	?
attributes	?	?
audits	0.58927% <0.00000%> (-0.00002%)	0.00000 <0.00000> (ø)
authentication	?	?
cas	?	?
casconfiguration	5.94935% <0.00000%> (+0.02900%)	0.00000 <0.00000> (ø)
cassandra	?	?
couchbase	?	?
dynamodb	9.49869% <0.00000%> (?)	0.00000 <0.00000> (?)
ehcache	3.36723% <0.00000%> (-0.00011%)	0.00000 <0.00000> (ø)
expirationpolicy	0.84696% <0.00000%> (?)	0.00000 <0.00000> (?)
filesystem	10.50886% <0.00000%> (-0.08282%)	0.00000 <0.00000> (ø)
hazelcast	3.75206% <0.00000%> (-0.00012%)	0.00000 <0.00000> (ø)
ignite	2.93946% <0.00000%> (?)	0.00000 <0.00000> (?)
infinispan	2.73846% <0.00000%> (?)	0.00000 <0.00000> (?)
influxdb	0.54460% <0.00000%> (?)	0.00000 <0.00000> (?)
jdbc	?	?
jmx	?	?
logout	?	?
mail	?	?
mariadb	?	?
memcached	?	?
metrics	1.49979% <0.00000%> (?)	0.00000 <0.00000> (?)
mfa	?	?
mongodb	?	?
mssqlserver	?	?
mysql	?	?
oauth	?	?
oidc	?	?
oracle	?	?
passwordops	?	?
radius	?	?
redis	?	?
registeredservice	?	?
restfulapi	?	?
saml	?	?
simple	?	?
sms	?	?
spnego	?	?
uma	?	?
utility	?	?
web	?	?
webapp	?	?
webflow	?	?
webflowactions	?	?
webflowconfig	?	?
webflowevents	?	?
webflowmfaactions	?	?
wsfederation	?	?
x509	?	?
zookeeper	?	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...ntrollers/logout/OidcLogoutEndpointController.java	0.00000% <0.00000%> (-94.11764%)	0.00000 <0.00000> (-13.00000)
.../org/apereo/cas/oidc/config/OidcConfiguration.java	0.00000% <ø> (-95.00000%)	0.00000 <0.00000> (-74.00000)
...i/src/main/java/org/apereo/cas/util/JsonUtils.java	0.00000% <0.00000%> (-100.00000%)	0.00000% <0.00000%> (-4.00000%)
...y/src/main/java/org/apereo/cas/CasJettyBanner.java	0.00000% <0.00000%> (-100.00000%)	0.00000% <0.00000%> (-2.00000%)
.../src/main/java/org/apereo/cas/CasTomcatBanner.java	0.00000% <0.00000%> (-100.00000%)	0.00000% <0.00000%> (-2.00000%)
...rc/main/java/org/apereo/cas/CasUndertowBanner.java	0.00000% <0.00000%> (-100.00000%)	0.00000% <0.00000%> (-2.00000%)
...in/java/org/apereo/cas/audit/AuditableContext.java	0.00000% <0.00000%> (-100.00000%)	0.00000% <0.00000%> (-9.00000%)
.../java/org/apereo/cas/web/view/DynamicHtmlView.java	0.00000% <0.00000%> (-100.00000%)	0.00000% <0.00000%> (-2.00000%)
...a/org/apereo/cas/rest/BadRestRequestException.java	0.00000% <0.00000%> (-100.00000%)	0.00000% <0.00000%> (-1.00000%)
.../java/org/apereo/cas/DefaultMessageDescriptor.java	0.00000% <0.00000%> (-100.00000%)	0.00000% <0.00000%> (-1.00000%)
... and 2013 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 109adbf...7ceb632. Read the comment docs.

[Interessierter]

Hi Misagh, I fixed the styling problems (with some I would disagree but ok, its not my project) and an embarassing bug and now the CI is mostly fine: https://github.com/Interessierter/cas/actions (the test failures are unrelated to my changes: at the time running there seem to be a regression with the registeredservice-tests within the official master branch and there are some infrastructure-related failures for MacOS and couchbase I did not investigate) - could you have another look if this is fine now for you?

I did merge the changes from the feature branch into master branch for easily triggering the CI on my fork. Running the CI is actually quite useful (although it really runs forever: ;( ) and should be mentioned on the Contributor Guide (I wanted to add such section but this guide seems to be not part of this repo)

GitHub

Interessierter/cas

Apereo CAS - Enterprise Single Sign On for all earthlings and beyond. - Interessierter/cas

[mmoayyed]

could you have another look if this is fine now for you?

Thanks much. Make sure you have merged with the latest master branch. I'll take a look.

and should be mentioned on the Contributor Guide

It is. The CI system is Github Actions, and it's common sense for one to be familiar with GH actions and figure out where the workflow configuration and triggers are. We are not going to document how GH action works and what triggers are available and where the workflow files are, etc and how the rules work. GH does that. Having said that, if you prefer, you can always post a PR to the Apereo blog and document your experience for others.

(I wanted to add such section but this guide seems to be not part of this repo)

It is. See gh-pages branch.

[mmoayyed]

Please use the service matching strategy here. See #5123 as an example.

[Interessierter]

ok, made the changes you requested except that one with the matchingStrategy because it is already there (just one line above! otherwise you have to elaborate more about what you mean)

[mmoayyed]

There is:

val matchResult = registeredService.matches(postLogoutRedirectUrl)
 || urls.stream().anyMatch(url -> postLogoutRedirectUrl.matches(url));

Should be (pseudocode):

val matchResult = registeredService.matches(postLogoutRedirectUrl)
 || urls.stream().anyMatch(url -> registeredService.getMatchingStrategy().matches(url, postLogoutRedirectUrl));

Watch out for getMatchingStrategy which might be null.

[Interessierter]

why?!? OIDC RP-initiated logout is something different to the (assumed, this is very sparsely documented) purpose of org.apereo.cas.services.RegisteredServiceMatchingStrategy - I would have to make RegisteredServices from each of the configured logout URLs just to do this check for no good reason (this is different to #5123 where I fully agree that should be done with the matchingStrategy)

[mmoayyed]

The matching strategy of the service is and should be controlled via the RegisteredServiceMatchingStrategy API, and not an ad-hoc matches call. So it can be customizable and pluggable, and is consistent with how the matching strategy works for a service body. It's possible to imagine a matching strategy for logouts per service (or globally) but that would be overkill I think. So, we could defer to the matching strategy of service, and also work out a global matching strategy for all services so you wouldn't have to customize every application.

There is a possibility (though remote) that one would want to have a different matching strategy for logouts separate from what operates on redirect-uri, but I think that is overkill. So, let's stick with the matching strategy API for now, and possibly come up with a global option to control matching strategies.

Correction: Or do this as a custom patch in a way that works best for your use case.

[Interessierter]

I do not think this is wise, as a custom matching strategy will more likely fail on completely artifical RegisteredServices (remember: I need to create one for each configured logoutURL just for this) but OK, as you wish, have pushed the requested changes

[mmoayyed]

more likely fail on completely artificial RegisteredServices

Could you elaborate please? I am not sure I follow.

What I have in mind is, conceptually you have a service that does matching operations. The purpose of the matching API is to exactly handle this sort of thing, and allow customizations and injections on a per application basis. You can of course decide whether the match would operate on the full text with matches or be partial via find or something else entirely. I dont follow the failure scenarios here by using this api.

The danger with using this API is that a deployment may want to use a "full" matching strategy on redirect-uris, and yet use some other custom strategy for logout uris. That's what I was referring to as "overkill" and possibly very suspicious. This does not sound like something you have to work with, and certainly not something we'd want to support (at least not yet!)

And yes, you have to do the customization for every application to define your own matching strategy. If you have a large number of services, this would be impractical. All true. So I was suggesting that we keep the "per service" option and use the API that is available for now, and then fall back onto creating a more global option, so you would not have to create this matching strategy for every service every time. This is also consistent with how everything else works with CAS; There is a global option, and then there is the ability to override it on a per application basis. In this case, we just have the per-application bit. The global stuff would come in later, and I am happy to work with you on that, possibly in a separate PR to handle the global route. Or if you prefer, you can punt that over to me.

And the global option would be something like: cas.service-registry.matching-strategy=XYZ. Or some such.

I think this works, but please let me know if I am missing something.

[Interessierter]

kindly have a look at my latest commit (3fd4f4f) , i think this illustrates it: In order to do the configuredLogoutURL.matches(postLogoutRedirectUrl) check with the configured MatchingStrategy of the service in question I have to create an artificial RegisteredService (RS) because that is what the API is (which is ok because it was, like i previously mentioned, presumably designed for something different). So now I use a matching strategy from a RS to match another (the artificially created) RS - this begs for problems.

IMHO you should withdraw your request of matching the postLogoutRedirectUrl with the matchingStrategy of the RegisteredService, because like I said, it is something different (e.g. to #5123 which is fully in line with the design intention of the strategy). Unless there is really a possibility to configure the logoutURL-matching differently it makes (IMHO) no sense at all to use the RS.matchingStrategy for this. So until then it should be just

val matchResult = registeredService.matches(postLogoutRedirectUrl)
 || urls.stream().anyMatch(url -> postLogoutRedirectUrl.matches(url));

again.

[mmoayyed]

Thanks for the notes. It does clearly illustrate the issue.

What you have in the patch is really a non-issue, because of course the matching API can be very slightly and humbly adjusted to not require a "pointless" service. That said, I do see your point and would have no problem reverting back to what was the original change, not because it's inapplicable but mainly because the solution is almost identical as what exists now and I see the argument for not complicating matters any more. It would be great to have others like @hdeadman or @leleuj chime in. I appreciate your patience here; let's pause for the time being. Thanks for your patience.

[hdeadman]

I am not sure I understand all the issues being discussed here but if the allowed logout URL is changing from an exact URL to a regular expression, would that allow a case where someone has configured an allowed logout URL of https://www.google.com but then this change would allow me to register https://www-google.com and then induce people to click on a trusted link that redirected to my malicious domain? I think its good to support it regular expressions but should it be an option or just documented in the release notes?

[Interessierter]

Thanks for your input! The scenario you described IMHO isnt possible because before the OIDC postLogoutRedirectUrl (PLRU) is checked against the configured logoutUrl (LU) the OIDC IdToken is checked and the service that the LU are retrieved from is determined from the IdToken, not just the PLRU (i.e. you just cannot easily "take over" another service).

@mmoayyed now how to proceed?
What about the following options:

1. accept my initial solution by leaving the functionality as-is and making the PLRU-matching configurable with a simple bean (best option IMHO, as CAS is a Spring Boot app it should take advantage of this, why needs everything explicitly configured in DB?)
2. introduce little syntax in the configured LU-string, e.g. https://oauth.example.org/logout,(re)https://www.acme.com/.* the first URL is interpreted as literal, the second because of the (re) prefix as a regex
3. introduce a new org.apereo.cas.services.RegisteredService#matchesLogoutUrl(java.lang.String) operation (and all that follows, like implementing it in the RegisteredServiceMatchingStrategy) that acts like matches() except it is using the configured logoutUrl for its check instead of serviceId. This is more work in many unrelated places :(

This was supposed to be a very simple yet useful addition to CAS (fully implemented, no side effects, with tests!) for a specific usecase but unfortunately due to the numerous change requests this has now already taken way more time then I imagined - I would be glad if we can soon finalize it - please advise!

[mmoayyed]

Apologies for the delay. Having reviewed this some more and discussed internally, I think the best option for now is one of the following:

• Back to the initial proposal to define a simple functional interface with a default implementation that keeps the current functionality. Alternative implementations as conditional beans could do regex-matching as was there before. This is almost the same as the initial proposal, except that the "matcher" api is properly defined, and is not used a predicate.

Or

• For now, use regex matching by default by doing a matches() call with no possibility of customization as a bean, to keep things simple OOTB.

It's possible that we might consider changing this in the future; to introduce APIs for logout URL matching, or control this behavior globally, etc. For now, it's best to move on with immediate progress and not let those future ambitions be the enemy of the good.

[Interessierter]

Hi @mmoayyed thanks for your reply! I choose variant 1 because it keeps the current functionality for all others which is safest (although like I said I think the scenario that Hal mentioned is not possible there are certain others possible if someone has taken over a website completely and is able to obtain control over the IdToken and what postLogoutRedirectUrl then the issue with . in currently configured logoutUrls may be a problem).

I pushed the branch and also merged it to master in my repo-fork so when the CI build there is done and you're OK with it I would be most happy if you accept the PR.

Of course I am fine with future changes/refactorings of this - as long as the functionality (for us now we need to match the postLogoutRedirectUrl with a regex against the configured logoutUrl) is kept.
Would you agree that I port this to the 6.3-branch?

[mmoayyed]

LGTM. Minor comments on formatting and structure

[mmoayyed]

Thanks for the follow-up adjustments. Once this is merged, I see no issues with porting this back to 6.3. I can cherry-pick on your behalf, or you're welcome to submit a separate PR.

[Interessierter]

made the requested adjustments and also fixed the code style issues (sorry, I have to work on a corporate on-demand antivirus scan infested machine, IntelliJ isnt really responsive when working with a large project like cas )

[mmoayyed]

Wonderful, thanks very much. Let's proceed.

As ever, you're most welcome to submit another PR to handle the change for 6.3.x, or you could let me know and I can always cherry-pick on your behalf, preserving credit.

[Interessierter]

would be nice if you could cherry-pick it into 6.3 - thanks in advance!