Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring to fix issue #3232 #3233

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Refactoring to fix issue #3232 #3233

wants to merge 3 commits into from

Conversation

strehle
Copy link
Member

@strehle strehle commented Jan 16, 2025

Refactoring to fix issue #3232

Move the check for existing secrets to the
grant flow (runtime) and remove it from REST (configuration).

We support now private_key_jwt next to secret, so check all parts in REST is too less and will get complicated. Fix a potential security issue because we must not create a token based on no authentication

@strehle strehle force-pushed the fix/issue/3232 branch 2 times, most recently from 9485cf2 to 5410f9d Compare January 16, 2025 09:43
@strehle
Copy link
Member Author

strehle commented Jan 16, 2025

@strehle strehle marked this pull request as ready for review January 16, 2025 09:54
@strehle strehle requested review from a team January 16, 2025 10:34
Move the check for existing secrets to the
grant flow (runtime) and remove it from REST (configuration).

We support now private_key_jwt next to secret, so check all parts in REST is too less and will get complicated.
Fix a potential security issue because we must not create a token based on no authentication
import static org.cloudfoundry.identity.uaa.oauth.token.TokenConstants.GRANT_TYPE_JWT_BEARER;

public class JwtTokenGranter extends AbstractTokenGranter {
final DefaultSecurityContextAccessor defaultSecurityContextAccessor;

private static final List<String> ALLOWED_AUTH_METHODS = List.of(CLIENT_AUTH_SECRET, CLIENT_AUTH_PRIVATE_KEY_JWT);
//comment I would add here CLIENT_AUTH_EMPTY to allow empty secret for jwt bearer, same as allowed for password
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this, the change is not change but only a refactoring.

As mentioned currently we reject in clients for JWT bearer empty secret but that is strange and we should discuss

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Pending Review | Discussion
Development

Successfully merging this pull request may close these issues.

Cleanup UAA Client Secret Setting and Ensure Authentication on Client Credentials Grant Flow
1 participant