Restructure authentication policies. #1232
Conversation
|
At this point, I have removed the oso and jmespath crates and shifted all policy decisions into the new |
|
The I have also implemented the new strategy outlined in #1229 with a tweak: the OpenID Connect provider now only resolves the ID and role name from the claims. The roles themselves are defined in the configuration and include both the permissions and possible limitations for the CAs access is allowed to. This means you can’t include the allowed CAs in the OpenID Connect claims any more but hopefully this is fine. Still missing for this PR to leave draft state are documentation adjustments, both internally and in the manual. |
|
Both implementation and documentation are done now. This now needs some testing, in particular the OpenID Connect parts, but it is ready for review now. |
|
Alright, I‘m calling this done. Now have at it ;) |
| # or readwrite depending on which GUID was matched. The GUIDs for your groups | ||
| # will be different than those used in this example, see your Krill log for the | ||
| # GUIDs to match on. | ||
| # [[auth_openidconnect.claims]] |
There was a problem hiding this comment.
I believe the examples for Azure and the others are outdated now that you've split id_claims and role_claims.
| # For array claim values, the first element is | ||
| # used. | ||
| # | ||
| # subst No This field describes a transformation of a |
There was a problem hiding this comment.
I think it might be nice to add (maybe as an example) that this may just be a literal rather than a substitution if used without match. I.e. the case "if you can log in, you're admin".
| @@ -1022,7 +1021,7 @@ pub async fn metrics(req: Request) -> RoutingResult { | |||
|
|
|||
| //------------ Publication --------------------------------------------------- | |||
|
|
|||
| /// Handle RFC8181 queries and return the appropriate response. | |||
| /// MyHandle RFC8181 queries and return the appropriate response. | |||
There was a problem hiding this comment.
I don't think this was intentional :-)
| } | ||
|
|
||
| Err(Self::internal_error( | ||
| format!("OpenID Connect: no value found for '{}' claim.", dest), |
There was a problem hiding this comment.
This error message also bubbles up when a user that doesn't match any roles successfully logs in, which might be confusing.
| // and don't result in an obvious timing difference between | ||
| // the two scenarios which could potentially | ||
| // be used to discover user names. | ||
| if encoded_hash != user_password_hash { |
There was a problem hiding this comment.
I know this was not changed recently, but I don't like the "succeed unless it fails" flow for authentication. I prefer an explicit "fail unless it succeeds" flow, as if authentication becomes more complex in the future, then making a small mistake where the checks are all bypassed becomes easier.
| IdTokenAdditionalClaim, | ||
| UserInfoStandardClaim, | ||
| UserInfoAdditionalClaim, | ||
| fn default_id_claims() -> Vec<TransformationRule> { |
There was a problem hiding this comment.
Perhaps make it more explicit in the config/manual that this is the default (and does not need to be manually configured)
This PR restructures how authentication policies are used in Krill. It removes the use of Oso and its policy definition language and instead switches to simple, straightforward mappings between permissions, roles, and users.
The existing concept of roles is augmented to serve as the central configuration option for limiting a user’s access to certain action and resources. Roles are now user configurable via the new
auth_rolesconfiguration directive. For each role, a set of permissions has to be provided. Optionally, a list of resource handles (vulgo: CAs) can be given in which case access is limited to these resources.The authentication providers now assign one of these roles to each logged in user.
The OpenID Connect provider now only determines claims for “id,” i.e., the user name, and the “role.” Since we replaced the previous use of JMES paths with custom functions with a more stringent model of matching and substitution, the configuration had to change in a non-compatible way, anyway, so we cleaned it up a bit and switched from a map to an array for the claims.
For the config file provider, this was already possible by adding a “role” attribute. This has now been changed into a “role” field of the user details. In order to make upgrading seamless, the “role” attribute is still accepted but a deprecation warning is logged. Since the
auth_usersconfiguration is not used for the OpenID Connect provider any more, thepassword_hashandsaltfields of the user details are now mandatory.Custom policies have been removed.
This currently is an unfinished draft PR.