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. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel