Received-SPF: pass (google.com: domain of linux-nfs+bounces-362-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Wed, 6 Dec 2023 10:47:44 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: Olga Kornievskaia <aglo@umich.edu>
Cc: Olga Kornievskaia <kolga@netapp.com>, Steve Dickson <SteveD@redhat.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: changes to struct rpc_gss_sec
Message-ID: <ZXCXoMQS04uoSDB8@tissot.1015granger.net>
References: <97AE695C-8F9F-4E9C-9460-427C284FBD32@oracle.com>
 <CAN-5tyHxvTevgM38q94W4e+rBzYu7tWqDHVMNcFQ5GT3uNArCw@mail.gmail.com>
 <6F0CCBAF-29E1-4720-A7DC-9F43751B56E7@oracle.com>
 <CAN-5tyGqSXyeu+LXWVu_J=A8CLW01c2wDKSfBeqFaaWLxnOAyw@mail.gmail.com>
 <ZWU/BvRX6BcnSQPj@tissot.1015granger.net>
 <CAN-5tyHi8app4K9aRj1paF=zG6PHLs3w8=C6iH4mZgduxvaAbA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAN-5tyHi8app4K9aRj1paF=zG6PHLs3w8=C6iH4mZgduxvaAbA@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?dXRPY05XT3doMzBNMUdsTVZLblVNOXpPYVBGU2kycHBkNFR5elg3c3ZSTUhN?=
 =?utf-8?B?a05UaVd0eUVaRzg4aytNRm40aDd3WmpzT3d3VUcvbWlJOFRCN2FKdHpsS0pF?=
 =?utf-8?B?Q1U1TCtwWXFqMHpzL1NWaUVQdUQyZlp5TGRmRTFBLzBmZ3dBMTltNTBsaCtD?=
 =?utf-8?B?SkRBOGpsVjdVS1dFWEwxdUVweXRoV0liM0FZUEJSSmdieURMeUhFR295WlF2?=
 =?utf-8?B?ZVJoWkRoZ05ROGVkTkRFRUFXZWNOamZDMFZtWkdINmwyRm1Pd1lSWUI2czFJ?=
 =?utf-8?B?MkdBWENCT09hdjFHOEp3ZXFTb3RqZWIzWG8rTUZVeTk4bUtudmlpS1NMMmRH?=
 =?utf-8?B?SStkN2t6MFExREtva1k2NGdJRVc5K21rVVhISDdKZzdyYnNMait3OHNxWmkr?=
 =?utf-8?B?TzlmK3JtR3gwL05oaUJQRjRIcEQwWDYvWVZxeVV1UlBydXcwZ081N1IxcGMx?=
 =?utf-8?B?aU0yREV5dVEyWTRtaDIyL3ROWUh6NlVpWFBQVmFhcXZKZlYvM003aFN0WERR?=
 =?utf-8?B?MTh5SWp6WHc2dmE4c01VaVgvNmtSbEh4dHQwSEpuWDU5VHpnRWpJNGJRWTJH?=
 =?utf-8?B?VU02ODhFU2hVQVB1Rm9OTjZHdFRJQWJWOGVJTlE2aHJMNTZINlArR25KQ0gy?=
 =?utf-8?B?WGdKT01pZnlmRGI5SldYeDdocEJwclpFTzJxYXZxOXVhOGRxQ1F5Z1VKSnJW?=
 =?utf-8?B?dGREUkJIOFZEUkZLaW1UdUY4QTJ6aXRYaXVkS2ZJMFc4b1NxVzJlVDJqTHhY?=
 =?utf-8?B?cWYyVngzQUR6RVpVUGFua0EvRTZVaG1HeE9qSXJPYmQxVWxNNUUvbkUzaGJy?=
 =?utf-8?B?RUhOT3lRUXBwYmZQT0JaeHlqekdkZU1uZGZLSUE4SW9pRjN0NzA5SEZTc1hN?=
 =?utf-8?B?SUZUQ1hiNWZVbFFtZytHNENwL3NQUjdqdkhmM0RwZzhoTklSbTBVSHhNQ2dv?=
 =?utf-8?B?ZGU5NU9RcWFLRXBPaWlWT0lDd3pGMjhIVlI2VkxnakJFdVhGQkpSOTI3VW5F?=
 =?utf-8?B?UWFMaHIwWXE3ZTdyNmExazltOU13eHZMczhCbEhBZmZPd1d6Q2M0cXkwSDVu?=
 =?utf-8?B?dmJUdzVJSFZNMGw3dEkwWklTU3VkK0pRd2VseFhaL0V6NlJPR3NyLzBuRC9K?=
 =?utf-8?B?VzdjNnpmNVlUYkNOQWJmZ3QzTllWVkdCMFZ6NzdFR0t5R0lzTlJ4NzVESW02?=
 =?utf-8?B?dHhRbjRHZmpkS3VOMVNnSHE3aFY5Z0h5RXlaNUxXR01ZdWc2eHNtOFFGTUJR?=
 =?utf-8?B?WmJnNjgweWtIc28zTittUmVwUTNBRzNRdkFMNmZyQUlxZStaSVRDYlowdHJM?=
 =?utf-8?B?bjBBYmtTRWV4SU10V3k5eTBHRzNablkwTUkwM2VUY0RheDU0Y1FDQitDOEVW?=
 =?utf-8?B?M3laSnNneWk0MTFqNUNEYUUyeFVuMjd6RU9pT0lCZGlFRWlTV3J1cDRhWHhR?=
 =?utf-8?B?Z2ZFYXFoa0kzK0Zzamlpc1pXcnA5WEx1eXNKaVZzVS90NXlQcmNKTjcvTmcv?=
 =?utf-8?B?NkVlYnlsMlhuRndVeEIxUE1vT0wwR3JtOVVuUHFVK1RSdFM2ZnNqamdycHFK?=
 =?utf-8?B?SzErbXFYM1BCUXI2WHZLOEFHZE1NRE5HRjNxNUpuVktRS1hEOUpUK2p4ZzlR?=
 =?utf-8?B?Qk1SbUg3OVZaTTdVb3djVkVJaDQzak93ZVkwZW16T0xTbFoyTy9TR2o5WHVS?=
 =?utf-8?B?UkUzek52eWl5MWhrUmFYS3BVSlJCdGU1RmxsaGgyR2pjTXU3UGZTb1N3cGo3?=
 =?utf-8?B?cE9Yb29jTm5sS2p4bXdIanJjY2ZxSGJwQjRabkYvNzlwR3grMmh4QkNYVEtz?=
 =?utf-8?B?TEJ1K205UUZPbERFR3d1OFlKV0hpcnRZMWpZMHY3RTVUWm1TajNLUlgyOTRE?=
 =?utf-8?B?WWpuS2E5WnR0aTAwVHNwYXg3YWVWaCtKQzczMkk3cERVT1NUcXJuM0JqZDY4?=
 =?utf-8?B?ZEhGbk4yUXZJdVJjRFMrUXVpZnE5ZHBVM3hoMlhuemdaTGpKcU1FaGtidmxP?=
 =?utf-8?B?MTBmVjgwMk1ORWp3czJPbnpsd2FDMWc2SDYzZm1aVk1JUk85WFBqaHgzVHgr?=
 =?utf-8?B?TndWS0NxUTFMYWltRnhjd0tDY1UxUVlRYXdnRXZPZmRWYmRvWks0VHRYMnlM?=
 =?utf-8?B?OHJIQm80ZFU2UGttb2tvTk52MnlQWlFqMHZHL1RrQlFjaFQzWmZrVXZCZllU?=
 =?utf-8?B?VFE9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	8L9uBSUBhO9k8reDYEGgPuG0siKqzvXxfskqrF+XRfebt83r7+6H6lO/xeMJ9NfWYM0PnqJ0Y+UxBlmhSMe+cLLfQRm8jNPpGna0MjmcIyOlcd/UrNCk/qJmozGFYaJzHyWoCvXDOF78WBDsxSrgACaqixnD7n44bcMMPFB5WQbWQjsXBRy4p39butCliFv4p+HSa9k350SROGmj5M2uAEgH51wo8Ox8wkGyM3vGkuuA2dI0u4nFEdydHDaEucZOAQlzoQVUqzVgp+9XMQGamWquSPGmu7Ju2wSkMEN6b9dADRS0mxVWcGyaKXX1LThjXMpmQMqM04v3sLjUPNQEytPf68Xilh0K/51nFZrdpiRbJVQDT76a9uctfFzqtV7JBe8zz0PUuekrzp+czCU1n5PeXBLyxiWdqmTEiG/Cgc1KHHzoxTeaCxBzwtdV07ImhLfd/I3u40i46QA/uuOCc6p1BUIUva7qvUDKfATlZO7P79DJte1XNyzwe82MGKCpavpmWEtQWBbBZGK1jXOLLSadPzWHAve8W1g6nPh2u2xiM563qWikKbb/ufcymSuuNuZxswsH7ggeffNkD13H6kZ21CY2RkFmupfHNBIoRbdcTCCTeYZrPNHQ33DMy/Ns9qIJED9uP+q412UncASU5ODJqNkyePtHPSdpruKX9FgNUfmLLOT0FpxsVj7jpwJ0AvBn/BG8nYgupViYDyMiZUnrOdik2iXVTUw/XRpb4CdRc1JePv4G6ls5tyU0k0Rc0LgLPN2tqDy4B7h4rkXQkOswAKd4RvuUGWkhw7YEDXeF4tfYqFwUroKja4lWLBmm3rED3K4cmq893ojrDvwdozKgF+IhXTuuyAMcdVCBXsQ=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 04e50b0c-e8cd-446c-02d9-08dbf672b1de
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Dec 2023 15:47:47.5459
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: pUDZJM1kdESolh3Qkt+wK+NMOHmmi/AwAlJpniAJ+n1zryTcZu5w7FROO1PQnHg+FZ2dkmUuS3srgPffTkgpwg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB5718
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-12-06_13,2023-12-06_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 malwarescore=0 phishscore=0
 bulkscore=0 adultscore=0 spamscore=0 suspectscore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2312060128
X-Proofpoint-GUID: Ss0ArprzPXjf8vxaoStHS4jFGeJ5TRRs
X-Proofpoint-ORIG-GUID: Ss0ArprzPXjf8vxaoStHS4jFGeJ5TRRs

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@oracle.com> 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@oracle.com> 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@umich.edu> 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@oracle.com> 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