On Tue, Dec 05, 2023 at 03:43:16PM -0500, Olga Kornievskaia wrote:
> On Mon, Nov 27, 2023 at 8:15 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
> >
> > On Mon, Nov 27, 2023 at 11:42:08AM -1000, Olga Kornievskaia wrote:
> > > Hi Chuck,
> > >
> > > Given that rpc_gss_secreate() was written by you I hope you dont mind
> > > questions. I believe gssd can't use the new api because it is
> > > insufficient. Specifically, authgss_create_default() takes in a
> > > structure which is populated with the correct (kerberos) credential we
> > > need to be using for the gss context establishment.
> > > rpc_gss_seccreate() sets the sec.cred = GSS_C_NO_CREDENTIAL. If you
> > > believe I'm incorrect in my assessment that rpc_gss_secreate please
> > > let me know.
> >
> > Caller can pass its preferred credential in via the *req parameter
> > (parameter 6). Then rpc_gss_seccreate(3t) does this:
> >
> > 846 if (req) {
> > 847 sec.req_flags = req->req_flags;
> > 848 gd->time_req = req->time_req;
> > 849 sec.cred = req->my_cred;
> > 850 gd->icb = req->input_channel_bindings;
> > 851 }
> >
> > Wouldn't that work?
>
> Actually this does not work. However, this does:
> if (req) {
> sec.req_flags = req->req_flags;
> gd->time_req = req->time_req;
> gd->sec.cred = req->my_cred;
> gd->icb = req->input_channel_bindings;
> }
I think this is a bug, and your fix is correct but maybe not
complete. I think you also need:
gd->sec.req_flags = req->req_flags;
> Existing code is such that cred in gd->sec.cred is always null. I'm
> 100% certain of that but I can't explain why sec.cred=req->my_cred is
> not working.
The misunderstanding is that:
gd->sec = sec;
copies a pointer to sec. It actually copies the content of @sec.
As it stands, @sec is not subsequently used after it is copied into
gd->sec, so any changes to @sec's fields are ignored. Thus the
"if (req)" clause needs to update the copy in gd->sec, not @sec
itself.
Fixes: 5f1fe4dde861 ("Pass time_req and input_channel_bindings through to init_sec_context")
> This is current code:
> in authgss_refresh()
> rpc_gss_sec:
> mechanism_OID: { 1 2 134 72 134 247 18 1 2 2 }
> qop: 0
> service: 1
> cred: (nil)
>
> Fixed libtirpc code as above (or code using authgss_create_default()):
> in authgss_refresh()
> rpc_gss_sec:
> mechanism_OID: { 1 2 134 72 134 247 18 1 2 2 }
> qop: 0
> service: 1
> cred: 0xffff9002e000
>
> If I were to fix the libtirpc this way, then I can use
> rpc_gss_seccreate from gssd and not use my previous changes. But it
> still requires changes to libtirpc. How is that not different from
> what's already there (as in committed)?
>
> > > As far as I can see, current libtirpc needs to be modified no matter
> > > what.
> >
> > I'm not convinced of that yet.
>
> See above?
>
> > > Perhaps there needs to be some config magic to demand the use of
> > > higher version of libtirpc for the new code but it would be just a
> > > different way an upstream nfs-utils won't build without an appropriate
> > > libtirpc version.
> >
> > Agreed, 4b272471937d ("gssd: handle KRB5_AP_ERR_BAD_INTEGRITY for
> > machine credentials") should have added some config magic.
> >
> > If the libtirpc version is too low, then the new GSS status checking
> > logic you added can't be used, and it should fall back to the old
> > logic via #ifdef.
> >
> >
> > > I would imagine distros would build matching
> > > libtirpc and nfs-utils that would either both not have the fix or have
> > > the fix.
> >
> > I don't think distros work that way unless they are forced to.
> > There doesn't seem to be a reason why the nfs-utils change has
> > to break things -- we can do this without the ABI disruption.
> >
> > The change to struct rpc_gss_sec can't remain. IMO both
> >
> > f5b6e6fdb1e6 ("gss-api: expose gss major/minor error in authgss_refresh()")
> >
> > and
> >
> > 4b272471937d ("gssd: handle KRB5_AP_ERR_BAD_INTEGRITY for machine credentials")
> >
> > need to be reverted first.
> >
> > Then a patch that replaces the authgss_create_default(3) call with
> > rpc_gss_seccreate(3t) can be applied, as long as it contains new
> > configure.ac logic to fall back to using authgss_create_default(3)
> > if rpc_gss_seccreate(3t) is not available in libtirpc.
> >
> > That should enable nfs-utils to be built no matter what version of
> > libtirpc is available in the build environment.
> >
> >
> > > On Wed, Nov 22, 2023 at 4:31 AM Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote:
> > > >
> > > > Possibly because authgss_create_default() was the API
> > > > available to gssd back in the day. rpc_gss_seccreate(3t)
> > > > is newer. That would be my guess.
> > > >
> > > >
> > > > > On Nov 22, 2023, at 1:07 AM, Olga Kornievskaia <aglo@xxxxxxxxx> wrote:
> > > > >
> > > > > Hi Chuck,
> > > > >
> > > > > A quick reply as I'm on vacation but I can take a look when I get
> > > > > back. I'm just thinking there must be a reason why gssd is using the
> > > > > authgss api and not calling the rpc_gss one.
> > > > >
> > > > > On Tue, Nov 21, 2023 at 6:59 AM Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote:
> > > > >>
> > > > >> Hey Olga-
> > > > >>
> > > > >> I see that f5b6e6fdb1e6 ("gss-api: expose gss major/minor error in
> > > > >> authgss_refresh()") added a couple of fields in structure rpc_gss_sec.
> > > > >> Later, there are some nfs-utils changes that start using those fields.
> > > > >>
> > > > >> That breaks building the latest upstream nfs-utils on Fedora 38, whose
> > > > >> current libtirpc doesn't have those new fields.
> > > > >>
> > > > >> IMO struct rpc_gss_sec is part of the libtirpc API/ABI, thus we really
> > > > >> shouldn't change it.
> > > > >>
> > > > >> Instead, if gssd needs GSS status codes, can't it call
> > > > >> rpc_gss_seccreate(3), which explicitly takes a struct
> > > > >> rpc_gss_options_ret_t * argument?
> > > > >>
> > > > >> ie, just replace the authgss_create_default() call with a call to
> > > > >> rpc_gss_seccreate(3) ....
> > > > >>
> > > > >>
> > > > >> --
> > > > >> Chuck Lever
> > > > >>
> > > > >>
> > > > >>
> > > >
> > > > --
> > > > Chuck Lever
> > > >
> > > >
> >
> > --
> > Chuck Lever
--
Chuck Lever
