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
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 <[email protected]> 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
>
>
>
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 <[email protected]> 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 <[email protected]> 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
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.
As far as I can see, current libtirpc needs to be modified no matter
what. 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. I would imagine distros would build matching
libtirpc and nfs-utils that would either both not have the fix or have
the fix.
On Wed, Nov 22, 2023 at 4:31 AM Chuck Lever III <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
>
>
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?
> As far as I can see, current libtirpc needs to be modified no matter
> what.
I'm not convinced of that yet.
> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
On Mon, Nov 27, 2023 at 8:15 PM Chuck Lever <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
On Tue, Dec 05, 2023 at 03:43:16PM -0500, Olga Kornievskaia wrote:
> On Mon, Nov 27, 2023 at 8:15 PM Chuck Lever <[email protected]> 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)?
Very simple: The libtirpc implementation we use belongs to the Linux
community. The TI-RPC API that it implements does not; it's used in
several other OSes (including TI-RPC API implementations that appear
in other libraries on Linux), and it is publicly documented.
Thus we cannot alter the public structures and function synopses
unless all implementations do.
I need to audit the library code, but I think the direction of your
suggested fix to rpc_gss_seccreate(3t) is sensible.
--
Chuck Lever
On Tue, Dec 05, 2023 at 03:43:16PM -0500, Olga Kornievskaia wrote:
> On Mon, Nov 27, 2023 at 8:15 PM Chuck Lever <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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