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;
}
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. 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
