Skip to content

Conversation

@computezrmle
Copy link
Contributor

Recently there were a lot of comments and suggestions regarding where to locate the VirtualBox configuration files.

See:
#6015
#6016
#6018

This PR is intended to combine them into a working solution on Linux/MacOS/Windows.

Major objectives:

  1. use VirtualBox's default file locations where possible
  2. use a well defined file location if (1.) is not possible

(1.) mitigates a possible race between VirtualBox Manager (launched by the user) and BOINC VMs (launched via VBoxHeadless).
Since the first VirtualBox process that starts up for a distinct user determines which vbox profile is used, all of them should use the same vbox profile.
(1.) also ensures that an interactive user can monitor the running VMs, e.g. on Windows.

(2.) must be used in environments where BOINC has not enough permissions to access the default vbox profile location, e.g. from inside MacOS's sandbox.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

@Crystal-Pellet
Copy link

Tested the Windows version of this PR and concluded that VirtualBox is using again the default Oracle VirtualBox location for its xml- and log-files.
Unnecessary .VirtualBox and/or VirtualBox directories are not longer created in BOINC's datadirectory.

So OK to this version of vboxwrapper.

@davidpanderson
Copy link
Contributor

Can you please also add comments (in vbox_common.h) for these fields:
string virtualbox_home_directory;
string virtualbox_scratch_directory;
string virtualbox_install_directory;
string virtualbox_guest_additions;

... saying what they're used for and how they're determined.

Also: I don't understand why these are elements of VBOX_BASE and not global vars.
(more generally I don't understand why we need VBOX_JOB, VBOX_VM, and VBOX_BASE at all.
But that's beyond the scope of this).

@computezrmle
Copy link
Contributor Author

Can you please also add comments (in vbox_common.h) for these fields:
string virtualbox_home_directory;
string virtualbox_scratch_directory;
string virtualbox_install_directory;
string virtualbox_guest_additions;

As for virtualbox_scratch_directory
This is unused, hence I removed it.

Added comments for the others.
These variables have been introduced more than a decade ago together with comments explaining what they are used for.
Over the years the original comments obviously got lost.

VBOX_JOB, VBOX_VM, and VBOX_BASE are also used for more than a decade.
Changing this should indeed not be the scope of this PR.

@codecov
Copy link

codecov bot commented Feb 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 11.95%. Comparing base (ab9ae86) to head (4216226).
Report is 68 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #6116      +/-   ##
============================================
- Coverage     11.96%   11.95%   -0.01%     
  Complexity     1068     1068              
============================================
  Files           278      278              
  Lines         36940    36938       -2     
  Branches       8529     8529              
============================================
- Hits           4419     4416       -3     
  Misses        32121    32121              
- Partials        400      401       +1     

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@lfield
Copy link
Contributor

lfield commented Mar 3, 2025

LGTM I endorse the test from Crystal-Pellet

@AenBleidd
Copy link
Member

@davidpanderson, could you please review this again? Thank you in advance.

@davidpanderson
Copy link
Contributor

The added comments are good, but they should say

  • why does vboxwrapper need to know where the vbox profile directory is?
  • Does this directory always exist already? In what situations does vboxwrapper have to create it? Why?
  • If vboxwrapper needs to create it, where does it put it, and what's it called?

vboxwrapper may not be able to create the profile directory in the user's home directory.
All platforms have a 'sandboxed' mode in which apps (including vboxwrapper)
run under a user account that can't access anything outside the BOINC data dir.
On Mac it can't even write in that directory - only in subdirectories.
That's why we were putting the profile dir in /projects/.

Whatever the change is, it needs to be tested on

  • Windows: secure install
  • Linux: secure install (I believe our .rpm does this by default)
  • Mac: standard install (which is secure)

@computezrmle
Copy link
Contributor Author

The added comments are good, but they should say

  • why does vboxwrapper need to know where the vbox profile directory is?
  • Does this directory always exist already? In what situations does vboxwrapper have to create it? Why?
  • If vboxwrapper needs to create it, where does it put it, and what's it called?

I think the comments are detailed enough.
Especially since they already include a link to the VirtualBox technical documentation.

// https://docs.oracle.com/en/virtualization/virtualbox/6.1/admin/TechnicalBackground.html#3.1.3.-Summary-of-Configuration-Data-Locations

vboxwrapper may not be able to create the profile directory in the user's home directory.

Vboxwrapper runs under that username, hence can write to it's own home directory.

In a sandbox environment the profile now goes to /projects/VirtualBox.
This was the agreement for MacOS as far as I understand the PRs mentioned above.
I don't see a reason why it should be different on other platforms.

Whatever the change is, it needs to be tested on

  • Windows: secure install
  • Linux: secure install (I believe our .rpm does this by default)
  • Mac: standard install (which is secure)

It has been tested on all platforms with real tasks from LHC@home.

@davidpanderson
Copy link
Contributor

Has it been tested with secure installs on Win and Linux?

@computezrmle
Copy link
Contributor Author

Depends on what you exactly mean by 'secure installs'.
Is it (on Linux) the installation as a service without a home directory?
If so, this works.
I didn't test it with this artifact but with the vboxwrapper before this PR which writes it's profile to /projects.

On Windows I don't know whether this has explicitly been tested.
I don't see a reason why it shouldn't work since it uses the same non-default location that is used on Linux and for the MacOS sandbox.

@davidpanderson
Copy link
Contributor

In the Windows installer there's a secure install option.
That creates an unprivileged user (without a home directory, I believe)
and BOINC jobs run as that user

@computezrmle
Copy link
Contributor Author

I don't know if a user on Windows requires a classical home directory, but AFAIK Windows always creates a user profile when a new user account is added.
Windows automatically stores the profile location in %USERPROFILE% which can now be used by vboxwrapper to put the VirtualBox profile in.

@Crystal-Pellet
Copy link

In the Windows installer there's a secure install option. That creates an unprivileged user (without a home directory, I believe) and BOINC jobs run as that user

If you mean 'Service install' with the secure option:
There is a boinc_master user created under the C:\Users and BOINC is running with the account of boinc_master.
Although a .VirtualBox directory is created under C:\Users\boinc_master's directory, VirtualBox cannot be used because of Access Denied.

2025-03-04 11:52:41 (10996): 
Command: VBoxManage -q list systemproperties
Exit Code: -2147024891
Output:
VBoxManage.exe: error: Failed to create the VirtualBox object!
VBoxManage.exe: error: The object is not ready
VBoxManage.exe: error: Details: code E_ACCESSDENIED (0x80070005), component VirtualBoxClientWrap, interface IVirtualBoxClient

It's just like with GPU-tasks, don't use BOINC's service install when you plan to run VBox-tasks.

@computezrmle
Copy link
Contributor Author

I wonder if boinc_master has enough access rights (via group membership) to meet the requirements mentioned in the VirtualBox manual.
If not it has to be solved in the installer rather than in vboxwrapper.

https://www.virtualbox.org/manual/topics/installation.html#install-win-installdir-req

Windows Installation Directory Security Requirements

The installation directory on Windows hosts must meet certain security requirements, in order to be accepted by the Windows installer.

This also applies for upgrades of Oracle VirtualBox.
For example, when installing Oracle VirtualBox into a custom location at X:\Data\MyPrograms\Oracle VirtualBox, all parent directories of this path (namely X:\Data and X:\Data\MyPrograms) must meet the following Discretionary Access Control List (DACL).

        Users               S-1-5-32-545:(OI)(CI)(RX)
        Users               S-1-5-32-545:(DE,WD,AD,WEA,WA)
        Authenticated Users S-1-5-11:(OI)(CI)(RX)
        Authenticated Users S-1-5-11:(DE,WD,AD,WEA,WA)
      

Directory inheritance must also be disabled for all parent directories.
You can use the icacls Windows command line tool to modify a directory to meet the security requirements. For example:

      icacls <Directory> /reset /t /c
      icacls <Directory> /inheritance:d /t /c
      icacls <Directory> /grant *S-1-5-32-545:(OI)(CI)(RX)
      icacls <Directory> /deny  *S-1-5-32-545:(DE,WD,AD,WEA,WA)
      icacls <Directory> /grant *S-1-5-11:(OI)(CI)(RX)
      icacls <Directory> /deny  *S-1-5-11:(DE,WD,AD,WEA,WA)
      

Note that these commands must be repeated for all parent directories (X:\Data and X:\Data\MyPrograms in this example).

@AenBleidd
Copy link
Member

I agree this should be solved separately in the installer.
Please open a separate ticket for that.

@computezrmle
Copy link
Contributor Author

Anything else that needs to be done?
Crystal-Pellet's last test shows that it works as intended in connection with with a secure install.
The fact that VirtualBox finally fails is not caused by vboxwrapper but most likely by a permissions issue.

There is a boinc_master user created under the C:\Users
(...) BOINC is running with the account of boinc_master.
(...) a .VirtualBox directory is created under C:\Users\boinc_master's directory

@computezrmle
Copy link
Contributor Author

@davidpanderson Please review again.
@AenBleidd After approval by reviewers please publish an official revision.

Thanks in advance.

@davidpanderson
Copy link
Contributor

The comments are not complete, but as long as it works, fine w/ me.

@davidpanderson davidpanderson merged commit 7579572 into BOINC:master Mar 12, 2025
153 checks passed
@github-project-automation github-project-automation bot moved this from In progress to Merged in Client/Manager Mar 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants