diff --git a/pkg/filesystem/virtual/nfsv4/base_program.go b/pkg/filesystem/virtual/nfsv4/base_program.go index ff507ecd..b6a0d633 100644 --- a/pkg/filesystem/virtual/nfsv4/base_program.go +++ b/pkg/filesystem/virtual/nfsv4/base_program.go @@ -949,13 +949,7 @@ func (s *compoundState) opClose(args *nfsv4.Close4args) nfsv4.Close4res { transaction, lastResponse, st := oos.startTransaction(p, args.Seqid, &ll, unconfirmedOpenOwnerPolicyDeny) if st != nfsv4.NFS4_OK { if r, ok := lastResponse.(nfsv4.Close4res); ok { - // Only return a cached response if the request - // contains the same state ID as provided - // originally. - // - // More details: RFC 7530, section 9.1.9, bullet - // point 3. - if okResponse, ok := lastResponse.(*nfsv4.Close4res_NFS4_OK); !ok || (okResponse.OpenStateid.Other == args.OpenStateid.Other && okResponse.OpenStateid.Seqid == nextSeqID(args.OpenStateid.Seqid)) { + if okResponse, ok := lastResponse.(*nfsv4.Close4res_NFS4_OK); !ok || isNextStateID(&okResponse.OpenStateid, &args.OpenStateid) { return r } } @@ -1168,13 +1162,7 @@ func (s *compoundState) opLock(args *nfsv4.Lock4args) nfsv4.Lock4res { transaction, lastResponse, st := los.startTransaction(p, owner.LockSeqid, false) if st != nfsv4.NFS4_OK { if r, ok := lastResponse.(nfsv4.Lock4res); ok { - // Only return a cached response if the - // request contains the same state ID as - // provided originally. - // - // More details: RFC 7530, section - // 9.1.9, bullet point 3. - if okResponse, ok := lastResponse.(*nfsv4.Lock4res_NFS4_OK); !ok || (okResponse.Resok4.LockStateid.Other == owner.LockStateid.Other && okResponse.Resok4.LockStateid.Seqid != nextSeqID(owner.LockStateid.Seqid)) { + if okResponse, ok := lastResponse.(*nfsv4.Lock4res_NFS4_OK); !ok || isNextStateID(&okResponse.Resok4.LockStateid, &owner.LockStateid) { return r } } @@ -1385,13 +1373,7 @@ func (s *compoundState) opLocku(args *nfsv4.Locku4args) nfsv4.Locku4res { transaction, lastResponse, st := los.startTransaction(p, args.Seqid, false) if st != nfsv4.NFS4_OK { if r, ok := lastResponse.(nfsv4.Locku4res); ok { - // Only return a cached response if the request - // contains the same state ID as provided - // originally. - // - // More details: RFC 7530, section 9.1.9, bullet - // point 3. - if okResponse, ok := lastResponse.(*nfsv4.Locku4res_NFS4_OK); !ok || (okResponse.LockStateid.Other == args.LockStateid.Other && okResponse.LockStateid.Seqid != nextSeqID(args.LockStateid.Seqid)) { + if okResponse, ok := lastResponse.(*nfsv4.Locku4res_NFS4_OK); !ok || isNextStateID(&okResponse.LockStateid, &args.LockStateid) { return r } } @@ -1725,13 +1707,7 @@ func (s *compoundState) opOpenConfirm(args *nfsv4.OpenConfirm4args) nfsv4.OpenCo transaction, lastResponse, st := oos.startTransaction(p, args.Seqid, &ll, unconfirmedOpenOwnerPolicyAllow) if st != nfsv4.NFS4_OK { if r, ok := lastResponse.(nfsv4.OpenConfirm4res); ok { - // Only return a cached response if the request - // contains the same state ID as provided - // originally. - // - // More details: RFC 7530, section 9.1.9, bullet - // point 3. - if okResponse, ok := lastResponse.(*nfsv4.OpenConfirm4res_NFS4_OK); !ok && (okResponse.Resok4.OpenStateid.Other == args.OpenStateid.Other && okResponse.Resok4.OpenStateid.Seqid == nextSeqID(args.OpenStateid.Seqid)) { + if okResponse, ok := lastResponse.(*nfsv4.OpenConfirm4res_NFS4_OK); !ok || isNextStateID(&okResponse.Resok4.OpenStateid, &args.OpenStateid) { return r } } @@ -1780,7 +1756,9 @@ func (s *compoundState) opOpenDowngrade(args *nfsv4.OpenDowngrade4args) nfsv4.Op transaction, lastResponse, st := oos.startTransaction(p, args.Seqid, &ll, unconfirmedOpenOwnerPolicyDeny) if st != nfsv4.NFS4_OK { if r, ok := lastResponse.(nfsv4.OpenDowngrade4res); ok { - return r + if okResponse, ok := lastResponse.(*nfsv4.OpenDowngrade4res_NFS4_OK); !ok || isNextStateID(&okResponse.Resok4.OpenStateid, &args.OpenStateid) { + return r + } } return &nfsv4.OpenDowngrade4res_default{Status: st} } @@ -1893,8 +1871,11 @@ func (s *compoundState) opReaddir(ctx context.Context, args *nfsv4.Readdir4args) } // Validate the cookie verifier. + // TODO: The macOS NFSv4 client may sometimes not set the cookie + // verifier properly, so we allow it to be zero. Remove this + // logic in due time. Issue: rdar://91034875 p := s.program - if args.Cookie != 0 && args.Cookieverf != p.rebootVerifier { + if args.Cookie != 0 && args.Cookieverf != p.rebootVerifier && (args.Cookieverf != nfsv4.Verifier4{}) { return &nfsv4.Readdir4res_default{Status: nfsv4.NFS4ERR_NOT_SAME} } @@ -3036,6 +3017,20 @@ func nextSeqID(seqID nfsv4.Seqid4) nfsv4.Seqid4 { return seqID + 1 } +// isNextStateID returns true if state ID a is the successor of state ID b. +// +// At a minimum, the standard states that when returning a cached +// response to operations such as CLOSE, LOCK, LOCKU, OPEN_CONFIRM, and +// OPEN_DOWNGRADE, it is sufficient to compare the original operation +// type and the operation's sequence ID. This function can be used to +// increase strictness by ensuring that the state IDs in the request +// also match the originally provided values. +// +// More details: RFC 7530, section 9.1.9, bullet point 3. +func isNextStateID(a *nfsv4.Stateid4, b *nfsv4.Stateid4) bool { + return a.Other == b.Other && a.Seqid == nextSeqID(b.Seqid) +} + // toShareMask converts NFSv4 share_access values that are part of OPEN // and OPEN_DOWNGRADE requests to our equivalent ShareMask values. func shareAccessToShareMask(in uint32) (virtual.ShareMask, nfsv4.Nfsstat4) { diff --git a/pkg/filesystem/virtual/nfsv4/base_program_test.go b/pkg/filesystem/virtual/nfsv4/base_program_test.go index 923747d1..44f77644 100644 --- a/pkg/filesystem/virtual/nfsv4/base_program_test.go +++ b/pkg/filesystem/virtual/nfsv4/base_program_test.go @@ -2764,7 +2764,122 @@ func TestBaseProgramCompound_OP_OPENATTR(t *testing.T) { }) } -// TODO: OPEN_CONFIRM +func TestBaseProgramCompound_OP_OPEN_CONFIRM(t *testing.T) { + ctrl, ctx := gomock.WithContext(context.Background(), t) + + rootDirectory := mock.NewMockVirtualDirectory(ctrl) + rootDirectory.EXPECT().VirtualGetAttributes(gomock.Any(), virtual.AttributesMaskFileHandle, gomock.Any()). + Do(func(ctx context.Context, requested virtual.AttributesMask, attributes *virtual.Attributes) { + attributes.SetFileHandle([]byte{0x2e, 0x8d, 0x48, 0x03, 0xc4, 0xc3, 0x2d, 0x6c}) + }) + handleResolver := mock.NewMockHandleResolver(ctrl) + randomNumberGenerator := mock.NewMockSingleThreadedGenerator(ctrl) + rebootVerifier := nfsv4_xdr.Verifier4{0x42, 0xa8, 0x3f, 0xd1, 0xde, 0x65, 0x74, 0x2a} + stateIDOtherPrefix := [...]byte{0xfa, 0xc3, 0xf7, 0x18} + clock := mock.NewMockClock(ctrl) + program := nfsv4.NewBaseProgram(rootDirectory, handleResolver.Call, randomNumberGenerator, rebootVerifier, stateIDOtherPrefix, clock, 2*time.Minute, time.Minute) + + clock.EXPECT().Now().Return(time.Unix(1000, 0)) + clock.EXPECT().Now().Return(time.Unix(1001, 0)) + setClientIDForTesting(ctx, t, randomNumberGenerator, program, 0x2e5550c498b2b463) + + t.Run("RetransmissionSuccess", func(t *testing.T) { + // It should be valid to send OPEN_CONFIRM repeatedly in + // case of connection drops. + leaf := mock.NewMockVirtualLeaf(ctrl) + clock.EXPECT().Now().Return(time.Unix(1002, 0)) + clock.EXPECT().Now().Return(time.Unix(1003, 0)) + openUnconfirmedFileForTesting( + ctx, + t, + randomNumberGenerator, + program, + rootDirectory, + leaf, + nfsv4_xdr.NfsFh4{0xff, 0x27, 0xc7, 0x8f, 0xd5, 0x6a, 0xfb, 0xee}, + /* shortClientID = */ 0x2e5550c498b2b463, + /* seqID = */ 1205, + /* stateIDOther = */ [...]byte{ + 0xfa, 0xc3, 0xf7, 0x18, + 0x80, 0x57, 0x5b, 0x95, + 0x08, 0x16, 0x41, 0x0a, + }) + + for i := int64(0); i < 10; i++ { + clock.EXPECT().Now().Return(time.Unix(1004+i*2, 0)) + clock.EXPECT().Now().Return(time.Unix(1005+i*2, 0)) + openConfirmForTesting( + ctx, + t, + randomNumberGenerator, + program, + nfsv4_xdr.NfsFh4{0xff, 0x27, 0xc7, 0x8f, 0xd5, 0x6a, 0xfb, 0xee}, + /* seqID = */ 1206, + /* stateIDOther = */ [...]byte{ + 0xfa, 0xc3, 0xf7, 0x18, + 0x80, 0x57, 0x5b, 0x95, + 0x08, 0x16, 0x41, 0x0a, + }) + } + }) + + t.Run("RetransmissionWithMismatchingStateID", func(t *testing.T) { + // At a minimum, the standard states that when returning + // a cached response it is sufficient to compare the + // original operation type and sequence ID. Let's be a + // bit more strict and actually check whether the + // provided state ID matches the one that was provided + // as part of the original request. + // + // More details: RFC 7530, section 9.1.9, bullet point 3. + clock.EXPECT().Now().Return(time.Unix(1024, 0)) + clock.EXPECT().Now().Return(time.Unix(1025, 0)) + + res, err := program.NfsV4Nfsproc4Compound(ctx, &nfsv4_xdr.Compound4args{ + Tag: "close", + Argarray: []nfsv4_xdr.NfsArgop4{ + &nfsv4_xdr.NfsArgop4_OP_PUTFH{ + Opputfh: nfsv4_xdr.Putfh4args{ + Object: nfsv4_xdr.NfsFh4{0xff, 0x27, 0xc7, 0x8f, 0xd5, 0x6a, 0xfb, 0xee}, + }, + }, + &nfsv4_xdr.NfsArgop4_OP_OPEN_CONFIRM{ + OpopenConfirm: nfsv4_xdr.OpenConfirm4args{ + Seqid: 1206, + OpenStateid: nfsv4_xdr.Stateid4{ + Seqid: 3, + Other: [...]byte{ + 0xfa, 0xc3, 0xf7, 0x18, + 0x80, 0x57, 0x5b, 0x95, + 0x08, 0x16, 0x41, 0x0a, + }, + }, + }, + }, + }, + }) + require.NoError(t, err) + require.Equal(t, &nfsv4_xdr.Compound4res{ + Tag: "close", + Resarray: []nfsv4_xdr.NfsResop4{ + &nfsv4_xdr.NfsResop4_OP_PUTFH{ + Opputfh: nfsv4_xdr.Putfh4res{ + Status: nfsv4_xdr.NFS4_OK, + }, + }, + &nfsv4_xdr.NfsResop4_OP_OPEN_CONFIRM{ + OpopenConfirm: &nfsv4_xdr.OpenConfirm4res_default{ + Status: nfsv4_xdr.NFS4ERR_BAD_SEQID, + }, + }, + }, + Status: nfsv4_xdr.NFS4ERR_BAD_SEQID, + }, res) + }) + + // TODO: Any more cases we want to test? +} + // TODO: OPEN_DOWNGRADE // TODO: PUTFH // TODO: PUTPUBFH