Add CIFS/SMB based file sharing for windows #3308

anjannath · 2022-08-22T10:16:23Z

Solution/Idea

the default CIFS/SMB feature provided from windows
can be used to share directories over the network
we can use it to share files between host and crc
vm

Adds the following config options:

enable-shared-dirs (was present on macOS and linux now added to windows)
smb-share-password

Testing

use the msi from this PR to install crc
run crc config set enable-shared-dirs true and follow the instructions from the validation msg (it should ask to configure smb-share-password first)
run crc config set preset podman and crc setup
crc start with podman preset and bundle from Add cifs-utils package to hyperV bundle snc#565 should succeed (download from openshift CI artifacts)
container with a volume mount should work, e.g: podman run -v /mnt/home/crc/conf:/etc/http/conf httpd

packaging/windows/scripts/passwordless_share_disable.ps1

anjannath · 2022-08-22T11:54:01Z

still need to test on a clean vm if the msi is able to set everything up, and also verify the assumption that Private network profile has "File and Printer Sharing" enabled by default on window 10

anjannath · 2022-08-24T08:20:33Z

@gbraad PTAL

packaging/windows/product.wxs.template

cfergeau · 2022-08-24T12:51:41Z

packaging/windows/product.wxs.template

            Before="EnableSMBFeature"
            Sequence="execute"/>
        <CustomAction Id="EnableSMBFeature" BinaryKey="WixCA" DllEntry="WixQuietExec64" Execute="deferred" Impersonate="no" Return="ignore" />
+        <SetProperty Action="CAEnableFileAndPrinterSharing"


Is it required to enable the sharing both for private and public? We are going to only use it for host -> VM communication, so maybe one of the 2 profiles will be enough?

i think its needed to enable for both, because if we don't enable for both then we have a pre-condition that users need to be using the profile for which we enabled this to use shared dirs which i wanted avoid,

the way the profiles are set is If user's don't select a profile (when first time connecting to a new network) then windows will set the "Public" profile

which is a security feature. you don't wanna bypass this

Its not a bypass, its just that I want the shared folders to be accessible in both "Private" and "Public" network profiles as we cannot predict what profile will be active on the user's machine

also the share is created giving access to only the login/current user and needs a password to access

It is a bypass as you do not need 'public' access, as that is based on what the user has chosen.
If the users clicks this away, that is a 'user error'.

we cannot predict what profile will be active on the user's machine

Yes you can. Inform the user of proper use.

This is why you need to check for reachability of the share before using.

Password or not, that is a different problem

It is a bypass as you do not need 'public' access,

The network profiles in windows are about what type of network you are connected to (home aka private network vs cafe wifi aka public network) and not in the sense of if something is publicly accessible, based on the user's selection of the network profile some default settings are applied

Enabling it for both the profiles, lets the user have shared folders without worrying about what network profile is currently active, otherwise if we only enable it for one profile, "Private" network profile, as said we need the pre-condition "To use shared folders in windows users need to set the Private profile as the active network profile"

This is why you need to check for reachability of the share before using.

If we have properly setup the SMB share, there won't be any need to do this check, if we only enable sharing for "Private" network we can just check if the current active network profile is "Private"

based on the user's selection of the network profile some default settings are applied

And that is the point. I am not unfamiliar to these settings:

It is based on the user's selection, with the default being 'Public', which prevents access to these services, unless correctly set.

as said we need the pre-condition "To use shared folders in windows users need to set the Private profile as the active network profile"

Can you check for this?

yes there's a cmd Get-NetConnectionProfile but it also needs an interface (seems profiles are per interface, so we need to first find the active interface)
https://docs.microsoft.com/en-us/powershell/module/netconnection/get-netconnectionprofile?view=windowsserver2022-ps#example-1-get-a-connection-profile

packaging/windows/product.wxs.template

pkg/crc/config/settings.go

cfergeau · 2022-08-24T13:26:12Z

pkg/drivers/hyperv/hyperv_windows.go

+		dir.Target = convertToUnixPath(dir.Target)
+		dir.Type = "cifs"
+		dir.Tag = smbShareUNCPathFromTag(dir.Tag)
+


Imo this should be done in pkg/crc/machine/hyperv/driver_windows.go / pkg/crc/machine/config/driver.go when we are setting up the driver, rather than doing it lazily in GetSharedDirs. Better to have valid directory sharing information from the start.

removed the existing SharedDirs struct setup from pkg/crc/machine/config/driver.go and added to machine/vfkit/driver_darwin.go and machine/libvirt/driver_linux.go its the same code duplicated cba3a6c

next commit follows the same pattern and configures shared dirs for hyperv in machine/hyperv/driver_windows.go

pkg/drivers/hyperv/hyperv_windows.go

cfergeau · 2022-08-24T13:30:33Z

pkg/drivers/hyperv/hyperv_windows.go

+
+func convertToUnixPath(path string) string {
+	// converts a path like c:\users\crc to /mnt/c/users/crc
+	return fmt.Sprintf("/mnt/c%s", strings.TrimLeftFunc(filepath.ToSlash(path), func(r rune) bool {


The podman function does not hardcode c as the drive, maybe we could reuse this helper directly rather than reimplementing it? (assuming vendoring this code does not make the binary a lot bigger).

There's a 9MB increase in the output binary which is not a lot, but the amount of code that is pulled in is huge! thinking to just c&p the relevent part from that function

I'd rather see us re-use code. c&p is not just unethical, we are supposed to share and combine most of the codebase over time anyway.

Okay, apart from the extra code in vendor, we don't need the full functionality of that Function in https://github.com/containers/podman/blob/67c4068bb35fd1aad95b1701c94ed11183d0fd66/pkg/specgen/winpath.go#L23-L41

i still think just c&p the relevant line is better then vendoring the whole podman codebase!

cfergeau · 2022-08-24T13:33:06Z

pkg/drivers/hyperv/hyperv_windows.go

+func smbShareUNCPathFromTag(tag string) string {
+	hostname, err := cmdOut("hostname")
+	if err == nil {
+		return fmt.Sprintf("//%s.mshome.net/%s", strings.TrimSpace(hostname), tag)


Wondering if we could use an IP there instead of hostname.mshome.net? Not sure it's better, maybe it would make the code simpler?

we have to figure out the ip address on one of the interfaces of the host or the "Default Switch" the default switch name might be localized, but this gives the ip address for default switch (Get-NetIPAddress -InterfaceAlias "vEthernet (Default Switch)").IPv4Address

In minishift we extracted the IP address, verified reachability before allowing it to be used.

changed to use an ip address instead of hostname.mshome.net for the UNC path

pkg/drivers/hyperv/hyperv_windows.go

anjannath · 2022-08-29T18:11:37Z

pkg/crc/config/settings.go

 	KubeAdminPassword       = "kubeadmin-password"
 	Preset                  = "preset"
 	EnableSharedDirs        = "enable-shared-dirs"
+	SharedDirPassword       = "shared-dir-password" // #nosec G101


@gbraad Changed this to shared-dir-password instead of smb-share-password more generic, can be reused when we do the sshfs

password aren't shared among dirs, as they apply to each individual share.

ahh, but i couldn't find a way to set password per share in windows, either from the GUI or from the the smbshare powerhsell cmdlets

Windows bases that on the UNC path; they can be assigned a different user and password combination. In the current case of CIFS use you only deal with a USERFOLDER and not 'arbitrary' options given by the user.

gbraad · 2022-08-30T03:56:18Z

Just based on some of the comments from Christophe I say there is still some open issues; code re-use, IP address for use, etc.

packaging/windows/product.wxs.template

gbraad · 2022-08-30T04:06:58Z

pkg/crc/config/settings.go

+
+	// Shared directories configs
+	if runtime.GOOS == "windows" {
+		validateSmbPasswordSet := func(value interface{}) (bool, string) {


Why is this not a regular named function inside a _windows.go file?

Its following the same pattern that's in the config package.. and we can't use a _windows.go in there without a lot of refactor because of how we are registering the settings

so it is impossible to create a file named settings_windows.go and place this inside as a named function ?

yeah looks like, if we do that we have to duplicate all of the common settings in settings_windows.go

or RegisterSettings could call a registerOSSpecificSettings(), this should avoid the duplication.

would prefer to do this refactor separate from this PR :) moved the function definition near the other similar functions, hopefully the code looks a bit better then before

gbraad · 2022-08-30T04:08:23Z

pkg/crc/machine/config/config.go

+	SSHKeyPath        string
+	KubeConfig        string
+	SharedDirs        []string
+	SharedDirPassword string


so when we deal with sshfs, you are only able to use a single password for each of them?

similar to #3308 (comment) i was not thinking about sshfs internally that much, was only thinking about the user facing config options as of now, when implementing sshfs we might have to change this

gbraad · 2022-08-30T04:08:41Z

pkg/crc/machine/config/config.go

+	KubeConfig        string
+	SharedDirs        []string
+	SharedDirPassword string
+	SharedDirUsername string


ditto for the username?

gbraad · 2022-08-30T04:09:51Z

pkg/crc/machine/driver.go

+
+	sharedDir := []libmachine.SharedDir{}
+	for _, dir := range driver.SharedDirs {
+		dir.Password = password


so we assume all shares share the same password.
Which username is associated... do not see this relation in this part of code.

yes, for windows its based on the user account, so shares themselves don't have passwords i am not considering about sshfs here, when we implement that we might have to change this

gbraad · 2022-08-30T04:11:33Z

pkg/crc/machine/hyperv/driver_windows.go

+			Target:   convertToUnixPath(dir),
+			Tag:      smbShareUNCPathFromTag("crc-dir0"), // smb share 'crc-dir0' is created in the msi
+			Type:     "cifs",
+			Username: machineConfig.SharedDirUsername,


so we associate the username at this point, but not the password? Why do we need to do this in the previous part in an update function?

because we don't want to store the password in the machine config file, so that's handled by the update function

If you are concerned about the password, this is an association as a credential (username + password). It would make more sense to place the username in the update function too. Otherwise you are still 'leaking' information about the user.

Its not as much about leaking information but storing a secret on disk unencrypted which we want to prevent, on the user's machine any non-privilege process can just run whoami and get that username, so it should be fine to store in the machine config?

btw this was suggested in #3308 (comment)

any non-privilege process can just run whoami and get that username,

People share the virtual machine folder as we do not offer an export function.

ahh, but that'd be a very special unsupported use case, assume most of our users don't do this, also we don't test it

It turns out to be different from what I suggested earlier, but maybe this should all be coming from the keyring then?

only the password needs to be securely stored, username we can derive during start, we just skip writing it to machine config json same as the password

anjannath · 2022-09-29T11:01:11Z

PR rebased on top of #3333

gbraad

Approved for podman bundle
Unable to test for the OpenShift bundle

Follow-up issues needed for the initial flow. Can't be enabled by default as an explicit password is needed.

anjannath · 2022-09-30T13:53:54Z

/retest

anjannath · 2022-10-10T08:29:23Z

Approved for podman bundle Unable to test for the OpenShift bundle

tested with the new 4.11.7 openshift bundle and working as expected

cfergeau

Was this waiting for a second ACK? Gerard approved this a while ago.

cfergeau · 2022-10-10T16:11:09Z

pkg/crc/config/settings.go

+		if !cfg.Get(HostNetworkAccess).AsBool() {
+			return false, fmt.Sprintf("%s can only be used with %s set to 'true'",
+				EnableSharedDirs, HostNetworkAccess)
+		}


Is there any way to restrict this to only the ports needed for file sharing?
The VM having access to all the services running on the host, and also knowing the user login/password does not sound great from a security perspective. We've removed a big part of the isolation between host and guest imo.

i don't know if its possible to filter for ports, i guess that will also need changes to gvisor-tap-vsock

maybe we could try adding a :port to the NAT table entries setup during daemon start and see if that works!

cfergeau · 2022-10-10T16:11:40Z

pkg/crc/config/callbacks.go

+func RequiresCleanupAndSetupMsg(key string, _ interface{}) string {
+	return fmt.Sprintf("Changes to configuration property '%s' are only applied during 'crc setup'.\n"+
+		"Please run 'crc cleanup' followed by 'crc setup' for this configuration to take effect.", key)
+}


Follow-up issue to make this more dynamic would be useful to have.

openshift-ci · 2022-10-10T16:12:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau, gbraad

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cfergeau,gbraad]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

This enbles file and printer sharing for the 'Private' and 'Public' network profiles Sharing is enabled during install and disabled during uninstall

This creates a samba share named 'crc-dir0' of the logon user's home folder during installation

options needed for the shared dirs such as the source path, target path and types etc. differs between drivers and its best to configure those at the specific driver level although currently both libvirt and vfkit have the same options for their shared directories, hyperv will have different options to use smb instead of virtiofs

it allows users to mount the host's home folder at /mnt/c/users/$username directory in the crc vm to determine if folder sharing is supported/enabled it checks that a smb share named 'crc-dir0' exists which should've been created during msi installation since CIFS/SMB is a network file system and needs a username and password for getting access two new fields 'SharedDirUsername' and 'SharedDirPassword' is added to the MachineConfig and StartConfig structs

this adds a new error type MaskedSecretError to the errors package

currently we need this to get the username in windows for mounting the shared directories, which is CIFS/SMB based and needs the username for access

this adds an additional config 'shared-dir-password' to store the needed password for mounting the samba share and modifies the help message of 'enable-shared-dirs' config for linux and macOS the 'shared-dir-password' config is a secret config and is not listed in the reponse from the config api when getting all the config options

since we filter out the secret configs in the response from config api we need to also skip them in the api client test case to make it pass addition of the 'enable-shared-dirs' secret config makes the test fail in the previous commit this was missed in 105ad05

changes to 'host-network-access' only works when the daemon is restarted for this to happen user needs to run 'crc cleanup' and then 'crc setup' this adds a new function 'RequiresCleanupAndSetupMsg' config package

openshift-ci · 2022-10-11T05:24:58Z

New changes are detected. LGTM label has been removed.

openshift-ci bot requested review from cfergeau and praveenkumar August 22, 2022 10:16

anjannath mentioned this pull request Aug 22, 2022

Add cifs-utils package to hyperV bundle crc-org/snc#565

Merged

praveenkumar reviewed Aug 22, 2022

View reviewed changes

packaging/windows/scripts/passwordless_share_disable.ps1 Outdated Show resolved Hide resolved

anjannath force-pushed the cifs branch from 96a6947 to 6c86d34 Compare August 22, 2022 16:17

anjannath changed the title ~~Add CIFS/SMB based file sharing for windows~~ [WIP] Add CIFS/SMB based file sharing for windows Aug 23, 2022

openshift-ci bot added the do-not-merge/work-in-progress label Aug 23, 2022

anjannath force-pushed the cifs branch 4 times, most recently from 97fd763 to 2b4517a Compare August 24, 2022 08:08

anjannath changed the title ~~[WIP] Add CIFS/SMB based file sharing for windows~~ Add CIFS/SMB based file sharing for windows Aug 24, 2022

openshift-ci bot removed the do-not-merge/work-in-progress label Aug 24, 2022

anjannath added the os/windows label Aug 24, 2022

anjannath requested a review from gbraad August 24, 2022 08:20

anjannath mentioned this pull request Aug 24, 2022

CIFS/SMB based file sharing for windows #3230

Closed

cfergeau reviewed Aug 24, 2022

View reviewed changes

anjannath force-pushed the cifs branch 3 times, most recently from 526670c to 427a8cc Compare August 29, 2022 18:03

anjannath commented Aug 29, 2022

View reviewed changes

gbraad reviewed Aug 30, 2022

View reviewed changes

packaging/windows/product.wxs.template Outdated Show resolved Hide resolved

gbraad reviewed Aug 30, 2022

View reviewed changes

anjannath force-pushed the cifs branch from 549b6e5 to 2379463 Compare September 29, 2022 10:54

anjannath force-pushed the cifs branch from 764fcf7 to a58ab07 Compare September 29, 2022 15:42

gbraad approved these changes Sep 30, 2022

View reviewed changes

openshift-ci bot assigned gbraad Sep 30, 2022

openshift-ci bot added lgtm approved labels Sep 30, 2022

anjannath mentioned this pull request Sep 30, 2022

[task] initial flow for file sharing setup on windows #3371

Open

cfergeau approved these changes Oct 10, 2022

View reviewed changes

openshift-ci bot assigned cfergeau Oct 10, 2022

anjannath added 11 commits October 11, 2022 10:38

msi: Enable file and printer sharing

ddb0833

This enbles file and printer sharing for the 'Private' and 'Public' network profiles Sharing is enabled during install and disabled during uninstall

msi: Create a samba share of the user's home folder

8d05463

This creates a samba share named 'crc-dir0' of the logon user's home folder during installation

Remove extra whitespaces in product.wxs.template

655b199

update code-ready/machine go module

61a9663

Mask password in error returned when mounting CIFS share fails

17e9391

this adds a new error type MaskedSecretError to the errors package

Add helper GetCurrentUsername

3bdc52a

currently we need this to get the username in windows for mounting the shared directories, which is CIFS/SMB based and needs the username for access

Fix config change msg for config setting 'host-network-access'

88b2d94

changes to 'host-network-access' only works when the daemon is restarted for this to happen user needs to run 'crc cleanup' and then 'crc setup' this adds a new function 'RequiresCleanupAndSetupMsg' config package

anjannath force-pushed the cifs branch from a58ab07 to 88b2d94 Compare October 11, 2022 05:24

openshift-ci bot removed the lgtm label Oct 11, 2022

anjannath merged commit a1ff049 into crc-org:main Oct 11, 2022

anjannath deleted the cifs branch October 11, 2022 07:48

anjannath mentioned this pull request Mar 12, 2024

Mounting host volumes via hostPath #1409

Closed

Add CIFS/SMB based file sharing for windows #3308

Add CIFS/SMB based file sharing for windows #3308

Uh oh!

Conversation

anjannath commented Aug 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Solution/Idea

Testing

Uh oh!

Uh oh!

anjannath commented Aug 22, 2022

Uh oh!

anjannath commented Aug 24, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbraad Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anjannath Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anjannath Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbraad commented Aug 30, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anjannath Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

anjannath commented Aug 22, 2022 •

edited

Loading

gbraad Aug 30, 2022 •

edited

Loading

anjannath Aug 30, 2022 •

edited

Loading

anjannath Aug 25, 2022 •

edited

Loading

anjannath Aug 30, 2022 •

edited

Loading

gbraad Aug 30, 2022 •

edited

Loading

gbraad Aug 30, 2022 •

edited

Loading