fix client admin rest call with empty secret #2571
Conversation
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/186322610 The labels on this github issue will be updated when the story is started. |
strehle
requested review from
Tallicia,
peterhaochen47,
bruce-ricard,
swalchemist,
torsten-sap,
adrianhoelzl-sap and
tack-sap
...er/src/main/java/org/cloudfoundry/identity/uaa/scim/validate/UaaPasswordPolicyValidator.java
Show resolved
Hide resolved
server/src/main/java/org/cloudfoundry/identity/uaa/util/PasswordValidatorUtil.java
Show resolved
Hide resolved
| MessageResolver messageResolver) { | ||
| List<Rule> rules = new ArrayList<>(); | ||
|
|
||
| //length is always a rule. We do not allow blank password | ||
| int minLength = Math.max(1, policy.getMinLength()); |
There was a problem hiding this comment.
I don't understand this line. This seems to be the source of the problem. Why are we overriding the policy with this 1? This is what needs to be changed. If we want to enforce a minimum 1 policy, it should be in the policy.
There was a problem hiding this comment.
Please check the history and you can also use an older UAA, but even if you run default client policy, means 0 as minimum secret is allowed you cannot have
uaac client add newclient --name xuaa --scope openid --authorized_grant_types password --autoapprove true --secret ''
We agree that the empty secret still should be allowed but with hard coded 1 this does not work.
For user passwords maybe and therefore I have divided the check into user and client validator.
| public static PasswordValidator secretValidator(GenericPasswordPolicy<?> policy, | ||
| MessageResolver messageResolver) { | ||
| return internalValidator(policy, 0, messageResolver); | ||
| } | ||
|
|
||
| public static PasswordValidator userValidator(GenericPasswordPolicy<?> policy, | ||
| MessageResolver messageResolver) { | ||
| return internalValidator(policy, 1, messageResolver); | ||
| } | ||
|
|
||
| public static PasswordValidator validator(GenericPasswordPolicy<?> policy, | ||
| private static PasswordValidator internalValidator(GenericPasswordPolicy<?> policy, | ||
| int minimumLength, | ||
| MessageResolver messageResolver) { | ||
| List<Rule> rules = new ArrayList<>(); | ||
|
|
||
| //length is always a rule. We do not allow blank password | ||
| int minLength = Math.max(1, policy.getMinLength()); | ||
| //length is always a rule. We do not allow blank password, but a blank secret | ||
| int minLength = Math.max(minimumLength, policy.getMinLength()); |
There was a problem hiding this comment.
It really looks like this code should be in the PasswordValidator class.
There was a problem hiding this comment.
the code is in the class. ,
I dont was change an existing beviour. Existing was. For a user password at least 1 character is needed.
There was a problem hiding this comment.
For a user password at least 1 character is needed
This is ridiculous. A 1-character password and an empty password have literally the same level of security: none.
This line is completely useless.
| @@ -82,8 +82,7 @@ public void validate(String clientSecret) throws InvalidClientSecretException { | |||
| clientSecretPolicy = zone.getConfig().getClientSecretPolicy(); | |||
| } | |||
|
|
|||
| PasswordValidator clientSecretValidator = validator(clientSecretPolicy, | |||
There was a problem hiding this comment.
Is this the source of the regression that was introduced recently? That we started validating client secrets?
There was a problem hiding this comment.
no, the issue is seperate.
As mentioned before use an older UAA start it and run uaac to create an client with empty secret......
There was a problem hiding this comment.
I don't want to reproduce the issue. I'm trying to find out what new code is the source of the problem. It seems to me like it should be documented in the fix, so that people understand what is going on.
There was a problem hiding this comment.
well, after the fix, the documentation matches again, because it says
minLength defaults to 0, e.g. https://docs.cloudfoundry.org/api/uaa/version/76.23.0/index.html#identity-zones
But this was not the case
...est/java/org/cloudfoundry/identity/uaa/integration/ClientAdminEndpointsIntegrationTests.java
Show resolved
Hide resolved
.../test/java/org/cloudfoundry/identity/uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java
Outdated
Show resolved
Hide resolved
.../test/java/org/cloudfoundry/identity/uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java
Show resolved
Hide resolved
.../test/java/org/cloudfoundry/identity/uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java
Outdated
Show resolved
Hide resolved
Empty client secret is allowed if client secret policy allows it. Error says 1 char is needed, this is wrong Why 2 validators, distinguish between user and client policy validation. For users 1 minimum char for a password might be ok, for a client definitely not. The empty secret should be treated as public, but we cannot disallow it. Therefore, this here is a fix for a regression introduced with 76.22.0.
strehle
force-pushed
the
fix/issue/2570
branch
from
66841d6 to
e42ebfc
Compare
|
close this, related issue was #2570 |
Empty client secret is allowed if client secret policy allows it.
Error says 1 char is needed, this is wrong
Why 2 validators, distinguish between user and client policy validation.
For users 1 minimum char for a password might be ok, for a client definitely not.
The empty secret should be treated as public, but we cannot disallow it.
Therefore, this here is a fix for a regression introduced with 76.22.0.