From abbbed2349c9f205d41b9c24ce8c46d695990b57 Mon Sep 17 00:00:00 2001 From: Connor Black Date: Mon, 15 Jun 2026 17:45:50 -0400 Subject: [PATCH 1/2] nfs: don't reject WRITE on read-only-mode files (fixes git fsync EPERM on macOS) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The macOS `agentfs run` overlay is exposed over a localhost NFSv3 server. `nfsproc3_write` gated every WRITE on the file's current mode bits via `permissions::can_write`, returning NFS3ERR_ACCES when the owner-write bit was clear. That is wrong for NFSv3: the protocol is stateless and has no OPEN procedure, so the server never sees the open(2) that authorized the descriptor. POSIX checks write permission once, at open time; later write(2)/fsync(2) on an already-open writable fd must succeed regardless of the file's mode bits. git relies on exactly this pattern: it creates loose objects with `git_mkstemp_mode(tmp, 0444)` (read-only mode, writable fd), writes, then fsyncs in `close_loose_object()`. On macOS the NFS client buffers the write and only flushes it as a synchronous WRITE RPC at fsync/close time, which hit the can_write check and failed with "fatal: error when closing loose object file: Permission denied" — making `git add`/`git commit` unusable inside `agentfs run`. Remove the mode-bit gate from the WRITE handler. Write authorization is enforced where NFSv3 expects it: the client kernel checks permission at open time via the ACCESS procedure (`nfsproc3_access` -> `permissions::compute_access`), which is unchanged. This matches upstream nfsserve, whose WRITE handler performs no mode-bit check. Adds nfs_handlers_write_perm_test.rs driving nfsproc3_write end-to-end over a real AgentNFS VFS: asserts WRITE on a 0444-mode file owned by the caller returns NFS3_OK (regression: was NFS3ERR_ACCES) and that an ordinary 0644 file still writes. Verified with a freshly built binary: the documented fsync repro and a real `git add`/`git commit` now succeed inside `agentfs run`; cli (32) and sdk (101) test suites pass; clippy clean. --- cli/src/nfsserve/nfs_handlers.rs | 31 ++-- .../nfsserve/nfs_handlers_write_perm_test.rs | 149 ++++++++++++++++++ 2 files changed, 163 insertions(+), 17 deletions(-) create mode 100644 cli/src/nfsserve/nfs_handlers_write_perm_test.rs diff --git a/cli/src/nfsserve/nfs_handlers.rs b/cli/src/nfsserve/nfs_handlers.rs index 69230aa5..3c66e5cd 100644 --- a/cli/src/nfsserve/nfs_handlers.rs +++ b/cli/src/nfsserve/nfs_handlers.rs @@ -1313,23 +1313,16 @@ pub async fn nfsproc3_write( } }; - // Check write permission - if !permissions::can_write(&context.auth, &attr) { - debug!("write permission denied for uid={}", context.auth.uid); - let pre_obj_attr = nfs::pre_op_attr::attributes(nfs::wcc_attr { - size: attr.size, - mtime: attr.mtime, - ctime: attr.ctime, - }); - make_success_reply(xid).serialize(output)?; - nfs::nfsstat3::NFS3ERR_ACCES.serialize(output)?; - nfs::wcc_data { - before: pre_obj_attr, - after: nfs::post_op_attr::attributes(attr), - } - .serialize(output)?; - return Ok(()); - } + // Do NOT gate WRITE on the file's current mode bits. NFSv3 is stateless and + // has no OPEN procedure, so the server never sees the open(2) that authorized + // this descriptor. POSIX checks write permission once at open time; a later + // write(2)/fsync(2) on an already-open writable fd must succeed regardless of + // the file's mode (e.g. git creates loose objects mode 0444 via + // git_mkstemp_mode, writes, then fsyncs in close_loose_object()). Returning + // NFS3ERR_ACCES here surfaced as "Permission denied" on fsync/close. Write + // authorization is enforced where NFSv3 expects it: the client kernel checks + // at open time via the ACCESS procedure (permissions::compute_access). This + // matches upstream nfsserve, whose WRITE handler performs no mode-bit check. let pre_obj_attr = nfs::pre_op_attr::attributes(nfs::wcc_attr { size: attr.size, @@ -2996,3 +2989,7 @@ pub async fn nfsproc3_mknod( Ok(()) } + +#[cfg(test)] +#[path = "nfs_handlers_write_perm_test.rs"] +mod nfs_handlers_write_perm_test; diff --git a/cli/src/nfsserve/nfs_handlers_write_perm_test.rs b/cli/src/nfsserve/nfs_handlers_write_perm_test.rs new file mode 100644 index 00000000..926672e8 --- /dev/null +++ b/cli/src/nfsserve/nfs_handlers_write_perm_test.rs @@ -0,0 +1,149 @@ +//! Regression tests for write-permission handling in the NFS WRITE handler. +//! +//! These cover the macOS `agentfs run` failure where `fsync`/`close` on a file +//! whose mode bits lack owner-write (e.g. git loose objects created with +//! `git_mkstemp_mode(tmp, 0444)`) was rejected with NFS3ERR_ACCES even though the +//! descriptor was opened for writing. NFSv3 is stateless and never sees open(2), +//! so WRITE must not be gated on the file's current mode bits. + +use std::io::Cursor; +use std::sync::Arc; + +use num_traits::ToPrimitive; +use tokio::sync::Mutex; + +use crate::nfs::AgentNFS; +use crate::nfsserve::context::RPCContext; +use crate::nfsserve::nfs::{self, nfsstat3}; +use crate::nfsserve::rpc::{auth_unix, rpc_msg}; +use crate::nfsserve::transaction_tracker::TransactionTracker; +use crate::nfsserve::xdr::XDR; + +// Private items of the parent (nfs_handlers) module under test. +use super::{nfsproc3_write, WRITE3args}; + +use agentfs_sdk::{AgentFS, AgentFSOptions, FileSystem}; + +/// Non-root uid that will own the test files (so owner mode bits apply). +const OWNER_UID: u32 = 501; +const OWNER_GID: u32 = 20; + +fn status_code(status: &nfsstat3) -> u32 { + status.to_u32().expect("nfsstat3 -> u32") +} + +async fn make_context() -> (RPCContext, tempfile::TempDir) { + let dir = tempfile::tempdir().expect("tempdir"); + let db_path = dir.path().join("delta.db"); + let agentfs = AgentFS::open(AgentFSOptions::with_path(db_path.to_str().unwrap())) + .await + .expect("open AgentFS"); + let fs: Arc> = Arc::new(Mutex::new(agentfs.fs)); + let vfs = Arc::new(AgentNFS::new(fs)); + + let ctx = RPCContext { + local_port: 0, + client_addr: "127.0.0.1:0".to_string(), + auth: auth_unix { + stamp: 0, + machinename: Vec::new(), + uid: OWNER_UID, + gid: OWNER_GID, + gids: Vec::new(), + }, + vfs, + mount_signal: None, + export_name: Arc::new("/".to_string()), + transaction_tracker: Arc::new(TransactionTracker::new(std::time::Duration::from_secs(60))), + }; + (ctx, dir) +} + +/// Create a file in the root directory with the given mode, owned by OWNER_UID, +/// and return its fileid. +async fn create_file_with_mode(ctx: &RPCContext, name: &str, mode: u32) -> nfs::fileid3 { + let root = ctx.vfs.root_dir(); + let attr = nfs::sattr3 { + mode: nfs::set_mode3::mode(mode), + ..Default::default() + }; + let (fileid, fattr) = ctx + .vfs + .create(root, &name.as_bytes().into(), attr, &ctx.auth) + .await + .expect("create file"); + assert_eq!(fattr.mode & 0o777, mode & 0o777, "mode should be set"); + fileid +} + +/// Drive nfsproc3_write and return the NFS status code from the reply. +async fn run_write(ctx: &RPCContext, fileid: nfs::fileid3, offset: u64, data: &[u8]) -> nfsstat3 { + let args = WRITE3args { + file: ctx.vfs.id_to_fh(fileid), + offset, + count: data.len() as u32, + stable: 2, // FILE_SYNC + data: data.to_vec(), + }; + + let mut input = Vec::new(); + args.serialize(&mut input).expect("serialize WRITE3args"); + + let mut out = Vec::new(); + let mut reader = Cursor::new(input); + nfsproc3_write(0x1234, &mut reader, &mut out, ctx) + .await + .expect("handler ran"); + + // Reply layout: rpc_msg (success reply) followed by the nfsstat3 status. + let mut cursor = Cursor::new(out); + let mut reply = rpc_msg::default(); + reply.deserialize(&mut cursor).expect("deserialize rpc_msg"); + let mut status = nfsstat3::NFS3_OK; + status.deserialize(&mut cursor).expect("deserialize status"); + status +} + +/// THE REGRESSION: a file whose mode lacks owner-write (0o444) must still accept +/// a WRITE from its owner. Pre-fix this returned NFS3ERR_ACCES, which surfaced as +/// "Permission denied" on git's fsync/close of a loose object. +#[tokio::test] +async fn write_succeeds_on_readonly_mode_file() { + let (ctx, _dir) = make_context().await; + let fileid = create_file_with_mode(&ctx, "loose-object", 0o444).await; + + let status = run_write(&ctx, fileid, 0, &[b'x'; 64]).await; + assert_eq!( + status_code(&status), + status_code(&nfsstat3::NFS3_OK), + "WRITE on a 0444-mode file owned by the caller must succeed \ + (regression: was NFS3ERR_ACCES = {})", + status_code(&nfsstat3::NFS3ERR_ACCES) + ); + + // Guard against a vacuous test: confirm the bytes actually landed. + let (read_back, _eof) = ctx.vfs.read(fileid, 0, 64).await.expect("read back"); + assert_eq!( + read_back, + vec![b'x'; 64], + "written data should be persisted" + ); +} + +/// A writable-mode file (0o644) must also accept WRITE — guards against the fix +/// regressing the ordinary path. +#[tokio::test] +async fn write_succeeds_on_writable_mode_file() { + let (ctx, _dir) = make_context().await; + let fileid = create_file_with_mode(&ctx, "normal", 0o644).await; + + let status = run_write(&ctx, fileid, 0, b"hello").await; + assert_eq!( + status_code(&status), + status_code(&nfsstat3::NFS3_OK), + "WRITE on a 0644 file must succeed" + ); + + let (read_back, _eof) = ctx.vfs.read(fileid, 0, 5).await.expect("read back"); + assert_eq!(read_back, b"hello".to_vec()); +} From 83234189a05fcd93f6c70d899ba10190e3b6aaaf Mon Sep 17 00:00:00 2001 From: Connor Black Date: Fri, 19 Jun 2026 12:08:14 -0400 Subject: [PATCH 2/2] nfs: stop stale guarded SETATTR from mutating --- cli/src/nfsserve/nfs_handlers.rs | 19 ++++- .../nfs_handlers_setattr_guard_test.rs | 83 +++++++++++++++++++ .../nfsserve/nfs_handlers_write_perm_test.rs | 15 ++-- 3 files changed, 107 insertions(+), 10 deletions(-) create mode 100644 cli/src/nfsserve/nfs_handlers_setattr_guard_test.rs diff --git a/cli/src/nfsserve/nfs_handlers.rs b/cli/src/nfsserve/nfs_handlers.rs index 3c66e5cd..dc6ca4a6 100644 --- a/cli/src/nfsserve/nfs_handlers.rs +++ b/cli/src/nfsserve/nfs_handlers.rs @@ -1319,10 +1319,16 @@ pub async fn nfsproc3_write( // write(2)/fsync(2) on an already-open writable fd must succeed regardless of // the file's mode (e.g. git creates loose objects mode 0444 via // git_mkstemp_mode, writes, then fsyncs in close_loose_object()). Returning - // NFS3ERR_ACCES here surfaced as "Permission denied" on fsync/close. Write - // authorization is enforced where NFSv3 expects it: the client kernel checks - // at open time via the ACCESS procedure (permissions::compute_access). This - // matches upstream nfsserve, whose WRITE handler performs no mode-bit check. + // NFS3ERR_ACCES here surfaced as "Permission denied" on fsync/close. + // + // Permission enforcement here belongs to the NFS client kernel, not this + // WRITE handler: the client performs the POSIX open-time check before it + // hands back a writable fd. The server's ACCESS procedure + // (permissions::compute_access) exposes the file's permission bits so a + // client *may* pre-check, but per RFC 1813 ACCESS is advisory — clients are + // not required to call it and it does not gate later WRITEs — so it is not a + // server-side enforcement point. We therefore perform no mode-bit check on + // WRITE, matching upstream nfsserve. let pre_obj_attr = nfs::pre_op_attr::attributes(nfs::wcc_attr { size: attr.size, @@ -1841,6 +1847,7 @@ pub async fn nfsproc3_setattr( make_success_reply(xid).serialize(output)?; nfs::nfsstat3::NFS3ERR_NOT_SYNC.serialize(output)?; nfs::wcc_data::default().serialize(output)?; + return Ok(()); } } } @@ -2993,3 +3000,7 @@ pub async fn nfsproc3_mknod( #[cfg(test)] #[path = "nfs_handlers_write_perm_test.rs"] mod nfs_handlers_write_perm_test; + +#[cfg(test)] +#[path = "nfs_handlers_setattr_guard_test.rs"] +mod nfs_handlers_setattr_guard_test; diff --git a/cli/src/nfsserve/nfs_handlers_setattr_guard_test.rs b/cli/src/nfsserve/nfs_handlers_setattr_guard_test.rs new file mode 100644 index 00000000..d7c023c5 --- /dev/null +++ b/cli/src/nfsserve/nfs_handlers_setattr_guard_test.rs @@ -0,0 +1,83 @@ +//! Regression tests for guarded SETATTR handling. +//! +//! A stale `sattrguard3::obj_ctime` must cause `NFS3ERR_NOT_SYNC`, must not +//! apply the requested mutation, and must produce exactly one NFS reply. + +use crate::nfsserve::nfs::{self, nfsstat3}; +use crate::nfsserve::rpc::rpc_msg; +use crate::nfsserve::xdr::XDR; +use std::io::Cursor; + +use super::nfs_handlers_write_perm_test::{create_file_with_mode, make_context, status_code}; +use super::{nfsproc3_setattr, sattrguard3, SETATTR3args}; + +fn stale_guard(ctime: nfs::nfstime3) -> sattrguard3 { + sattrguard3::obj_ctime(nfs::nfstime3 { + seconds: ctime.seconds.wrapping_add(1), + nseconds: ctime.nseconds, + }) +} + +async fn run_setattr( + ctx: &crate::nfsserve::context::RPCContext, + fileid: nfs::fileid3, + new_attribute: nfs::sattr3, + guard: sattrguard3, +) -> (nfsstat3, usize, usize) { + let args = SETATTR3args { + object: ctx.vfs.id_to_fh(fileid), + new_attribute, + guard, + }; + + let mut input = Vec::new(); + args.serialize(&mut input).expect("serialize SETATTR3args"); + + let mut out = Vec::new(); + let mut reader = Cursor::new(input); + nfsproc3_setattr(0x5678, &mut reader, &mut out, ctx) + .await + .expect("handler ran"); + + let total_len = out.len(); + let mut cursor = Cursor::new(out); + let mut reply = rpc_msg::default(); + reply.deserialize(&mut cursor).expect("deserialize rpc_msg"); + let mut status = nfsstat3::NFS3_OK; + status.deserialize(&mut cursor).expect("deserialize status"); + let mut wcc = nfs::wcc_data::default(); + wcc.deserialize(&mut cursor).expect("deserialize wcc_data"); + + (status, cursor.position() as usize, total_len) +} + +#[tokio::test] +async fn stale_guarded_setattr_returns_not_sync_once_and_does_not_mutate() { + let (ctx, _dir) = make_context().await; + let fileid = create_file_with_mode(&ctx, "guarded", 0o644).await; + let attr_before = ctx.vfs.getattr(fileid).await.expect("getattr before"); + + let new_attribute = nfs::sattr3 { + mode: nfs::set_mode3::mode(0o600), + ..Default::default() + }; + let (status, consumed_len, total_len) = + run_setattr(&ctx, fileid, new_attribute, stale_guard(attr_before.ctime)).await; + + assert_eq!( + status_code(&status), + status_code(&nfsstat3::NFS3ERR_NOT_SYNC), + "stale guard must reject SETATTR with NFS3ERR_NOT_SYNC" + ); + assert_eq!( + consumed_len, total_len, + "handler must serialize exactly one SETATTR reply on guard mismatch" + ); + + let attr_after = ctx.vfs.getattr(fileid).await.expect("getattr after"); + assert_eq!( + attr_after.mode & 0o777, + attr_before.mode & 0o777, + "stale guarded SETATTR must not apply the requested mode change" + ); +} diff --git a/cli/src/nfsserve/nfs_handlers_write_perm_test.rs b/cli/src/nfsserve/nfs_handlers_write_perm_test.rs index 926672e8..2f2d5ac9 100644 --- a/cli/src/nfsserve/nfs_handlers_write_perm_test.rs +++ b/cli/src/nfsserve/nfs_handlers_write_perm_test.rs @@ -20,19 +20,22 @@ use crate::nfsserve::transaction_tracker::TransactionTracker; use crate::nfsserve::xdr::XDR; // Private items of the parent (nfs_handlers) module under test. -use super::{nfsproc3_write, WRITE3args}; +use super::{nfsproc3_write, stable_how, WRITE3args}; use agentfs_sdk::{AgentFS, AgentFSOptions, FileSystem}; -/// Non-root uid that will own the test files (so owner mode bits apply). +/// Arbitrary non-root uid/gid that own the test files. The exact values are +/// not significant — they only need to be non-zero so owner mode bits are +/// actually enforced (root would bypass the permission check). 501/20 happen +/// to be macOS-conventional. const OWNER_UID: u32 = 501; const OWNER_GID: u32 = 20; -fn status_code(status: &nfsstat3) -> u32 { +pub(super) fn status_code(status: &nfsstat3) -> u32 { status.to_u32().expect("nfsstat3 -> u32") } -async fn make_context() -> (RPCContext, tempfile::TempDir) { +pub(super) async fn make_context() -> (RPCContext, tempfile::TempDir) { let dir = tempfile::tempdir().expect("tempdir"); let db_path = dir.path().join("delta.db"); let agentfs = AgentFS::open(AgentFSOptions::with_path(db_path.to_str().unwrap())) @@ -61,7 +64,7 @@ async fn make_context() -> (RPCContext, tempfile::TempDir) { /// Create a file in the root directory with the given mode, owned by OWNER_UID, /// and return its fileid. -async fn create_file_with_mode(ctx: &RPCContext, name: &str, mode: u32) -> nfs::fileid3 { +pub(super) async fn create_file_with_mode(ctx: &RPCContext, name: &str, mode: u32) -> nfs::fileid3 { let root = ctx.vfs.root_dir(); let attr = nfs::sattr3 { mode: nfs::set_mode3::mode(mode), @@ -82,7 +85,7 @@ async fn run_write(ctx: &RPCContext, fileid: nfs::fileid3, offset: u64, data: &[ file: ctx.vfs.id_to_fh(fileid), offset, count: data.len() as u32, - stable: 2, // FILE_SYNC + stable: stable_how::FILE_SYNC as u32, data: data.to_vec(), };