Re: [Freeipa-devel] [PATCHES 0015-0019] changes to the way host TGT is obtained using keytab

2015-03-06 Thread Petr Vobornik

On 03/06/2015 02:45 PM, Martin Babinsky wrote:

On 03/06/2015 02:08 PM, Jan Cholasta wrote:

Hi Martin,

Dne 6.3.2015 v 13:05 Martin Babinsky napsal(a):

This series of patches for the master/4.1 branch attempts to implement
some of the Rob's and Petr Vobornik's ideas which originated from a
discussion on this list regarding my original patch fixing
https://fedorahosted.org/freeipa/ticket/4808.

I suppose that these patches are just a first iteration, we may further
discuss if this is the right thing to do.

Below is a quote from the original discussion just to get the context:


3) I think it would be nice to support ccache types other than FILE.

According to Petr Vobornik (see his reply), the user is limited mostly
to FILE ccache type, so I don't know if it will make sense to support
also other types.


Actually I agree with Honza. I wanted to say that with your 
implementation user can't use anything else - are limited to FILE type.

--
Petr Vobornik

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


[Freeipa-devel] [PATCHES 0015-0019] changes to the way host TGT is obtained using keytab

2015-03-06 Thread Martin Babinsky
This series of patches for the master/4.1 branch attempts to implement 
some of the Rob's and Petr Vobornik's ideas which originated from a 
discussion on this list regarding my original patch fixing 
https://fedorahosted.org/freeipa/ticket/4808.


I suppose that these patches are just a first iteration, we may further 
discuss if this is the right thing to do.


Below is a quote from the original discussion just to get the context:

--
Martin^3 Babinsky


Martin Babinsky wrote:

On 03/02/2015 04:28 PM, Rob Crittenden wrote:

Petr Vobornik wrote:

On 01/12/2015 05:45 PM, Martin Babinsky wrote:

related to ticket https://fedorahosted.org/freeipa/ticket/4808


this patch seems to be a bit forgotten.

It works, looks fine.

One minor issue: trailing whitespaces in the man page.

I also wonder if it shouldn't be used in other tools which call kinit
with keytab:
* ipa-client-automount:434
* ipa-client-install:2591 (this usage should be fine since it's used for
server installation)
* dcerpc.py:545
* rpcserver.py: 971, 981 (armor for web ui forms base auth)

Most importantly the ipa-client-automount because it's called from
ipa-client-install (if location is specified) and therefore it might
fail during client installation.

Or also, kinit call with admin creadentials worked for the user but I
wonder if it was just a coincidence and may break under slightly
different but similar conditions.


I think that's a fine idea. In fact there is already a function that
could be extended, kinit_hostprincipal().

rob



So in principle we could add multiple TGT retries to
kinit_hostprincipal() and then plug this function to all the places
Petr mentioned in order to provide this functionality each time TGT is
requested using keytab.

Do I understand it correctly?



Honestly I think I'd only do the retries on client installation.  I
don't know that the other uses would really benefit or need this.

But this is an opportunity to consolidate some code, so I guess the
approach I'd take is to add an option to kinit_hostprincipal of
retries=0 so that only a single kinit is done. The client installers
would pass in some value.

This change is quite a bit more invasive but it's also early in the
release cycle so the risk will be spread out.

rob
From fda86199e97aa661e4ee3e73858966c7086a3ee0 Mon Sep 17 00:00:00 2001
From: Martin Babinsky mbabi...@redhat.com
Date: Fri, 6 Mar 2015 12:40:42 +0100
Subject: [PATCH 1/5] modifications to ipautil.kinit_hostprincipal

1.) the function can now perform multiple attempts to get host TGT before
failing.

2.) instead of a directory name specifiying the location of credentials cache
the function now takes the full path including ccache filename.

---
 ipapython/ipautil.py | 46 --
 1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 4116d974e620341119b56fad3cff1bda48af3bab..90a8d4035bce218c4cd000c9434125131b311dd9 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -1175,27 +1175,45 @@ def wait_for_open_socket(socket_name, timeout=0):
 else:
 raise e
 
-def kinit_hostprincipal(keytab, ccachedir, principal):
+def kinit_hostprincipal(keytab, ccache_name, principal, attempts=1):
 
-Given a ccache directory and a principal kinit as that user.
+Given a ccache_name file and a principal kinit as that user.
+
+The optional parameter 'attempts' specifies how many times the credential
+initialization should be attempted before giving up and raising
+StandardError.
 
 This blindly overwrites the current CCNAME so if you need to save
 it do so before calling this function.
 
 Thus far this is used to kinit as the local host.
 
-try:
-ccache_file = 'FILE:%s/ccache' % ccachedir
-krbcontext = krbV.default_context()
-ktab = krbV.Keytab(name=keytab, context=krbcontext)
-princ = krbV.Principal(name=principal, context=krbcontext)
-os.environ['KRB5CCNAME'] = ccache_file
-ccache = krbV.CCache(name=ccache_file, context=krbcontext, primary_principal=princ)
-ccache.init(princ)
-ccache.init_creds_keytab(keytab=ktab, principal=princ)
-return ccache_file
-except krbV.Krb5Error, e:
-raise StandardError('Error initializing principal %s in %s: %s' % (principal, keytab, str(e)))
+curr_attempt = 0
+ccache_file = 'FILE:%s' % ccache_name
+root_logger.debug(Initializing principal %s % principal)
+while True:
+curr_attempt += 1
+try:
+krbcontext = krbV.default_context()
+ktab = krbV.Keytab(name=keytab, context=krbcontext)
+princ = krbV.Principal(name=principal, context=krbcontext)
+os.environ['KRB5CCNAME'] = ccache_file
+ccache = krbV.CCache(name=ccache_file, context=krbcontext,
+ primary_principal=princ)
+ccache.init(princ)
+ 

Re: [Freeipa-devel] [PATCHES 0015-0019] changes to the way host TGT is obtained using keytab

2015-03-06 Thread Martin Babinsky

On 03/06/2015 02:08 PM, Jan Cholasta wrote:

Hi Martin,

Dne 6.3.2015 v 13:05 Martin Babinsky napsal(a):

This series of patches for the master/4.1 branch attempts to implement
some of the Rob's and Petr Vobornik's ideas which originated from a
discussion on this list regarding my original patch fixing
https://fedorahosted.org/freeipa/ticket/4808.

I suppose that these patches are just a first iteration, we may further
discuss if this is the right thing to do.

Below is a quote from the original discussion just to get the context:


1) Why 5 patches for 2 changes (kinit_hostprincipal instead of exec
kinit, ipa-client-install --kinit-attempts)?

Will squish them to a smaller number (2-3)

2) IMO a for loop would be better than an infinite while loop:

 for attempt in range(attempts):
 try:
 # kinit
 ...
 except krbV.Krb5Error as e:
 # kinit failed
 ...
 else:
 break
 else:
 # max attempts reached
 ...


That's true. Infinite loops are tad scary anyway.

3) I think it would be nice to support ccache types other than FILE.
According to Petr Vobornik (see his reply), the user is limited mostly 
to FILE ccache type, so I don't know if it will make sense to support 
also other types.


4) I would prefer if you kept using the full ccache name returned from
kinit_hostprincipal when connecting to LDAP.

5) Given that the ccache path usually ends with /ccache, I would
retain the old way of calling kinit_hostprincipal. You can do something
like this to support all of the above:

 def kinit_hostprincipal(keytab, ccache_file, principal, attempts=1):
 if os.path.isdir(ccache_file):
 ccache_file = os.path.join(ccache_file, 'ccache')
 ...
 return ccache_file

(You don't need to prepend FILE:, as it is the default ccache type.)

Honza

Dumb me didn't realize that 'ccache_file' is a reference to an actual 
filesystem path and that the filename can be set dynamically depending 
on path type (directory vs. file).


Thank you for your comments. Will update the patches accordingly.

--
Martin^3 Babinsky

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


Re: [Freeipa-devel] [PATCHES 0015-0019] changes to the way host TGT is obtained using keytab

2015-03-06 Thread Petr Vobornik

On 03/06/2015 01:05 PM, Martin Babinsky wrote:

This series of patches for the master/4.1 branch attempts to implement
some of the Rob's and Petr Vobornik's ideas which originated from a
discussion on this list regarding my original patch fixing
https://fedorahosted.org/freeipa/ticket/4808.

I suppose that these patches are just a first iteration, we may further
discuss if this is the right thing to do.

Below is a quote from the original discussion just to get the context:



The original kinit_hostprincipal had `ccachedir` argument, the new one 
has `ccache_name`. But the new code still prepends FILE ccache type:


old: ccache_file = 'FILE:%s/ccache' % ccachedir
new: ccache_file = 'FILE:%s' % ccache_name

I would remove the line because I understand the use of 'ccache_name' 
name as equivalent of KRB5CCNAME and therefore I would expect that the 
value of this argument would be used to set the environment variable 
WITHOUT any modification. And mainly, user is limited only to FILE 
ccache type.


I also wonder if
  os.environ['KRB5CCNAME'] = ccache_file
has to be set when ccache is defined by krbV call:
   ccache =  krbV.CCache(name=ccache_file, ...

krbV snipped doesn't use it so maybe we can remove it.

https://git.fedorahosted.org/cgit/python-krbV.git/tree/krbV-code-snippets.py
--
Petr Vobornik

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


Re: [Freeipa-devel] [PATCHES 0015-0019] changes to the way host TGT is obtained using keytab

2015-03-06 Thread Jan Cholasta

Hi Martin,

Dne 6.3.2015 v 13:05 Martin Babinsky napsal(a):

This series of patches for the master/4.1 branch attempts to implement
some of the Rob's and Petr Vobornik's ideas which originated from a
discussion on this list regarding my original patch fixing
https://fedorahosted.org/freeipa/ticket/4808.

I suppose that these patches are just a first iteration, we may further
discuss if this is the right thing to do.

Below is a quote from the original discussion just to get the context:


1) Why 5 patches for 2 changes (kinit_hostprincipal instead of exec 
kinit, ipa-client-install --kinit-attempts)?


2) IMO a for loop would be better than an infinite while loop:

for attempt in range(attempts):
try:
# kinit
...
except krbV.Krb5Error as e:
# kinit failed
...
else:
break
else:
# max attempts reached
...

3) I think it would be nice to support ccache types other than FILE.

4) I would prefer if you kept using the full ccache name returned from 
kinit_hostprincipal when connecting to LDAP.


5) Given that the ccache path usually ends with /ccache, I would 
retain the old way of calling kinit_hostprincipal. You can do something 
like this to support all of the above:


def kinit_hostprincipal(keytab, ccache_file, principal, attempts=1):
if os.path.isdir(ccache_file):
ccache_file = os.path.join(ccache_file, 'ccache')
...
return ccache_file

(You don't need to prepend FILE:, as it is the default ccache type.)

Honza

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code