Skip to content

Commit

Permalink
Make our NFSv4 server more picky when it comes to retransmissions
Browse files Browse the repository at this point in the history 
RFC 7530 states that in order to determine whether a request is a
retransmission, it is sufficient to consider the seqid and operation
type. For well-behaving clients this is okay, but in case of
implementation flaws this can lead to hard to debug issues.

Let's put an additional safety belt in place, where we at least ensure
that the arguments provided by the client match up with what's in the
cached response.
  • Loading branch information
@EdSchouten
EdSchouten committed Jul 10, 2023
1 parent 2b8c1fb commit b1a8462
Show file tree
Hide file tree
Showing 2 changed files with 92 additions and 6 deletions.
40 changes: 36 additions & 4 deletions pkg/filesystem/virtual/nfsv4/base_program.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -949,7 +949,15 @@ 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 {
return r
// 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)) {
return r
}
}
return &nfsv4.Close4res_default{Status: st}
}
Expand Down Expand Up @@ -1160,7 +1168,15 @@ 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 {
return r
// 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)) {
return r
}
}
return &nfsv4.Lock4res_default{Status: st}
}
Expand Down Expand Up @@ -1369,7 +1385,15 @@ 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 {
return r
// 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)) {
return r
}
}
return &nfsv4.Locku4res_default{Status: st}
}
Expand Down Expand Up @@ -1701,7 +1725,15 @@ 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 {
return r
// 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)) {
return r
}
}
return &nfsv4.OpenConfirm4res_default{Status: st}
}
Expand Down
58 changes: 56 additions & 2 deletions pkg/filesystem/virtual/nfsv4/base_program_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -792,13 +792,67 @@ func TestBaseProgramCompound_OP_CLOSE(t *testing.T) {
}
})

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(1037, 0))
clock.EXPECT().Now().Return(time.Unix(1038, 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{0x1f, 0x5b, 0x1f, 0x0e, 0x8c, 0xf4, 0xf5, 0x40},
},
},
&nfsv4_xdr.NfsArgop4_OP_CLOSE{
Opclose: nfsv4_xdr.Close4args{
Seqid: 244,
OpenStateid: nfsv4_xdr.Stateid4{
Seqid: 3,
Other: [...]byte{
0xf5, 0x47, 0xa8, 0x88,
0x74, 0x62, 0xab, 0x46,
0x26, 0x1d, 0x14, 0x7f,
},
},
},
},
},
})
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_CLOSE{
Opclose: &nfsv4_xdr.Close4res_default{
Status: nfsv4_xdr.NFS4ERR_BAD_SEQID,
},
},
},
Status: nfsv4_xdr.NFS4ERR_BAD_SEQID,
}, res)
})

t.Run("CloseAfterClosed", func(t *testing.T) {
// We should no longer be able to interact with the
// state ID after closing it. Attempting to close a file
// that has already been closed should just return
// NFS4ERR_BAD_STATEID.
clock.EXPECT().Now().Return(time.Unix(1037, 0))
clock.EXPECT().Now().Return(time.Unix(1038, 0))
clock.EXPECT().Now().Return(time.Unix(1039, 0))
clock.EXPECT().Now().Return(time.Unix(1040, 0))

res, err := program.NfsV4Nfsproc4Compound(ctx, &nfsv4_xdr.Compound4args{
Tag: "close",
Expand Down

0 comments on commit b1a8462

Please sign in to comment.