diff --git a/cli/src/nfsserve/nfs_handlers.rs b/cli/src/nfsserve/nfs_handlers.rs index 69230aa5..dc6ca4a6 100644 --- a/cli/src/nfsserve/nfs_handlers.rs +++ b/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; 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 new file mode 100644 index 00000000..2f2d5ac9 --- /dev/null +++ b/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> = 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()); +}