credentials_krb5: use gss_acquire_cred for client-side GSSAPI use case

Mon Mar 6 13:47:04 UTC 2017

On Mon, 2017-03-06 at 08:30 -0500, Simo wrote:
> On Mon, 2017-03-06 at 07:23 +0100, Stefan Metzmacher wrote:
> > Am 05.03.2017 um 20:01 schrieb Simo:
> > > On Sat, 2017-03-04 at 11:19 -0500, Simo wrote:
> > > > On Fri, 2017-03-03 at 13:24 +0100, Stefan Metzmacher wrote:
> > > > > Am 03.03.2017 um 13:07 schrieb Alexander Bokovoy:
> > > > > > On pe, 03 maalis 2017, Stefan Metzmacher wrote:
> > > > > > > Hi Alexander,
> > > > > > > 
> > > > > > > > > > > Attached patch is needed for upcoming FreeIPA 4.5
> > > > > > > > > > > release
> > > > > > > > > > > to allow use
> > > > > > > > > > > of Samba Python bindings in a privile separation
> > > > > > > > > > > mode
> > > > > > > > > > > provided by
> > > > > > > > > > > GSS-proxy (https://pagure.io/gssproxy). FreeIPA
> > > > > > > > > > > bug
> > > > > > > > > > > is
> > > > > > > > > > > here:
> > > > > > > > > > > https://pagure.io/freeipa/issue/6671, Samba bug
> > > > > > > > > > > is
> > > > > > > > > > > https://bugzilla.samba.org/show_bug.cgi?id=12611
> > > > > > > > > > > 
> > > > > > > > > > > Please see more details in the commit message.
> > > > > > > > > > 
> > > > > > > > > > Please have a look at
> > > > > > > > > > https://bugzilla.samba.org/show_bug.cgi?id=12480
> > > > > > > > > > for the reasons why we can't use
> > > > > > > > > > gss_acquire_cred().
> > > > > > > > > > 
> > > > > > > > > > There needs to be another solution, sorry.
> > > > > > > > > 
> > > > > > > > > As gss_acquire_cred_from() seems to be handled by
> > > > > > > > > gssproxy,
> > > > > > > > > I guess we need a wrapper in
> > > > > > > > > lib/krb5_wrap/gss_samba.[ch]
> > > > > > > > > that uses gss_acquire_cred_from() if available and
> > > > > > > > > gss_krb5_import_cred() otherwise.
> > > > > > > > > 
> > > > > > > > > And that wrapper needs to be used everywhere we
> > > > > > > > > currently
> > > > > > > > > use gss_krb5_import_cred(). It should also hide the
> > > > > > > > > mess
> > > > > > > > > we currently use in gse_init_server() to work arround
> > > > > > > > > the broken gss_krb5_import_cred() server side.
> > > > > > > > 
> > > > > > > > That was my initial idea but the problem here is that
> > > > > > > > we
> > > > > > > > deal
> > > > > > > > with it 
> > > > > > > > differently in different contexts. In client context we
> > > > > > > > use
> > > > > > > > GSSAPI creds
> > > > > > > > while in other places we use KRB5 ccache handle.
> > > > > > > > 
> > > > > > > > This particular code path is isolated from other use of
> > > > > > > > gss_krb5_import_cred(). And it does not need to specify
> > > > > > > > explicit
> > > > > > > > credential cache because it is running in a client
> > > > > > > > context
> > > > > > > > and
> > > > > > > > expects
> > > > > > > > to use the default ccache.
> > > > > > > 
> > > > > > > That's wrong this code path operates on multiple MEMORY
> > > > > > > caches
> > > > > > > in smbtorture.
> > > > > > > 
> > > > > > > gss_acquire_cred() never used again for initiator creds
> > > > > > > and only for acceptor creds as a fallback for a broken
> > > > > > > gss_krb5_import_cred().
> > > > > > > 
> > > > > > > So all places should use the wrapper that uses
> > > > > > > gss_acquire_cred_from() if available. It should
> > > > > > > otherwise use gss_krb5_import_cred() with
> > > > > > > a fallback only for the keytab only case
> > > > > > > to
> > > > > > > gsskrb5_register_acceptor_identity()/gss_acquire_cred().
> > > > > > 
> > > > > > So, what about Andreas' code from
> > > > > > https://git.samba.org/?p=asn/samba.git;a=shortlog;h=refs/he
> > > > > > ad
> > > > > > s/ma
> > > > > > st
> > > > > > er-cli_creds
> > > > > > ?
> > > > > 
> > > > > I know I also had some of this in my master3-libsmb-tmp,
> > > > > but skipped it as it was not required yet.
> > > > > 
> > > > > > The only issue I see there is that when gensec_gssapi calls
> > > > > > into
> > > > > > cli_credentials_get_client_gss_creds() with NULL ccache, it
> > > > > > wants
> > > > > > to get
> > > > > > the creds from the default ccache but Andreas' code will
> > > > > > try
> > > > > > to
> > > > > > create a
> > > > > > new MEMORY: ccache. This is wrong for the use case we have
> > > > > > with
> > > > > > cli_credentials_get_client_gss_creds() because this ccache
> > > > > > is
> > > > > > empty
> > > > > > and
> > > > > > default one contains actual credentials.
> > > > > 
> > > > > I think I'd preferr to have a wrapper that behaves like
> > > > > gss_krb5_import_cred() on heimdal, just
> > > > > 
> > > > > uint32_t smb_gss_krb5_import_cred(OM_uint32 *minor_status,
> > > > >                      krb5_ccache id,
> > > > >                      krb5_principal keytab_principal,
> > > > >                      krb5_keytab keytab,
> > > > >                      gss_cred_id_t *cred);
> > > > > 
> > > > > And change the callers just to use it without any additional
> > > > > logic change.
> > > > > 
> > > > > That would be much easier to understand.
> > > > 
> > > > Doesn't this mean the code would always try to
> > > > krb5_cc_resolve()
> > > > the
> > > > ccache ?
> > 
> > Yes, and that's exactly what I want. We should do such things
> > explicit
> > in order to avoid unexpected magic depending on the used kerberos
> > library.
> 
> Can you explain what magic you refer to ?
> What's your goal here ? Keep in mind that in MIT kerberos
> gss_krb5_import_cred() is implemented by calling the acquire_cred
> paths
> internally anyway, so the solution for any "magic" is likely the same
> in either case.
> 
> > > > I would rather have instead an interface that looks like
> > > > gss_acquire_cred_from(), and then simulate it if it is not
> > > > available.
> > > > 
> > > > That would be a much cleaner gssapi-like interface, and will
> > > > help
> > > > starting to untangle the mess we have with code always mixing
> > > > lower
> > > > level krb5 calls, which we should avoid where possible and
> > > > higher
> > > > level
> > > > gssapi calls which we should prefer in most cases, except when
> > > > the
> > > > implementatrion has limitation we cannot deal with regular
> > > > gssapi
> > > > calls
> > > > (and when we do we should fix it in gssapi instead of carrying
> > > > workarounds forever).
> > > 
> > > In this patch[1] I added a patch on top of yours to detect
> > > gss_acquire_cred_from() that implements
> > > smb_gss_acquire_cred_from()
> > > using gss_krb5_import_cred() to emulate it if not available.
> > > 
> > > I have not tested this or changed the rest of the code from
> > > gss_krb5_import_cred() to smb_gss_acquire_cred_from(), but would
> > > do
> > > it
> > > if you think this route is ok.
> > 
> > Is there a reason why you used the prototype of gss_add_cred_from()
> > instead of gss_acquire_cred_from() in order to implement
> > smb_gss_add_cred_from()?
> 
> No, I just used the wrong name as I had it in my hands, I can change
> the name to smb_gss_add_cred_from(), or change the prototype to be
> the
> same of gss_acquired_cred_from(), probably the latter is actually
> better now that you point it out, it will remove some playing around
> with input/output parms on the fallback to regular
> gss_acquire_cred().
> 
> Going to push a new change there to do that.

Done.

> > I actually think the simpler gss_krb5_import_cred() interface
> > keeps the logic in our callers simpler.
> 
> But it keeps our interfaces tied to resolve ccache manually, that
> cannot be done with interposer mechanisms, they will just fail.
> The whole point of these patches is to allow interposition, so we
> need
> to move toward removing direct krb5 calls as much as possible.
> 
> > We can revisit this, one we would start to use gssapi for anything
> > other than kerberos.
> 
> We should do that too.
> 