Fix devlxd image export #13730

markylaing · 2024-07-09T15:55:22Z

This was caused by calling CheckPermission without considering that the handler is called internally by devlxd.

In this PR I have updated all image handlers that have AllowUntrusted set to true. The handling of authentication/authorization is clarified with comments indicating what should happen when the caller is:

A guest instance.
An untrusted client.
An untrusted client with an image secret.
An authenticated client.

Comments contextual of whether the image is public, private, or cached.

Closes #13678
Closes #13167

markylaing · 2024-07-09T16:06:33Z

@tomponline @MusicDin I'm guessing since devlxd image export was broken we don't have tests for it? I've tested it manually and from the speed of the image download in the guest it must have come from the host (the guest downloaded ubuntu:22.04 in about 3 seconds - I wish my internet connection was that fast 😆).

Another thing I wan't to be absolutely certain of is that an authenticated client can see public images in a project they don't have access to.

Shall I mark as draft for now while I work on tests? Otherwise I can put up a separate PR.

MusicDin · 2024-07-09T16:23:06Z

Thanks for this 🚀

I'm not sure how to reliably test it within CI. The instance needs internet access to fetch the fingerprint for the given image name/alias. Haven't tested, but maybe if fingerprint is provided instead of an alias, it will check for the image directly on the host. This would be handy as we can simply prevent instance's access to the internet and ensure image download does not fail.

Otherwise, I can think only of limiting the time for the image download to ensure it was fetched from the host (maybe this can be more reliable if we use some bigger images like Ubuntu).

For local development you can error out after this block:

lxd/client/simplestreams_images.go

Lines 70 to 76 in 9df996e

    
           devlxdHTTP, err := unixHTTPClient(nil, "/dev/lxd/sock", nil) 
        
           if err == nil { 
        
           	resp, err := lxdDownloadImage(fingerprint, unixURI, r.httpUserAgent, devlxdHTTP.Do, req) 
        
           	if err == nil { 
        
           		return resp, nil 
        
           	} 
        
           }

See the change in client/simplestream_images in this commit: 43fbdcc. This throws an error if image is not found on the host.

Also merging this PR should close this issue #13595

markylaing · 2024-07-10T06:54:51Z

Thanks for this 🚀

I'm not sure how to reliably test it within CI. The instance needs internet access to fetch the fingerprint for the given image name/alias. Haven't tested, but maybe if fingerprint is provided instead of an alias, it will check for the image directly on the host. This would be handy as we can simply prevent instance's access to the internet and ensure image download does not fail.

Otherwise, I can think only of limiting the time for the image download to ensure it was fetched from the host (maybe this can be more reliable if we use some bigger images like Ubuntu).

For local development you can error out after this block:

lxd/client/simplestreams_images.go

Lines 70 to 76 in 9df996e

devlxdHTTP, err := unixHTTPClient(nil, "/dev/lxd/sock", nil)

if err == nil {

resp, err := lxdDownloadImage(fingerprint, unixURI, r.httpUserAgent, devlxdHTTP.Do, req)

if err == nil {

return resp, nil

}

}

See the change in client/simplestream_images in this commit: 43fbdcc. This throws an error if image is not found on the host.

Also merging this PR should close this issue #13595

I think we should avoid calling out to external remotes if at all possible. I'll investigate whether it is possible to trick the guest LXD by using images:${test_image_full_fingerprint}. I'm hoping it will first see that the image is not cached, then call the devlxd endpoint with the full fingerprint without first trying to resolve the fingerprint as an alias. We'll see :)

markylaing · 2024-07-10T07:48:30Z

I think we should avoid calling out to external remotes if at all possible. I'll investigate whether it is possible to trick the guest LXD by using images:${test_image_full_fingerprint}. I'm hoping it will first see that the image is not cached, then call the devlxd endpoint with the full fingerprint without first trying to resolve the fingerprint as an alias. We'll see :)

My last comment was a bit shortsighted 😆. We can't run nested LXD in the busy box test image. We'll have to put tests for this into the lxd-ci repo instead.

tomponline · 2024-07-10T07:51:26Z

My last comment was a bit shortsighted 😆. We can't run nested LXD in the busy box test image. We'll have to put tests for this into the lxd-ci repo instead.

This is fine by me

markylaing · 2024-07-10T12:58:47Z

Marking as draft for a bit. Still adding tests. I need to raise a couple of issues on the back of this as well.

markylaing · 2024-07-12T12:36:16Z

@tomponline this is ready for review. The associated test PR is here: canonical/lxd-ci#240

lxd/images.go

tomponline · 2024-07-12T14:24:39Z

lxd/images.go

+	trusted := auth.IsTrusted(r.Context())
+
+	// Unauthenticated clients that do not provide a secret may only view public images.
+	// Devlxd queries can be for private images but only if they are cached.


Why are only private cached images allowed? I feel like this comment could do with expanding.

I've updated the comments here but I'm still not sure I've made it super clear to be honest.

Devlxd queries can be for public images because the image is accessible to anyone. Devlxd queries can be for private images only when the image is cached from a public remote. E.g. if the host has ubuntu:22.04 downloaded, we cache it but don't make it public.

Thanks, that makes sense.

We don't want to trigger a download of an image on the host.

BTW are we restricting which project images are accessible by /dev/lxd requests to only those of the instance's effective image project?

Currently are devlxd requests are for the default project.

I think this warrants a separate issue though. As I understand it, all cached images are stored in the default project. If I launch an ubuntu:22.04 container in project test say, and then create another ubuntu:22.04 inside it, then restricting the devlxd request to only project test would miss the cached image and download it via the ubuntu remote instead.

Another thing that doesn't make sense to me is how to actually make an image public on a host that is accessible to the nested LXD. The nested LXD doesn't have the host added as a remote, so it can't resolve the alias of the public image. As far as I can tell, this feature only works for cached images.

OK thanks, yes please can you create a separate issue.

I'm not sure that "all cached images are stored in the default project" but if /dev/lxd only servers from the default project currently then we dont need to change it in this PR.

Another thing that doesn't make sense to me is how to actually make an image public on a host that is accessible to the nested LXD. The nested LXD doesn't have the host added as a remote, so it can't resolve the alias of the public image. As far as I can tell, this feature only works for cached images.

Does lxc image import server:fingerprint work?

OK thanks, yes please can you create a separate issue.

I'm not sure that "all cached images are stored in the default project" but if /dev/lxd only servers from the default project currently then we dont need to change it in this PR.

#13768

Another thing that doesn't make sense to me is how to actually make an image public on a host that is accessible to the nested LXD. The nested LXD doesn't have the host added as a remote, so it can't resolve the alias of the public image. As far as I can tell, this feature only works for cached images.

Does lxc image import server:fingerprint work?

In canonical/lxd-ci#240 I experimented with creating an image on the host and making it public. This would prove that image export over devlxd was working because the image would not be present anywhere else. The problem is that you can't specify the host as a remote (e.g. what do you put as "server" in the command you posted), so I tried out a few things related to this comment. The methods I tried were:

(naively) lxc init "ubuntu:{host_image_fingerprint}" c1. This doesn't work because the client tries to resolve the image against the ubuntu remote.

Using lxc query -X POST /1.0/instances and providing an image source with the fingerprint of the public image on the host. For this you need to provide a remote URL, so I added the ubuntu URL. This fails because the guest LXD tries to get the image from the remote to perform architecture checks (here).

Downloading the image directly via cURL. This doesn't really work because the downloaded octet-stream needs to be split into rootfs and metadata files before you can do anything. I also didn't like this because it doesn't actually test that the guest will download an image from the host when requested, only that the socket itself works.

markylaing · 2024-07-15T12:00:29Z

Static analysis failing because I touched lxd/devlxd.go which has some pre-existing lint errors. Will fix shortly.

markylaing · 2024-07-15T12:48:38Z

Static analysis failing because I touched lxd/devlxd.go which has some pre-existing lint errors. Will fix shortly.

Now fixed.

Signed-off-by: Mark Laing <[email protected]>

…nts. Signed-off-by: Mark Laing <[email protected]>

Signed-off-by: Mark Laing <[email protected]>

lxd/devlxd.go

Signed-off-by: Mark Laing <[email protected]>

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 <[email protected]>

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 <[email protected]>

Additionally, if the `net.Conn` in `(*ConnPidMapper).ConnStateHandler` is not a (*net.UnixConn), return early to avoid a panic. Signed-off-by: Mark Laing <[email protected]>

Signed-off-by: Mark Laing <[email protected]>

Additionally standardise the error message. Signed-off-by: Mark Laing <[email protected]>

Signed-off-by: Mark Laing <[email protected]>

markylaing · 2024-07-16T11:51:43Z

@tomponline I've clarified the commits to devlxd.go and addressed your other comments.

tomponline

Thanks!

Adds tests for the devlxd socket in a container. These tests are based on the devlxd tests for VMs, with additional testing for image export over the socket. Note: I wasn't sure where else to add this test other than in the github workflow. Also note: This test will fail on edge until canonical/lxd#13730 is merged.

markylaing added the Bug Confirmed to be a bug label Jul 9, 2024

markylaing requested a review from MusicDin July 9, 2024 15:55

markylaing self-assigned this Jul 9, 2024

markylaing requested a review from tomponline as a code owner July 9, 2024 15:55

markylaing marked this pull request as draft July 10, 2024 12:58

markylaing force-pushed the check-trusted-follow-up branch from b71f344 to 2426d4d Compare July 11, 2024 11:26

markylaing mentioned this pull request Jul 12, 2024

Test image export over devlxd socket canonical/lxd-ci#240

Merged

markylaing marked this pull request as ready for review July 12, 2024 12:35

tomponline reviewed Jul 12, 2024

View reviewed changes

lxd/images.go Outdated Show resolved Hide resolved

tomponline reviewed Jul 12, 2024

View reviewed changes

lxd/images.go Show resolved Hide resolved

tomponline reviewed Jul 12, 2024

View reviewed changes

lxd/images.go Outdated Show resolved Hide resolved

tomponline reviewed Jul 12, 2024

View reviewed changes

markylaing force-pushed the check-trusted-follow-up branch from b4657aa to 931f1c1 Compare July 15, 2024 10:33

markylaing requested a review from tomponline July 15, 2024 10:34

markylaing mentioned this pull request Jul 15, 2024

Investigate whether image export requests over devlxd should use the instances' project #13768

Open

markylaing force-pushed the check-trusted-follow-up branch from 931f1c1 to 31b5ffa Compare July 15, 2024 12:48

markylaing marked this pull request as draft July 15, 2024 12:49

markylaing added 4 commits July 15, 2024 13:49

lxd: Use constant for devlxd remote address.

ef230a4

Signed-off-by: Mark Laing <[email protected]>

lxd: Clarify authentication/authorization for viewing/exporting images.

4564d6e

Signed-off-by: Mark Laing <[email protected]>

test/suites: Test public image behaviour for trusted, restricted clie…

55b3b15

…nts. Signed-off-by: Mark Laing <[email protected]>

lxd: Send image-retrieved lifecycle event when rootfs file is present.

24111f6

Signed-off-by: Mark Laing <[email protected]>

markylaing force-pushed the check-trusted-follow-up branch from 31b5ffa to caf60bd Compare July 15, 2024 12:49

markylaing marked this pull request as ready for review July 15, 2024 12:50

tomponline reviewed Jul 15, 2024

View reviewed changes

lxd/devlxd.go Show resolved Hide resolved

markylaing force-pushed the check-trusted-follow-up branch from caf60bd to ebb2cbc Compare July 16, 2024 11:36

markylaing added 8 commits July 16, 2024 12:38

lxd: Define devlxd handler functions.

7e02344

Signed-off-by: Mark Laing <[email protected]>

lxd: Define a type for a devlxd handler function.

76632d1

Signed-off-by: Mark Laing <[email protected]>

lxd: Fix lint errors (revive: unchecked-type-assertion).

44a7e7e

Additionally, if the `net.Conn` in `(*ConnPidMapper).ConnStateHandler` is not a (*net.UnixConn), return early to avoid a panic. Signed-off-by: Mark Laing <[email protected]>

lxd: Remove log formatting and use log context instead.

7d7beb8

Signed-off-by: Mark Laing <[email protected]>

lxd: Rename pidNotInContainerErr (revive: error-naming).

11fae9d

Additionally standardise the error message. Signed-off-by: Mark Laing <[email protected]>

lxd: Add comments to exported types/methods (revive: exported).

f58a9cd

Signed-off-by: Mark Laing <[email protected]>

markylaing force-pushed the check-trusted-follow-up branch from ebb2cbc to f58a9cd Compare July 16, 2024 11:50

tomponline approved these changes Jul 16, 2024

View reviewed changes

tomponline merged commit 499f5d1 into canonical:main Jul 16, 2024
26 of 28 checks passed

markylaing mentioned this pull request Jul 16, 2024

security.devlxd.images regressed somewhere after 5.0 #13595

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix devlxd image export #13730

Fix devlxd image export #13730

markylaing commented Jul 9, 2024

markylaing commented Jul 9, 2024

MusicDin commented Jul 9, 2024 •

edited

Loading

markylaing commented Jul 10, 2024

markylaing commented Jul 10, 2024

tomponline commented Jul 10, 2024

markylaing commented Jul 10, 2024

markylaing commented Jul 12, 2024

tomponline Jul 12, 2024

markylaing Jul 15, 2024

tomponline Jul 15, 2024

markylaing Jul 15, 2024

markylaing Jul 15, 2024

tomponline Jul 15, 2024

tomponline Jul 15, 2024 •

edited

Loading

markylaing Jul 15, 2024

markylaing commented Jul 15, 2024

markylaing commented Jul 15, 2024

markylaing commented Jul 16, 2024

tomponline left a comment

Fix devlxd image export #13730

Fix devlxd image export #13730

Conversation

markylaing commented Jul 9, 2024

markylaing commented Jul 9, 2024

MusicDin commented Jul 9, 2024 • edited Loading

markylaing commented Jul 10, 2024

markylaing commented Jul 10, 2024

tomponline commented Jul 10, 2024

markylaing commented Jul 10, 2024

markylaing commented Jul 12, 2024

tomponline Jul 12, 2024

Choose a reason for hiding this comment

markylaing Jul 15, 2024

Choose a reason for hiding this comment

tomponline Jul 15, 2024

Choose a reason for hiding this comment

markylaing Jul 15, 2024

Choose a reason for hiding this comment

markylaing Jul 15, 2024

Choose a reason for hiding this comment

tomponline Jul 15, 2024

Choose a reason for hiding this comment

tomponline Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

markylaing Jul 15, 2024

Choose a reason for hiding this comment

markylaing commented Jul 15, 2024

markylaing commented Jul 15, 2024

markylaing commented Jul 16, 2024

tomponline left a comment

Choose a reason for hiding this comment

MusicDin commented Jul 9, 2024 •

edited

Loading

tomponline Jul 15, 2024 •

edited

Loading