Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Auth: Use util for determining if the caller is a server administrator #13703

Merged
merged 8 commits into from
Jul 5, 2024
Merged

Auth: Use util for determining if the caller is a server administrator #13703

merged 8 commits into from
Jul 5, 2024

Conversation

markylaing
Copy link
Contributor

This avoids repeating similar logic in multiple places by additionally checking the identity cache for unrestricted clients when checking for admin privileges. See #13702 (comment)

@markylaing markylaing self-assigned this Jul 5, 2024
tomponline
tomponline reviewed Jul 5, 2024
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a lot cleaner. Tests are failing though

@markylaing markylaing marked this pull request as draft July 5, 2024 10:03
@markylaing
Copy link
Contributor Author

This looks a lot cleaner. Tests are failing though

Yeah marked as draft and fixing now.

@markylaing
lxd/auth: Add GetIdentityFromCtx util.
b37458b 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/auth: Handle unrestricted clients when checking caller privilege.
7c18075 
Additionally, rename `IsRootUserFromCtx` to `IsServerAdmin`.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/auth/drivers: Update TLS driver to use new utils.
a2da5a8 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/auth/drivers: Update OpenFGA driver to use new utils.
6345965 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/auth/drivers: Ignore the identities' project list in OpenFGA driver.
cba65fb 
When fine-grained authorization is enabled for TLS users, we will introduce
a new certificate type for which the "restricted" and "project" fields will
be considered deprecated. This simplifies permission management for these
users.

These two for loops granted operator permission in each project in the identities'
project list. They were added in preparation for TLS fine-grained auth under the
erroneous assumption that fine-grained auth would be enabled alongside existing
TLS auth methods, which will not be the case.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd: Update allowProjectResourceList to use new auth utils.
92468bb 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/identity: Move ValidateAuthenticationMethod to identity package.
500b905 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/identity: Validate authentication method when getting cache entries.
365078f 
When `core.trust_ca_certificates` is enabled, we check the identity cache
for a certificate with a matching fingerprint anyway. This is in case the
certificate does exist in the truststore and was previously restricted.

If the caller erroneously uses the new authentication method
`auth.AuthenticationMethodPKI` instead of `api.AuthenticationMethodTLS`
the identity will not be found in the cache. Returning a Not Found error
in this instance tells the authorizer that they should have admin privileges!

Adding validation on the authentication method when getting or setting cache
entries will surface these errors more transparently.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@tomponline
Copy link
Member

@markylaing cool what was the issue in the end?

@markylaing
Copy link
Contributor Author

markylaing commented Jul 5, 2024 •
edited
Loading

@markylaing cool what was the issue in the end?

Ah yes I meant to update you. It was a little bit insidious actually.

In the new util auth.GetIdentityFromCtx, the value of auth.GetAuthenticationMethod was passed in directly as the authentication method. If the authentication method was auth.AuthenticationMethodPKI, then the identity would not be found (because there are no identities with that authentication method). This led to a "not found" error being returned from GetIdentityFromCtx, which gives server admin access.

The fix was to use api.AuthenticationMethodTLS if the protocol was auth.AuthenticationMethodPKI in the call to (identity.Cache).Get here.

The last two commits move the ValidateAuthenticationMethod function from the auth package to the identity package (avoiding circular imports) and then validate the authentication method that (identity.Cache).Get is called with, returning a "bad request" error instead. This will surface the error much more transparently, should it happen again.

@markylaing markylaing marked this pull request as ready for review July 5, 2024 12:53
tomponline
tomponline approved these changes Jul 5, 2024
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent thanks

@tomponline tomponline merged commit 50fdcee into canonical:main Jul 5, 2024
28 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomponline tomponline tomponline approved these changes

Assignees

@markylaing markylaing

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@markylaing @tomponline