feature: add client_auth_method=none into tokens for clients with empty secret - outdated

[strehle]

add client_auth_method=none into tokens for clients with empty secret

e.g. cf client.
This allows clients to see, whats really used.
According to https://oauth.net/2.1/ the empty secret can be treated as public usage

Advantage is, that public to public exchanges can now work, see #2193 .
If we restrict with #2193 the user_token grant, then public to public cannot be allowed without this feature

Similar PR #2447, which introduces a new method for empty

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/185787203

The labels on this github issue will be updated when the story is started.

[hsinn0]

@strehle, Just based on the title of this PR and #2447, it appears that the two PRs are conflicting each other? Is the goal to merging only one of the two PRs? Or am I missing something?
Also, could you point to the doc section where it says "the empty secret can be treated as public usage"?

[strehle]

@strehle, Just based on the title of this PR and #2447, it appears that the two PRs are conflicting each other? Is the goal to merging only one of the two PRs? Or am I missing something? Also, could you point to the doc section where it says "the empty secret can be treated as public usage"?

There are 2 PRs which same goal but different approaches. Goal. Add information about "dummy" authentication to clients so that we have this information in case of token exchanges.

Approaches:
a) Set empty to client_auth_method, see #2447
b) Set none to client_auth_method, this PR. #2435

With this information we can later fix the issue from #2193 .

The names are taken from https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication . So none is defined for public flows. If we do empty secret authentication, then internally it is an authentication, but with a dummy secret, so I currently would vote vor option a) , but wanted to have both PRs to check with you (from vmware) what would be your choice.

[peterhaochen47]

I think comparing these two options:
#2435 (client_auth_method=none)
#2447 (client_auth_method=empty)
I have a slight preference for client_auth_method=none, reasons:

• From the OIDC spec, the none value signals a "Public Client." I think we should disclose our clients with empty secrets as essentially the same as public clients. Even though our internal logic might put clients with empty secrets through a different code path than clients with no secrets, to our users, these two client types are practically both public clients & have the same level of security risks.
• I think it might not be intuitive to the users what client_auth_method=empty mean exactly (they might be confused about why the auth method itself is empty); whereas the meaning of client_auth_method=none is more intuitive (it means unauthenticated) and uses the terminology from the spec.
• In the long term, I think we probably should just disallow empty string secrets (in a major version)? Since this config has created such confusions by being within the grey area between public and confidential clients. If so, then only client_auth_method=none is needed.

[Tallicia]

I concur with Peter's recomendation to use #2435 (client_auth_method=none) Is there any prerequisities pending to merge 2435, or do we have a taret release/milestone to associate it with?

[strehle]

I have to rebase it , but will you notify asap it is ready

[Tallicia]

Thanks @strehle !

[strehle]

@peterhaochen47 @Tallicia
I need your attention once again.

I copied some tests from the brach or PR
#2447
into this PR and found another reason why I created method empty in addition to none,
see example integration test
https://github.com/cloudfoundry/uaa/pull/2435/files#diff-b3558228379bf58ca90c1d7627edf8bc79e569bb7ab9f4b47db0a99c00bb8a94

RefreshTokenSupportIntegrationTests.java

This will fail here in the PR because:
With none we have no chance to distinguesh between NULL secret and EMPTY.

I introduced the public flow (with option allowpublic) and I also added the support for the refresh flow for public .
This means. If you create a token with autz.code flow and NULL (no secret in request) secret and then you do a refresh flow also without a secret then this is allowed if allowpublic is set.

But we have the existing behaviour with cf and empty secret and here we allow refresh but we dont treat this as public.
If we now set the empty secret to none then the refresh flow with cf client breaks. And I dont know if we should do this ?
Therefore I created empty as name because then we can allow a refresh flow with empty secret but we dis-allow it with none secret.

So question: what should we do ? disallow refresh for cf clients ? or better go with empty ?

[Tallicia]

I'm adding this to the agenda for tomorrow's meeting to better understand. Mostly I'm curious why we can not distinguish between NULL and empty/""

[strehle]

I'm curious why we can not distinguish between NULL and empty/""

In this PR we set none and then the refresh flow for the cf client with an empty secret fails. Because by default the cf client does not have the allowpublic flag set to true and then the normal refresh flow fails.

Here is the feature where a refresh flow without any secret can work #2138

But if we now set the empty secret to the mode that it seems it is public, then we loose the state to distinguish
This PR keeps the state: #2504