[PATCH 0/6] nfs-utils: handle BAD_INTEGRITY ERROR

[PATCH 0/6] nfs-utils: handle BAD_INTEGRITY ERROR

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

This patch series is re-work of the previous patch series that handles
gss error for bad integrity. In this version, gssd is changed to use
rpc_gss_seccreate() function in tirpc which exposes the gss errors to
the caller. This functionality is further checked with configure for the
presence of this function in the tirpc library.

Note that the current libtirpc (1.3.4 version) needs a fix to
rpc_gss_seccreate() to work correctly for the gssd that passes in
credentials to be used for the gss context establishement.

Olga Kornievskaia (6):
  gssd: revert commit a5f3b7ccb01c
  gssd: revert commit 513630d720bd
  gssd: switch to using rpc_gss_seccreate()
  gssd: handle KRB5_AP_ERR_BAD_INTEGRITY for machine credentials
  gssd: handle KRB5_AP_ERR_BAD_INTEGRITY for user credentials
  configure: check for rpc_gss_seccreate

 aclocal/libtirpc.m4    |  5 +++++
 utils/gssd/gssd_proc.c | 26 +++++++++++++++++++++++---
 2 files changed, 28 insertions(+), 3 deletions(-)

-- 
2.39.1

[PATCH 1/6] gssd: revert commit a5f3b7ccb01c

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

In preparation for using rpc_gss_seccreate() function, revert commit
a5f3b7ccb01c "gssd: handle KRB5_AP_ERR_BAD_INTEGRITY for user
credentials"

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 utils/gssd/gssd_proc.c |  2 --
 utils/gssd/krb5_util.c | 42 ------------------------------------------
 utils/gssd/krb5_util.h |  1 -
 3 files changed, 45 deletions(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index a96647df..e5cc1d98 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -419,8 +419,6 @@ create_auth_rpc_client(struct clnt_info *clp,
 			if (cred == GSS_C_NO_CREDENTIAL)
 				retval = gssd_refresh_krb5_machine_credential(clp->servername,
 					"*", NULL, 1);
-			else
-				retval = gssd_k5_remove_bad_service_cred(clp->servername);
 			if (!retval) {
 				auth = authgss_create_default(rpc_clnt, tgtname,
 						&sec);
diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
index 6f66ef4f..f6ce1fec 100644
--- a/utils/gssd/krb5_util.c
+++ b/utils/gssd/krb5_util.c
@@ -1553,48 +1553,6 @@ gssd_acquire_user_cred(gss_cred_id_t *gss_cred)
 	return ret;
 }
 
-/* Removed a service ticket for nfs/<name> from the ticket cache
- */
-int
-gssd_k5_remove_bad_service_cred(char *name)
-{
-        krb5_creds in_creds, out_creds;
-        krb5_error_code ret;
-        krb5_context context;
-        krb5_ccache cache;
-        krb5_principal principal;
-        int retflags = KRB5_TC_MATCH_SRV_NAMEONLY;
-        char srvname[1024];
-
-        ret = krb5_init_context(&context);
-        if (ret)
-                goto out_cred;
-        ret = krb5_cc_default(context, &cache);
-        if (ret)
-                goto out_free_context;
-        ret = krb5_cc_get_principal(context, cache, &principal);
-        if (ret)
-                goto out_close_cache;
-        memset(&in_creds, 0, sizeof(in_creds));
-        in_creds.client = principal;
-        sprintf(srvname, "nfs/%s", name);
-        ret = krb5_parse_name(context, srvname, &in_creds.server);
-        if (ret)
-                goto out_free_principal;
-        ret = krb5_cc_retrieve_cred(context, cache, retflags, &in_creds, &out_creds);
-        if (ret)
-                goto out_free_principal;
-        ret = krb5_cc_remove_cred(context, cache, 0, &out_creds);
-out_free_principal:
-        krb5_free_principal(context, principal);
-out_close_cache:
-        krb5_cc_close(context, cache);
-out_free_context:
-        krb5_free_context(context);
-out_cred:
-        return ret;
-}
-
 #ifdef HAVE_SET_ALLOWABLE_ENCTYPES
 /*
  * this routine obtains a credentials handle via gss_acquire_cred()
diff --git a/utils/gssd/krb5_util.h b/utils/gssd/krb5_util.h
index 7ef87018..62c91a0e 100644
--- a/utils/gssd/krb5_util.h
+++ b/utils/gssd/krb5_util.h
@@ -22,7 +22,6 @@ char *gssd_k5_err_msg(krb5_context context, krb5_error_code code);
 void gssd_k5_get_default_realm(char **def_realm);
 
 int gssd_acquire_user_cred(gss_cred_id_t *gss_cred);
-int gssd_k5_remove_bad_service_cred(char *srvname);
 
 #ifdef HAVE_SET_ALLOWABLE_ENCTYPES
 extern int limit_to_legacy_enctypes;
-- 
2.39.1

[PATCH 1/6] gssd: revert commit a5f3b7ccb01c

[Petr Vorel]

From: Olga Kornievskaia <olga.kornievskaia@gmail.com>

From: Olga Kornievskaia <kolga@netapp.com>

Hi Olga,

> Subject "[PATCH 1/6] gssd: revert commit a5f3b7ccb01c"

Also a5f3b7ccb01c does not exist in git tree.
You probably meant 14ee48785f97dbb90dd199698d838da66c319605.

Kind regards,
Petr

[PATCH 2/6] gssd: revert commit 513630d720bd

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

In preparation for using rpc_gss_seccreate(), revert commit 513630d720bd
"gssd: handle KRB5_AP_ERR_BAD_INTEGRITY for machine credentials"

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 utils/gssd/gssd_proc.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index e5cc1d98..4fb6b72d 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -412,27 +412,13 @@ create_auth_rpc_client(struct clnt_info *clp,
 		tid, tgtname);
 	auth = authgss_create_default(rpc_clnt, tgtname, &sec);
 	if (!auth) {
-		if (sec.minor_status == KRB5KRB_AP_ERR_BAD_INTEGRITY) {
-			printerr(2, "WARNING: server=%s failed context "
-				 "creation with KRB5_AP_ERR_BAD_INTEGRITY\n",
-				 clp->servername);
-			if (cred == GSS_C_NO_CREDENTIAL)
-				retval = gssd_refresh_krb5_machine_credential(clp->servername,
-					"*", NULL, 1);
-			if (!retval) {
-				auth = authgss_create_default(rpc_clnt, tgtname,
-						&sec);
-				if (auth)
-					goto success;
-			}
-		}
 		/* Our caller should print appropriate message */
 		printerr(2, "WARNING: Failed to create krb5 context for "
 			    "user with uid %d for server %s\n",
 			 uid, tgtname);
 		goto out_fail;
 	}
-success:
+
 	/* Success !!! */
 	rpc_clnt->cl_auth = auth;
 	*clnt_return = rpc_clnt;
-- 
2.39.1

[PATCH 2/6] gssd: revert commit 513630d720bd

[Petr Vorel]

From: Olga Kornievskaia <olga.kornievskaia@gmail.com>

From: Olga Kornievskaia <kolga@netapp.com>

> In preparation for using rpc_gss_seccreate(), revert commit 513630d720bd
> "gssd: handle KRB5_AP_ERR_BAD_INTEGRITY for machine credentials"

Hi Olga,

Subject "[PATCH 2/6] gssd: revert commit 513630d720bd"
=> commit 513630d720bd does not exists. You probably meant to revert
4b272471937d6662e608dcf2b70dbc4b6dee76a0. Please next time revert on rebased
master to get correct git hash.

Kind regards,
Petr

[PATCH 3/6] gssd: switch to using rpc_gss_seccreate()

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

If available from the libtirpc library, switch to using
rpc_gss_seccreate() instead of authgss_create_default() which does not
expose gss error codes.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 utils/gssd/gssd_proc.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 4fb6b72d..99761157 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -70,6 +70,9 @@
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <syscall.h>
+#ifdef HAVE_TIRPC_GSS_SECCREATE
+#include <rpc/rpcsec_gss.h>
+#endif
 
 #include "gssd.h"
 #include "err_util.h"
@@ -330,6 +333,11 @@ create_auth_rpc_client(struct clnt_info *clp,
 	struct timeval	timeout;
 	struct sockaddr		*addr = (struct sockaddr *) &clp->addr;
 	socklen_t		salen;
+#ifdef HAVE_TIRPC_GSS_SECCREATE
+	rpc_gss_options_req_t	req;
+	rpc_gss_options_ret_t	ret;
+	char			mechanism[] = "kerberos_v5";
+#endif
 	pthread_t tid = pthread_self();
 
 	sec.qop = GSS_C_QOP_DEFAULT;
@@ -410,7 +418,14 @@ create_auth_rpc_client(struct clnt_info *clp,
 
 	printerr(3, "create_auth_rpc_client(0x%lx): creating context with server %s\n", 
 		tid, tgtname);
+#ifdef HAVE_TIRPC_GSS_SECCREATE
+	memset(&req, 0, sizeof(req));
+	req.my_cred = sec.cred;
+	auth = rpc_gss_seccreate(rpc_clnt, tgtname, mechanism,
+			rpcsec_gss_svc_none, NULL, &req, &ret);
+#else
 	auth = authgss_create_default(rpc_clnt, tgtname, &sec);
+#endif
 	if (!auth) {
 		/* Our caller should print appropriate message */
 		printerr(2, "WARNING: Failed to create krb5 context for "
-- 
2.39.1

[PATCH 4/6] gssd: handle KRB5_AP_ERR_BAD_INTEGRITY for machine credentials

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

During context establishment, when the client received
KRB5_AP_ERR_BAD_INTEGRITY error, it might be due to the server
updating its key material. To handle such error, get a new
service ticket and re-try the AP_REQ.

This functionality relies on the new API in libtirpc that
exposes the gss errors.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 utils/gssd/gssd_proc.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 99761157..29600a3f 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -427,13 +427,32 @@ create_auth_rpc_client(struct clnt_info *clp,
 	auth = authgss_create_default(rpc_clnt, tgtname, &sec);
 #endif
 	if (!auth) {
+#ifdef HAVE_TIRPC_GSS_SECCREATE
+		if (ret.minor_status == KRB5KRB_AP_ERR_BAD_INTEGRITY) {
+			printerr(2, "WARNING: server=%s failed context "
+				 "creation with KRB5_AP_ERR_BAD_INTEGRITY\n",
+				 clp->servername);
+			if (cred == GSS_C_NO_CREDENTIAL)
+				retval = gssd_refresh_krb5_machine_credential(clp->servername,
+					"*", NULL, 1);
+			if (!retval) {
+				auth = rpc_gss_seccreate(rpc_clnt, tgtname,
+						mechanism, rpcsec_gss_svc_none,
+						NULL, &req, &ret);
+				if (auth)
+					goto success;
+			}
+		}
+#endif
 		/* Our caller should print appropriate message */
 		printerr(2, "WARNING: Failed to create krb5 context for "
 			    "user with uid %d for server %s\n",
 			 uid, tgtname);
 		goto out_fail;
 	}
-
+#ifdef HAVE_TIRPC_GSS_SECCREATE
+success:
+#endif
 	/* Success !!! */
 	rpc_clnt->cl_auth = auth;
 	*clnt_return = rpc_clnt;
-- 
2.39.1

[PATCH 5/6] gssd: handle KRB5_AP_ERR_BAD_INTEGRITY for user credentials

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

Unlike the machine credential case, we can't throw away the ticket
cache and use the keytab to renew the credentials. Instead, we
need to remove the service ticket for the server that returned
KRB5_AP_ERR_BAD_INTEGRITY and try again.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 utils/gssd/gssd_proc.c |  2 ++
 utils/gssd/krb5_util.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 utils/gssd/krb5_util.h |  1 +
 3 files changed, 45 insertions(+)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 29600a3f..7629de0b 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -435,6 +435,8 @@ create_auth_rpc_client(struct clnt_info *clp,
 			if (cred == GSS_C_NO_CREDENTIAL)
 				retval = gssd_refresh_krb5_machine_credential(clp->servername,
 					"*", NULL, 1);
+			else
+				retval = gssd_k5_remove_bad_service_cred(clp->servername);
 			if (!retval) {
 				auth = rpc_gss_seccreate(rpc_clnt, tgtname,
 						mechanism, rpcsec_gss_svc_none,
diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
index f6ce1fec..6f66ef4f 100644
--- a/utils/gssd/krb5_util.c
+++ b/utils/gssd/krb5_util.c
@@ -1553,6 +1553,48 @@ gssd_acquire_user_cred(gss_cred_id_t *gss_cred)
 	return ret;
 }
 
+/* Removed a service ticket for nfs/<name> from the ticket cache
+ */
+int
+gssd_k5_remove_bad_service_cred(char *name)
+{
+        krb5_creds in_creds, out_creds;
+        krb5_error_code ret;
+        krb5_context context;
+        krb5_ccache cache;
+        krb5_principal principal;
+        int retflags = KRB5_TC_MATCH_SRV_NAMEONLY;
+        char srvname[1024];
+
+        ret = krb5_init_context(&context);
+        if (ret)
+                goto out_cred;
+        ret = krb5_cc_default(context, &cache);
+        if (ret)
+                goto out_free_context;
+        ret = krb5_cc_get_principal(context, cache, &principal);
+        if (ret)
+                goto out_close_cache;
+        memset(&in_creds, 0, sizeof(in_creds));
+        in_creds.client = principal;
+        sprintf(srvname, "nfs/%s", name);
+        ret = krb5_parse_name(context, srvname, &in_creds.server);
+        if (ret)
+                goto out_free_principal;
+        ret = krb5_cc_retrieve_cred(context, cache, retflags, &in_creds, &out_creds);
+        if (ret)
+                goto out_free_principal;
+        ret = krb5_cc_remove_cred(context, cache, 0, &out_creds);
+out_free_principal:
+        krb5_free_principal(context, principal);
+out_close_cache:
+        krb5_cc_close(context, cache);
+out_free_context:
+        krb5_free_context(context);
+out_cred:
+        return ret;
+}
+
 #ifdef HAVE_SET_ALLOWABLE_ENCTYPES
 /*
  * this routine obtains a credentials handle via gss_acquire_cred()
diff --git a/utils/gssd/krb5_util.h b/utils/gssd/krb5_util.h
index 62c91a0e..7ef87018 100644
--- a/utils/gssd/krb5_util.h
+++ b/utils/gssd/krb5_util.h
@@ -22,6 +22,7 @@ char *gssd_k5_err_msg(krb5_context context, krb5_error_code code);
 void gssd_k5_get_default_realm(char **def_realm);
 
 int gssd_acquire_user_cred(gss_cred_id_t *gss_cred);
+int gssd_k5_remove_bad_service_cred(char *srvname);
 
 #ifdef HAVE_SET_ALLOWABLE_ENCTYPES
 extern int limit_to_legacy_enctypes;
-- 
2.39.1

[PATCH 6/6] configure: check for rpc_gss_seccreate

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

If we have rpc_gss_sccreate in tirpc library define
HAVE_TIRPC_GSS_SECCREATE, which would allow us to handle bad_integrity
errors.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 aclocal/libtirpc.m4 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/aclocal/libtirpc.m4 b/aclocal/libtirpc.m4
index bddae022..ef48a2ae 100644
--- a/aclocal/libtirpc.m4
+++ b/aclocal/libtirpc.m4
@@ -26,6 +26,11 @@ AC_DEFUN([AC_LIBTIRPC], [
                                     [Define to 1 if your tirpc library provides libtirpc_set_debug])],,
                          [${LIBS}])])
 
+     AS_IF([test -n "${LIBTIRPC}"],
+           [AC_CHECK_LIB([tirpc], [rpc_gss_seccreate],
+                         [AC_DEFINE([HAVE_TIRPC_GSS_SECCREATE], [1],
+                                    [Define to 1 if your tirpc library provides rpc_gss_seccreate])],,
+                         [${LIBS}])])
   AC_SUBST([AM_CPPFLAGS])
   AC_SUBST(LIBTIRPC)
 
-- 
2.39.1

Re: [PATCH 6/6] configure: check for rpc_gss_seccreate

[Chuck Lever]

On Wed, Dec 06, 2023 at 04:33:32PM -0500, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> If we have rpc_gss_sccreate in tirpc library define
> HAVE_TIRPC_GSS_SECCREATE, which would allow us to handle bad_integrity
> errors.
> 
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  aclocal/libtirpc.m4 | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/aclocal/libtirpc.m4 b/aclocal/libtirpc.m4
> index bddae022..ef48a2ae 100644
> --- a/aclocal/libtirpc.m4
> +++ b/aclocal/libtirpc.m4
> @@ -26,6 +26,11 @@ AC_DEFUN([AC_LIBTIRPC], [
>                                      [Define to 1 if your tirpc library provides libtirpc_set_debug])],,
>                           [${LIBS}])])
>  
> +     AS_IF([test -n "${LIBTIRPC}"],
> +           [AC_CHECK_LIB([tirpc], [rpc_gss_seccreate],
> +                         [AC_DEFINE([HAVE_TIRPC_GSS_SECCREATE], [1],
> +                                    [Define to 1 if your tirpc library provides rpc_gss_seccreate])],,
> +                         [${LIBS}])])
>    AC_SUBST([AM_CPPFLAGS])
>    AC_SUBST(LIBTIRPC)

It would be better for distributors if this checked that the local
version of libtirpc has the rpc_gss_seccreate fix that you sent.
The PKG_CHECK_MODULES macro should work for that, once you know the
version number of libtirpc that will have that fix.

Also, this patch should come either before "gssd: switch to using
rpc_gss_seccreate()" or this change should be squashed into that
patch, IMO.


-- 
Chuck Lever

Re: [PATCH 6/6] configure: check for rpc_gss_seccreate

[Olga Kornievskaia]

On Thu, Dec 7, 2023 at 9:44 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On Wed, Dec 06, 2023 at 04:33:32PM -0500, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > If we have rpc_gss_sccreate in tirpc library define
> > HAVE_TIRPC_GSS_SECCREATE, which would allow us to handle bad_integrity
> > errors.
> >
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> >  aclocal/libtirpc.m4 | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/aclocal/libtirpc.m4 b/aclocal/libtirpc.m4
> > index bddae022..ef48a2ae 100644
> > --- a/aclocal/libtirpc.m4
> > +++ b/aclocal/libtirpc.m4
> > @@ -26,6 +26,11 @@ AC_DEFUN([AC_LIBTIRPC], [
> >                                      [Define to 1 if your tirpc library provides libtirpc_set_debug])],,
> >                           [${LIBS}])])
> >
> > +     AS_IF([test -n "${LIBTIRPC}"],
> > +           [AC_CHECK_LIB([tirpc], [rpc_gss_seccreate],
> > +                         [AC_DEFINE([HAVE_TIRPC_GSS_SECCREATE], [1],
> > +                                    [Define to 1 if your tirpc library provides rpc_gss_seccreate])],,
> > +                         [${LIBS}])])
> >    AC_SUBST([AM_CPPFLAGS])
> >    AC_SUBST(LIBTIRPC)
>
> It would be better for distributors if this checked that the local
> version of libtirpc has the rpc_gss_seccreate fix that you sent.
> The PKG_CHECK_MODULES macro should work for that, once you know the
> version number of libtirpc that will have that fix.
>
> Also, this patch should come either before "gssd: switch to using
> rpc_gss_seccreate()" or this change should be squashed into that
> patch, IMO.

I can certainly re-arrange the order (if Steve wants me to re-send an
ordered list).  I attempted to address your comment to  check for
existence of the function or fallback to the old way. I'm not sure I'm
capable of producing something that depends on distro versioning (or
am I supposed to be)? I think this goes back to me hoping that a
distro would create matching set of libtirpc and nfs-utils rpms...

Thank you for the reviews!

>
>
> --
> Chuck Lever

Re: [PATCH 6/6] configure: check for rpc_gss_seccreate

[Chuck Lever]

On Thu, Dec 07, 2023 at 05:21:50PM -0500, Olga Kornievskaia wrote:
> On Thu, Dec 7, 2023 at 9:44 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> >
> > On Wed, Dec 06, 2023 at 04:33:32PM -0500, Olga Kornievskaia wrote:
> > > From: Olga Kornievskaia <kolga@netapp.com>
> > >
> > > If we have rpc_gss_sccreate in tirpc library define
> > > HAVE_TIRPC_GSS_SECCREATE, which would allow us to handle bad_integrity
> > > errors.
> > >
> > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > > ---
> > >  aclocal/libtirpc.m4 | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/aclocal/libtirpc.m4 b/aclocal/libtirpc.m4
> > > index bddae022..ef48a2ae 100644
> > > --- a/aclocal/libtirpc.m4
> > > +++ b/aclocal/libtirpc.m4
> > > @@ -26,6 +26,11 @@ AC_DEFUN([AC_LIBTIRPC], [
> > >                                      [Define to 1 if your tirpc library provides libtirpc_set_debug])],,
> > >                           [${LIBS}])])
> > >
> > > +     AS_IF([test -n "${LIBTIRPC}"],
> > > +           [AC_CHECK_LIB([tirpc], [rpc_gss_seccreate],
> > > +                         [AC_DEFINE([HAVE_TIRPC_GSS_SECCREATE], [1],
> > > +                                    [Define to 1 if your tirpc library provides rpc_gss_seccreate])],,
> > > +                         [${LIBS}])])
> > >    AC_SUBST([AM_CPPFLAGS])
> > >    AC_SUBST(LIBTIRPC)
> >
> > It would be better for distributors if this checked that the local
> > version of libtirpc has the rpc_gss_seccreate fix that you sent.
> > The PKG_CHECK_MODULES macro should work for that, once you know the
> > version number of libtirpc that will have that fix.
> >
> > Also, this patch should come either before "gssd: switch to using
> > rpc_gss_seccreate()" or this change should be squashed into that
> > patch, IMO.
> 
> I can certainly re-arrange the order (if Steve wants me to re-send an
> ordered list).  I attempted to address your comment to  check for
> existence of the function or fallback to the old way.

A comment that I made when I thought no changes to rpc_gss_seccreate(3t)
would be needed.... But you found and fixed a bug there.


> I'm not sure I'm
> capable of producing something that depends on distro versioning (or
> am I supposed to be)?

I think this series truly needs to check the libtirpc version.
Otherwise the build will complete successfully, gssd will use
rpc_gss_seccreate(), but it will be broken.

Grep for PKG_CHECK_MODULES in the other files in aclocal/ and you
should find a pattern to use.


> I think this goes back to me hoping that a
> distro would create matching set of libtirpc and nfs-utils rpms...

IME distros don't work that way.


-- 
Chuck Lever

Re: [PATCH 6/6] configure: check for rpc_gss_seccreate

[Olga Kornievskaia]

On Thu, Dec 7, 2023 at 5:27 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On Thu, Dec 07, 2023 at 05:21:50PM -0500, Olga Kornievskaia wrote:
> > On Thu, Dec 7, 2023 at 9:44 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> > >
> > > On Wed, Dec 06, 2023 at 04:33:32PM -0500, Olga Kornievskaia wrote:
> > > > From: Olga Kornievskaia <kolga@netapp.com>
> > > >
> > > > If we have rpc_gss_sccreate in tirpc library define
> > > > HAVE_TIRPC_GSS_SECCREATE, which would allow us to handle bad_integrity
> > > > errors.
> > > >
> > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > > > ---
> > > >  aclocal/libtirpc.m4 | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/aclocal/libtirpc.m4 b/aclocal/libtirpc.m4
> > > > index bddae022..ef48a2ae 100644
> > > > --- a/aclocal/libtirpc.m4
> > > > +++ b/aclocal/libtirpc.m4
> > > > @@ -26,6 +26,11 @@ AC_DEFUN([AC_LIBTIRPC], [
> > > >                                      [Define to 1 if your tirpc library provides libtirpc_set_debug])],,
> > > >                           [${LIBS}])])
> > > >
> > > > +     AS_IF([test -n "${LIBTIRPC}"],
> > > > +           [AC_CHECK_LIB([tirpc], [rpc_gss_seccreate],
> > > > +                         [AC_DEFINE([HAVE_TIRPC_GSS_SECCREATE], [1],
> > > > +                                    [Define to 1 if your tirpc library provides rpc_gss_seccreate])],,
> > > > +                         [${LIBS}])])
> > > >    AC_SUBST([AM_CPPFLAGS])
> > > >    AC_SUBST(LIBTIRPC)
> > >
> > > It would be better for distributors if this checked that the local
> > > version of libtirpc has the rpc_gss_seccreate fix that you sent.
> > > The PKG_CHECK_MODULES macro should work for that, once you know the
> > > version number of libtirpc that will have that fix.
> > >
> > > Also, this patch should come either before "gssd: switch to using
> > > rpc_gss_seccreate()" or this change should be squashed into that
> > > patch, IMO.
> >
> > I can certainly re-arrange the order (if Steve wants me to re-send an
> > ordered list).  I attempted to address your comment to  check for
> > existence of the function or fallback to the old way.
>
> A comment that I made when I thought no changes to rpc_gss_seccreate(3t)
> would be needed.... But you found and fixed a bug there.
>
>
> > I'm not sure I'm
> > capable of producing something that depends on distro versioning (or
> > am I supposed to be)?
>
> I think this series truly needs to check the libtirpc version.
> Otherwise the build will complete successfully, gssd will use
> rpc_gss_seccreate(), but it will be broken.
>
> Grep for PKG_CHECK_MODULES in the other files in aclocal/ and you
> should find a pattern to use.

Yes but I won't know the version number of libtirpc (version or rpm
package) for which to check? It seems like libtirpc changes needs to
be checked in (btw I'm assuming a new version would need to be
generated), then (if that's it or libtirpc version and package version
are different things there might be more) this particular patch could
be generated. Isn't that correct?

Steve, I could really use your guidance on steps to be done here.

Thank you.

>
>
> > I think this goes back to me hoping that a
> > distro would create matching set of libtirpc and nfs-utils rpms...
>
> IME distros don't work that way.
>
>
> --
> Chuck Lever

Re: [PATCH 6/6] configure: check for rpc_gss_seccreate

[Steve Dickson]

On 12/8/23 9:26 AM, Olga Kornievskaia wrote:
> On Thu, Dec 7, 2023 at 5:27 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On Thu, Dec 07, 2023 at 05:21:50PM -0500, Olga Kornievskaia wrote:
>>> On Thu, Dec 7, 2023 at 9:44 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>
>>>> On Wed, Dec 06, 2023 at 04:33:32PM -0500, Olga Kornievskaia wrote:
>>>>> From: Olga Kornievskaia <kolga@netapp.com>
>>>>>
>>>>> If we have rpc_gss_sccreate in tirpc library define
>>>>> HAVE_TIRPC_GSS_SECCREATE, which would allow us to handle bad_integrity
>>>>> errors.
>>>>>
>>>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>>>>> ---
>>>>>   aclocal/libtirpc.m4 | 5 +++++
>>>>>   1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/aclocal/libtirpc.m4 b/aclocal/libtirpc.m4
>>>>> index bddae022..ef48a2ae 100644
>>>>> --- a/aclocal/libtirpc.m4
>>>>> +++ b/aclocal/libtirpc.m4
>>>>> @@ -26,6 +26,11 @@ AC_DEFUN([AC_LIBTIRPC], [
>>>>>                                       [Define to 1 if your tirpc library provides libtirpc_set_debug])],,
>>>>>                            [${LIBS}])])
>>>>>
>>>>> +     AS_IF([test -n "${LIBTIRPC}"],
>>>>> +           [AC_CHECK_LIB([tirpc], [rpc_gss_seccreate],
>>>>> +                         [AC_DEFINE([HAVE_TIRPC_GSS_SECCREATE], [1],
>>>>> +                                    [Define to 1 if your tirpc library provides rpc_gss_seccreate])],,
>>>>> +                         [${LIBS}])])
>>>>>     AC_SUBST([AM_CPPFLAGS])
>>>>>     AC_SUBST(LIBTIRPC)
>>>>
>>>> It would be better for distributors if this checked that the local
>>>> version of libtirpc has the rpc_gss_seccreate fix that you sent.
>>>> The PKG_CHECK_MODULES macro should work for that, once you know the
>>>> version number of libtirpc that will have that fix.
>>>>
>>>> Also, this patch should come either before "gssd: switch to using
>>>> rpc_gss_seccreate()" or this change should be squashed into that
>>>> patch, IMO.
>>>
>>> I can certainly re-arrange the order (if Steve wants me to re-send an
>>> ordered list).  I attempted to address your comment to  check for
>>> existence of the function or fallback to the old way.
>>
>> A comment that I made when I thought no changes to rpc_gss_seccreate(3t)
>> would be needed.... But you found and fixed a bug there.
>>
>>
>>> I'm not sure I'm
>>> capable of producing something that depends on distro versioning (or
>>> am I supposed to be)?
>>
>> I think this series truly needs to check the libtirpc version.
>> Otherwise the build will complete successfully, gssd will use
>> rpc_gss_seccreate(), but it will be broken.
>>
>> Grep for PKG_CHECK_MODULES in the other files in aclocal/ and you
>> should find a pattern to use.
> 
> Yes but I won't know the version number of libtirpc (version or rpm
> package) for which to check? It seems like libtirpc changes needs to
> be checked in (btw I'm assuming a new version would need to be
> generated), then (if that's it or libtirpc version and package version
> are different things there might be more) this particular patch could
> be generated. Isn't that correct?
> 
> Steve, I could really use your guidance on steps to be done here.
Again... The versions "on who is on first and what is on second" :-)
Is not an upstream problem... It is a distro problem...

Let me take a closer look...

> 
> Thank you.
> 
>>
>>
>>> I think this goes back to me hoping that a
>>> distro would create matching set of libtirpc and nfs-utils rpms...
We do... upstream creates tar balls... distros create rpm
that have requirements for certain versions of things.

steved.

>>
>> IME distros don't work that way.
>>
>>
>> --
>> Chuck Lever
> 

Re: [PATCH 6/6] configure: check for rpc_gss_seccreate

[Chuck Lever]

On Fri, Dec 08, 2023 at 10:01:29AM -0500, Steve Dickson wrote:
> 
> 
> On 12/8/23 9:26 AM, Olga Kornievskaia wrote:
> > On Thu, Dec 7, 2023 at 5:27 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> > > 
> > > On Thu, Dec 07, 2023 at 05:21:50PM -0500, Olga Kornievskaia wrote:
> > > > On Thu, Dec 7, 2023 at 9:44 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> > > > > 
> > > > > On Wed, Dec 06, 2023 at 04:33:32PM -0500, Olga Kornievskaia wrote:
> > > > > > From: Olga Kornievskaia <kolga@netapp.com>
> > > > > > 
> > > > > > If we have rpc_gss_sccreate in tirpc library define
> > > > > > HAVE_TIRPC_GSS_SECCREATE, which would allow us to handle bad_integrity
> > > > > > errors.
> > > > > > 
> > > > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > > > > > ---
> > > > > >   aclocal/libtirpc.m4 | 5 +++++
> > > > > >   1 file changed, 5 insertions(+)
> > > > > > 
> > > > > > diff --git a/aclocal/libtirpc.m4 b/aclocal/libtirpc.m4
> > > > > > index bddae022..ef48a2ae 100644
> > > > > > --- a/aclocal/libtirpc.m4
> > > > > > +++ b/aclocal/libtirpc.m4
> > > > > > @@ -26,6 +26,11 @@ AC_DEFUN([AC_LIBTIRPC], [
> > > > > >                                       [Define to 1 if your tirpc library provides libtirpc_set_debug])],,
> > > > > >                            [${LIBS}])])
> > > > > > 
> > > > > > +     AS_IF([test -n "${LIBTIRPC}"],
> > > > > > +           [AC_CHECK_LIB([tirpc], [rpc_gss_seccreate],
> > > > > > +                         [AC_DEFINE([HAVE_TIRPC_GSS_SECCREATE], [1],
> > > > > > +                                    [Define to 1 if your tirpc library provides rpc_gss_seccreate])],,
> > > > > > +                         [${LIBS}])])
> > > > > >     AC_SUBST([AM_CPPFLAGS])
> > > > > >     AC_SUBST(LIBTIRPC)
> > > > > 
> > > > > It would be better for distributors if this checked that the local
> > > > > version of libtirpc has the rpc_gss_seccreate fix that you sent.
> > > > > The PKG_CHECK_MODULES macro should work for that, once you know the
> > > > > version number of libtirpc that will have that fix.
> > > > > 
> > > > > Also, this patch should come either before "gssd: switch to using
> > > > > rpc_gss_seccreate()" or this change should be squashed into that
> > > > > patch, IMO.
> > > > 
> > > > I can certainly re-arrange the order (if Steve wants me to re-send an
> > > > ordered list).  I attempted to address your comment to  check for
> > > > existence of the function or fallback to the old way.
> > > 
> > > A comment that I made when I thought no changes to rpc_gss_seccreate(3t)
> > > would be needed.... But you found and fixed a bug there.
> > > 
> > > 
> > > > I'm not sure I'm
> > > > capable of producing something that depends on distro versioning (or
> > > > am I supposed to be)?
> > > 
> > > I think this series truly needs to check the libtirpc version.
> > > Otherwise the build will complete successfully, gssd will use
> > > rpc_gss_seccreate(), but it will be broken.
> > > 
> > > Grep for PKG_CHECK_MODULES in the other files in aclocal/ and you
> > > should find a pattern to use.
> > 
> > Yes but I won't know the version number of libtirpc (version or rpm
> > package) for which to check? It seems like libtirpc changes needs to
> > be checked in (btw I'm assuming a new version would need to be
> > generated), then (if that's it or libtirpc version and package version
> > are different things there might be more) this particular patch could
> > be generated. Isn't that correct?

1. Commit the reverts to nfs-utils, and cut a release. This enables
   nfs-utils to build everywhere again -- it addresses an immediate
   bug.

2. Commit your libtirpc patches, and cut a release. This fixes the
   ABI issue in libtirpc, and you now have a known-good library
   release version number to use.

3. Update the libtirpc aclocal check in nfs-utils to use that
   version number, and commit the rest of your fixes. This fix will
   then appear in the next nfs-utils release.


> > Steve, I could really use your guidance on steps to be done here.
> Again... The versions "on who is on first and what is on second" :-)
> Is not an upstream problem... It is a distro problem...

I think distros will be less likely to upgrade if there are LEGO
blocks laying on the floor that they accidentally step on. And my
impression is that distros want the config to break rather than
that hidden bugs leak into their production builds.

This is clearly something upstream can flag, and we should because
otherwise, the breakage can be silent or frustrating to debug. This
is a security issue, really, since it directly involves gssd.

I mean, why bother to have all of the autoconf machinery if upstream
doesn't care about checking library versions?


> Let me take a closer look...
> 
> > 
> > Thank you.
> > 
> > > 
> > > 
> > > > I think this goes back to me hoping that a
> > > > distro would create matching set of libtirpc and nfs-utils rpms...
> We do... upstream creates tar balls... distros create rpm
> that have requirements for certain versions of things.

That is typically the case. I'm concerned about the few times
it isn't, or if there are testing gaps.


-- 
Chuck Lever

Re: [PATCH 6/6] configure: check for rpc_gss_seccreate

[Steve Dickson]

On 12/7/23 9:44 AM, Chuck Lever wrote:
> On Wed, Dec 06, 2023 at 04:33:32PM -0500, Olga Kornievskaia wrote:
>> From: Olga Kornievskaia <kolga@netapp.com>
>>
>> If we have rpc_gss_sccreate in tirpc library define
>> HAVE_TIRPC_GSS_SECCREATE, which would allow us to handle bad_integrity
>> errors.
>>
>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>> ---
>>   aclocal/libtirpc.m4 | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/aclocal/libtirpc.m4 b/aclocal/libtirpc.m4
>> index bddae022..ef48a2ae 100644
>> --- a/aclocal/libtirpc.m4
>> +++ b/aclocal/libtirpc.m4
>> @@ -26,6 +26,11 @@ AC_DEFUN([AC_LIBTIRPC], [
>>                                       [Define to 1 if your tirpc library provides libtirpc_set_debug])],,
>>                            [${LIBS}])])
>>   
>> +     AS_IF([test -n "${LIBTIRPC}"],
>> +           [AC_CHECK_LIB([tirpc], [rpc_gss_seccreate],
>> +                         [AC_DEFINE([HAVE_TIRPC_GSS_SECCREATE], [1],
>> +                                    [Define to 1 if your tirpc library provides rpc_gss_seccreate])],,
>> +                         [${LIBS}])])
>>     AC_SUBST([AM_CPPFLAGS])
>>     AC_SUBST(LIBTIRPC)
> 
> It would be better for distributors if this checked that the local
> version of libtirpc has the rpc_gss_seccreate fix that you sent.
> The PKG_CHECK_MODULES macro should work for that, once you know the
> version number of libtirpc that will have that fix.
This is some the distro need to worried about... Not upstream.
Yes... do to the recent changes to libtirpc, I did need to
add a requirement to nfs-utils (in Fedora) so it would compile.

> 
> Also, this patch should come either before "gssd: switch to using
> rpc_gss_seccreate()" or this change should be squashed into that
> patch, IMO.Taking a quick look... The patches seem fine... but I will
take a closer look over the weekend.

steved
> 

Re: [PATCH 0/6] nfs-utils: handle BAD_INTEGRITY ERROR

[Chuck Lever]

On Wed, Dec 06, 2023 at 04:33:26PM -0500, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> This patch series is re-work of the previous patch series that handles
> gss error for bad integrity. In this version, gssd is changed to use
> rpc_gss_seccreate() function in tirpc which exposes the gss errors to
> the caller. This functionality is further checked with configure for the
> presence of this function in the tirpc library.
> 
> Note that the current libtirpc (1.3.4 version) needs a fix to
> rpc_gss_seccreate() to work correctly for the gssd that passes in
> credentials to be used for the gss context establishement.
> 
> Olga Kornievskaia (6):
>   gssd: revert commit a5f3b7ccb01c
>   gssd: revert commit 513630d720bd
>   gssd: switch to using rpc_gss_seccreate()
>   gssd: handle KRB5_AP_ERR_BAD_INTEGRITY for machine credentials
>   gssd: handle KRB5_AP_ERR_BAD_INTEGRITY for user credentials
>   configure: check for rpc_gss_seccreate
> 
>  aclocal/libtirpc.m4    |  5 +++++
>  utils/gssd/gssd_proc.c | 26 +++++++++++++++++++++++---
>  2 files changed, 28 insertions(+), 3 deletions(-)

The added error reporting is very nice. I'm glad we could make it
work.

Reviewed-by: Chuck Lever <chuck.lever@oracle.com>

-- 
Chuck Lever

Re: [PATCH 0/6] nfs-utils: handle BAD_INTEGRITY ERROR

[Steve Dickson]

On 12/6/23 4:33 PM, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> This patch series is re-work of the previous patch series that handles
> gss error for bad integrity. In this version, gssd is changed to use
> rpc_gss_seccreate() function in tirpc which exposes the gss errors to
> the caller. This functionality is further checked with configure for the
> presence of this function in the tirpc library.
> 
> Note that the current libtirpc (1.3.4 version) needs a fix to
> rpc_gss_seccreate() to work correctly for the gssd that passes in
> credentials to be used for the gss context establishement.
> 
> Olga Kornievskaia (6):
>    gssd: revert commit a5f3b7ccb01c
>    gssd: revert commit 513630d720bd
>    gssd: switch to using rpc_gss_seccreate()
>    gssd: handle KRB5_AP_ERR_BAD_INTEGRITY for machine credentials
>    gssd: handle KRB5_AP_ERR_BAD_INTEGRITY for user credentials
>    configure: check for rpc_gss_seccreate
> 
>   aclocal/libtirpc.m4    |  5 +++++
>   utils/gssd/gssd_proc.c | 26 +++++++++++++++++++++++---
>   2 files changed, 28 insertions(+), 3 deletions(-)
> 
Committed... (tag: nfs-utils-2-7-1-rc3)

steved.