Skip to content

[OMEGA-470] Fix: Memory transfer permissions - #377

Merged
TossSky merged 10 commits into
singnet:mainfrom
timur-ashkenov:fix/memory-transfer-permissions
Oct 6, 2026
Merged

TossSky merged 10 commits into
singnet:mainfrom
timur-ashkenov:fix/memory-transfer-permissions

Conversation

@timur-ashkenov

Copy link
Copy Markdown
Collaborator

Description

Fixes memory-transfer startup failures without making archive directories world-writable.
Memory transfer now requires a pre-created Linux directory with a private group, setgid mode 2770, and no ACL access. The launcher validates ownership, mode, GID, symlinks, and ACL state before startup. The container entrypoint remains root for Nginx/bootstrap, while only nobody agent/import/recovery processes receive the configured supplementary transfer group.
Adds --memory-transfer-gid and updates the wizard and documentation with the required Linux setup.

How Has This Been Tested?

  • ./.venv/bin/python -m pytest -q tests
    Result: 78 passed, 1 skipped on macOS.
  • ./.venv/bin/python -m pytest -q tests/test_omegaclaw_launcher.py
    Result: 7 passed, 1 skipped.
  • bash -n scripts/omega
  • bash -n entrypoint.sh
  • git diff --check
  • Built omega:memory-transfer-test locally on macOS.
    The Linux Docker integration test is skipped on macOS by design because Docker Desktop does not provide native Linux host UID/GID/ACL semantics.

Checklist

  • PR contains autogenerated code
  • Self-review completed
  • Test scenarios above are passed with the version of the code from PR

@paul-v-snet paul-v-snet left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A few things I noticed. I may be missing some context, so please correct me where I'm wrong.

  1. su --supp-group with a numeric GID
    As far as I can tell, util-linux su resolves --supp-group only by name: add_supp_group() calls getgrnam() (su-common.c#L1005-L1017). If so, su --supp-group 1001 nobody would fail with group 1001 does not exist unless the container's /etc/group already has that group. Since recover always goes through agent_su, I think the container might exit at startup whenever a transfer dir is configured. When I tried it, even --supp-group 100 was rejected, though users exists there. The preflight probably doesn't catch this because it uses docker run --group-add, which accepts numeric GIDs. Since the main container runs with -d, the failure would likely only show up in docker logs.
    Also, per su(1), the first -G group becomes the primary group unless -g is given, so the agent seems to lose nogroup.
  2. ACL check looks like it's always bypassed
    The heredoc has else without a colon, so Python raises a SyntaxError and exits with 1. That's treated as "no ACL". On a dir with setfacl -m u:nobody:rwx, the check passed for me and printed a traceback. The python3 stub in the tests ("-" -> exit 1) would hide this.
  3. Not sure the original issue is fully addressed
    The installer no longer creates the dir, but it also doesn't seem to validate the path, mode or GID. As far as I can see, all checks still run in start() at the end of the wizard, after docker pull. So I think a misprepared dir would still fail at the same point, just with a different message. Maybe the installer could run the same checks up front, default the GID to the dir's st_gid, and point to the setup docs?
  4. The uname != Linux check seems to turn the feature off on macOS / Docker Desktop, where bind mounts might have worked before. Is that intended?

Comment thread scripts/omega Outdated
Comment thread entrypoint.sh Outdated
timur-ashkenov and others added 3 commits October 2, 2026 16:22
Co-authored-by: paul-v-snet <paul.vasiletz@singularitynet.io>
…-permissions

# Conflicts:
#	tests/test_omegaclaw_launcher.py
@kabirkbr

kabirkbr commented Oct 2, 2026 •

Copy link
Copy Markdown

2,5 cents from me:

For the release, I suggest the fix should:

  • check the folder in the installer when the user gives it, and on failure explain what to do or let the user skip memory transfer, so the wizard does not fail at the end (OMEGA-470)
  • not use 0777
  • not block it by OS: memory transfer is new in this release, so let the write check decide; @timur-ashkenov could you try it on your Mac? If it passes there, allow it; if not, I suggest to document "Linux only for now"...
  • print a warning before a memory import, since with --memory-mode overwrite (the default) an import replaces the agent's current memory, so the user knows there is a risk of losing agent memory and takes extra care;

From @paul-v-snet's review, these should be solved before testing: the numeric GID in su --supp-group (point 1, the agent may not start at all), and the installer checks before docker pull (point 3). Point 2 looks fixed by the applied suggestion. The branch also has a conflict with main now.

Running the agent as the launching user instead of nobody would remove the need for the group, but it changes how Omega is launched, so it goes to a separate ticket, not this PR.

This answer was prepared with the help of my AI assistant.

@timur-ashkenov timur-ashkenov left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed the review feedback in this branch:

  • fixed the numeric MEMORY_TRANSFER_GID handling in entrypoint.sh: the numeric GID is resolved or created as a group name before it is passed to su --supp-group; nogroup remains the agent’s primary group;
  • kept the Linux memory-transfer directory model private and group-based, without 0777;
  • moved directory validation to the installer flow, before Docker image pulling, and let the user skip memory transfer if the directory is not prepared;
  • added a warning before an overwrite-mode memory import;
  • added coverage for the installer validation, Linux-only contract, preflight setup, and import warning.

I also tested the proposed entrypoint.sh ownership/mode workaround on macOS Docker. A simple write can pass, but the actual memory export still fails under Omega’s Landlock policy. Therefore the secure bind-mount memory-transfer flow remains supported on native Linux only for now.

@paul-v-snet paul-v-snet left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please check and fix it if that is the case

Comment thread scripts/omega Outdated
@timur-ashkenov

Copy link
Copy Markdown
Collaborator Author

Good catch. This was not caught because my local testing ran from a repository checkout, where scripts/memory_transfer.py was available next to scripts/omega. The launcher is now self-contained again: it creates one private temporary validator and both the installer and runtime paths use that same validator. I also added a Linux-only runtime test; it is skipped locally on macOS and will run in Linux CI.

paul-v-snet
paul-v-snet previously approved these changes Oct 5, 2026
@TossSky

TossSky commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

Tested: 9025f74 and an image built from it on a Linux host. The launcher ran as a regular user in the docker group, through the install wizard (curl ... | bash) and through scripts/omega start, with the WebSocket channel and Anthropic. For the run without the fix I used the launcher from 7c0e87a with the v0.1.20 release candidate image ee0618a.

The agent does not start with a group the image does not have

entrypoint.sh:46 looks the GID up with getent group. When the image has no group with that GID, getent exits with 2. entrypoint.sh:2 turns on set -euo pipefail, so the entrypoint stops on line 46 and never reaches groupadd on line 49. The container exits with code 2 and empty docker logs, while the wizard and start exit with 0.

The setup steps in docs/reference-memory-portability.md create a new host group, and the image does not have it. I followed those steps with GID 4470, and the agent never started. GID 100 (users) and GID 0 exist in the image and work.

With || true added inside the command substitution on line 46, the entrypoint creates omega-transfer-4470, and the checks below pass with GID 4470 too. test_container_user_can_write_group_protected_transfer_directory bypasses the entrypoint, so it passes either way.

What I checked

  • Without the fix, answering y in the wizard with a new path creates drwxrwxr-x 1000 1000, and the wizard fails with "--memory-transfer-dir must be writable by container user 65534:65534"
  • With the fix, the wizard rejects a missing directory and a directory without setgid, prints the reason and asks the question again
  • start stops before docker pull for a directory without setgid, with access for other users, without group write, with a different GID, without --memory-transfer-gid, for a relative path, a missing directory, /tmp, an access ACL, a default ACL, a symbolic link, and a directory owned by another user. A path with a space works
  • With GID 100, and with GID 4470 after the one-line change, the agent runs as nobody with nogroup and the transfer group as a supplementary group, and keeps the group after docker restart
  • /memory-export writes the archive as -rw-r--r--, owner 65534, group of the directory. The SHA-256 in the reply matches the file, and the operator reads and deletes it without sudo
  • Overwrite, append, history-only and vector-only imports from the directory finish with "memory_portability: import complete", and the agent stays up. After an overwrite import into an empty memory volume, the agent recalls a word it was told before the export. The overwrite warning appears for overwrite and not for append

Smaller things

  • --memory-transfer-gid 0 passes the checks, and the agent runs with root as a supplementary group. A directory that root creates without -g belongs to group 0, so for such a directory the GID that README.md:107 reads with stat -c %g is 0. In the current image the root group adds write access to four directories under /usr/share/man.
  • --memory-transfer-dir /path/to/link/ with a trailing slash passes the symlink check on scripts/omega:37, because os.path.islink follows the link when the path ends with a slash. The wizard drops the slash, so only the flag is affected.
  • When the wizard rejects a directory, it says "Prepare the directory and try again, or answer n at the next prompt to continue without memory transfer." It does not say how to prepare it. The steps are only in docs/reference-memory-portability.md, and the wizard does not point to it.

Verdict: FAIL

@TossSky

TossSky commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

Tested: e82ea6f and an image built from it on a Linux host. The launcher ran as a regular user in the docker group, through the install wizard and through scripts/omega start, with the WebSocket channel and Anthropic. The transfer directory was prepared as in the docs, with a group (GID 4470) that the image does not have.

What I checked

  • The agent starts with GID 4470 and runs as nobody with the transfer group as a supplementary group, also after docker restart
  • The wizard rejects a missing directory, a directory without setgid and GID 0, and asks again. For a directory without setgid it shows the reason and the link to the docs
  • start stops before docker pull for GID 0, for a symbolic link written with a trailing slash, a dot or a double slash, and for the unsafe directories from the first round
  • /memory-export writes the archive into the directory, the SHA-256 in the reply matches the file, and the operator reads and deletes it without sudo
  • Overwrite, append, history-only and vector-only imports from the directory finish with "memory_portability: import complete". After an overwrite import into an empty memory volume, the agent recalls a word it was told before the export
  • tests/pytest.sh gives 99 passed, 2 skipped, run_mandatory gives 166 passed and run_optional gives 6 passed

Verdict: PASS

@TossSky

TossSky commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

@paul-v-snet

Can you approve this PR please?

@TossSky
TossSky merged commit bca50ca into singnet:main Oct 6, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants