From 2f7a2021635848e89dd9928aa03e131a77b33e8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Graber?= Date: Tue, 5 Dec 2023 23:23:18 -0500 Subject: [PATCH 1/9] lxd: Use expanded cert fingerprint in authorizer check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stéphane Graber (cherry picked from commit 48325541674a6e7b0cecb5609c87ca45815f2550) Signed-off-by: Mark Laing License: Apache-2.0 --- lxd/certificates.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lxd/certificates.go b/lxd/certificates.go index 1ca0a9f73bbe..033d48c788e4 100644 --- a/lxd/certificates.go +++ b/lxd/certificates.go @@ -1031,7 +1031,7 @@ func certificateDelete(d *Daemon, r *http.Request) response.Response { } var userCanEditCertificate bool - err = s.Authorizer.CheckPermission(r.Context(), r, entity.CertificateURL(fingerprint), auth.EntitlementCanDelete) + err = s.Authorizer.CheckPermission(r.Context(), r, entity.CertificateURL(certInfo.Fingerprint), auth.EntitlementCanDelete) if err == nil { userCanEditCertificate = true } else if api.StatusErrorCheck(err, http.StatusForbidden) { From 7161cc0fe0bf5f286a90c13b4dfb744880a939d3 Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Tue, 13 Feb 2024 13:28:28 +0000 Subject: [PATCH 2/9] lxd: Check certificate view permission directly in handler. Because non-admin users are able to update their own certificate (only the certificate itself, non of it's LXD properties) permission checks for certificates tend to happen in the handlers themselves. In the case of `certificateGet`, writing a separate access handler would result in an extra database query to expand the fingerprint. So check permission after getting the full cert to keep things simple. Signed-off-by: Mark Laing --- lxd/certificates.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lxd/certificates.go b/lxd/certificates.go index 033d48c788e4..7c90b5931e63 100644 --- a/lxd/certificates.go +++ b/lxd/certificates.go @@ -48,7 +48,7 @@ var certificateCmd = APIEndpoint{ Path: "certificates/{fingerprint}", Delete: APIEndpointAction{Handler: certificateDelete, AccessHandler: allowAuthenticated}, - Get: APIEndpointAction{Handler: certificateGet, AccessHandler: allowPermission(entity.TypeCertificate, auth.EntitlementCanView, "fingerprint")}, + Get: APIEndpointAction{Handler: certificateGet, AccessHandler: allowAuthenticated}, Patch: APIEndpointAction{Handler: certificatePatch, AccessHandler: allowAuthenticated}, Put: APIEndpointAction{Handler: certificatePut, AccessHandler: allowAuthenticated}, } @@ -697,13 +697,14 @@ func certificatesPost(d *Daemon, r *http.Request) response.Response { // "500": // $ref: "#/responses/InternalServerError" func certificateGet(d *Daemon, r *http.Request) response.Response { + s := d.State() fingerprint, err := url.PathUnescape(mux.Vars(r)["fingerprint"]) if err != nil { return response.SmartError(err) } var cert *api.Certificate - err = d.State().DB.Cluster.Transaction(context.TODO(), func(ctx context.Context, tx *db.ClusterTx) error { + err = s.DB.Cluster.Transaction(context.TODO(), func(ctx context.Context, tx *db.ClusterTx) error { dbCertInfo, err := dbCluster.GetCertificateByFingerprintPrefix(ctx, tx.Tx(), fingerprint) if err != nil { return err @@ -716,6 +717,11 @@ func certificateGet(d *Daemon, r *http.Request) response.Response { return response.SmartError(err) } + err = s.Authorizer.CheckPermission(r.Context(), r, entity.CertificateURL(cert.Fingerprint), auth.EntitlementCanView) + if err != nil { + return response.SmartError(err) + } + return response.SyncResponseETag(true, cert, cert) } From 7bd8bf6e2b32b58c7faf599652691e224a155619 Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Mon, 19 Feb 2024 23:04:40 +0000 Subject: [PATCH 3/9] lxd/request: Add a generic methods for getting and setting context values. Signed-off-by: Mark Laing --- lxd/request/context.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 lxd/request/context.go diff --git a/lxd/request/context.go b/lxd/request/context.go new file mode 100644 index 000000000000..bd2882297586 --- /dev/null +++ b/lxd/request/context.go @@ -0,0 +1,29 @@ +package request + +import ( + "context" + "fmt" + "net/http" +) + +// GetCtxValue gets a value of type T from the context using the given key. +func GetCtxValue[T any](ctx context.Context, key CtxKey) (T, error) { + var empty T + valueAny := ctx.Value(key) + if valueAny == nil { + return empty, fmt.Errorf("Failed to get expected value %q from context", key) + } + + value, ok := valueAny.(T) + if !ok { + return empty, fmt.Errorf("Value for context key %q has incorrect type (expected %T, got %T)", key, empty, valueAny) + } + + return value, nil +} + +// SetCtxValue sets the given value in the request context with the given key. +func SetCtxValue(r *http.Request, key CtxKey, value any) { + rWithCtx := r.WithContext(context.WithValue(r.Context(), key, value)) + *r = *rWithCtx +} From 47676f61a523c1e5df449377cb5b634434e4bb38 Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Tue, 20 Feb 2024 11:51:15 +0000 Subject: [PATCH 4/9] lxd/request: Adds an effective project name context key. Signed-off-by: Mark Laing --- lxd/request/const.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lxd/request/const.go b/lxd/request/const.go index 8ba8b21cfe95..c993e0105c9c 100644 --- a/lxd/request/const.go +++ b/lxd/request/const.go @@ -37,6 +37,11 @@ const ( // This contains groups defined by the identity provider if the identity authenticated with OIDC on another cluster // member. CtxForwardedIdentityProviderGroups CtxKey = "identity_provider_groups" + + // CtxEffectiveProjectName is used to indicate that the effective project of a resource is different from the project + // specified in the URL. (For example, if a project has `features.networks=false`, any networks in this project actually + // belong to the default project). + CtxEffectiveProjectName CtxKey = "effective_project_name" ) // Headers. From 1b39a9b67e9776ac0f2afa2689b4cfcdcaf883fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Graber?= Date: Mon, 4 Dec 2023 15:41:18 -0500 Subject: [PATCH 5/9] lxd/images: Perform access control after fingerprint expansion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stéphane Graber (cherry picked from commit 504471e673714100768c93771e9e78c67b6f8804) Signed-off-by: Mark Laing License: Apache-2.0 --- lxd/images.go | 49 +++++++++++++++++++++++++------------------------ 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/lxd/images.go b/lxd/images.go index 2023d678502e..c5fa08e4cf11 100644 --- a/lxd/images.go +++ b/lxd/images.go @@ -2980,17 +2980,7 @@ func imageGet(d *Daemon, r *http.Request) response.Response { return response.SmartError(err) } - var userCanViewImage bool - err = s.Authorizer.CheckPermission(r.Context(), r, entity.ImageURL(projectName, fingerprint), auth.EntitlementCanView) - if err == nil { - userCanViewImage = true - } else if !api.StatusErrorCheck(err, http.StatusForbidden) { - return response.SmartError(err) - } - - public := d.checkTrustedClient(r) != nil || !userCanViewImage - secret := r.FormValue("secret") - + // Get the image (expand partial fingerprints). var info *api.Image err = s.DB.Cluster.Transaction(r.Context(), func(ctx context.Context, tx *db.ClusterTx) error { info, err = doImageGet(ctx, tx, projectName, fingerprint, false) @@ -3004,6 +2994,17 @@ func imageGet(d *Daemon, r *http.Request) response.Response { return response.SmartError(err) } + var userCanViewImage bool + err = s.Authorizer.CheckPermission(r.Context(), r, entity.ImageURL(projectName, info.Fingerprint), auth.EntitlementCanView) + if err == nil { + userCanViewImage = true + } else if !api.StatusErrorCheck(err, http.StatusForbidden) { + return response.SmartError(err) + } + + public := d.checkTrustedClient(r) != nil || !userCanViewImage + secret := r.FormValue("secret") + op, err := imageValidSecret(s, r, projectName, info.Fingerprint, secret) if err != nil { return response.SmartError(err) @@ -3997,20 +3998,8 @@ func imageExport(d *Daemon, r *http.Request) response.Response { return response.SmartError(err) } - // Access control. - var userCanViewImage bool - err = s.Authorizer.CheckPermission(r.Context(), r, entity.ImageURL(projectName, fingerprint), auth.EntitlementCanView) - if err == nil { - userCanViewImage = true - } else if !api.StatusErrorCheck(err, http.StatusForbidden) { - return response.SmartError(err) - } - - public := d.checkTrustedClient(r) != nil || !userCanViewImage - secret := r.FormValue("secret") - + // Get the image (expand the fingerprint). var imgInfo *api.Image - err = s.DB.Cluster.Transaction(context.TODO(), func(ctx context.Context, tx *db.ClusterTx) error { // Get the image (expand the fingerprint). _, imgInfo, err = tx.GetImage(ctx, fingerprint, dbCluster.ImageFilter{Project: &projectName}) @@ -4021,6 +4010,18 @@ func imageExport(d *Daemon, r *http.Request) response.Response { return response.SmartError(err) } + // Access control. + var userCanViewImage bool + err = s.Authorizer.CheckPermission(r.Context(), r, entity.ImageURL(projectName, imgInfo.Fingerprint), auth.EntitlementCanView) + if err == nil { + userCanViewImage = true + } else if !api.StatusErrorCheck(err, http.StatusForbidden) { + return response.SmartError(err) + } + + public := d.checkTrustedClient(r) != nil || !userCanViewImage + secret := r.FormValue("secret") + if r.RemoteAddr == "@devlxd" { if !imgInfo.Public && !imgInfo.Cached { return response.NotFound(fmt.Errorf("Image %q not found", fingerprint)) From ae11a7db680a049741ebe0aa02dfd694c3156b3d Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Tue, 20 Feb 2024 20:05:17 +0000 Subject: [PATCH 6/9] lxd/auth: Get correct project name in CheckPermission. Signed-off-by: Mark Laing --- lxd/auth/driver_tls.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lxd/auth/driver_tls.go b/lxd/auth/driver_tls.go index 828892d83e62..9c25ed7652d4 100644 --- a/lxd/auth/driver_tls.go +++ b/lxd/auth/driver_tls.go @@ -68,11 +68,15 @@ func (t *tls) CheckPermission(ctx context.Context, r *http.Request, entityURL *a return api.StatusErrorf(http.StatusForbidden, "Certificate is restricted") } - entityType, projectName, _, _, err := entity.ParseURL(entityURL.URL) + entityType, projectName, _, pathArgs, err := entity.ParseURL(entityURL.URL) if err != nil { return fmt.Errorf("Failed to parse entity URL: %w", err) } + if entityType == entity.TypeProject { + projectName = pathArgs[0] + } + // Check server level object types switch entityType { case entity.TypeServer: From 46890b572865c1d23df631472aacc6240fb646ab Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Tue, 20 Feb 2024 20:10:01 +0000 Subject: [PATCH 7/9] lxd/auth: Allow for effective projects when listing resources. Note that this approach works with the `all-projects` query parameter because `all-projects` is not allowed for restricted clients and unrestricted clients are returned a permission checker earlier in the method. Signed-off-by: Mark Laing --- lxd/auth/driver_tls.go | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/lxd/auth/driver_tls.go b/lxd/auth/driver_tls.go index 9c25ed7652d4..ce2ada41cffa 100644 --- a/lxd/auth/driver_tls.go +++ b/lxd/auth/driver_tls.go @@ -7,6 +7,7 @@ import ( "net/http" "github.com/canonical/lxd/lxd/identity" + "github.com/canonical/lxd/lxd/request" "github.com/canonical/lxd/shared" "github.com/canonical/lxd/shared/api" "github.com/canonical/lxd/shared/entity" @@ -169,6 +170,8 @@ func (t *tls) GetPermissionChecker(ctx context.Context, r *http.Request, entitle return nil, api.StatusErrorf(http.StatusForbidden, "User does not have permissions for project %q", details.projectName) } + effectiveProject, _ := request.GetCtxValue[string](r.Context(), request.CtxEffectiveProjectName) + // Filter objects by project. return func(entityURL *api.URL) bool { eType, project, _, pathArgs, err := entity.ParseURL(entityURL.URL) @@ -177,15 +180,23 @@ func (t *tls) GetPermissionChecker(ctx context.Context, r *http.Request, entitle return false } + // GetPermissionChecker can only be used to check permissions on entities of the same type, e.g. a list of instances. + if eType != entityType { + logger.Warn("Permission checker received URL with unexpected entity type", logger.Ctx{"expected": entityType, "actual": eType, "entity_url": entityURL}) + return false + } + + // If it's a project URL, the project name is in the path, not the query parameter. if eType == entity.TypeProject { project = pathArgs[0] } - if eType != entityType { - logger.Warn("Permission checker received URL with unexpected entity type", logger.Ctx{"expected": entityType, "actual": eType, "entity_url": entityURL}) - return false + // If an effective project has been set in the request context. We expect all entities to be in that project. + if effectiveProject != "" { + return project == effectiveProject } + // Otherwise, check if the project is in the list of allowed projects for the entity. return shared.ValueInSlice(project, id.Projects) }, nil } From b62c8b5e944faafa10cdd795086f68cde6b7d65d Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Tue, 20 Feb 2024 20:10:43 +0000 Subject: [PATCH 8/9] lxd: Set effective project when listing project resources. Signed-off-by: Mark Laing --- lxd/images.go | 38 +++++++++++++++++++++++++++++++++++++- lxd/network_acls.go | 1 + lxd/network_zones.go | 1 + lxd/networks.go | 2 ++ lxd/profiles.go | 1 + lxd/storage_buckets.go | 1 + lxd/storage_volumes.go | 7 ++++++- 7 files changed, 49 insertions(+), 2 deletions(-) diff --git a/lxd/images.go b/lxd/images.go index c5fa08e4cf11..58f0c6e0feae 100644 --- a/lxd/images.go +++ b/lxd/images.go @@ -1617,7 +1617,24 @@ func imagesGet(d *Daemon, r *http.Request) response.Response { filterStr := r.FormValue("filter") s := d.State() + var effectiveProjectName string + err := s.DB.Cluster.Transaction(r.Context(), func(ctx context.Context, tx *db.ClusterTx) error { + hasImages, err := dbCluster.ProjectHasImages(ctx, tx.Tx(), projectName) + if err != nil { + return err + } + if !hasImages { + effectiveProjectName = api.ProjectDefaultName + } + + return nil + }) + if err != nil { + return response.SmartError(err) + } + + request.SetCtxValue(r, request.CtxEffectiveProjectName, effectiveProjectName) hasPermission, authorizationErr := s.Authorizer.GetPermissionChecker(r.Context(), r, auth.EntitlementCanView, entity.TypeImage) if authorizationErr != nil && !api.StatusErrorCheck(authorizationErr, http.StatusForbidden) { return response.SmartError(authorizationErr) @@ -3427,10 +3444,29 @@ func imageAliasesPost(d *Daemon, r *http.Request) response.Response { // "500": // $ref: "#/responses/InternalServerError" func imageAliasesGet(d *Daemon, r *http.Request) response.Response { - projectName := request.ProjectParam(r) recursion := util.IsRecursionRequest(r) s := d.State() + + projectName := request.ProjectParam(r) + var effectiveProjectName string + err := s.DB.Cluster.Transaction(r.Context(), func(ctx context.Context, tx *db.ClusterTx) error { + projectHasImages, err := dbCluster.ProjectHasImages(ctx, tx.Tx(), projectName) + if err != nil { + return err + } + + if !projectHasImages { + effectiveProjectName = api.ProjectDefaultName + } + + return nil + }) + if err != nil { + return response.SmartError(err) + } + + request.SetCtxValue(r, request.CtxEffectiveProjectName, effectiveProjectName) userHasPermission, err := s.Authorizer.GetPermissionChecker(r.Context(), r, auth.EntitlementCanView, entity.TypeImageAlias) if err != nil { return response.InternalError(fmt.Errorf("Failed to get a permission checker: %w", err)) diff --git a/lxd/network_acls.go b/lxd/network_acls.go index 85683fb8a95b..577963b7767a 100644 --- a/lxd/network_acls.go +++ b/lxd/network_acls.go @@ -167,6 +167,7 @@ func networkACLsGet(d *Daemon, r *http.Request) response.Response { return response.InternalError(err) } + request.SetCtxValue(r, request.CtxEffectiveProjectName, projectName) userHasPermission, err := s.Authorizer.GetPermissionChecker(r.Context(), r, auth.EntitlementCanView, entity.TypeNetworkACL) if err != nil { return response.SmartError(err) diff --git a/lxd/network_zones.go b/lxd/network_zones.go index e945ef93930c..73a0c1249344 100644 --- a/lxd/network_zones.go +++ b/lxd/network_zones.go @@ -156,6 +156,7 @@ func networkZonesGet(d *Daemon, r *http.Request) response.Response { return response.InternalError(err) } + request.SetCtxValue(r, request.CtxEffectiveProjectName, projectName) userHasPermission, err := s.Authorizer.GetPermissionChecker(r.Context(), r, auth.EntitlementCanView, entity.TypeNetworkZone) if err != nil { return response.InternalError(err) diff --git a/lxd/networks.go b/lxd/networks.go index 39deb4849ec4..99bb8d65703a 100644 --- a/lxd/networks.go +++ b/lxd/networks.go @@ -176,6 +176,8 @@ func networksGet(d *Daemon, r *http.Request) response.Response { return response.SmartError(err) } + request.SetCtxValue(r, request.CtxEffectiveProjectName, projectName) + recursion := util.IsRecursionRequest(r) var networkNames []string diff --git a/lxd/profiles.go b/lxd/profiles.go index dab67a0d6181..b12bc9ac9252 100644 --- a/lxd/profiles.go +++ b/lxd/profiles.go @@ -152,6 +152,7 @@ func profilesGet(d *Daemon, r *http.Request) response.Response { recursion := util.IsRecursionRequest(r) + request.SetCtxValue(r, request.CtxEffectiveProjectName, p.Name) userHasPermission, err := s.Authorizer.GetPermissionChecker(r.Context(), r, auth.EntitlementCanView, entity.TypeProfile) if err != nil { return response.InternalError(err) diff --git a/lxd/storage_buckets.go b/lxd/storage_buckets.go index 76ffd52261ad..0448133ebabe 100644 --- a/lxd/storage_buckets.go +++ b/lxd/storage_buckets.go @@ -196,6 +196,7 @@ func storagePoolBucketsGet(d *Daemon, r *http.Request) response.Response { return response.SmartError(err) } + request.SetCtxValue(r, request.CtxEffectiveProjectName, bucketProjectName) userHasPermission, err := s.Authorizer.GetPermissionChecker(r.Context(), r, auth.EntitlementCanView, entity.TypeStorageBucket) if err != nil { return response.SmartError(err) diff --git a/lxd/storage_volumes.go b/lxd/storage_volumes.go index 9f309c2bcd59..b88b99821ee3 100644 --- a/lxd/storage_volumes.go +++ b/lxd/storage_volumes.go @@ -345,9 +345,9 @@ func storagePoolVolumesGet(d *Daemon, r *http.Request) response.Response { var dbVolumes []*db.StorageVolume var projectImages []string + var customVolProjectName string err = s.DB.Cluster.Transaction(r.Context(), func(ctx context.Context, tx *db.ClusterTx) error { - var customVolProjectName string if !allProjects { dbProject, err := cluster.GetProject(ctx, tx.Tx(), requestProjectName) @@ -453,6 +453,11 @@ func storagePoolVolumesGet(d *Daemon, r *http.Request) response.Response { return volA.Name < volB.Name }) + // If we're requesting for just one project, set the effective project name of volumes in this project. + if !allProjects { + request.SetCtxValue(r, request.CtxEffectiveProjectName, customVolProjectName) + } + userHasPermission, err := s.Authorizer.GetPermissionChecker(r.Context(), r, auth.EntitlementCanView, entity.TypeStorageVolume) if err != nil { return response.SmartError(err) From 40d791b5a0028cd493084586c23f11da09f57a16 Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Tue, 20 Feb 2024 20:48:58 +0000 Subject: [PATCH 9/9] test/suites: Ensure restricted clients can list inherited resources. Signed-off-by: Mark Laing --- test/suites/tls_restrictions.sh | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test/suites/tls_restrictions.sh b/test/suites/tls_restrictions.sh index ae6ac95de79f..ab72b2c35ce5 100644 --- a/test/suites/tls_restrictions.sh +++ b/test/suites/tls_restrictions.sh @@ -46,6 +46,36 @@ test_tls_restrictions() { ! lxc_remote project create localhost:blah1 || false + # Ensure we can create and view resources that are not enabled for the project (e.g. their effective project is + # the default project). + + # Networks are disabled when projects are created. + lxc_remote network create localhost:blah-network --project blah + lxc_remote network show localhost:blah-network --project blah + lxc_remote network list localhost: --project blah | grep blah-network + lxc_remote network rm localhost:blah-network --project blah + + # Network zones are disabled when projects are created. + lxc_remote network zone create localhost:blah-zone --project blah + lxc_remote network zone show localhost:blah-zone --project blah + lxc_remote network zone list localhost: --project blah | grep blah-zone + lxc_remote network zone delete localhost:blah-zone --project blah + + # Unset the profiles feature (the default is false). + lxc project unset blah features.profiles + lxc_remote profile create localhost:blah-profile --project blah + lxc_remote profile show localhost:blah-profile --project blah + lxc_remote profile list localhost: --project blah | grep blah-profile + lxc_remote profile delete localhost:blah-profile --project blah + + # Unset the storage volumes feature (the default is false). + lxc project unset blah features.storage.volumes + lxc_remote storage volume create "localhost:${pool_name}" blah-volume --project blah + lxc_remote storage volume show "localhost:${pool_name}" blah-volume --project blah + lxc_remote storage volume list "localhost:${pool_name}" --project blah + lxc_remote storage volume list "localhost:${pool_name}" --project blah | grep blah-volume + lxc_remote storage volume delete "localhost:${pool_name}" blah-volume --project blah + # Cleanup lxc config trust show "${FINGERPRINT}" | sed -e "s/restricted: true/restricted: false/" | lxc config trust edit "${FINGERPRINT}" lxc project delete blah