Repository navigation
Admin API v2: add stricter validation constrains for Client resource - #48935
Conversation
|
@michalvavrik @vmuzikar I think what you have here looks good. What's here does not look like representation specific validation - it's based upon existing logic / constraints, correct? I think there was also a mention that v2 may want stricter validation than what v1 allows, but that would need its own issue as that creates situations where v1 clients would not be valid in v2, and we need to have a complete story around that in particular for the admin ui. Related to #48786 (comment) - having the validations captured in annotations definitely works well with what we're already doing in admin api v2 given the usage of the hibernate validator and the exposure of the validation metadata in the openapi, but it is not reusable by non-v2 scenarios. What kind of follow-ups would be good to capture? Things like - being able to reuse the validation annotations directly on the Entities, a way to introspect the hibernate persistence metadata, enhancing ModelValidationException and the general exception mapping logic (EntityManagerProxy.convert) to make the mapping to representation easier, etc. |
Yes, I have tried to identify states that are allowed by our API but fails later, like when you try to use the client. Or which fails down on the database later, which doesn't provide as clear validation exceptions as we do (typically I got 500 when DB insert failed due to exceeded value of the column length). However, if someone does updates / patches in steps (like first set standard flow, then remember redirect URIs). That is intentional, because we want to prevent illegal states that won't work.
I agree. At minimal, we need to ensure that validation messages at showed in the Admin UI (when user tries to save wrong values), but ideally we could be step ahead and adjust UI parts (like input max length, min length and so on). Sounds like a follow-up.
I will defer this note to @vmuzikar . |
fe56b31 to
f2bf75c
Compare
There was a problem hiding this comment.
Thanks for the PR, Michal! Just two nits to consider (but nothing serious that would block this). Thus, approving.
f2bf75c to
ed8991e
Compare
|
I'd like to look into this ASAP as well. |
There was a problem hiding this comment.
@michalvavrik Overall looks good, nice work. Added some minor comments.
| public enum NameIdFormat { | ||
| USERNAME, EMAIL, PERSISTENT, TRANSIENT; | ||
|
|
||
| @JsonValue | ||
| public String toJson() { | ||
| return name().toLowerCase(); | ||
| } | ||
|
|
||
| @JsonCreator | ||
| public static NameIdFormat fromJson(String value) { | ||
| return value == null ? null : valueOf(value.toUpperCase()); | ||
| } | ||
| } | ||
|
|
||
| public enum SignatureAlgorithm { | ||
| RSA_SHA1, RSA_SHA256, RSA_SHA256_MGF1, RSA_SHA512, RSA_SHA512_MGF1, DSA_SHA1; | ||
|
|
||
| @JsonCreator | ||
| public static SignatureAlgorithm fromJson(String value) { | ||
| return value == null ? null : valueOf(value); | ||
| } | ||
| } |
There was a problem hiding this comment.
Just a note. Obviously the enums should be somewhere in the core codebase, the API layer should not be the source of truth for this. But it's ok for the MVP, let's keep as is.
| CanonicalizationMethod.INCLUSIVE_11, | ||
| CanonicalizationMethod.INCLUSIVE_11_WITH_COMMENTS |
There was a problem hiding this comment.
Where is INCLUSIVE_11 coming from? It doesn't seem to be possible to set this value in the UI?
There was a problem hiding this comment.
Where is
INCLUSIVE_11coming from? It doesn't seem to be possible to set this value in the UI?
My understanding is that Keycloak does not restrict valid values. It stores the canonicalization method and passes it to this code:
then what is valid value is determined by XML DSIG provider and the default provider registers all 6 constants that I listed in here. If you want to restrict it to UI only, I can do that. However, even of the 4 listed values in UI, but I only found tests for 2 of them (EXCLUSIVE_WITH_COMMENTS and EXCLUSIVE), so I don't have explicit argument against the ones I included (like argument it is not tested...which it is not as well).
It is your call, let me know.
ef0a1b7 to
9529a54
Compare
9529a54 to
b57b0ef
Compare
* Closes: keycloak#48786 * Changes: | Schema#Property | New Validation Constraint | Reasoning | |-----------------|--------------------------|-----------| | BaseClientRepresentation#clientId | `@Size(min=1, max=255)` | DB VARCHAR(255) | | BaseClientRepresentation#displayName | `@Size(max=255)` | DB NVARCHAR(255) | | BaseClientRepresentation#description | `@Size(max=255)` | DB NVARCHAR(255) | | BaseClientRepresentation#appUrl | `@Size(max=255)` | DB VARCHAR(255) | | BaseClientRepresentation#redirectUris | `@Size(max=100)` on set, `@Size(max=255)` on elements | DB VARCHAR(255) per element, bound collection | | BaseClientRepresentation#roles | `@Size(max=300)` on set, `@Size(max=255)` on elements | DB VARCHAR(255) per element, bound collection | | OIDCClientRepresentation#webOrigins | `@Size(max=100)` on set, `@Size(max=255)` on elements | DB VARCHAR(255) per element, bound collection | | OIDCClientRepresentation#webOrigins | `@ValidWebOrigin` | RFC 6454 format, invalid origins silently break CORS | | OIDCClientRepresentation#serviceAccountRoles | `@Size(max=300)` on set, `@Size(max=255)` on elements | DB VARCHAR(255) per element, bound collection | | OIDCClientRepresentation.Auth#method | `@NotBlank` | must be specified when auth object is present | | OIDCClientRepresentation.Auth#method | `@ValidAuthMethod` | open SPI, dynamic check against registered providers | | OIDCClientRepresentation.Auth#secret | `@Size(min=6, max=255)` | DB VARCHAR(255), min=6 prevents trivially weak secrets | | OIDCClientRepresentation.Auth#certificate | `@Size(max=65536)` | NCLOB unlimited, practical upper bound | | OIDCClientRepresentation (class-level) | `@ConfidentialFlowsRequireAuth` | SERVICE_ACCOUNT/TOKEN_EXCHANGE fail at runtime without auth | | OIDCClientRepresentation (class-level) | `@RedirectFlowsRequireUris` | STANDARD/IMPLICIT fail at runtime without redirect URIs | | OIDCClientRepresentation (class-level) | `@ServiceAccountRolesRequireFlow` | roles silently ignored without SERVICE_ACCOUNT flow | | SAMLClientRepresentation#nameIdFormat | type changed to enum `NameIdFormat` | SAML 2.0 spec frozen since 2005, unlikely to change | | SAMLClientRepresentation#signatureAlgorithm | type changed to enum | enum could change due to PQC, but we can simply update enum | | SAMLClientRepresentation#signatureCanonicalizationMethod | `@ValidCanonicalizationMethod` | finite set from JDK constants, W3C standard unchanged since 2008 | | SAMLClientRepresentation#signingCertificate | `@Size(max=65536)` | NCLOB unlimited, practical upper bound | Signed-off-by: Michal Vavřík <michal.vavrik@aol.com>
b57b0ef to
0b164b9
Compare
* Closes: keycloak#48786 * Changes: | Schema#Property | New Validation Constraint | Reasoning | |-----------------|--------------------------|-----------| | BaseClientRepresentation#clientId | `@Size(min=1, max=255)` | DB VARCHAR(255) | | BaseClientRepresentation#displayName | `@Size(max=255)` | DB NVARCHAR(255) | | BaseClientRepresentation#description | `@Size(max=255)` | DB NVARCHAR(255) | | BaseClientRepresentation#appUrl | `@Size(max=255)` | DB VARCHAR(255) | | BaseClientRepresentation#redirectUris | `@Size(max=100)` on set, `@Size(max=255)` on elements | DB VARCHAR(255) per element, bound collection | | BaseClientRepresentation#roles | `@Size(max=300)` on set, `@Size(max=255)` on elements | DB VARCHAR(255) per element, bound collection | | OIDCClientRepresentation#webOrigins | `@Size(max=100)` on set, `@Size(max=255)` on elements | DB VARCHAR(255) per element, bound collection | | OIDCClientRepresentation#webOrigins | `@ValidWebOrigin` | RFC 6454 format, invalid origins silently break CORS | | OIDCClientRepresentation#serviceAccountRoles | `@Size(max=300)` on set, `@Size(max=255)` on elements | DB VARCHAR(255) per element, bound collection | | OIDCClientRepresentation.Auth#method | `@NotBlank` | must be specified when auth object is present | | OIDCClientRepresentation.Auth#method | `@ValidAuthMethod` | open SPI, dynamic check against registered providers | | OIDCClientRepresentation.Auth#secret | `@Size(min=6, max=255)` | DB VARCHAR(255), min=6 prevents trivially weak secrets | | OIDCClientRepresentation.Auth#certificate | `@Size(max=65536)` | NCLOB unlimited, practical upper bound | | OIDCClientRepresentation (class-level) | `@ConfidentialFlowsRequireAuth` | SERVICE_ACCOUNT/TOKEN_EXCHANGE fail at runtime without auth | | OIDCClientRepresentation (class-level) | `@RedirectFlowsRequireUris` | STANDARD/IMPLICIT fail at runtime without redirect URIs | | OIDCClientRepresentation (class-level) | `@ServiceAccountRolesRequireFlow` | roles silently ignored without SERVICE_ACCOUNT flow | | SAMLClientRepresentation#nameIdFormat | type changed to enum `NameIdFormat` | SAML 2.0 spec frozen since 2005, unlikely to change | | SAMLClientRepresentation#signatureAlgorithm | type changed to enum | enum could change due to PQC, but we can simply update enum | | SAMLClientRepresentation#signatureCanonicalizationMethod | `@ValidCanonicalizationMethod` | finite set from JDK constants, W3C standard unchanged since 2008 | | SAMLClientRepresentation#signingCertificate | `@Size(max=65536)` | NCLOB unlimited, practical upper bound | Signed-off-by: Michal Vavřík <michal.vavrik@aol.com>
@Size(min=1, max=255)@Size(max=255)@Size(max=255)@Size(max=255)@Size(max=100)on set,@Size(max=255)on elements@Size(max=300)on set,@Size(max=255)on elements@Size(max=100)on set,@Size(max=255)on elements@ValidWebOrigin@Size(max=300)on set,@Size(max=255)on elements@NotBlank@ValidAuthMethod@Size(min=6, max=255)@Size(max=65536)@ConfidentialFlowsRequireAuth@RedirectFlowsRequireUris@ServiceAccountRolesRequireFlowNameIdFormatSignatureAlgorithm@ValidCanonicalizationMethod@Size(max=65536)I have full analysis of constrains I have considered but did not add if anyone is interested, shout.