On Thu, Jun 27, 2013 at 06:38:02PM +0200, Sumit Bose wrote:
On Thu, Jun 27, 2013 at 06:23:22PM +0200, Jakub Hrozek wrote:
> On Thu, Jun 27, 2013 at 05:23:58PM +0200, Sumit Bose wrote:
> > On Thu, Jun 27, 2013 at 05:09:52PM +0200, Sumit Bose wrote:
> > > On Thu, Jun 27, 2013 at 04:37:09PM +0200, Jakub Hrozek wrote:
> > > > On Thu, Jun 27, 2013 at 04:00:28PM +0200, Sumit Bose wrote:
> > > > > On Thu, Jun 27, 2013 at 01:27:28PM +0200, Jakub Hrozek wrote:
> > > > > > Hi,
> > > > > >
> > > > > > during testing I found out that we mishandle UPNs for
subdomain users
> > > > > > when using Kerberos authentication.
> > > > > >
> > > > > > If there is no userPrincipal attribute we guess based on
username@REALM.
> > > > > > But for subdomain users the username is already qualified,
so so you end
> > > > > > up with username@DOMAIN@REALM. Currently first login works
fine because
> > > > > > krb5 auth code treats the result as an enterprise
principal. But if you
> > > > > > are checking existing ccache then the krb5 code errors out
because one of
> > > > > > the krb5_cc_* functions treats username@DOMAIN@REALM as
invalid principal.
> > > > > >
> > > > > > The attached patch checks if the username is already
qualified and
> > > > > > replaces the domain name with realm name when guessing the
UPN. I really
> > > > > > don't like the result because parsing out is inherently
fragile. I think
> > > > > > we should store the plain username in an additional sysdb
attribute,
> > > > > > too.
> > > > >
> > > > > ...
> > > > >
> > > > > > }
> > > > > >
> > > > > > errno_t krb5_get_simple_upn(TALLOC_CTX *mem_ctx, struct
krb5_ctx *krb5_ctx,
> > > > > > - const char *domain_name, const
char *username,
> > > > > > + struct sss_domain_info *dom,
const char *username,
> > > > > > const char *user_dom, char
**_upn)
> > > > > > {
> > > > > > const char *realm = NULL;
> > > > > > char *uc_dom = NULL;
> > > > > > char *upn;
> > > > > > + char *name;
> > > > > > + char *domname;
> > > > > > + TALLOC_CTX *tmp_ctx = NULL;
> > > > >
> > > > > I'm not sure a tmp_ctx is really needed here.
> > > >
> > > > I would prefer tmp_ctx because there's quite a couple of
allocations and
> > > > using tmp_ctx is simpler than keeping track of individual allocated
> > > > strings.
> > > >
> > > > I just forgot to use it in some (most) places in the function. New
patch
> > > > is attached that allocates on top of tmp_ctx and then only steals
the
> > > > result.
> > >
> > > ACK
> >
> > ah, sorry, just one more comment. Maybe if sss_parse_name() fails we
> > should just use username and continue as before instead of failing
> > completely.
> >
> > bye,
> > Sumit
>
> Thanks, that's true. New patch attached.
Thank you, ACK again.
bye,
Sumit
Pushed to master.