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

Make volumes accessible on SELinux systems by adding relabel option. #3289

Merged
merged 4 commits into from
Dec 13, 2021

Conversation

tmds
Copy link
Contributor

@tmds tmds commented Nov 2, 2021

The volumes mounted by the plugin are not accessible in the container on SELinux systems like Fedora and RHEL.
They need to be relabeled.

For more info see the Labeling Volume Mounts section in https://docs.podman.io/en/latest/markdown/podman-run.1.html.

@bwateratmsft ptal.

@tmds tmds requested a review from a team as a code owner November 2, 2021 20:23
@bwateratmsft
Copy link
Collaborator

bwateratmsft commented Nov 3, 2021

I'm reluctant to have this flag added by default to all volume mounts in all docker-run tasks, especially given how ominously the docs warn about the z and Z options ("Use extreme caution...", emphasis theirs). An alternative option would be to add rw,z and ro,z as options here, which would allow end users to specify these flags if they wish. It should be noted, you can specify those values today, it will work but it will give a yellow squiggle (and a warning) (only when the document is open):

image
image

We have logic to implicitly add several volume mappings as needed for debugging .NET and Python. If there are conflicts; i.e. if the user specifies a volume mapping to the same destination in the container, the user's input will be respected. In the example above I'm taking advantage of that logic to get /src mapped with rw,z.

Do you think updating package.json with rw,z and ro,z as enum options is sufficient? Or is this likely a narrow enough scenario that the yellow squiggles can just be ignored, with no changes to the extension?

@tmds
Copy link
Contributor Author

tmds commented Nov 4, 2021

We have logic to implicitly add several volume mappings as needed for debugging .NET and Python.

More changes are needed besides updating package.json to allow the 'z' option.
I'm unable to debug .NET applications because the volumes used by the plugin don't set the z flag.

especially given how ominously the docs warn about the z and Z options

I'll look into this a bit deeper and get back to you.

@bwateratmsft
Copy link
Collaborator

In addition to the changes to package.json, you'll need to update your tasks.json to override the default volume mappings. Something like this:

        {
            "type": "docker-run",
            "label": "docker-run: debug",
            "dependsOn": [
                "docker-build: debug"
            ],
            "dockerRun": {
                "volumes": [
                    {
                        "containerPath": "/src",
                        "localPath": "${workspaceFolder}",
                        "permissions": "rw,z"
                    },
                    {
                        "containerPath": "/app",
                        "localPath": "${workspaceFolder}",
                        "permissions": "rw,z"
                    },
                    {
                        "containerPath": "/remote_debugger",
                        "localPath": "~/.vsdbg",
                        "permissions": "ro,z"
                    },
                    {
                        "containerPath": "/root/.nuget/packages",
                        "localPath": "~/.nuget/packages",
                        "permissions": "ro,z"
                    },
                ]
            },
            "netCore": {
                "appProject": "${workspaceFolder}/Net6.csproj",
                "enableDebugging": true
            }
        },

This should work today albeit with the warning squiggles. For a Windows host, just replace the ~ in the localPath's with %USERPROFILE%.

@tmds
Copy link
Contributor Author

tmds commented Nov 4, 2021

I'll look into this a bit deeper and get back to you.

The comment in the docker docs is out-of-date with the implementation. It will not accept any of the paths mentioned for relabeling.

Trying to relabel them will cause docker to fail.

That means it is not a good idea to unconditionally add the 'z' as the PR is currently doing.

We have logic to implicitly add several volume mappings as needed for debugging .NET and Python.

We should make a change that the volumes used for debugging are relabeled. Then they work out of the box on systems with SELinux.

@bwateratmsft do you have a suggestion on how to change DockerContainerVolume to support the relabling?

export interface DockerContainerVolume {
    localPath: string;
    containerPath: string;
    permissions?: 'ro' | 'rw';
}

@bwateratmsft
Copy link
Collaborator

Should we set it so that that flag is only added when enableDebugging is set to true? I suppose it's sorta moot since all these implicit volumes are only added if it's true.

For DockerContainerVolume you could just add values ro,z and rw,z, same as for package.json.

@tmds
Copy link
Contributor Author

tmds commented Nov 9, 2021

@bwateratmsft I've added the ,z flag to the debug volumes.

@tmds
Copy link
Contributor Author

tmds commented Nov 10, 2021

@bwateratmsft I had reverted the wrong commit. I've fixed it now.

Copy link
Collaborator

@bwateratmsft bwateratmsft left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@tmds
Copy link
Contributor Author

tmds commented Dec 13, 2021

@bwateratmsft I've addressed your feedback. ptal.

Copy link
Collaborator

@bwateratmsft bwateratmsft left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @tmds!

@bwateratmsft
Copy link
Collaborator

This is now released in Docker extension version 1.19.0.

@microsoft microsoft locked and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants