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.

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

Effective project handling #13886

Merged
merged 21 commits into from
Aug 27, 2024

Conversation

markylaing
Copy link
Contributor

@markylaing markylaing commented Aug 7, 2024

Implements the following:

  1. Always sets the effective project when querying entities that have associated features.* project config. This includes:
    i. Adding network, network zone, image, and profile specific access handlers.
    ii. For each entity type above, setting details in the request context to avoid repeated calculation (same pattern as for storage buckets and volumes).
  2. Always uses the request project in calls to (Authorizer).CheckPermission and in calls to the auth.PermissionChecker returned by (Authorizer).GetPermissionChecker.
  3. In the TLS driver, we remove effective project handling (we always expect calls to use the request project, this is what we check in their allowed project list).
  4. In the OpenFGA driver, overwrite the request project with the effective project in calls to the embedded OpenFGA server. But do not "punch through" to the default project like with the TLS driver, as these permissions can be managed by an administrator.
  5. Increased test coverage for project features with TLS authorization.
  6. Adds tests for handling of project features with fine-grained authorization.

Closes #13863

@markylaing markylaing force-pushed the effective-project-handling branch 3 times, most recently from e21d4c4 to 5f08719 Compare August 8, 2024 15:29
test/suites/auth.sh Fixed Show fixed Hide fixed
@github-actions github-actions bot added the Documentation Documentation needs updating label Aug 8, 2024
Copy link

github-actions bot commented Aug 8, 2024

Heads up @mionaalex - the "Documentation" label was applied to this issue.

@markylaing
Copy link
Contributor Author

markylaing commented Aug 8, 2024

I've created this issue to refactor the image database methods. For now there is an extra database call in image and image alias handlers to resolve the effective project and set it in the request context before the access check.

Edit: Whoops I forgot to add the link - #13896

@markylaing markylaing marked this pull request as ready for review August 8, 2024 18:48
@markylaing markylaing self-assigned this Aug 8, 2024
@markylaing markylaing force-pushed the effective-project-handling branch 2 times, most recently from 6f634ea to 9b516bf Compare August 8, 2024 19:02
@tomponline
Copy link
Member

I've created this issue to refactor the image database methods. For now there is an extra database call in image and image alias handlers to resolve the effective project and set it in the request context before the access check.

Missing link?

@markylaing
Copy link
Contributor Author

I've created this issue to refactor the image database methods. For now there is an extra database call in image and image alias handlers to resolve the effective project and set it in the request context before the access check.

Missing link?

Whoops, I've added it to the original comment.

test/includes/storage.sh Show resolved Hide resolved
test/suites/auth.sh Outdated Show resolved Hide resolved
test/includes/storage.sh Outdated Show resolved Hide resolved
test/suites/auth.sh Outdated Show resolved Hide resolved
test/suites/auth.sh Outdated Show resolved Hide resolved
test/suites/tls_restrictions.sh Outdated Show resolved Hide resolved
test/suites/tls_restrictions.sh Outdated Show resolved Hide resolved
test/suites/tls_restrictions.sh Outdated Show resolved Hide resolved
test/suites/tls_restrictions.sh Outdated Show resolved Hide resolved
test/suites/tls_restrictions.sh Outdated Show resolved Hide resolved
simondeziel
simondeziel previously approved these changes Aug 13, 2024
lxd/profiles.go Outdated Show resolved Hide resolved
lxd/profiles.go Outdated Show resolved Hide resolved
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.

So in general 2 questions:

  1. Can we avoid passing *Daemon to addFooDetailsToRequestContext?
  2. Why are the not found errors from project.FooRecord() being masked, and if there is a reason, we should have a comment explaining it, as its non-obvious.

@tomponline
Copy link
Member

@markylaing please can you look at whether func eventsSocket(s *state.State, r *http.Request, w http.ResponseWriter) error { is doing the right thing (WRT to projects/all projects ) whilst you're working in this area? Thanks

@markylaing
Copy link
Contributor Author

@markylaing please can you look at whether func eventsSocket(s *state.State, r *http.Request, w http.ResponseWriter) error { is doing the right thing (WRT to projects/all projects ) whilst you're working in this area? Thanks

The events socket is doing the right thing WRT project/all-projects. If all-projects is passed, we get a permission checker with can_view_events on projects and use that for each event that is to be sent to the client. If a project name is given, we check that the identity has can_view_events on that project and deny the request if they don't, then only send events that are in that project.

This breaks down for effective projects of course. If an event occurs in the default project because the feature is not enabled in another project, we can't resolve the original request project because there may be more than one project with that feature disabled.

@tomponline
Copy link
Member

This breaks down for effective projects of course. If an event occurs in the default project because the feature is not enabled in another project, we can't resolve the original request project because there may be more than one project with that feature disabled.

Mmm yeah that is what I was getting at. So this would mean that specifying a specific project which has certain features disabled would mean missing events for entities related to those features ? But it wouldn't mean seeing events that one shouldnt?

lxd/response/smart.go Outdated Show resolved Hide resolved
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.

I think we should decouple this PR from the 404 not found discussion so as not to prevent this from being merged. Can you rework this PR to not do any error masking and then we can revisit the discussion as a separate issue?

Adds the effective project to the request context for all profile
related requests. This follows the pattern in storage volumes and
storage buckets, whereby details about the profile that are needed for
the access check are added to the request context so that they do not
need to be recalculated.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Additionally, ensure that the effective project is always set in
the request context before calling `GetPermissionChecker`

Signed-off-by: Mark Laing <mark.laing@canonical.com>
The current usage of effective project in the TLS authorizer checks
that the project in the given entity URL is the same as the effective
project. But since the effective project is always set (for entity
type with feature flags), this means we're not checking that the entity
is in identities' project list. (This is ok, because the caller cannot
list entities in a project that they do not have access to, but is not
correct).

We now expect that all URLs passed into the PermissionChecker returned by
this function will contain the request project, so we can just check it
against the project list.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
The OpenFGA driver will not allow "punching through" to the default
project. Instead we will re-write the given entity URL to use the
effective project when performing the auth check. An administrator can
allow individual permissions against resources in the effective project
if necessary.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
This commit copies the majority of the updated `tls_restrictions` test
and asserts the behaviour of the fine-grained authorization driver.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
…ive projects.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@@ -3001,7 +3081,13 @@ func imageGet(d *Daemon, r *http.Request) response.Response {
// Get the image. We need to do this before the permission check because the URL in the permission check will not
// work with partial fingerprints.
var info *api.Image
effectiveProjectName := projectName
Copy link
Member

@tomponline tomponline Aug 27, 2024

Choose a reason for hiding this comment

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

var effectiveProjectName string would be clearer here no? Showing that its empty until its loaded from the DB below, rather than defaulting to the request project? (infact you seem to have switched to that pattern further down in this file).

@tomponline tomponline merged commit e4715fe into canonical:main Aug 27, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Documentation needs updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handling of project features.* in authorization
3 participants