nfs: don't reject WRITE on read-only-mode files (fixes git fsync EPERM on macOS)

cli/src/nfsserve/nfs_handlers.rs

@@ -1313,23 +1313,22 @@ 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.
+    //
+    // 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,
@@ -1848,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(());
             }
         }
     }
@@ -2996,3 +2996,11 @@ pub async fn nfsproc3_mknod(
 
     Ok(())
 }
+
+#[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;

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"
+    );
+}

cli/src/nfsserve/nfs_handlers_write_perm_test.rs

@@ -0,0 +1,152 @@
+//! 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, stable_how, WRITE3args};
+
+use agentfs_sdk::{AgentFS, AgentFSOptions, FileSystem};
+
+/// 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;
+
+pub(super) fn status_code(status: &nfsstat3) -> u32 {
+    status.to_u32().expect("nfsstat3 -> u32")
+}
+
+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()))
+        .await
+        .expect("open AgentFS");
+    let fs: Arc<Mutex<dyn FileSystem>> = 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.
+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),
+        ..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: stable_how::FILE_SYNC as u32,
+        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());
+}