Received-SPF: pass (google.com: domain of linux-nfs+bounces-2209-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 5 Mar 2024 08:30:03 -0500
From: Scott Mayhew <smayhew@redhat.com>
To: Olga Kornievskaia <aglo@umich.edu>
Cc: linux-nfs@vger.kernel.org
Subject: Re: [nfs-utils PATCH 2/2] gssd: add a "backoff" feature to
 limit_krb5_enctypes()
Message-ID: <ZeceW1uO1s_GtZHj@aion>
References: <20240228222253.1080880-1-smayhew@redhat.com>
 <20240228222253.1080880-3-smayhew@redhat.com>
 <CAN-5tyHaP9OXNPJ2ZX=M7ktqLgfXZttk+zym5-DYzi6+vv_B5g@mail.gmail.com>
 <ZeYHp8BygJQDkrv1@aion>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <ZeYHp8BygJQDkrv1@aion>

Re-adding the list.

On Mon, 04 Mar 2024, Scott Mayhew wrote:

> On Mon, 04 Mar 2024, Olga Kornievskaia wrote:
>=20
> > On Wed, Feb 28, 2024 at 5:23=E2=80=AFPM Scott Mayhew <smayhew@redhat.co=
m> 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 en=
try
> > > that's not one of the newer encryption types, and then use that as the
> > > start of the list.
> >=20
> > 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.
>=20
> I'm willing to drop this patch if that's the consensus.  Or we could
> still it behind yet another config option.
>=20
> I'd still like to see the first one get merged though.
>=20
> -Scott
> >=20
> > >
> > > 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[] =3D "kerberos_v5";
> > > +#endif
> > > +#ifdef HAVE_SET_ALLOWABLE_ENCTYPES
> > > +       bool                    backoff =3D false;
> > > +       struct rpc_err          err;
> > >  #endif
> > >         pthread_t tid =3D pthread_self();
> > >
> > > @@ -354,14 +358,14 @@ create_auth_rpc_client(struct clnt_info *clp,
> > >                 goto out_fail;
> > >         }
> > >
> > > -
> > >         if (authtype =3D=3D AUTHTYPE_KRB5) {
> > >  #ifdef HAVE_SET_ALLOWABLE_ENCTYPES
> > > +again:
> > >                 /*
> > >                  * Do this before creating rpc connection since we wo=
n't need
> > >                  * rpc connection if it fails!
> > >                  */
> > > -               if (limit_krb5_enctypes(&sec)) {
> > > +               if (limit_krb5_enctypes(&sec, backoff)) {
> > >                         printerr(1, "WARNING: Failed while limiting k=
rb5 "
> > >                                     "encryption types for user with u=
id %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 =3D=3D ECONNRESET && !backoff) {
> > > +                       backoff =3D true;
> > > +                       goto again;
> > > +               }
> > >  #endif
> > >                 /* Our caller should print appropriate message */
> > >                 printerr(2, "WARNING: Failed to create krb5 context f=
or "
> > > 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[] =3D { 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 =3D -1;
> > > +       int i, j;
> > > +       bool done =3D false;
> > > +
> > > +       if (backoff && sec->cred !=3D GSS_C_NO_CREDENTIAL) {
> > > +               printerr(2, "%s: backoff: releasing old cred\n", __fu=
nc__);
> > > +               maj_stat =3D gss_release_cred(&min_stat, &sec->cred);
> > > +               if (maj_stat !=3D GSS_S_COMPLETE) {
> > > +                       printerr(2, "%s: gss_release_cred() failed\n"=
, __func__);
> > > +                       return -1;
> > > +               }
> > > +       }
> > >
> > >         if (sec->cred =3D=3D GSS_C_NO_CREDENTIAL) {
> > >                 err =3D gssd_acquire_krb5_cred(&sec->cred);
> > > @@ -1718,6 +1729,33 @@ limit_krb5_enctypes(struct rpc_gss_sec *sec)
> > >                 set_enctypes =3D krb5_enctypes;
> > >         }
> > >
> > > +       if (backoff) {
> > > +               j =3D num_set_enctypes;
> > > +               for (i =3D 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 en=
ctype %d\n",
> > > +                                        __func__, *set_enctypes);
> > > +                               set_enctypes++;
> > > +                               num_set_enctypes--;
> > > +                               break;
> > > +                       default:
> > > +                               done =3D 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 =3D gss_set_allowable_enctypes(&min_stat, sec->cred,
> > >                                 &krb5oid, num_set_enctypes, set_encty=
pes);
> > >
> > > 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
> > >
> > >
> >=20