From ef230a4523ddbd9f179bd5cd8e6fe09168bdfb55 Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Mon, 15 Jul 2024 11:12:24 +0100 Subject: [PATCH 01/12] lxd: Use constant for devlxd remote address. Signed-off-by: Mark Laing --- lxd/daemon.go | 2 +- lxd/devlxd.go | 4 +++- lxd/images.go | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lxd/daemon.go b/lxd/daemon.go index 5d62a22d786b..4ecdac10bf4b 100644 --- a/lxd/daemon.go +++ b/lxd/daemon.go @@ -388,7 +388,7 @@ func (d *Daemon) Authenticate(w http.ResponseWriter, r *http.Request) (trusted b } // Devlxd unix socket credentials on main API. - if r.RemoteAddr == "@devlxd" { + if r.RemoteAddr == devlxdRemoteAddress { return false, "", "", nil, fmt.Errorf("Main API query can't come from /dev/lxd socket") } diff --git a/lxd/devlxd.go b/lxd/devlxd.go index 82902ea70997..1a62f31aff22 100644 --- a/lxd/devlxd.go +++ b/lxd/devlxd.go @@ -30,6 +30,8 @@ import ( "github.com/canonical/lxd/shared/ws" ) +const devlxdRemoteAddress = "@devlxd" + type hoistFunc func(f func(*Daemon, instance.Instance, http.ResponseWriter, *http.Request) response.Response, d *Daemon) func(http.ResponseWriter, *http.Request) // DevLxdServer creates an http.Server capable of handling requests against the @@ -101,7 +103,7 @@ var devlxdImageExport = devLxdHandler{"/1.0/images/{fingerprint}/export", func(d } // Use by security checks to distinguish devlxd vs lxd APIs - r.RemoteAddr = "@devlxd" + r.RemoteAddr = devlxdRemoteAddress resp := imageExport(d, r) diff --git a/lxd/images.go b/lxd/images.go index ff35a2696095..8291c63651e3 100644 --- a/lxd/images.go +++ b/lxd/images.go @@ -4033,7 +4033,7 @@ func imageExport(d *Daemon, r *http.Request) response.Response { secret := r.FormValue("secret") - if r.RemoteAddr == "@devlxd" { + if r.RemoteAddr == devlxdRemoteAddress { if !imgInfo.Public && !imgInfo.Cached { return response.NotFound(fmt.Errorf("Image %q not found", fingerprint)) } From 4564d6e76ebaec3d700e047f4678bd8c792dff7b Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Tue, 9 Jul 2024 16:41:50 +0100 Subject: [PATCH 02/12] lxd: Clarify authentication/authorization for viewing/exporting images. Signed-off-by: Mark Laing --- lxd/images.go | 160 ++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 116 insertions(+), 44 deletions(-) diff --git a/lxd/images.go b/lxd/images.go index 8291c63651e3..db5d45e846a4 100644 --- a/lxd/images.go +++ b/lxd/images.go @@ -973,6 +973,7 @@ func imagesPost(d *Daemon, r *http.Request) response.Response { projectName := request.ProjectParam(r) + // If the client is not authenticated, CheckPermission will return a http.StatusForbidden api.StatusError. var userCanCreateImages bool err := s.Authorizer.CheckPermission(r.Context(), entity.ProjectURL(projectName), auth.EntitlementCanCreateImages) if err != nil && !auth.IsDeniedError(err) { @@ -1631,14 +1632,12 @@ func imagesGet(d *Daemon, r *http.Request) response.Response { request.SetCtxValue(r, request.CtxEffectiveProjectName, effectiveProjectName) - // Check if the caller is authenticated via the request context. - trusted, err := request.GetCtxValue[bool](r.Context(), request.CtxTrusted) - if err != nil { - return response.InternalError(fmt.Errorf("Failed getting authentication status: %w", err)) - } + // If the caller is not trusted, we only want to list public images. + publicOnly := !auth.IsTrusted(r.Context()) // Get a permission checker. If the caller is not authenticated, the permission checker will deny all. - // However, the permission checker will not be called for public images. + // However, the permission checker is only called when an image is private. Both trusted and untrusted clients will + // still see public images. canViewImage, err := s.Authorizer.GetPermissionChecker(r.Context(), auth.EntitlementCanView, entity.TypeImage) if err != nil { return response.SmartError(err) @@ -1651,7 +1650,7 @@ func imagesGet(d *Daemon, r *http.Request) response.Response { var result any err = s.DB.Cluster.Transaction(r.Context(), func(ctx context.Context, tx *db.ClusterTx) error { - result, err = doImagesGet(ctx, tx, util.IsRecursionRequest(r), projectName, !trusted, clauses, canViewImage) + result, err = doImagesGet(ctx, tx, util.IsRecursionRequest(r), projectName, publicOnly, clauses, canViewImage) if err != nil { return err } @@ -2993,37 +2992,65 @@ func imageGet(d *Daemon, r *http.Request) response.Response { return response.SmartError(err) } - // Get the image (expand partial fingerprints). + trusted := auth.IsTrusted(r.Context()) + secret := r.FormValue("secret") + + // Unauthenticated clients that do not provide a secret may only view public images. + publicOnly := !trusted && secret == "" + + // 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 err = s.DB.Cluster.Transaction(r.Context(), func(ctx context.Context, tx *db.ClusterTx) error { - info, err = doImageGet(ctx, tx, projectName, fingerprint, false) + info, err = doImageGet(ctx, tx, projectName, fingerprint, publicOnly) if err != nil { return err } return nil }) - if err != nil { + if err != nil && api.StatusErrorCheck(err, http.StatusNotFound) { + // Return a generic not found. This is so that the caller cannot determine the existence of an image by the + // contents of the error message. + return response.NotFound(nil) + } else if err != nil { return response.SmartError(err) } + // Access check. var userCanViewImage bool - err = s.Authorizer.CheckPermission(r.Context(), entity.ImageURL(projectName, info.Fingerprint), auth.EntitlementCanView) - if err != nil && !auth.IsDeniedError(err) { - return response.SmartError(err) - } else if err == nil { - userCanViewImage = true - } + if secret != "" { + // If a secret was provided, validate it regardless of whether the image is public or the caller has sufficient + // privilege. This is to ensure the image token operation is cancelled. + op, err := imageValidSecret(s, r, projectName, info.Fingerprint, secret) + if err != nil { + return response.SmartError(err) + } - secret := r.FormValue("secret") + // If an operation was found the caller has access, otherwise continue to other access checks. + if op != nil { + userCanViewImage = true + } + } - op, err := imageValidSecret(s, r, projectName, info.Fingerprint, secret) - if err != nil { - return response.SmartError(err) + // No operation found for the secret. Perform other access checks. + if !userCanViewImage { + if info.Public { + // If the image is public any client can view it. + userCanViewImage = true + } else { + // Otherwise perform an access check with the full image fingerprint. + err = s.Authorizer.CheckPermission(r.Context(), entity.ImageURL(projectName, info.Fingerprint), auth.EntitlementCanView) + if err != nil && !auth.IsDeniedError(err) { + return response.SmartError(err) + } else if err == nil { + userCanViewImage = true + } + } } - // If the caller does not have permission to view the image and the secret was invalid, return generic not found. - if !info.Public && !userCanViewImage && op == nil { + // If the client still cannot view the image, return a generic not found error. + if !userCanViewImage { return response.NotFound(nil) } @@ -3597,6 +3624,8 @@ func imageAliasGet(d *Daemon, r *http.Request) response.Response { s := d.State() + // Set `userCanViewImageAlias` to true only when the caller is authenticated and can view the alias. + // We don't abort the request if this is false because the image alias may be for a public image. var userCanViewImageAlias bool err = s.Authorizer.CheckPermission(r.Context(), entity.ImageAliasURL(projectName, name), auth.EntitlementCanView) if err != nil && !auth.IsDeniedError(err) { @@ -3607,12 +3636,16 @@ func imageAliasGet(d *Daemon, r *http.Request) response.Response { var alias api.ImageAliasesEntry err = d.State().DB.Cluster.Transaction(r.Context(), func(ctx context.Context, tx *db.ClusterTx) error { + // If `userCanViewImageAlias` is false, the query will be restricted to public images only. _, alias, err = tx.GetImageAlias(ctx, projectName, name, userCanViewImageAlias) return err }) - if err != nil { + if err != nil && !api.StatusErrorCheck(err, http.StatusNotFound) { return response.SmartError(err) + } else if err != nil { + // Return a generic not found error. + return response.NotFound(nil) } return response.SyncResponseETag(true, alias, alias) @@ -4010,43 +4043,82 @@ func imageExport(d *Daemon, r *http.Request) response.Response { return response.SmartError(err) } - // Get the image (expand the fingerprint). + isDevLXDQuery := r.RemoteAddr == devlxdRemoteAddress + secret := r.FormValue("secret") + trusted := auth.IsTrusted(r.Context()) + + // Unauthenticated remote clients that do not provide a secret may only view public images. + // For devlxd, we allow querying for private images. We'll subsequently perform additional access checks. + publicOnly := !trusted && secret == "" && !isDevLXDQuery + + // 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 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}) + err = s.DB.Cluster.Transaction(r.Context(), func(ctx context.Context, tx *db.ClusterTx) error { + filter := dbCluster.ImageFilter{Project: &projectName} + if publicOnly { + filter.Public = &publicOnly + } + _, imgInfo, err = tx.GetImage(ctx, fingerprint, filter) return err }) - if err != nil { + if err != nil && api.StatusErrorCheck(err, http.StatusNotFound) { + // Return a generic not found. This is so that the caller cannot determine the existence of an image by the + // contents of the error message. + return response.NotFound(nil) + } else if err != nil { return response.SmartError(err) } // Access control. var userCanViewImage bool - err = s.Authorizer.CheckPermission(r.Context(), entity.ImageURL(projectName, imgInfo.Fingerprint), auth.EntitlementCanView) - if err != nil && !auth.IsDeniedError(err) { - return response.SmartError(err) - } else if err == nil { - userCanViewImage = true - } - - secret := r.FormValue("secret") - - if r.RemoteAddr == devlxdRemoteAddress { - if !imgInfo.Public && !imgInfo.Cached { - return response.NotFound(fmt.Errorf("Image %q not found", fingerprint)) - } - } else { + if secret != "" { + // If a secret was provided, validate it regardless of whether the image is public or the caller has sufficient + // privilege. This is to ensure the image token operation is cancelled. op, err := imageValidSecret(s, r, projectName, imgInfo.Fingerprint, secret) if err != nil { return response.SmartError(err) } - // If the image is not public and the caller cannot view it, return a generic not found error. - if !imgInfo.Public && !userCanViewImage && op == nil { + // If an operation was found the caller has access, otherwise continue to other access checks. + if op != nil { + userCanViewImage = true + } + } + + if isDevLXDQuery { + // A devlxd query must contain the full fingerprint of the image (no partials). + if fingerprint != imgInfo.Fingerprint { + return response.NotFound(nil) + } + + // A devlxd query must be for a public or cached image. + if !(imgInfo.Public || imgInfo.Cached) { return response.NotFound(nil) } + + userCanViewImage = true + } + + if !userCanViewImage { + if imgInfo.Public { + // If the image is public any client can view it. + userCanViewImage = true + } else { + // Otherwise perform an access check with the full image fingerprint. + err = s.Authorizer.CheckPermission(r.Context(), entity.ImageURL(projectName, imgInfo.Fingerprint), auth.EntitlementCanView) + if err != nil && !auth.IsDeniedError(err) { + return response.SmartError(err) + } else if err == nil { + userCanViewImage = true + } + } + } + + // If the client still cannot view the image, return a generic not found error. + if !userCanViewImage { + return response.NotFound(nil) } var address string From 55b3b157a3f92932334bdd8e613837271f1fa0cc Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Thu, 11 Jul 2024 12:12:17 +0100 Subject: [PATCH 03/12] test/suites: Test public image behaviour for trusted, restricted clients. Signed-off-by: Mark Laing --- test/suites/tls_restrictions.sh | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/test/suites/tls_restrictions.sh b/test/suites/tls_restrictions.sh index 5f807b83e9ed..d898d864b078 100644 --- a/test/suites/tls_restrictions.sh +++ b/test/suites/tls_restrictions.sh @@ -60,17 +60,34 @@ test_tls_restrictions() { ! lxc_remote storage volume list "localhost:${pool_name}" --project default || false ! lxc_remote storage bucket list "localhost:${pool_name}" --project default || false - # Can still list images as some may be public. There are no public images in the default project now, - # so the list should be empty - [ "$(lxc_remote image list localhost --project default --format csv)" = "" ] + ### Validate images. + test_image_fingerprint="$(lxc image info testimage --project default | awk '/^Fingerprint/ {print $2}')" - # Set up the test image in the blah project (ensure_import_testimage imports the image into the current project). - lxc project switch blah && ensure_import_testimage && lxc project switch default + # We can always list images, but there are no public images in the default project now, so the list should be empty. + [ "$(lxc_remote image list localhost: --project default --format csv)" = "" ] + ! lxc_remote image show localhost:testimage --project default || false + + # Set the image to public and ensure we can view it. + lxc image show testimage --project default | sed -e "s/public: false/public: true/" | lxc image edit testimage --project default + [ "$(lxc_remote image list localhost: --project default --format csv | wc -l)" = 1 ] + lxc_remote image show localhost:testimage --project default + + # Check we can export the public image: + lxc image export localhost:testimage "${LXD_DIR}/" --project default + [ "${test_image_fingerprint}" = "$(sha256sum "${LXD_DIR}/${test_image_fingerprint}.tar.xz" | cut -d' ' -f1)" ] + rm "${LXD_DIR}/${test_image_fingerprint}.tar.xz" + + # While the image is public, copy it to the blah project and create an alias for it. + lxc_remote image copy localhost:testimage localhost: --project default --target-project blah + lxc_remote image alias create localhost:testimage "${test_image_fingerprint}" --project blah + + # Restore privacy on the test image in the default project. + lxc image show testimage --project default | sed -e "s/public: true/public: false/" | lxc image edit testimage --project default # Set up a profile in the blah project. Additionally ensures restricted TLS clients can edit profiles in projects they have access to. lxc profile show default | lxc_remote profile edit localhost:default --project blah - # Create an instance. + # Create an instance (using the test image copied from the default project while it was public). lxc_remote init testimage localhost:blah-instance --project blah # Create a custom volume. From 24111f6af1946a8e8936d33b3e8df3d0f5c865cd Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Fri, 12 Jul 2024 13:30:15 +0100 Subject: [PATCH 04/12] lxd: Send image-retrieved lifecycle event when rootfs file is present. Signed-off-by: Mark Laing --- lxd/images.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lxd/images.go b/lxd/images.go index db5d45e846a4..341ae033a6d6 100644 --- a/lxd/images.go +++ b/lxd/images.go @@ -4178,6 +4178,9 @@ func imageExport(d *Daemon, r *http.Request) response.Response { files[1].Path = rootfsPath files[1].Filename = filename + requestor := request.CreateRequestor(r) + s.Events.SendLifecycle(projectName, lifecycle.ImageRetrieved.Event(imgInfo.Fingerprint, projectName, requestor, nil)) + return response.FileResponse(r, files, nil) } From 7e023445bacb13bb1b53775dc8eac0abec19fb69 Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Tue, 16 Jul 2024 12:16:39 +0100 Subject: [PATCH 05/12] lxd: Define devlxd handler functions. Signed-off-by: Mark Laing --- lxd/devlxd.go | 41 +++++++++++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/lxd/devlxd.go b/lxd/devlxd.go index 1a62f31aff22..d943364f75a9 100644 --- a/lxd/devlxd.go +++ b/lxd/devlxd.go @@ -56,7 +56,9 @@ type devLxdHandler struct { f func(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response } -var devlxdConfigGet = devLxdHandler{"/1.0/config", func(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { +var devlxdConfigGet = devLxdHandler{"/1.0/config", devlxdConfigGetHandler} + +func devlxdConfigGetHandler(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { if shared.IsFalse(c.ExpandedConfig()["security.devlxd"]) { return response.DevLxdErrorResponse(api.StatusErrorf(http.StatusForbidden, "not authorized"), c.Type() == instancetype.VM) } @@ -69,9 +71,11 @@ var devlxdConfigGet = devLxdHandler{"/1.0/config", func(d *Daemon, c instance.In } return response.DevLxdResponse(http.StatusOK, filtered, "json", c.Type() == instancetype.VM) -}} +} -var devlxdConfigKeyGet = devLxdHandler{"/1.0/config/{key}", func(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { +var devlxdConfigKeyGet = devLxdHandler{"/1.0/config/{key}", devlxdConfigKeyGetHandler} + +func devlxdConfigKeyGetHandler(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { if shared.IsFalse(c.ExpandedConfig()["security.devlxd"]) { return response.DevLxdErrorResponse(api.StatusErrorf(http.StatusForbidden, "not authorized"), c.Type() == instancetype.VM) } @@ -91,9 +95,11 @@ var devlxdConfigKeyGet = devLxdHandler{"/1.0/config/{key}", func(d *Daemon, c in } return response.DevLxdResponse(http.StatusOK, value, "raw", c.Type() == instancetype.VM) -}} +} -var devlxdImageExport = devLxdHandler{"/1.0/images/{fingerprint}/export", func(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { +var devlxdImageExport = devLxdHandler{"/1.0/images/{fingerprint}/export", devlxdImageExportHandler} + +func devlxdImageExportHandler(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { if shared.IsFalse(c.ExpandedConfig()["security.devlxd"]) { return response.DevLxdErrorResponse(api.StatusErrorf(http.StatusForbidden, "not authorized"), c.Type() == instancetype.VM) } @@ -113,9 +119,11 @@ var devlxdImageExport = devLxdHandler{"/1.0/images/{fingerprint}/export", func(d } return response.DevLxdResponse(http.StatusOK, "", "raw", c.Type() == instancetype.VM) -}} +} + +var devlxdMetadataGet = devLxdHandler{"/1.0/meta-data", devlxdMetadataGetHandler} -var devlxdMetadataGet = devLxdHandler{"/1.0/meta-data", func(d *Daemon, inst instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { +func devlxdMetadataGetHandler(d *Daemon, inst instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { if shared.IsFalse(inst.ExpandedConfig()["security.devlxd"]) { return response.DevLxdErrorResponse(api.StatusErrorf(http.StatusForbidden, "not authorized"), inst.Type() == instancetype.VM) } @@ -123,9 +131,11 @@ var devlxdMetadataGet = devLxdHandler{"/1.0/meta-data", func(d *Daemon, inst ins value := inst.ExpandedConfig()["user.meta-data"] return response.DevLxdResponse(http.StatusOK, fmt.Sprintf("#cloud-config\ninstance-id: %s\nlocal-hostname: %s\n%s", inst.CloudInitID(), inst.Name(), value), "raw", inst.Type() == instancetype.VM) -}} +} + +var devlxdEventsGet = devLxdHandler{"/1.0/events", devlxdEventsGetHandler} -var devlxdEventsGet = devLxdHandler{"/1.0/events", func(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { +func devlxdEventsGetHandler(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { if shared.IsFalse(c.ExpandedConfig()["security.devlxd"]) { return response.DevLxdErrorResponse(api.StatusErrorf(http.StatusForbidden, "not authorized"), c.Type() == instancetype.VM) } @@ -180,9 +190,11 @@ var devlxdEventsGet = devLxdHandler{"/1.0/events", func(d *Daemon, c instance.In listener.Wait(r.Context()) return resp -}} +} -var devlxdAPIHandler = devLxdHandler{"/1.0", func(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { +var devlxdAPIHandler = devLxdHandler{"/1.0", devlxdAPIHandlerFunc} + +func devlxdAPIHandlerFunc(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { s := d.State() if r.Method == "GET" { @@ -238,10 +250,11 @@ var devlxdAPIHandler = devLxdHandler{"/1.0", func(d *Daemon, c instance.Instance } return response.DevLxdErrorResponse(api.StatusErrorf(http.StatusMethodNotAllowed, fmt.Sprintf("method %q not allowed", r.Method)), c.Type() == instancetype.VM) +} -}} +var devlxdDevicesGet = devLxdHandler{"/1.0/devices", devlxdDevicesGetHandler} -var devlxdDevicesGet = devLxdHandler{"/1.0/devices", func(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { +func devlxdDevicesGetHandler(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { if shared.IsFalse(c.ExpandedConfig()["security.devlxd"]) { return response.DevLxdErrorResponse(api.StatusErrorf(http.StatusForbidden, "not authorized"), c.Type() == instancetype.VM) } @@ -258,7 +271,7 @@ var devlxdDevicesGet = devLxdHandler{"/1.0/devices", func(d *Daemon, c instance. } return response.DevLxdResponse(http.StatusOK, c.ExpandedDevices(), "json", c.Type() == instancetype.VM) -}} +} var handlers = []devLxdHandler{ {"/", func(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { From 76632d14be40e623177b6dda916d19ccd37041a0 Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Tue, 16 Jul 2024 12:17:43 +0100 Subject: [PATCH 06/12] lxd: Define a type for a devlxd handler function. Signed-off-by: Mark Laing --- lxd/devlxd.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lxd/devlxd.go b/lxd/devlxd.go index d943364f75a9..71169a01b02d 100644 --- a/lxd/devlxd.go +++ b/lxd/devlxd.go @@ -34,6 +34,8 @@ const devlxdRemoteAddress = "@devlxd" type hoistFunc func(f func(*Daemon, instance.Instance, http.ResponseWriter, *http.Request) response.Response, d *Daemon) func(http.ResponseWriter, *http.Request) +type devlxdHandlerFunc func(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response + // DevLxdServer creates an http.Server capable of handling requests against the // /dev/lxd Unix socket endpoint created inside containers. func devLxdServer(d *Daemon) *http.Server { From 1b454b49e13d14f99a284e7d95885ed2f8262e9b Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Tue, 16 Jul 2024 12:18:26 +0100 Subject: [PATCH 07/12] lxd: Rename the `f` field of the devlxdHandler type. Go conventions state that single letter variable names should be used for receivers, for familiar types (e.g. r for io.Reader or *http.Request) or in very short scopes such as loops. They shouldn't be used for field names. See https://google.github.io/styleguide/go/decisions.html#single-letter-variable-names. Signed-off-by: Mark Laing --- lxd/devlxd.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lxd/devlxd.go b/lxd/devlxd.go index 71169a01b02d..ea91ba1379da 100644 --- a/lxd/devlxd.go +++ b/lxd/devlxd.go @@ -55,7 +55,7 @@ type devLxdHandler struct { * server side right now either, I went the simple route to avoid * needless noise. */ - f func(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response + handlerFunc devlxdHandlerFunc } var devlxdConfigGet = devLxdHandler{"/1.0/config", devlxdConfigGetHandler} @@ -329,7 +329,7 @@ func devLxdAPI(d *Daemon, f hoistFunc) http.Handler { m.UseEncodedPath() // Allow encoded values in path segments. for _, handler := range handlers { - m.HandleFunc(handler.path, f(handler.f, d)) + m.HandleFunc(handler.path, f(handler.handlerFunc, d)) } return m From 80f3210c96e5a96fa809801a7b644fc03efdf591 Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Tue, 16 Jul 2024 12:29:06 +0100 Subject: [PATCH 08/12] lxd: Specify field names in devlxd handler definitions. Field names are optional if private according to the Go style guide (https://google.github.io/styleguide/go/decisions.html#field-names), but for consistency in our code-base we should include the field names. Signed-off-by: Mark Laing --- lxd/devlxd.go | 44 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/lxd/devlxd.go b/lxd/devlxd.go index ea91ba1379da..140f1153903b 100644 --- a/lxd/devlxd.go +++ b/lxd/devlxd.go @@ -58,7 +58,10 @@ type devLxdHandler struct { handlerFunc devlxdHandlerFunc } -var devlxdConfigGet = devLxdHandler{"/1.0/config", devlxdConfigGetHandler} +var devlxdConfigGet = devLxdHandler{ + path: "/1.0/config", + handlerFunc: devlxdConfigGetHandler, +} func devlxdConfigGetHandler(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { if shared.IsFalse(c.ExpandedConfig()["security.devlxd"]) { @@ -75,7 +78,10 @@ func devlxdConfigGetHandler(d *Daemon, c instance.Instance, w http.ResponseWrite return response.DevLxdResponse(http.StatusOK, filtered, "json", c.Type() == instancetype.VM) } -var devlxdConfigKeyGet = devLxdHandler{"/1.0/config/{key}", devlxdConfigKeyGetHandler} +var devlxdConfigKeyGet = devLxdHandler{ + path: "/1.0/config/{key}", + handlerFunc: devlxdConfigKeyGetHandler, +} func devlxdConfigKeyGetHandler(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { if shared.IsFalse(c.ExpandedConfig()["security.devlxd"]) { @@ -99,7 +105,10 @@ func devlxdConfigKeyGetHandler(d *Daemon, c instance.Instance, w http.ResponseWr return response.DevLxdResponse(http.StatusOK, value, "raw", c.Type() == instancetype.VM) } -var devlxdImageExport = devLxdHandler{"/1.0/images/{fingerprint}/export", devlxdImageExportHandler} +var devlxdImageExport = devLxdHandler{ + path: "/1.0/images/{fingerprint}/export", + handlerFunc: devlxdImageExportHandler, +} func devlxdImageExportHandler(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { if shared.IsFalse(c.ExpandedConfig()["security.devlxd"]) { @@ -123,7 +132,10 @@ func devlxdImageExportHandler(d *Daemon, c instance.Instance, w http.ResponseWri return response.DevLxdResponse(http.StatusOK, "", "raw", c.Type() == instancetype.VM) } -var devlxdMetadataGet = devLxdHandler{"/1.0/meta-data", devlxdMetadataGetHandler} +var devlxdMetadataGet = devLxdHandler{ + path: "/1.0/meta-data", + handlerFunc: devlxdMetadataGetHandler, +} func devlxdMetadataGetHandler(d *Daemon, inst instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { if shared.IsFalse(inst.ExpandedConfig()["security.devlxd"]) { @@ -135,7 +147,10 @@ func devlxdMetadataGetHandler(d *Daemon, inst instance.Instance, w http.Response return response.DevLxdResponse(http.StatusOK, fmt.Sprintf("#cloud-config\ninstance-id: %s\nlocal-hostname: %s\n%s", inst.CloudInitID(), inst.Name(), value), "raw", inst.Type() == instancetype.VM) } -var devlxdEventsGet = devLxdHandler{"/1.0/events", devlxdEventsGetHandler} +var devlxdEventsGet = devLxdHandler{ + path: "/1.0/events", + handlerFunc: devlxdEventsGetHandler, +} func devlxdEventsGetHandler(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { if shared.IsFalse(c.ExpandedConfig()["security.devlxd"]) { @@ -194,7 +209,10 @@ func devlxdEventsGetHandler(d *Daemon, c instance.Instance, w http.ResponseWrite return resp } -var devlxdAPIHandler = devLxdHandler{"/1.0", devlxdAPIHandlerFunc} +var devlxdAPIHandler = devLxdHandler{ + path: "/1.0", + handlerFunc: devlxdAPIHandlerFunc, +} func devlxdAPIHandlerFunc(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { s := d.State() @@ -254,7 +272,10 @@ func devlxdAPIHandlerFunc(d *Daemon, c instance.Instance, w http.ResponseWriter, return response.DevLxdErrorResponse(api.StatusErrorf(http.StatusMethodNotAllowed, fmt.Sprintf("method %q not allowed", r.Method)), c.Type() == instancetype.VM) } -var devlxdDevicesGet = devLxdHandler{"/1.0/devices", devlxdDevicesGetHandler} +var devlxdDevicesGet = devLxdHandler{ + path: "/1.0/devices", + handlerFunc: devlxdDevicesGetHandler, +} func devlxdDevicesGetHandler(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { if shared.IsFalse(c.ExpandedConfig()["security.devlxd"]) { @@ -276,9 +297,12 @@ func devlxdDevicesGetHandler(d *Daemon, c instance.Instance, w http.ResponseWrit } var handlers = []devLxdHandler{ - {"/", func(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { - return response.DevLxdResponse(http.StatusOK, []string{"/1.0"}, "json", c.Type() == instancetype.VM) - }}, + { + path: "/", + handlerFunc: func(d *Daemon, c instance.Instance, w http.ResponseWriter, r *http.Request) response.Response { + return response.DevLxdResponse(http.StatusOK, []string{"/1.0"}, "json", c.Type() == instancetype.VM) + }, + }, devlxdAPIHandler, devlxdConfigGet, devlxdConfigKeyGet, From 44a7e7e67929fa166baef5be47b6b867c36ade1b Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Mon, 15 Jul 2024 13:29:53 +0100 Subject: [PATCH 09/12] lxd: Fix lint errors (revive: unchecked-type-assertion). Additionally, if the `net.Conn` in `(*ConnPidMapper).ConnStateHandler` is not a (*net.UnixConn), return early to avoid a panic. Signed-off-by: Mark Laing --- lxd/devlxd.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/lxd/devlxd.go b/lxd/devlxd.go index 140f1153903b..8f21c77aa1bb 100644 --- a/lxd/devlxd.go +++ b/lxd/devlxd.go @@ -392,7 +392,12 @@ type ConnPidMapper struct { } func (m *ConnPidMapper) ConnStateHandler(conn net.Conn, state http.ConnState) { - unixConn := conn.(*net.UnixConn) + unixConn, _ := conn.(*net.UnixConn) + if unixConn == nil { + logger.Error("Invalid type for devlxd connection", logger.Ctx{"conn_type": fmt.Sprintf("%T", conn)}) + return + } + switch state { case http.StateNew: cred, err := ucred.GetCred(unixConn) @@ -478,7 +483,9 @@ func findContainerForPid(pid int32, s *state.State) (instance.Container, error) return nil, fmt.Errorf("Instance is not container type") } - return inst.(instance.Container), nil + // Explicitly ignore type assertion check. We've just checked that it's a container. + c, _ := inst.(instance.Container) + return c, nil } status, err := os.ReadFile(fmt.Sprintf("/proc/%d/status", pid)) @@ -531,7 +538,9 @@ func findContainerForPid(pid int32, s *state.State) (instance.Container, error) } if origPidNs == pidNs { - return inst.(instance.Container), nil + // Explicitly ignore type assertion check. The instance must be a container if we've found it via the process ID. + c, _ := inst.(instance.Container) + return c, nil } } From 7d7beb8b131a31720cb2b383d8a85f4bf810f5ea Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Mon, 15 Jul 2024 13:33:21 +0100 Subject: [PATCH 10/12] lxd: Remove log formatting and use log context instead. Signed-off-by: Mark Laing --- lxd/devlxd.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lxd/devlxd.go b/lxd/devlxd.go index 8f21c77aa1bb..017bcb47b78c 100644 --- a/lxd/devlxd.go +++ b/lxd/devlxd.go @@ -402,7 +402,7 @@ func (m *ConnPidMapper) ConnStateHandler(conn net.Conn, state http.ConnState) { case http.StateNew: cred, err := ucred.GetCred(unixConn) if err != nil { - logger.Debugf("Error getting ucred for conn %s", err) + logger.Debug("Error getting ucred for devlxd connection", logger.Ctx{"error": err}) } else { m.mLock.Lock() m.m[unixConn] = cred @@ -430,7 +430,7 @@ func (m *ConnPidMapper) ConnStateHandler(conn net.Conn, state http.ConnState) { delete(m.m, unixConn) m.mLock.Unlock() default: - logger.Debugf("Unknown state for connection %s", state) + logger.Debug("Unknown state for devlxd connection", logger.Ctx{"state": state.String()}) } } From 11fae9d2efb761a5ec8ddd220d4410e5a90911fc Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Mon, 15 Jul 2024 13:36:06 +0100 Subject: [PATCH 11/12] lxd: Rename pidNotInContainerErr (revive: error-naming). Additionally standardise the error message. Signed-off-by: Mark Laing --- lxd/devlxd.go | 7 ++++--- lxd/devlxd_test.go | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/lxd/devlxd.go b/lxd/devlxd.go index 017bcb47b78c..692324785885 100644 --- a/lxd/devlxd.go +++ b/lxd/devlxd.go @@ -2,6 +2,7 @@ package main import ( "encoding/json" + "errors" "fmt" "net" "net/http" @@ -317,7 +318,7 @@ func hoistReq(f func(*Daemon, instance.Instance, http.ResponseWriter, *http.Requ conn := ucred.GetConnFromContext(r.Context()) cred, ok := pidMapper.m[conn.(*net.UnixConn)] if !ok { - http.Error(w, pidNotInContainerErr.Error(), http.StatusInternalServerError) + http.Error(w, errPIDNotInContainer.Error(), http.StatusInternalServerError) return } @@ -434,7 +435,7 @@ func (m *ConnPidMapper) ConnStateHandler(conn net.Conn, state http.ConnState) { } } -var pidNotInContainerErr = fmt.Errorf("pid not in container?") +var errPIDNotInContainer = errors.New("Process ID not found in container") func findContainerForPid(pid int32, s *state.State) (instance.Container, error) { /* @@ -544,5 +545,5 @@ func findContainerForPid(pid int32, s *state.State) (instance.Container, error) } } - return nil, pidNotInContainerErr + return nil, errPIDNotInContainer } diff --git a/lxd/devlxd_test.go b/lxd/devlxd_test.go index 1ad667c4ac8a..6c21bae7c03f 100644 --- a/lxd/devlxd_test.go +++ b/lxd/devlxd_test.go @@ -169,7 +169,7 @@ func TestHttpRequest(t *testing.T) { t.Fatal(err) } - if !strings.Contains(string(resp), pidNotInContainerErr.Error()) { + if !strings.Contains(string(resp), errPIDNotInContainer.Error()) { t.Fatal("resp error not expected: ", string(resp)) } } From f58a9cd1d206f7b730f267f1b10c6bbf71935428 Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Mon, 15 Jul 2024 13:46:39 +0100 Subject: [PATCH 12/12] lxd: Add comments to exported types/methods (revive: exported). Signed-off-by: Mark Laing --- lxd/devlxd.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lxd/devlxd.go b/lxd/devlxd.go index 692324785885..45322a2b2196 100644 --- a/lxd/devlxd.go +++ b/lxd/devlxd.go @@ -387,11 +387,15 @@ func devLxdAPI(d *Daemon, f hoistFunc) http.Handler { */ var pidMapper = ConnPidMapper{m: map[*net.UnixConn]*unix.Ucred{}} +// ConnPidMapper is threadsafe cache of unix connections to process IDs. We use this in hoistReq to determine +// the instance that the connection has been made from. type ConnPidMapper struct { m map[*net.UnixConn]*unix.Ucred mLock sync.Mutex } +// ConnStateHandler is used in the `ConnState` field of the devlxd http.Server so that we can cache the process ID of the +// caller when a new connection is made and delete it when the connection is closed. func (m *ConnPidMapper) ConnStateHandler(conn net.Conn, state http.ConnState) { unixConn, _ := conn.(*net.UnixConn) if unixConn == nil {