Re: [nfs-utils PATCH 2/2] gssd: add a "backoff" feature to limit_krb5_enctypes()

[Scott Mayhew <smayhew@redhat.com>]

Re-adding the list.

On Mon, 04 Mar 2024, Scott Mayhew wrote:

> On Mon, 04 Mar 2024, Olga Kornievskaia wrote:
> 
> > On Wed, Feb 28, 2024 at 5:23 PM Scott Mayhew <smayhew@redhat.com> wrote:
> > >
> > > If the NFS server reset the connection when we tried to create a GSS
> > > context with it, then there's a good chance that we used an encryption
> > > type that it didn't support.
> > >
> > > Add a one time backoff/retry mechanism, where we adjust the list of
> > > encryption types that we set via gss_set_allowable_enctypes().  We can
> > > do this easily because the list of encryption types should be ordered
> > > from highest preference to lowest.  We just need to find the first entry
> > > that's not one of the newer encryption types, and then use that as the
> > > start of the list.
> > 
> > This doesn't seem like a good idea because it opens up a security
> > problem that a connection reset (by an attacker) during context
> > establishment would force the client to downgrade its security.
> 
> I'm willing to drop this patch if that's the consensus.  Or we could
> still it behind yet another config option.
> 
> I'd still like to see the first one get merged though.
> 
> -Scott
> > 
> > >
> > > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > > ---
> > >  utils/gssd/gssd_proc.c | 15 +++++++++++++--
> > >  utils/gssd/krb5_util.c | 40 +++++++++++++++++++++++++++++++++++++++-
> > >  utils/gssd/krb5_util.h |  2 +-
> > >  3 files changed, 53 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> > > index 7629de0b..0da54598 100644
> > > --- a/utils/gssd/gssd_proc.c
> > > +++ b/utils/gssd/gssd_proc.c
> > > @@ -337,6 +337,10 @@ create_auth_rpc_client(struct clnt_info *clp,
> > >         rpc_gss_options_req_t   req;
> > >         rpc_gss_options_ret_t   ret;
> > >         char                    mechanism[] = "kerberos_v5";
> > > +#endif
> > > +#ifdef HAVE_SET_ALLOWABLE_ENCTYPES
> > > +       bool                    backoff = false;
> > > +       struct rpc_err          err;
> > >  #endif
> > >         pthread_t tid = pthread_self();
> > >
> > > @@ -354,14 +358,14 @@ create_auth_rpc_client(struct clnt_info *clp,
> > >                 goto out_fail;
> > >         }
> > >
> > > -
> > >         if (authtype == AUTHTYPE_KRB5) {
> > >  #ifdef HAVE_SET_ALLOWABLE_ENCTYPES
> > > +again:
> > >                 /*
> > >                  * Do this before creating rpc connection since we won't need
> > >                  * rpc connection if it fails!
> > >                  */
> > > -               if (limit_krb5_enctypes(&sec)) {
> > > +               if (limit_krb5_enctypes(&sec, backoff)) {
> > >                         printerr(1, "WARNING: Failed while limiting krb5 "
> > >                                     "encryption types for user with uid %d\n",
> > >                                  uid);
> > > @@ -445,6 +449,13 @@ create_auth_rpc_client(struct clnt_info *clp,
> > >                                         goto success;
> > >                         }
> > >                 }
> > > +#endif
> > > +#ifdef HAVE_SET_ALLOWABLE_ENCTYPES
> > > +               clnt_geterr(rpc_clnt, &err);
> > > +               if (err.re_errno == ECONNRESET && !backoff) {
> > > +                       backoff = true;
> > > +                       goto again;
> > > +               }
> > >  #endif
> > >                 /* Our caller should print appropriate message */
> > >                 printerr(2, "WARNING: Failed to create krb5 context for "
> > > diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
> > > index 57b3cf8a..5502e74e 100644
> > > --- a/utils/gssd/krb5_util.c
> > > +++ b/utils/gssd/krb5_util.c
> > > @@ -1675,7 +1675,7 @@ out:
> > >   */
> > >
> > >  int
> > > -limit_krb5_enctypes(struct rpc_gss_sec *sec)
> > > +limit_krb5_enctypes(struct rpc_gss_sec *sec, bool backoff)
> > >  {
> > >         u_int maj_stat, min_stat;
> > >         krb5_enctype enctypes[] = { ENCTYPE_DES_CBC_CRC,
> > > @@ -1689,6 +1689,17 @@ limit_krb5_enctypes(struct rpc_gss_sec *sec)
> > >         int num_set_enctypes;
> > >         krb5_enctype *set_enctypes;
> > >         int err = -1;
> > > +       int i, j;
> > > +       bool done = false;
> > > +
> > > +       if (backoff && sec->cred != GSS_C_NO_CREDENTIAL) {
> > > +               printerr(2, "%s: backoff: releasing old cred\n", __func__);
> > > +               maj_stat = gss_release_cred(&min_stat, &sec->cred);
> > > +               if (maj_stat != GSS_S_COMPLETE) {
> > > +                       printerr(2, "%s: gss_release_cred() failed\n", __func__);
> > > +                       return -1;
> > > +               }
> > > +       }
> > >
> > >         if (sec->cred == GSS_C_NO_CREDENTIAL) {
> > >                 err = gssd_acquire_krb5_cred(&sec->cred);
> > > @@ -1718,6 +1729,33 @@ limit_krb5_enctypes(struct rpc_gss_sec *sec)
> > >                 set_enctypes = krb5_enctypes;
> > >         }
> > >
> > > +       if (backoff) {
> > > +               j = num_set_enctypes;
> > > +               for (i = 0; i < j && !done; i++) {
> > > +                       switch (*set_enctypes) {
> > > +                       case ENCTYPE_AES128_CTS_HMAC_SHA256_128:
> > > +                       case ENCTYPE_AES256_CTS_HMAC_SHA384_192:
> > > +                       case ENCTYPE_CAMELLIA128_CTS_CMAC:
> > > +                       case ENCTYPE_CAMELLIA256_CTS_CMAC:
> > > +                               printerr(2, "%s: backoff: removing enctype %d\n",
> > > +                                        __func__, *set_enctypes);
> > > +                               set_enctypes++;
> > > +                               num_set_enctypes--;
> > > +                               break;
> > > +                       default:
> > > +                               done = true;
> > > +                               break;
> > > +                       }
> > > +               }
> > > +               printerr(2, "%s: backoff: %d remaining enctypes\n",
> > > +                        __func__, num_set_enctypes);
> > > +               if (!num_set_enctypes) {
> > > +                       printerr(0, "%s: no remaining enctypes after backoff\n",
> > > +                                __func__);
> > > +                       return -1;
> > > +               }
> > > +       }
> > > +
> > >         maj_stat = gss_set_allowable_enctypes(&min_stat, sec->cred,
> > >                                 &krb5oid, num_set_enctypes, set_enctypes);
> > >
> > > diff --git a/utils/gssd/krb5_util.h b/utils/gssd/krb5_util.h
> > > index 40ad3233..0be0c500 100644
> > > --- a/utils/gssd/krb5_util.h
> > > +++ b/utils/gssd/krb5_util.h
> > > @@ -26,7 +26,7 @@ int gssd_k5_remove_bad_service_cred(char *srvname);
> > >
> > >  #ifdef HAVE_SET_ALLOWABLE_ENCTYPES
> > >  extern int limit_to_legacy_enctypes;
> > > -int limit_krb5_enctypes(struct rpc_gss_sec *sec);
> > > +int limit_krb5_enctypes(struct rpc_gss_sec *sec, bool backoff);
> > >  int get_allowed_enctypes(void);
> > >  #endif
> > >
> > > --
> > > 2.43.0
> > >
> > >
> >