Remove validation from OIDC endpoint #37159
Conversation
This change removes validation of query parameters from the /api/security/v1/oidc endpoint. It was discovered during manual testing that Google's OP is sending extra parameters than the ones identified in https://tools.ietf.org/html/rfc6749#section-4.1.2 which is refernced by https://openid.net/specs/openid-connect-core-1_0.html#AuthResponse (for instance auth_user and session_state). The existing validation rules only allowed the expected query parameters but this means that Kibana wouldn't be able to complete OpenID Connect authentication with Google acting as the OP. As dictated in the standard (RFC6749), "The client MUST ignore unrecognized response parameters." so we should allow but discard any extra parameters we do not recognize and not throw an error.
💚 Build Succeeded |
|
ACK: will review today |
| state: Joi.string() | ||
| }) | ||
| } | ||
| auth: false |
There was a problem hiding this comment.
question: can we just use Joi.object().keys({...}).unknown()?
question: is there any reason why we can't merge these two routes (GET and POST) into one with method: ['GET', 'POST']
There was a problem hiding this comment.
can we just use Joi.object().keys({...}).unknown()?
Not sure. I'd have to try this out. Are you thinking that we could at least fail early (in validation) when a request reaches kibana with no parameters ? Sounds like a good idea.
question: is there any reason why we can't merge these two routes (GET and POST) into one with method: ['GET', 'POST']
Good point. Previously we had different validation that's why I had two routes, but now we could merge them
There was a problem hiding this comment.
Not sure. I'd have to try this out. Are you thinking that we could at least fail early (in validation) when a request reaches kibana with no parameters ? Sounds like a good idea.
Not exactly, to forbid empty object (when we don't have any query string parameters), I believe we need to add Joi.object().....min(1). My idea was more to have a basic validation for keys where feasible, e.g. Joi.string().uri() for target_link_uri and error_uri or Joi.string().alphanum().min(1) for iss... or string().required() if we have a required field or something like this.
There was a problem hiding this comment.
Gotcha. I added validation for the URIs and specific for the issuer but I don't think we can add any for code, state.
💚 Build Succeeded |
|
The squashed commit message was a |
UI fail. It lost my commit message by failing to merge and when I clicked refresh, it went ahead and merged with the PR description as message :/This change adjusts validation of query parameters in the /api/security/v1/oidc endpoint. It was discovered during manual testing that Google's OP is sending extra parameters than the ones identified in https://tools.ietf.org/html/rfc6749#section-4.1.2 which is refernced by https://openid.net/specs/openid-connect-core-1_0.html#AuthResponse (for instance auth_user and session_state). The existing validation rules only allowed the expected query parameters but this means that Kibana wouldn't be able to complete OpenID Connect authentication with Google acting as the OP. As dictated in the standard (RFC6749), "The client MUST ignore unrecognized response parameters." so we should allow but discard any extra parameters we do not recognize and not throw an error. Furthermore, it adds stricter validation for the issuer and all pararameters of type URI when these are present.
This change adjusts validation of query parameters in the /api/security/v1/oidc endpoint. It was discovered during manual testing that Google's OP is sending extra parameters than the ones identified in https://tools.ietf.org/html/rfc6749#section-4.1.2 which is refernced by https://openid.net/specs/openid-connect-core-1_0.html#AuthResponse (for instance auth_user and session_state). The existing validation rules only allowed the expected query parameters but this means that Kibana wouldn't be able to complete OpenID Connect authentication with Google acting as the OP. As dictated in the standard (RFC6749), "The client MUST ignore unrecognized response parameters." so we should allow but discard any extra parameters we do not recognize and not throw an error. Furthermore, it adds stricter validation for the issuer and all pararameters of type URI when these are present.
azasypkin
added
Feature:Security/Authentication
|
Pinging @elastic/kibana-security |
* Remove validation from OIDC endpoint This change removes validation of query parameters from the /api/security/v1/oidc endpoint. It was discovered during manual testing that Google's OP is sending extra parameters than the ones identified in https://tools.ietf.org/html/rfc6749#section-4.1.2 which is refernced by https://openid.net/specs/openid-connect-core-1_0.html#AuthResponse (for instance auth_user and session_state). The existing validation rules only allowed the expected query parameters but this means that Kibana wouldn't be able to complete OpenID Connect authentication with Google acting as the OP. As dictated in the standard (RFC6749), "The client MUST ignore unrecognized response parameters." so we should allow but discard any extra parameters we do not recognize and not throw an error. * address feedback * address feedback * add validation for the issuer
This change removes validation of query parameters from the
/api/security/v1/oidc endpoint. It was discovered during manual
testing that Google's OP is sending extra parameters than the ones
identified in https://tools.ietf.org/html/rfc6749#section-4.1.2
which is referenced by
https://openid.net/specs/openid-connect-core-1_0.html#AuthResponse
(for instance
auth_userandsession_state). The existing validationrules only allowed the expected query parameters but this
means that Kibana wouldn't be able to complete OpenID Connect
authentication with Google acting as the OP.
As dictated in the standard (RFC6749), "The client MUST ignore
unrecognized response parameters." and for 3rd Party initiated SSO in
https://openid.net/specs/openid-connect-core-1_0.html#ThirdPartyInitiatedLogin
"her parameters MAY be sent, if defined by extensions. Any parameters used
that are not understood MUST be ignored by the Client."
so we should allow but discard any extra parameters we do not recognize and
not throw an error.
Resolves #37160