Skip to content

remove enabling of file sharing and creation of smbshare from MSI #4620

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

remove enabling of file sharing and creation of smbshare from MSI #4620

wants to merge 3 commits into from

Conversation

anjannath
Copy link
Member

@anjannath anjannath commented Feb 14, 2025 •
edited
Loading

fixes #4619

@openshift-ci openshift-ci bot requested review from cfergeau and gbraad February 14, 2025 11:28
@anjannath anjannath changed the title wip: remove enabling of file sharing a creation of smbshare from MSI remove enabling of file sharing a creation of smbshare from MSI Feb 18, 2025
@anjannath anjannath changed the title remove enabling of file sharing a creation of smbshare from MSI remove enabling of file sharing and creation of smbshare from MSI Feb 18, 2025
@praveenkumar
Copy link
Member

@anjannath didn't we have this as part of preflight before and then moved it to MSI? if yes then we might to revert those commits instead of this?

@anjannath
Copy link
Member Author

@anjannath didn't we have this as part of preflight before and then moved it to MSI? if yes then we might to revert those commits instead of this?

no, i also thought the same and i did check the logs before creating the PR, seems this was never added as preflight checks, i think because when we added this SMB sharing support we were already moving every privileged operation on windows already to MSI so when we added support for SMB directory sharing this was only added to the MSI..

$ git log --grep="smb" --grep="cifs" --oneline
fff474a33 msi: remove the smb share created during installation
2aa7059b4 msi: ignore errors from custom action creating the smb shared dir
4a52aef4e cifs: Add 'enable-shared-dirs' config for windows
b4cfc0ec5 add CIFS/SMB based file sharing for windows
2785de06d move initialization of SharedDirs to driver's CreatHost function

cfergeau
cfergeau reviewed Apr 10, 2025
View reviewed changes
Copy link
Contributor

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

this should remove at least of the warnings reported by virustotal for
the MSI:

One concern I have is if virustotal starts reporting this against the crc binary instead of reporting that against the msi

pkg/crc/preflight/preflight_darwin_test.go
assert.Len(t, getPreflightChecks(true, network.UserNetworkingMode, constants.GetDefaultBundlePath(preset.OpenShift), preset.OpenShift, false), 20)
assert.Len(t, getPreflightChecks(true, network.UserNetworkingMode, constants.GetDefaultBundlePath(preset.OpenShift), preset.OpenShift, false), 20)
assert.Len(t, getPreflightChecks(true, network.UserNetworkingMode, constants.GetDefaultBundlePath(preset.OpenShift), preset.OpenShift, false, false), 20)
assert.Len(t, getPreflightChecks(true, network.UserNetworkingMode, constants.GetDefaultBundlePath(preset.OpenShift), preset.OpenShift, false, false), 20)
Copy link
Contributor

Choose a reason for hiding this comment

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

There are several changes to getPreflightChecks calls in this commit to add one argument, they seem to belong to the previous commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

packaging/windows/product.wxs.template
@@ -92,24 +92,6 @@
Before="AddUserToHypervAdminGroup"
Sequence="execute"/>
<CustomAction Id="AddUserToHypervAdminGroup" BinaryKey="WixCA" DllEntry="WixQuietExec64" Execute="deferred" Impersonate="no" Return="ignore" />
<SetProperty Action="CACreateSMBShare"
Id="CreateSMBShare"
Value="&quot;[POWERSHELLEXE]&quot; -NonInteractive -ExecutionPolicy Bypass -NoProfile -Command &quot;New-SmbShare -Name '[SHAREDDIRNAME]' -Path '[USERFOLDER]' -FullAccess '[LogonUser]'&quot;"
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you should also be removing

        <Property Id="USERFOLDER">
            <DirectorySearch Id="userProfileSearch" Depth="0" Path="[%USERPROFILE]" />
        </Property>

        <Property Id="SHAREDDIRNAME" Secure="yes">crc-dir0</Property>

and possibly other properties? (powershell, logonuser, …)

Copy link
Member Author

Choose a reason for hiding this comment

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

we need to keep powershell and logonuser these are used by the installHyperv custom action

anjannath added 3 commits May 15, 2025 16:47
@anjannath
remove enabling of file sharing and creation of smbshare from MSI
d830aa7 
this will be moved to the 'crc setup' command as we should only enable
file sharing and create the smb share for home dir when users have set
the config option 'enable-shared-dirs' to 'true'

this should remove at least of the warnings reported by virustotal for
the MSI:
https://www.virustotal.com/gui/file/31b402dcc1da24265074a21a26018d6cde8eef0b63c77a18f89eb079b6556790
@anjannath
preflight: add new label SharedDir for checks
8b82bdd 
this label will be used to filter out preflight checks
that should only run when 'enable-shared-dir' settings
is set to 'true'
@anjannath
preflight: add check, fix and cleanup funcs for enabling SMB share on…
dd18b3d 
… windows

earlier these were part of the MSI since we want to enable this only
when user has set 'enable-shared-dirs' setting, this is moved to the
preflight package where we can check this config value before hand

this should remove at least of the warnings reported by virustotal
for the MSI

https://www.virustotal.com/gui/file/31b402dcc1da24265074a21a26018d6cde8eef0b63c77a18f89eb079b6556790
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented May 15, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign gbraad for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfergeau cfergeau cfergeau left review comments

@gbraad gbraad Awaiting requested review from gbraad

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove automatic creation of shared folder from the MSI
3 participants
@anjannath @praveenkumar @cfergeau