[Freeipa-devel] [PATCH] Use more krb

[Simo Sorce]

On Mon, 2007-10-01 at 12:19 -0400, Rob Crittenden wrote:
> Simo Sorce wrote:
> > This patch changes a bit of code to rely more on kerberos.
> > 
> > It also changes ipa-adduser to something I think is a more useful
> > workflow, it does not require an email for example as we don't need it.
> > Instead it allows passing a principal name in case you need to create
> > specific one or want to avoid conflicts with an exiting one.
> 
> What about the other tools?

Working on it while I explore the code.
Any "must do" pointers are very welcome.

> > This patch also removes some classes we don't want to use by default for
> > users.
> > 
> > Note: this patch may not apply cleanly as a pull from upstream after the
> > commit required me a merge. I have the merge patch in my tree so just
> > ack/nack it and I will push both the patch and the merge patch at the
> > same time.
> 
> I'd rather ack the actual patch that is going to be committed.
> 
> Other comments in-line in the code, marked by []
> 

> [ E-mail is required in the GUI so I made it required here for 
> consistency. ]

Ah I see, then I guess we should remove the requirement in the GUI.
Service accounts may very well not have an email anyway.

> [ /home should not be hardcoded here. It is currently hardcoded on the 
> server side. This needs not be hardcoded anywhere but should be stored 
> in LDAP somewhere. I'd rather not have to fix both places in the future 
> though. ]

Uhmmm ... make sense, I will think how to best fix this in a following patch.

> [ Why not add groups at the same time? ]

I think it is a bit too much to list groups in interactive mode.
Unattended is different.

> diff -r e950c62a04f9 -r fbee5ea59a1f ipa-python/freeipa-python.spec
> --- a/ipa-python/freeipa-python.spec	Fri Sep 28 14:55:28 2007 -0400
> +++ b/ipa-python/freeipa-python.spec	Mon Oct 01 11:35:40 2007 -0400
> @@ -10,7 +10,7 @@ BuildRoot:      %{_tmppath}/%{name}-%{ve
>   BuildRoot: 
> %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
>   BuildArch: 	noarch
> 
> -Requires: python PyKerberos python-krbV
> +Requires: python PyKerberos
> 
> [ This looks like a mis-merge. python-krbV is required now ]

Seem it is not in the current pushed code.
Have you added this in some patches not yet pushed?

> @@ -350,8 +360,7 @@ class IPAServer:
>           # FIXME: What is the default group for users?
>           user['gidnumber'] = '501'
> 
> -        realm = ipa.config.config.get_realm()
> -        user['krbprincipalname'] = "%s@%s" % (user.get('uid'), realm)
> +        user['krbprincipalname'] = "%s@%s" % (user.get('uid'),
> self.realm)
> 
> [ You need to check to see if krbprincipalname is already set ]

Good catch thanks.

> -        user_dn = self.get_user_by_uid(uid, ['dn', 'uid', 
> 'objectclass'], opts)
> -        if user_dn is None:
> +        user = self.get_user_by_principal(principal, 
> ['krbprincipalname'], opts)
> +        if user is None or user['krbprincipalname'] != principal:
>               raise ipaerror.gen_exception(ipaerror.LDAP_NOT_FOUND)
> 
> [ If you do a search on principal how could user['principalname] ever != 
> principal? ]

Just paranoia checking :-)

Simo.