cas icon indicating copy to clipboard operation
cas copied to clipboard

OIDC service scopes applied to all token requests

Open sbearcsiro opened this issue 2 years ago • 7 comments

I noticed that the application and OIDC service scope limits were not applied to OIDC client credentials grants, ie client credentials grant requests could get assign any scope to the access tokens. I've attempted to fix it in the DefaultTokenGenerator classes by removing scopes from the requested scopes that aren't allowed by the application / service. Subsequent testing showed that the OIDC service scope limits could be avoided by using the /oauth2.0/ token endpoints instead, so I've pushed scopes down into the OAuth services.

I'm not sure if there's anywhere I've missed where a similar scope calculation needs to occur or whether this could be solved in a better way, happy to take suggestions.

sbearcsiro avatar Mar 09 '22 16:03 sbearcsiro

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id Secret Commit Filename
- Generic High Entropy Secret a38001ff8da2eeaea318b02afc1ad4491feba706 support/cas-server-support-oidc/src/test/java/org/apereo/cas/oidc/web/OidcTokenGeneratorTests.java View secret
- Generic High Entropy Secret a38001ff8da2eeaea318b02afc1ad4491feba706 support/cas-server-support-oidc/src/test/java/org/apereo/cas/oidc/web/OidcTokenGeneratorTests.java View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

gitguardian[bot] avatar Mar 09 '22 16:03 gitguardian[bot]

Thanks for the patch.

I've pushed scopes down into the OAuth services.

I don't think this is something we can reasonably take in via a pull request. The work to handle this is quite large, and I do see this PR is missing quite a few more places to run the same check, plus real integration tests, (unit tests alone are not enough here), plus documentation, etc. To accommodate all of this, (and I am not sure you have the bandwidth to handle this all), it will take quite some time, and the reviews would be very difficult since the number of modified files would continue to grow.

Is there a smaller variant where you could just handle a fix for OIDC, and leave OAuth alone for the time being?

mmoayyed avatar Mar 11 '22 18:03 mmoayyed

@mmoayyed thanks for the feedback and review. I'm afraid I wouldn't have the bandwidth to handle fully adding scopes to the oauth support (as much as I'd like to).
I've reworked this patch to remove the oauth scopes field and created a(n almost) functional type that can evaluate the allowed scopes for a token request context.

You also mentioned it missing places to run the same check, did they pertain to OIDC and are they still missing in this patch? If so, any hints where they might be?

sbearcsiro avatar Mar 16 '22 16:03 sbearcsiro

This pull request has been open and inactive for 7 days. It will be closed shortly.

github-actions[bot] avatar Mar 27 '22 00:03 github-actions[bot]

@mmoayyed any thoughts on this?

sbearcsiro avatar Mar 28 '22 00:03 sbearcsiro

Haven't had time to give this a proper review. Please merge with master once and I'll see if I can offer suggestions later this week or the next.

mmoayyed avatar Mar 28 '22 11:03 mmoayyed

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Aug 23 '22 04:08 CLAassistant

This appears to have been taken care of now, and should be more or less correct with 6.6 or the current master. Please follow up if you see otherwise. Thank you!

mmoayyed avatar Oct 14 '22 12:10 mmoayyed