* Re: GSSAPI fix for pynfs nfs4.1 client code
2021-09-30 21:11 ` J. Bruce Fields
@ 2021-09-30 21:25 ` J. Bruce Fields
2021-10-01 6:27 ` Volodymyr Khomenko
2021-10-01 6:12 ` Volodymyr Khomenko
2021-10-01 6:49 ` Volodymyr Khomenko
2 siblings, 1 reply; 12+ messages in thread
From: J. Bruce Fields @ 2021-09-30 21:25 UTC (permalink / raw)
To: Volodymyr Khomenko; +Cc: linux-nfs
On Thu, Sep 30, 2021 at 05:11:23PM -0400, J. Bruce Fields wrote:
> On Thu, Sep 30, 2021 at 06:22:09PM +0300, Volodymyr Khomenko wrote:
> > commit b77dc49c775756f08bdd0c6ebbe67a96f0ffe41f
> > Author: Volodymyr Khomenko <volodymyr@vastdata.com>
> > Date: Thu Sep 30 17:53:04 2021 +0300
> >
> > Fixed GSSContext to start sequence numbering from 1
> >
> > GSS sequence number 0 is usually used by NFS4 NULL request
> > during GSS context establishment (but ignored by server).
> > Client should never reuse GSS sequence number, so using
> > 0 for the next real operation (EXCHANGE_ID) is possible but
> > looks suspicious. Fixed the code so numbering for operations
> > is done from 1 to avoid confusion.
>
> So, I can verify that --security=krb5 works after this patch but not
> before, good. But why is that? As you say, the server is supposed to
> ignore the sequence number on context creation requests. And 0 is valid
> sequence number as far as I know.
Looking at the network--my server's not responding to the first data
message.
I think the Linux server just has a bug. I'll make a patch....
--b.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: GSSAPI fix for pynfs nfs4.1 client code
2021-09-30 21:25 ` J. Bruce Fields
@ 2021-10-01 6:27 ` Volodymyr Khomenko
0 siblings, 0 replies; 12+ messages in thread
From: Volodymyr Khomenko @ 2021-10-01 6:27 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-nfs
> Looking at the network--my server's not responding to the first data
> message.
I see that linux client (when doing mount -t nfs -o
"vers=4.1,sec=krb") always does the same
(i.e. the very 1st packet with EXCHANGE_ID operation comes with GSS
sequence number=1,
then CREATE_SESSION uses seq_num=2, and so on). If your server works
normally with a regular mount,
I don't think that my fix is not related at all to this problem
volodymyr.
On Fri, Oct 1, 2021 at 12:25 AM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Thu, Sep 30, 2021 at 05:11:23PM -0400, J. Bruce Fields wrote:
> > On Thu, Sep 30, 2021 at 06:22:09PM +0300, Volodymyr Khomenko wrote:
> > > commit b77dc49c775756f08bdd0c6ebbe67a96f0ffe41f
> > > Author: Volodymyr Khomenko <volodymyr@vastdata.com>
> > > Date: Thu Sep 30 17:53:04 2021 +0300
> > >
> > > Fixed GSSContext to start sequence numbering from 1
> > >
> > > GSS sequence number 0 is usually used by NFS4 NULL request
> > > during GSS context establishment (but ignored by server).
> > > Client should never reuse GSS sequence number, so using
> > > 0 for the next real operation (EXCHANGE_ID) is possible but
> > > looks suspicious. Fixed the code so numbering for operations
> > > is done from 1 to avoid confusion.
> >
> > So, I can verify that --security=krb5 works after this patch but not
> > before, good. But why is that? As you say, the server is supposed to
> > ignore the sequence number on context creation requests. And 0 is valid
> > sequence number as far as I know.
>
> Looking at the network--my server's not responding to the first data
> message.
>
> I think the Linux server just has a bug. I'll make a patch....
>
> --b.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: GSSAPI fix for pynfs nfs4.1 client code
2021-09-30 21:11 ` J. Bruce Fields
2021-09-30 21:25 ` J. Bruce Fields
@ 2021-10-01 6:12 ` Volodymyr Khomenko
2021-10-01 6:49 ` Volodymyr Khomenko
2 siblings, 0 replies; 12+ messages in thread
From: Volodymyr Khomenko @ 2021-10-01 6:12 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-nfs
> So, I can verify that --security=krb5 works after this patch but not
> before, good. But why is that? As you say, the server is supposed to
> ignore the sequence number on context creation requests. And 0 is valid
> sequence number as far as I know.
It is very confusing to have two different requests with the same GSS
sequence number.
Yes, one of them is ignored and technically this is possible, but it
makes more mess in the traffic
and it's more hard to analyze it manually.
Moveover, Linux client does the same (EXCHANGE_ID comes with seq_num=1)
when mounting NFS4 - I guess this is a good approach to follow.
volodymyr.
On Fri, Oct 1, 2021 at 12:11 AM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Thu, Sep 30, 2021 at 06:22:09PM +0300, Volodymyr Khomenko wrote:
> > commit b77dc49c775756f08bdd0c6ebbe67a96f0ffe41f
> > Author: Volodymyr Khomenko <volodymyr@vastdata.com>
> > Date: Thu Sep 30 17:53:04 2021 +0300
> >
> > Fixed GSSContext to start sequence numbering from 1
> >
> > GSS sequence number 0 is usually used by NFS4 NULL request
> > during GSS context establishment (but ignored by server).
> > Client should never reuse GSS sequence number, so using
> > 0 for the next real operation (EXCHANGE_ID) is possible but
> > looks suspicious. Fixed the code so numbering for operations
> > is done from 1 to avoid confusion.
>
> So, I can verify that --security=krb5 works after this patch but not
> before, good. But why is that? As you say, the server is supposed to
> ignore the sequence number on context creation requests. And 0 is valid
> sequence number as far as I know.
>
> --b.
>
> >
> > Signed-off-by: Volodymyr Khomenko <volodymyr@vastdata.com>
> >
> > diff --git a/rpc/security.py b/rpc/security.py
> > index 0682f43..86f6592 100644
> > --- a/rpc/security.py
> > +++ b/rpc/security.py
> > @@ -174,7 +174,9 @@ class GSSContext(object):
> > def __init__(self, context_ptr):
> > self.lock = threading.Lock()
> > self.ptr = context_ptr
> > - self.seqid = 0 # client - next seqid to use
> > + # Note - seqid=0 is usually used during GSS context establishment,
> > + # to have the unique number we need to use the next value now.
> > + self.seqid = 1 # client - next seqid to use
> > self.highest = 0 # server - highest seqid seen
> > self.seen = 0 # server - bitmask of seen requests
> >
>
> > commit a612cf9897f0fa5b5de94885e00ef9293e93ffa3
> > Author: Volodymyr Khomenko <volodymyr@vastdata.com>
> > Date: Thu Sep 30 16:29:07 2021 +0300
> >
> > Fixed gssapi usage (RPCGSS) for nfs4.1 client
> >
> > gssapi library used in the code has been changed and
> > current code is not compatible with API of new library version.
> > Fixed the code to work with recent gssapi (tested with 1.6.2).
> > Tested with krb5, krb5i and krb5p security:
> > ./nfs4.1/testserver.py server.fqdn:/export --maketree --security=krb5 all
> >
> > Signed-off-by: Volodymyr Khomenko <volodymyr@vastdata.com>
> >
> > diff --git a/rpc/security.py b/rpc/security.py
> > index fe4390c..0682f43 100644
> > --- a/rpc/security.py
> > +++ b/rpc/security.py
> > @@ -10,6 +10,7 @@ from . import gss_type
> > from .gss_type import rpc_gss_init_res
> > try:
> > import gssapi
> > + from gssapi.raw.misc import GSSError
> > except ImportError:
> > print("Could not find gssapi module, proceeding without")
> > gssapi = None
> > @@ -242,11 +243,11 @@ class AuthGss(AuthNone):
> >
> > def init_cred(self, call, target="nfs@jupiter", source=None, oid=None):
> > # STUB - need intelligent way to set defaults
> > - good_major = [gssapi.GSS_S_COMPLETE, gssapi.GSS_S_CONTINUE_NEEDED]
> > + good_major = [GSS_S_COMPLETE, GSS_S_CONTINUE_NEEDED]
> > p = Packer()
> > up = GSSUnpacker('')
> > # Set target (of form nfs@SERVER)
> > - target = gssapi.Name(target, gssapi.NT_HOSTBASED_SERVICE)
> > + target = gssapi.Name(target, gssapi.NameType.hostbased_service)
> > # Set source (of form USERNAME)
> > if source is not None:
> > source = gssapi.Name(source, gssapi.NT_USER_NAME)
> > @@ -254,18 +255,26 @@ class AuthGss(AuthNone):
> > else:
> > # Just use default cred
> > gss_cred = None
> > - context = gssapi.Context()
> > - token = None
> > - handle = ''
> > + # RFC2203 5.2.2. Context Creation Requests
> > + # When GSS_Init_sec_context() is called, the parameters
> > + # replay_det_req_flag and sequence_req_flag must be turned off.
> > +
> > + # Note - by default, out_of_sequence_detection flag (sequence_req_flag) is used by gssapi.init_sec_context()
> > + # and we have 'An expected per-message token was not received' error (GSS_S_GAP_TOKEN).
> > + # To prevent this, we need to use default flags without out_of_sequence_detection bit.
> > + flags = gssapi.IntEnumFlagSet(gssapi.RequirementFlag, [gssapi.RequirementFlag.mutual_authentication])
> > + context = gssapi.SecurityContext(name=target, creds=gss_cred, flags=flags)
> > + input_token = None
> > + handle = b''
> > proc = RPCSEC_GSS_INIT
> > - while True:
> > + while not context.complete:
> > # Call initSecContext. If it returns COMPLETE, we are done.
> > # If it returns CONTINUE_NEEDED, we must send d['token']
> > # to the target, which will run it through acceptSecContext,
> > # and give us back a token we need to send through initSecContext.
> > # Repeat as necessary.
> > - token = context.init(target, token, gss_cred)
> > - if context.open:
> > + output_token = context.step(input_token)
> > + if context.complete:
> > # XXX if res.major == CONTINUE there is a bug in library code
> > # STUB - now what? Just use context?
> > # XXX need to use res.seq_window
> > @@ -277,16 +286,16 @@ class AuthGss(AuthNone):
> > gss_proc=proc)
> > proc = RPCSEC_GSS_CONTINUE_INIT
> > p.reset()
> > - p.pack_opaque(token)
> > + p.pack_opaque(output_token)
> > header, reply = call(p.get_buffer(), credinfo)
> > up.reset(reply)
> > res = up.unpack_rpc_gss_init_res()
> > up.done()
> > # res now holds relevent output from target's acceptSecContext call
> > if res.gss_major not in good_major:
> > - raise gssapi.Error(res.gss_major, res.gss_minor)
> > + raise GSSError(res.gss_major, res.gss_minor)
> > handle = res.handle # Should not change between calls
> > - token = res.gss_token # This needs to be sent to initSecContext
> > + input_token = res.gss_token # This needs to be sent to SecurityContext.step()
> > return CredInfo(self, context=handle)
> >
> > @staticmethod
> > @@ -361,7 +370,7 @@ class AuthGss(AuthNone):
> > except:
> > log_gss.exception("unsecure_data - initial unpacking")
> > raise rpclib.RPCUnsuccessfulReply(GARBAGE_ARGS)
> > - qop = context.verifyMIC(data, checksum)
> > + qop = context.verify_signature(data, checksum)
> > check_gssapi(qop)
> > data = pull_seqnum(data)
> > elif cred.service == rpc_gss_svc_privacy:
> > @@ -373,14 +382,14 @@ class AuthGss(AuthNone):
> > log_gss.exception("unsecure_data - initial unpacking")
> > raise rpclib.RPCUnsuccessfulReply(GARBAGE_ARGS)
> > # data, qop, conf = context.unwrap(data)
> > - data, qop = context.unwrap(data)
> > + data, encrypted, qop = context.unwrap(data)
> > check_gssapi(qop)
> > data = pull_seqnum(data)
> > else:
> > # Can't get here, but doesn't hurt
> > log_gss.error("Unknown service %i for RPCSEC_GSS" % cred.service)
> > - except gssapi.Error as e:
> > - log_gss.warn("unsecure_data: gssapi call returned %s" % e.name)
> > + except GSSError as e:
> > + log_gss.warn("unsecure_data: gssapi call returned %s" % str(e))
> > raise rpclib.RPCUnsuccessfulReply(GARBAGE_ARGS)
> > return data
> >
> > @@ -397,7 +406,7 @@ class AuthGss(AuthNone):
> > # data = opaque[gss_seq_num+data] + opaque[checksum]
> > p.pack_uint(cred.seq_num)
> > data = p.get_buffer() + data
> > - token = context.getMIC(data) # XXX BUG set qop
> > + token = context.get_signature(data) # XXX BUG set qop
> > p.reset()
> > p.pack_opaque(data)
> > p.pack_opaque(token)
> > @@ -406,16 +415,16 @@ class AuthGss(AuthNone):
> > # data = opaque[wrap([gss_seq_num+data])]
> > p.pack_uint(cred.seq_num)
> > data = p.get_buffer() + data
> > - token = context.wrap(data) # XXX BUG set qop
> > + wrap_res = context.wrap(data, encrypt=True) # XXX BUG set qop
> > p.reset()
> > - p.pack_opaque(token)
> > + p.pack_opaque(wrap_res.message)
> > data = p.get_buffer()
> > else:
> > # Can't get here, but doesn't hurt
> > log_gss.error("Unknown service %i for RPCSEC_GSS" % cred.service)
> > - except gssapi.Error as e:
> > + except GSSError as e:
> > # XXX What now?
> > - log_gss.warn("secure_data: gssapi call returned %s" % e.name)
> > + log_gss.warn("secure_data: gssapi call returned %s" % str(e))
> > raise
> > return data
> >
> > @@ -436,8 +445,8 @@ class AuthGss(AuthNone):
> > return rpclib.NULL_CRED
> > else:
> > data = self.partially_packed_header(xid, body)
> > - # XXX how handle gssapi.Error?
> > - token = self._get_context(body.cred.body.handle).getMIC(data)
> > + # XXX how handle GSSError?
> > + token = self._get_context(body.cred.body.handle).get_signature(data)
> > return opaque_auth(RPCSEC_GSS, token)
> >
> > def check_call_verf(self, xid, body):
> > @@ -448,10 +457,10 @@ class AuthGss(AuthNone):
> > return False
> > data = self.partially_packed_header(xid, body)
> > try:
> > - qop = self._get_context(body.cred.body.handle).verifyMIC(data, body.verf.body)
> > - except gssapi.Error as e:
> > + qop = self._get_context(body.cred.body.handle).verify_signature(data, body.verf.body)
> > + except GSSError as e:
> > log_gss.warn("Verifier checksum failed verification with %s" %
> > - e.name)
> > + str(e))
> > return False
> > body.cred.body.qop = qop # XXX Where store this?
> > log_gss.debug("verifier checks out (qop=%i)" % qop)
> > @@ -522,10 +531,10 @@ class AuthGss(AuthNone):
> > context = self._get_context(cred.body.handle)
> > try:
> > token = context.accept(token)
> > - except gssapi.Error as e:
> > + except GSSError as e:
> > log_gss.debug("RPCSEC_GSS_INIT failed (%s, %i)!" %
> > - (e.name, e.minor))
> > - res = rpc_gss_init_res('', e.major, e.minor, 0, '')
> > + (str(e), e.min_code))
> > + res = rpc_gss_init_res('', e.maj_code, e.min_code, 0, '')
> > else:
> > log_gss.debug("RPCSEC_GSS_*INIT succeeded!")
> > if first:
> > @@ -538,9 +547,9 @@ class AuthGss(AuthNone):
> > else:
> > handle = cred.body.handle
> > if context.open:
> > - major = gssapi.GSS_S_COMPLETE
> > + major = GSS_S_COMPLETE
> > else:
> > - major = gssapi.GSS_S_CONTINUE_NEEDED
> > + major = GSS_S_CONTINUE_NEEDED
> > res = rpc_gss_init_res(handle, major, 0, # XXX can't see minor
> > WINDOWSIZE, token)
> > # Prepare response
> > @@ -559,15 +568,15 @@ class AuthGss(AuthNone):
> > # NOTE this relies on GSS_S_COMPLETE == rpc.SUCCESS == 0
> > return rpclib.NULL_CRED
> > elif cred.gss_proc in (RPCSEC_GSS_INIT, RPCSEC_GSS_CONTINUE_INIT):
> > - # init requires getMIC(seq_window)
> > + # init requires get_signature(seq_window)
> > i = WINDOWSIZE
> > else:
> > - # Else return getMIC(cred.seq_num)
> > + # Else return get_signature(cred.seq_num)
> > i = cred.seq_num
> > p = Packer()
> > p.pack_uint(i)
> > # XXX BUG - need to set qop
> > - token = self._get_context(cred.handle).getMIC(p.get_buffer())
> > + token = self._get_context(cred.handle).get_signature(p.get_buffer())
> > return opaque_auth(RPCSEC_GSS, token)
> >
> > def check_reply_verf(self, msg, call_cred, data):
> > @@ -593,12 +602,12 @@ class AuthGss(AuthNone):
> > if res.gss_major != GSS_S_COMPLETE:
> > raise SecError("Expected NULL")
> > # BUG - context establishment is not finished on client
> > - # - so how get context? How run verifyMIC?
> > + # - so how get context? How run verify_signature?
> > # - This seems to be a protocol problem. Just ignore for now
> > else:
> > p = Packer()
> > p.pack_uint(call_cred.body.seq_num)
> > - qop = call_cred.context.verifyMIC(p.get_buffer(), verf.body)
> > + qop = call_cred.context.verify_signature(p.get_buffer(), verf.body)
> > if qop != call_cred.body.qop:
> > raise SecError("Mismatched qop")
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: GSSAPI fix for pynfs nfs4.1 client code
2021-09-30 21:11 ` J. Bruce Fields
2021-09-30 21:25 ` J. Bruce Fields
2021-10-01 6:12 ` Volodymyr Khomenko
@ 2021-10-01 6:49 ` Volodymyr Khomenko
2021-10-01 14:13 ` J. Bruce Fields
2 siblings, 1 reply; 12+ messages in thread
From: Volodymyr Khomenko @ 2021-10-01 6:49 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-nfs
> So, I can verify that --security=krb5 works after this patch but not
> before, good. But why is that? As you say, the server is supposed to
> ignore the sequence number on context creation requests. And 0 is valid
> sequence number as far as I know.
By design of RPCGSS we have a 'last seen seq_number' counter on the
server side per each GSS context
and we must not accept any packet that was already seen before (we
also have a bitmask of several last requests for this).
This 'last seen counter' is unsigned int32 (the same as seq_num) so we
can't just init it to -1 so next seq_num=0 will be valid.
The most obvious implementation is to init it last_seen_seq_num to 0
so the very 1st packet after NFS4 NULL must be 1 to differ from last
seen seq_number.
A better implementation (theoretically) can set this counter to
'undefined' state by additional flag, but this is more
resource-consuming
(you need to process is_inited flag + last_seen_seq_num instead of
just one counter).
If the last seen seq_number is undefined during GSS initialization,
then strictly speaking we can send ANY seq_num for the very 1st
request after NFS4 NULL.
volodymyr.
On Fri, Oct 1, 2021 at 12:11 AM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Thu, Sep 30, 2021 at 06:22:09PM +0300, Volodymyr Khomenko wrote:
> > commit b77dc49c775756f08bdd0c6ebbe67a96f0ffe41f
> > Author: Volodymyr Khomenko <volodymyr@vastdata.com>
> > Date: Thu Sep 30 17:53:04 2021 +0300
> >
> > Fixed GSSContext to start sequence numbering from 1
> >
> > GSS sequence number 0 is usually used by NFS4 NULL request
> > during GSS context establishment (but ignored by server).
> > Client should never reuse GSS sequence number, so using
> > 0 for the next real operation (EXCHANGE_ID) is possible but
> > looks suspicious. Fixed the code so numbering for operations
> > is done from 1 to avoid confusion.
>
> So, I can verify that --security=krb5 works after this patch but not
> before, good. But why is that? As you say, the server is supposed to
> ignore the sequence number on context creation requests. And 0 is valid
> sequence number as far as I know.
>
> --b.
>
> >
> > Signed-off-by: Volodymyr Khomenko <volodymyr@vastdata.com>
> >
> > diff --git a/rpc/security.py b/rpc/security.py
> > index 0682f43..86f6592 100644
> > --- a/rpc/security.py
> > +++ b/rpc/security.py
> > @@ -174,7 +174,9 @@ class GSSContext(object):
> > def __init__(self, context_ptr):
> > self.lock = threading.Lock()
> > self.ptr = context_ptr
> > - self.seqid = 0 # client - next seqid to use
> > + # Note - seqid=0 is usually used during GSS context establishment,
> > + # to have the unique number we need to use the next value now.
> > + self.seqid = 1 # client - next seqid to use
> > self.highest = 0 # server - highest seqid seen
> > self.seen = 0 # server - bitmask of seen requests
> >
>
> > commit a612cf9897f0fa5b5de94885e00ef9293e93ffa3
> > Author: Volodymyr Khomenko <volodymyr@vastdata.com>
> > Date: Thu Sep 30 16:29:07 2021 +0300
> >
> > Fixed gssapi usage (RPCGSS) for nfs4.1 client
> >
> > gssapi library used in the code has been changed and
> > current code is not compatible with API of new library version.
> > Fixed the code to work with recent gssapi (tested with 1.6.2).
> > Tested with krb5, krb5i and krb5p security:
> > ./nfs4.1/testserver.py server.fqdn:/export --maketree --security=krb5 all
> >
> > Signed-off-by: Volodymyr Khomenko <volodymyr@vastdata.com>
> >
> > diff --git a/rpc/security.py b/rpc/security.py
> > index fe4390c..0682f43 100644
> > --- a/rpc/security.py
> > +++ b/rpc/security.py
> > @@ -10,6 +10,7 @@ from . import gss_type
> > from .gss_type import rpc_gss_init_res
> > try:
> > import gssapi
> > + from gssapi.raw.misc import GSSError
> > except ImportError:
> > print("Could not find gssapi module, proceeding without")
> > gssapi = None
> > @@ -242,11 +243,11 @@ class AuthGss(AuthNone):
> >
> > def init_cred(self, call, target="nfs@jupiter", source=None, oid=None):
> > # STUB - need intelligent way to set defaults
> > - good_major = [gssapi.GSS_S_COMPLETE, gssapi.GSS_S_CONTINUE_NEEDED]
> > + good_major = [GSS_S_COMPLETE, GSS_S_CONTINUE_NEEDED]
> > p = Packer()
> > up = GSSUnpacker('')
> > # Set target (of form nfs@SERVER)
> > - target = gssapi.Name(target, gssapi.NT_HOSTBASED_SERVICE)
> > + target = gssapi.Name(target, gssapi.NameType.hostbased_service)
> > # Set source (of form USERNAME)
> > if source is not None:
> > source = gssapi.Name(source, gssapi.NT_USER_NAME)
> > @@ -254,18 +255,26 @@ class AuthGss(AuthNone):
> > else:
> > # Just use default cred
> > gss_cred = None
> > - context = gssapi.Context()
> > - token = None
> > - handle = ''
> > + # RFC2203 5.2.2. Context Creation Requests
> > + # When GSS_Init_sec_context() is called, the parameters
> > + # replay_det_req_flag and sequence_req_flag must be turned off.
> > +
> > + # Note - by default, out_of_sequence_detection flag (sequence_req_flag) is used by gssapi.init_sec_context()
> > + # and we have 'An expected per-message token was not received' error (GSS_S_GAP_TOKEN).
> > + # To prevent this, we need to use default flags without out_of_sequence_detection bit.
> > + flags = gssapi.IntEnumFlagSet(gssapi.RequirementFlag, [gssapi.RequirementFlag.mutual_authentication])
> > + context = gssapi.SecurityContext(name=target, creds=gss_cred, flags=flags)
> > + input_token = None
> > + handle = b''
> > proc = RPCSEC_GSS_INIT
> > - while True:
> > + while not context.complete:
> > # Call initSecContext. If it returns COMPLETE, we are done.
> > # If it returns CONTINUE_NEEDED, we must send d['token']
> > # to the target, which will run it through acceptSecContext,
> > # and give us back a token we need to send through initSecContext.
> > # Repeat as necessary.
> > - token = context.init(target, token, gss_cred)
> > - if context.open:
> > + output_token = context.step(input_token)
> > + if context.complete:
> > # XXX if res.major == CONTINUE there is a bug in library code
> > # STUB - now what? Just use context?
> > # XXX need to use res.seq_window
> > @@ -277,16 +286,16 @@ class AuthGss(AuthNone):
> > gss_proc=proc)
> > proc = RPCSEC_GSS_CONTINUE_INIT
> > p.reset()
> > - p.pack_opaque(token)
> > + p.pack_opaque(output_token)
> > header, reply = call(p.get_buffer(), credinfo)
> > up.reset(reply)
> > res = up.unpack_rpc_gss_init_res()
> > up.done()
> > # res now holds relevent output from target's acceptSecContext call
> > if res.gss_major not in good_major:
> > - raise gssapi.Error(res.gss_major, res.gss_minor)
> > + raise GSSError(res.gss_major, res.gss_minor)
> > handle = res.handle # Should not change between calls
> > - token = res.gss_token # This needs to be sent to initSecContext
> > + input_token = res.gss_token # This needs to be sent to SecurityContext.step()
> > return CredInfo(self, context=handle)
> >
> > @staticmethod
> > @@ -361,7 +370,7 @@ class AuthGss(AuthNone):
> > except:
> > log_gss.exception("unsecure_data - initial unpacking")
> > raise rpclib.RPCUnsuccessfulReply(GARBAGE_ARGS)
> > - qop = context.verifyMIC(data, checksum)
> > + qop = context.verify_signature(data, checksum)
> > check_gssapi(qop)
> > data = pull_seqnum(data)
> > elif cred.service == rpc_gss_svc_privacy:
> > @@ -373,14 +382,14 @@ class AuthGss(AuthNone):
> > log_gss.exception("unsecure_data - initial unpacking")
> > raise rpclib.RPCUnsuccessfulReply(GARBAGE_ARGS)
> > # data, qop, conf = context.unwrap(data)
> > - data, qop = context.unwrap(data)
> > + data, encrypted, qop = context.unwrap(data)
> > check_gssapi(qop)
> > data = pull_seqnum(data)
> > else:
> > # Can't get here, but doesn't hurt
> > log_gss.error("Unknown service %i for RPCSEC_GSS" % cred.service)
> > - except gssapi.Error as e:
> > - log_gss.warn("unsecure_data: gssapi call returned %s" % e.name)
> > + except GSSError as e:
> > + log_gss.warn("unsecure_data: gssapi call returned %s" % str(e))
> > raise rpclib.RPCUnsuccessfulReply(GARBAGE_ARGS)
> > return data
> >
> > @@ -397,7 +406,7 @@ class AuthGss(AuthNone):
> > # data = opaque[gss_seq_num+data] + opaque[checksum]
> > p.pack_uint(cred.seq_num)
> > data = p.get_buffer() + data
> > - token = context.getMIC(data) # XXX BUG set qop
> > + token = context.get_signature(data) # XXX BUG set qop
> > p.reset()
> > p.pack_opaque(data)
> > p.pack_opaque(token)
> > @@ -406,16 +415,16 @@ class AuthGss(AuthNone):
> > # data = opaque[wrap([gss_seq_num+data])]
> > p.pack_uint(cred.seq_num)
> > data = p.get_buffer() + data
> > - token = context.wrap(data) # XXX BUG set qop
> > + wrap_res = context.wrap(data, encrypt=True) # XXX BUG set qop
> > p.reset()
> > - p.pack_opaque(token)
> > + p.pack_opaque(wrap_res.message)
> > data = p.get_buffer()
> > else:
> > # Can't get here, but doesn't hurt
> > log_gss.error("Unknown service %i for RPCSEC_GSS" % cred.service)
> > - except gssapi.Error as e:
> > + except GSSError as e:
> > # XXX What now?
> > - log_gss.warn("secure_data: gssapi call returned %s" % e.name)
> > + log_gss.warn("secure_data: gssapi call returned %s" % str(e))
> > raise
> > return data
> >
> > @@ -436,8 +445,8 @@ class AuthGss(AuthNone):
> > return rpclib.NULL_CRED
> > else:
> > data = self.partially_packed_header(xid, body)
> > - # XXX how handle gssapi.Error?
> > - token = self._get_context(body.cred.body.handle).getMIC(data)
> > + # XXX how handle GSSError?
> > + token = self._get_context(body.cred.body.handle).get_signature(data)
> > return opaque_auth(RPCSEC_GSS, token)
> >
> > def check_call_verf(self, xid, body):
> > @@ -448,10 +457,10 @@ class AuthGss(AuthNone):
> > return False
> > data = self.partially_packed_header(xid, body)
> > try:
> > - qop = self._get_context(body.cred.body.handle).verifyMIC(data, body.verf.body)
> > - except gssapi.Error as e:
> > + qop = self._get_context(body.cred.body.handle).verify_signature(data, body.verf.body)
> > + except GSSError as e:
> > log_gss.warn("Verifier checksum failed verification with %s" %
> > - e.name)
> > + str(e))
> > return False
> > body.cred.body.qop = qop # XXX Where store this?
> > log_gss.debug("verifier checks out (qop=%i)" % qop)
> > @@ -522,10 +531,10 @@ class AuthGss(AuthNone):
> > context = self._get_context(cred.body.handle)
> > try:
> > token = context.accept(token)
> > - except gssapi.Error as e:
> > + except GSSError as e:
> > log_gss.debug("RPCSEC_GSS_INIT failed (%s, %i)!" %
> > - (e.name, e.minor))
> > - res = rpc_gss_init_res('', e.major, e.minor, 0, '')
> > + (str(e), e.min_code))
> > + res = rpc_gss_init_res('', e.maj_code, e.min_code, 0, '')
> > else:
> > log_gss.debug("RPCSEC_GSS_*INIT succeeded!")
> > if first:
> > @@ -538,9 +547,9 @@ class AuthGss(AuthNone):
> > else:
> > handle = cred.body.handle
> > if context.open:
> > - major = gssapi.GSS_S_COMPLETE
> > + major = GSS_S_COMPLETE
> > else:
> > - major = gssapi.GSS_S_CONTINUE_NEEDED
> > + major = GSS_S_CONTINUE_NEEDED
> > res = rpc_gss_init_res(handle, major, 0, # XXX can't see minor
> > WINDOWSIZE, token)
> > # Prepare response
> > @@ -559,15 +568,15 @@ class AuthGss(AuthNone):
> > # NOTE this relies on GSS_S_COMPLETE == rpc.SUCCESS == 0
> > return rpclib.NULL_CRED
> > elif cred.gss_proc in (RPCSEC_GSS_INIT, RPCSEC_GSS_CONTINUE_INIT):
> > - # init requires getMIC(seq_window)
> > + # init requires get_signature(seq_window)
> > i = WINDOWSIZE
> > else:
> > - # Else return getMIC(cred.seq_num)
> > + # Else return get_signature(cred.seq_num)
> > i = cred.seq_num
> > p = Packer()
> > p.pack_uint(i)
> > # XXX BUG - need to set qop
> > - token = self._get_context(cred.handle).getMIC(p.get_buffer())
> > + token = self._get_context(cred.handle).get_signature(p.get_buffer())
> > return opaque_auth(RPCSEC_GSS, token)
> >
> > def check_reply_verf(self, msg, call_cred, data):
> > @@ -593,12 +602,12 @@ class AuthGss(AuthNone):
> > if res.gss_major != GSS_S_COMPLETE:
> > raise SecError("Expected NULL")
> > # BUG - context establishment is not finished on client
> > - # - so how get context? How run verifyMIC?
> > + # - so how get context? How run verify_signature?
> > # - This seems to be a protocol problem. Just ignore for now
> > else:
> > p = Packer()
> > p.pack_uint(call_cred.body.seq_num)
> > - qop = call_cred.context.verifyMIC(p.get_buffer(), verf.body)
> > + qop = call_cred.context.verify_signature(p.get_buffer(), verf.body)
> > if qop != call_cred.body.qop:
> > raise SecError("Mismatched qop")
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: GSSAPI fix for pynfs nfs4.1 client code
2021-10-01 6:49 ` Volodymyr Khomenko
@ 2021-10-01 14:13 ` J. Bruce Fields
2021-10-01 14:38 ` Volodymyr Khomenko
0 siblings, 1 reply; 12+ messages in thread
From: J. Bruce Fields @ 2021-10-01 14:13 UTC (permalink / raw)
To: Volodymyr Khomenko; +Cc: linux-nfs
On Fri, Oct 01, 2021 at 09:49:50AM +0300, Volodymyr Khomenko wrote:
> > So, I can verify that --security=krb5 works after this patch but not
> > before, good. But why is that? As you say, the server is supposed to
> > ignore the sequence number on context creation requests. And 0 is valid
> > sequence number as far as I know.
>
> By design of RPCGSS we have a 'last seen seq_number' counter on the
> server side per each GSS context
> and we must not accept any packet that was already seen before (we
> also have a bitmask of several last requests for this).
> This 'last seen counter' is unsigned int32 (the same as seq_num) so we
> can't just init it to -1 so next seq_num=0 will be valid.
> The most obvious implementation is to init it last_seen_seq_num to 0
> so the very 1st packet after NFS4 NULL must be 1 to differ from last
> seen seq_number.
Note in theory gssapi mechanisms can require multiple round trips (in
the GSS_PROC_CONTINUE_INIT case), so this wouldn't actually avoid
duplicate sequence numbers.
In any case, the rfc is unambiguous here: "In a creation request, the
seq_num and service fields are undefined and both must be ignored by the
server."
> A better implementation (theoretically) can set this counter to
> 'undefined' state by additional flag, but this is more
> resource-consuming
> (you need to process is_inited flag + last_seen_seq_num instead of
> just one counter).
> If the last seen seq_number is undefined during GSS initialization,
> then strictly speaking we can send ANY seq_num for the very 1st
> request after NFS4 NULL.
Right, again, from RFC 2203, " The seq_num field can start at any value
below MAXSEQ."
It can be implemented without the need for an is_inited flag.
The initial sequence number of 0 really did find an actual bug in the
server, so pynfs is definitely doing its job in this case!
--b.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: GSSAPI fix for pynfs nfs4.1 client code
2021-10-01 14:13 ` J. Bruce Fields
@ 2021-10-01 14:38 ` Volodymyr Khomenko
2021-10-01 15:48 ` J. Bruce Fields
0 siblings, 1 reply; 12+ messages in thread
From: Volodymyr Khomenko @ 2021-10-01 14:38 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-nfs
> The seq_num field can start at any value below MAXSEQ
Yes, that's the statement I haven't found before in RFC...
Probably we also need to write a test starting the seq_num from a big
value (more than SEQUENCE_WINDOW)
to make sure that it is really implemented properly without
'is_inited' flag (so what's the initial value?).
However I still propose to keep the default behaviour of pynfs to be
the same as for linux NFS4 client.
I think the caller can change it when needed (to 0 or whatever
needed), but the default value should be good...
volodymyr.
On Fri, Oct 1, 2021 at 5:13 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Oct 01, 2021 at 09:49:50AM +0300, Volodymyr Khomenko wrote:
> > > So, I can verify that --security=krb5 works after this patch but not
> > > before, good. But why is that? As you say, the server is supposed to
> > > ignore the sequence number on context creation requests. And 0 is valid
> > > sequence number as far as I know.
> >
> > By design of RPCGSS we have a 'last seen seq_number' counter on the
> > server side per each GSS context
> > and we must not accept any packet that was already seen before (we
> > also have a bitmask of several last requests for this).
> > This 'last seen counter' is unsigned int32 (the same as seq_num) so we
> > can't just init it to -1 so next seq_num=0 will be valid.
> > The most obvious implementation is to init it last_seen_seq_num to 0
> > so the very 1st packet after NFS4 NULL must be 1 to differ from last
> > seen seq_number.
>
> Note in theory gssapi mechanisms can require multiple round trips (in
> the GSS_PROC_CONTINUE_INIT case), so this wouldn't actually avoid
> duplicate sequence numbers.
>
> In any case, the rfc is unambiguous here: "In a creation request, the
> seq_num and service fields are undefined and both must be ignored by the
> server."
>
> > A better implementation (theoretically) can set this counter to
> > 'undefined' state by additional flag, but this is more
> > resource-consuming
> > (you need to process is_inited flag + last_seen_seq_num instead of
> > just one counter).
> > If the last seen seq_number is undefined during GSS initialization,
> > then strictly speaking we can send ANY seq_num for the very 1st
> > request after NFS4 NULL.
>
> Right, again, from RFC 2203, " The seq_num field can start at any value
> below MAXSEQ."
>
> It can be implemented without the need for an is_inited flag.
>
> The initial sequence number of 0 really did find an actual bug in the
> server, so pynfs is definitely doing its job in this case!
>
> --b.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: GSSAPI fix for pynfs nfs4.1 client code
2021-10-01 14:38 ` Volodymyr Khomenko
@ 2021-10-01 15:48 ` J. Bruce Fields
2021-10-02 6:12 ` Volodymyr Khomenko
0 siblings, 1 reply; 12+ messages in thread
From: J. Bruce Fields @ 2021-10-01 15:48 UTC (permalink / raw)
To: Volodymyr Khomenko; +Cc: linux-nfs
On Fri, Oct 01, 2021 at 05:38:45PM +0300, Volodymyr Khomenko wrote:
> > The seq_num field can start at any value below MAXSEQ
> Yes, that's the statement I haven't found before in RFC...
> Probably we also need to write a test starting the seq_num from a big
> value (more than SEQUENCE_WINDOW)
> to make sure that it is really implemented properly without
> 'is_inited' flag (so what's the initial value?).
>
> However I still propose to keep the default behaviour of pynfs to be
> the same as for linux NFS4 client.
> I think the caller can change it when needed (to 0 or whatever
> needed), but the default value should be good...
That's what I'd choose if I were writing a "real" client. Everybody
already tests with the Linux client, so its behavior is a safe bet.
But I'd usually prefer a test client do different things than the client
everyone already tests with.
Like I say, the seqid=0 already caught a bug in my server, so I'm a fan.
(And it's a bug that would also trigger if any of the first 128 rpcs
were out of order. But that would probably manifest as some user
reporting "once in a blue moon my krb5 mounts hang" and I think it would
take a while to get from that report to this bug as the root cause. So
I'm glad pynfs hit it....)
--b.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: GSSAPI fix for pynfs nfs4.1 client code
2021-10-01 15:48 ` J. Bruce Fields
@ 2021-10-02 6:12 ` Volodymyr Khomenko
2021-10-02 20:38 ` J. Bruce Fields
0 siblings, 1 reply; 12+ messages in thread
From: Volodymyr Khomenko @ 2021-10-02 6:12 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-nfs
Ok, I see your point - yes, it's better that pynfs has "unusual but
still RFC-compliant" behaviour to catch the real bugs.
I will check how this behaviour (using seqid=0 or even something else
for EXCHANGE_ID) can be controlled from the caller side
if we want to override it.
P.S. Since the very 1st operation after NFS4 NULL is EXCHANGE_ID - it
should be only single operation
(client can't send few ECHANGE_ID because clientowner is only one per
mount) and next CREATE_SESSION can't be sent
until EXCHANGE_ID is replied from the server.
So the use-case of 'any of the first 128 rpcs were out of order' is
just a theoretical one and probably not possible in practice.
And thanks for handling the patch!
volodymyr.
On Fri, Oct 1, 2021 at 6:48 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Oct 01, 2021 at 05:38:45PM +0300, Volodymyr Khomenko wrote:
> > > The seq_num field can start at any value below MAXSEQ
> > Yes, that's the statement I haven't found before in RFC...
> > Probably we also need to write a test starting the seq_num from a big
> > value (more than SEQUENCE_WINDOW)
> > to make sure that it is really implemented properly without
> > 'is_inited' flag (so what's the initial value?).
> >
> > However I still propose to keep the default behaviour of pynfs to be
> > the same as for linux NFS4 client.
> > I think the caller can change it when needed (to 0 or whatever
> > needed), but the default value should be good...
>
> That's what I'd choose if I were writing a "real" client. Everybody
> already tests with the Linux client, so its behavior is a safe bet.
>
> But I'd usually prefer a test client do different things than the client
> everyone already tests with.
>
> Like I say, the seqid=0 already caught a bug in my server, so I'm a fan.
>
> (And it's a bug that would also trigger if any of the first 128 rpcs
> were out of order. But that would probably manifest as some user
> reporting "once in a blue moon my krb5 mounts hang" and I think it would
> take a while to get from that report to this bug as the root cause. So
> I'm glad pynfs hit it....)
>
> --b.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: GSSAPI fix for pynfs nfs4.1 client code
2021-10-02 6:12 ` Volodymyr Khomenko
@ 2021-10-02 20:38 ` J. Bruce Fields
0 siblings, 0 replies; 12+ messages in thread
From: J. Bruce Fields @ 2021-10-02 20:38 UTC (permalink / raw)
To: Volodymyr Khomenko; +Cc: linux-nfs
On Sat, Oct 02, 2021 at 09:12:25AM +0300, Volodymyr Khomenko wrote:
> P.S. Since the very 1st operation after NFS4 NULL is EXCHANGE_ID - it
> should be only single operation
> (client can't send few ECHANGE_ID because clientowner is only one per
> mount) and next CREATE_SESSION can't be sent
> until EXCHANGE_ID is replied from the server.
> So the use-case of 'any of the first 128 rpcs were out of order' is
> just a theoretical one and probably not possible in practice.
So our server uses a fixed-size gss sequence number window of 128. We
keep track of sd_max, the largest sequence number we've seen so far.
Given an incoming rpc with sequence number seqno, we check:
is seqno > sd_max?
This is the normal case for in-order sequence numbers;
update sd_max and our other sequence number data and
continue normal processing.
else is seqno < sd_max - 128?
Oops, this is definitely too old; drop the request.
else check our data about sequence numbers seen so far.
But our specific bug was we were doing the second check using unsigned
arithmetic, so if we hit the second check before sd_max hits 128, then
(sd_max - 128) is something very large, and we drop the request.
--b.
^ permalink raw reply [flat|nested] 12+ messages in thread