Implement OpenID Connect - #3
Conversation
Al2Klimov
left a comment
There was a problem hiding this comment.
No module.info yet due to the lack of versions of both this project itself and its dependencies. Apropos, I had to install snapshot packages of both IW2 and IPL.
ba53bc7 to
2483223
Compare
Add module.info please. Module version should be 1.0.0. Also add |
035e437 to
ad04ce6
Compare
| 'redirect_uri' => $login->config->redirect_url | ||
| ]])->getBody()->getContents()); | ||
|
|
||
| list($header, $payloadBase64Url, $signature) = explode('.', $tokens->id_token); |
There was a problem hiding this comment.
Hi, I know it's still in development and early. Just read over the PR and noticed this.
Shouldn't the token be verified using the signature? Otherwise is just some data claiming some username and groups.
There was a problem hiding this comment.
Verified with a public key? Where does latter origin from? Right, again from the very same discovery URL. That makes verification pointless, in contrast to HTTPS:
The module relies on HTTPS for transport security. Do not use HTTP providers in production!
There was a problem hiding this comment.
The referenced OWASP document addresses this:
Attacker Provided Public Key
The JSON Web Signature (JWS) standard (which defines the header and signatures used by JWTs) allows the key used to sign the token to be embedded in the header. If the library used to validate the token supports this, and doesn’t check the key against a list of approved keys, this allows an attacker to sign an JWT with an arbitrary key that they provide.
There are a variety of scripts that can be used to do this, such as jwk-node-jose.py or jwt_tool.
There was a problem hiding this comment.
I could also be wrong, it's a long spec and much to consider. I was just wondering.
If the ID Token is received via direct communication between the Client and the Token Endpoint (which it is in this flow), the TLS server validation MAY be used to validate the issuer in place of checking the token signature. The Client MUST validate the signature of all other ID Tokens according to JWS [JWS] using the algorithm specified in the JWT alg Header Parameter. The Client MUST use the keys provided by the Issuer.
https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation
|
|
||
| if ($username === null || $groups === null && ($login->config->map_groups ?? null) === 'y') { | ||
| // Requested claims may be not part of the ID token. Such must be retrieved from the userinfo endpoint. | ||
| // E.g. GitLab includes groups only in the userinfo endpoint (but groups_direct only in the ID token) |
There was a problem hiding this comment.
Even if the JWT is signed, it may have not everything we need, then we fall back to the userinfo_endpoint.
@martialblog Does the latter provide signed data?
There was a problem hiding this comment.
Hi, this is what the spec says
The sub (subject) Claim MUST always be returned in the UserInfo Response.
NOTE: Due to the possibility of token substitution attacks (see Section 16.11), the UserInfo Response is not guaranteed to be about the End-User identified by the sub (subject) element of the ID Token. The sub Claim in the UserInfo Response MUST be verified to exactly match the sub Claim in the ID Token; if they do not match, the UserInfo Response values MUST NOT be used.
https://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse
| $username = $userinfo->{$login->config->username_claim} ?? null; | ||
| } | ||
|
|
||
| if ($groups === null && ($login->config->map_groups ?? null) === 'y') { | ||
| $groups = $userinfo->{$login->config->groups_claim} ?? null; |
There was a problem hiding this comment.
My question just was whether these claims (or ideally all, as username_claim/groups_claim can be anything) from the userinfo endpoint, are signed. If not, JWT verification is pointless:
Even if the JWT is signed, it may have not everything we need, then we fall back to the userinfo_endpoint.
Regarding token substitution attacks in general:
The Implicit Flow of OAuth 2.0 [RFC6749] is not designed to mitigate this risk. (...) Client MUST NOT use the Implicit Flow without employing additional security mechanisms
We don't use The Implicit Flow at all. Icinga Web SSO only trusts info it directly obtains from ID providers. Hence:
The module relies on HTTPS for transport security. Do not use HTTP providers in production!
ad04ce6 to
329c109
Compare
|
|
||
| // "the TLS server validation MAY be used to validate the issuer in place of checking the token signature" | ||
| // -- https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation | ||
| list($header, $payloadBase64Url, $signature) = explode('.', $tokens->id_token); |
|
|
||
| Icinga Web SSO documentation is available at [icinga.com/docs](https://icinga.com/docs/icinga-sso-web/latest/). | ||
|
|
||
| ## Features |
There was a problem hiding this comment.
Hi,
currently the module doesn't support logout via openid-connect/logout so that a logout in the Icinga Web interface also closes the active user session.
An example use case would be, when a users groups are changed in the identity provider and needs to re-login. Currently the users would have to contact the identity provider to close the session (i.e. web interface).
If this is intentionally not supported, it maybe should be mentioned in the docs.
There was a problem hiding this comment.
So you mean that a logout in Icinga Web should also trigger one at e.g https://id.netways.de?
There was a problem hiding this comment.
Yes, this could be an optional setting maybe. So that the session is closed on the provider's side and the user redirected to the Icinga Web login. https://<PROVIDER>/protocol/openid-connect/logout?post_logout_redirect_uri=https%3A%2F%2F<ICINGAWEB>%2Flogin
I think this is the spec https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RPLogout
There was a problem hiding this comment.
It's quite common in generic OIDC integrations. e.g. https://grafana.com/docs/grafana/latest/setup-grafana/configure-access/configure-authentication/keycloak/#enable-single-logout
There was a problem hiding this comment.
Common or not, I consider this absolutely counter-intuitive. 🤯
Just imagine, I click to log out just from GitLab and my complete https://id.netways.de session gets lost which I didn't request. 🙈
There was a problem hiding this comment.
From what I understand it's the session associated with the RP.
RECOMMENDED. ID Token previously issued by the OP to the RP passed to the Logout Endpoint as a hint about the End-User's current authenticated session with the Client. This is used as an indication of the identity of the End-User that the RP is requesting be logged out by the OP.
There was a problem hiding this comment.
the adjective "RECOMMENDED", mean that there may exist valid reasons in particular circumstances to ignore a particular item
– https://datatracker.ietf.org/doc/html/rfc2119#section-3
My valid reason is that I, as a user, didn't request logout from https://id.netways.de/, but just from GitLab and I expect exactly that to happen.
End of story.
There was a problem hiding this comment.
Again, I think this is only for the RP session not for the entire provider.
If the ID Token's sid claim does not correspond to the RP's current session or a recent session at the OP, the OP SHOULD treat the logout request as suspect, and MAY decline to act upon it.
At the Logout Endpoint, the OP SHOULD ask the End-User whether to log out of the OP as well. Furthermore, the OP MUST ask the End-User this question if an id_token_hint was not provided or if the supplied ID Token does not belong to the current OP session with the RP and/or currently logged in End-User.
In any case, like I said: If this is intentionally not supported, it maybe should be mentioned in the docs.
There was a problem hiding this comment.
I think this is only for the RP session
Then I don't get what's the whole stuff for.
the session is closed on the provider's side and the user redirected to the Icinga Web login.
https://<PROVIDER>/protocol/openid-connect/logout?post_logout_redirect_uri=https%3A%2F%2F<ICINGAWEB>%2Flogin
That's not how Icinga Web works. Just look at AuthenticationController#logoutAction(), it
- expects all logout hooks to do their thing silently in the background
- redirects to the login page
If this is intentionally not supported, it maybe should be mentioned in the docs.
Counter-suggestion: After merging this, you'll make a docs PR.
| | OAuth Scopes | Read-only; configure these in the OIDC provider! | openid profile | | ||
| | Redirect URL | Auto-filled; configure the same in the OIDC provider! | https://example.com/sso/oidc/redirection-endpoint | | ||
| | Username Field | Claim that holds the username | preferred\_username | | ||
| | Map Groups | Import group memberships from the provider | yes/no | |
There was a problem hiding this comment.
Hi,
I think the group mapping could use some more docs.
Does the group from the provider need to exist in Icinga Web as a group?
If so, does the username from the provider need to be in this Icinga Web group?
Just so it is clear for the admins what they have to configure in Icinga Web.
Also "Import group memberships from the provider" could be confusing, since the module does not import anything, it "maps" the groups in the claim to existing Icinga Web Groups.
There was a problem hiding this comment.
Note, the description on the Form reads "Apply group memberships from the provider to Icinga Web" which is more accurate for example
| 'discovered' => $login->discovered | ||
| ]); | ||
|
|
||
| $user = (new User($username))->setGroups($groups ?? []); |
There was a problem hiding this comment.
the module does not import anything, it "maps" the groups in the claim to existing Icinga Web Groups.
Absolutely completely not.
Just like Icinga Web imports whichever LDAP groups a user is member of from MSAD, making memberships magically appear for the user, Web SSO imports such groups from any provider.
Does the group from the provider need to exist in Icinga Web as a group?
No. Just like with LDAP, nothing has to "exist in Icinga Web".
There was a problem hiding this comment.
Cool. The docs could state this. So that users don't have to guess.
There was a problem hiding this comment.
If the LDAP ones already do, yes. Otherwise, I wouldn't try to be more catholic than the Pope himself...
329c109 to
0071a67
Compare
|
(Replaced two scope fields with one.) |
0071a67 to
6ca0bed
Compare
|
(Make |
6ca0bed to
f173cf9
Compare
|
(Fixed the broken session refresh.) |
f173cf9 to
29b1f48
Compare
This adds the initial Icinga Web SSO implementation with OpenID Connect provider support: