[Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs

[Simo Sorce]

On Mon, 2014-06-09 at 13:39 -0400, Rob Crittenden wrote:
> Simo Sorce wrote:
> > This patch set is an initial implementation of ticket #3859
> > 
> > It seem to be working fine in my initial testing but I have not yet
> > tested all cases.
> > 
> > However I wonted to throw it on the list to get some initial feedback
> > about the choices I made wrt access control and ipa-getkeytab flags and
> > default behavior.
> > 
> > In particular, the current patch set would require us to make
> > host/service keytabs readable by the requesting party (whoever that is,
> > admin or host itself) in order to allow it to get back the actual
> > keytab. I am not sure this is ideal. Also write access to the keytab is
> > still all is needed to allow someone to change it.
> > 
> > Neither is ideal, but it was simpler as a first implementation. In
> > particular I think we should allow either permission indipendently, and
> > it should be something an admin can change. However I do not like
> > allowing normal writes or reads to these attributes, mostly because w/o
> > access to the master key nobody can really make sense of actually
> > reading out the contents of KrbPrincipalKey or could write a blob that
> > can be successfully decrypted.
> > 
> > So I was wondering if we might want to prevent both reading and writing
> > via LDAP (except via extended operations) and instead use another method
> > to determine access patterns.
> > 
> > As for ipa-getkeytab is everyone ok with tryin the new method first and
> > always falling back to the old one (if a password has been provided) ?
> 
> 0001
> 
> Needs a rebase. I did my best for testing.

Rebased.

> In check_service_name() why not just initialize name to NULL rather than
> assigning it twice? Or is the value of name undefined if krb5_* fails?

No it is set to NULL in case of failure actually. Fixed.

> get_realm_backend() should probably have a comment on why returning NULL
> is ok (either because there is no sdn or because there is no be). It
> appears that things will eventually fail in get_entry_by_principal()

Instead of adding complex explanations I immediately return with an
error if get_realm_backend() returns NULL.

> I think the Access Strategy comment needs to be expanded (or dropped) in
> is_allowed_to_access_keys()

I assume you mean is_allowed_to_access_attr(), and I am going to drop it
from that place, I carried in on when moving code around.

> In the fatal error in is_allowed_to_access_keys() should the error
> include the bound user and the resource they attempted to modify?

Uhm it does make sense to log what entry the user was trying to poke at,
but not in is_allowed_to_access_attr(), I will add a log in the calling
function.

> 0002
> 
> ACK
> 
> 0003
> 
> Nit: typo in commit, extedned

Fixed

> I'd prefer unique error messages for each ber decode failure.

Added unique error messages in the key encoding section where they were
missing.

> Why is write access always required in ipapwd_getkeytab()? Would there
> be a case where a principal can only re-fetch existing keys?

I do not see where write access is always required, I test first with
read access (SLAPI_ACL_READ) and then with write (SLAPI_ACL_WRITE)

> Since getnew is defined as a boolean in the ASN.1, is the conditional
> 
> if (getnew == 0)
> 
> preferred over just
> 
> if (getnew)?

Future proof if we want to change it to a non-boolean value for whatever
reason in the future ? :)

> Some errors read "Out of Memory" and others "allocation failed". I think
> with a fatal error the line number is included in the error. If I'm
> wrong then perhaps something specific to the allocation should be included.

Sorry I do not see where this occurs in patch 0003

> The error message to the call ber_decode_krb5_key_data() doesn't seem to
> match.

I am not sure what you mean, the error clearly state it can't decode the
keys

> 0004
> 
> More duplicated error messages, e.g. Missing reply control

Well in either case it is missing, I did not think we need to be that
detailed as something is clearly screwed up here, but ok, will make them
unique here too.

> 0005
> 
> More duplicated error messages, Failed to parse key data

Can't find this.

> Typo, Incopatible

Can't find this

> Functionality-wise.
> 
> Retrieving a keytab from a service without one fails in a strange way:
> 
> # ipa-getkeytab -r -s `hostname` -p test1/`hostname` -k /tmp/test.kt
> Operation failed! Insufficient access rights

Did you have access to retrieve the keytab ?

> Failed to get keytab
> 
> I suppose a new permission is needed to add the ability to re-fetch
> keytabs.

Yes.

> Should the admins group be able to do this out-of-the box?

No they shouldn't.

> Maybe this is ok, I don't know, but it looks wierd. I fetched with -r
> the same keytab a number of times and this is what it contains:
> 
> # klist -kt /tmp/test.kt
> Keytab name: FILE:/tmp/test.kt
> KVNO Timestamp           Principal
> ---- -------------------
> ------------------------------------------------------
>    1 06/09/2014 13:35:50 test1/grindle.example.com at EXAMPLE.COM
>    1 06/09/2014 13:35:50 test1/grindle.example.com at EXAMPLE.COM
>    1 06/09/2014 13:35:50 test1/grindle.example.com at EXAMPLE.COM
>    1 06/09/2014 13:35:50 test1/grindle.example.com at EXAMPLE.COM
>    1 06/09/2014 13:35:53 test1/grindle.example.com at EXAMPLE.COM
>    1 06/09/2014 13:35:53 test1/grindle.example.com at EXAMPLE.COM
>    1 06/09/2014 13:35:53 test1/grindle.example.com at EXAMPLE.COM
>    1 06/09/2014 13:35:53 test1/grindle.example.com at EXAMPLE.COM
>    1 06/09/2014 13:36:00 test1/grindle.example.com at EXAMPLE.COM
>    1 06/09/2014 13:36:00 test1/grindle.example.com at EXAMPLE.COM
>    1 06/09/2014 13:36:00 test1/grindle.example.com at EXAMPLE.COM
>    1 06/09/2014 13:36:00 test1/grindle.example.com at EXAMPLE.COM
>    1 06/09/2014 13:36:45 test1/grindle.example.com at EXAMPLE.COM
>    1 06/09/2014 13:36:45 test1/grindle.example.com at EXAMPLE.COM
>    1 06/09/2014 13:36:45 test1/grindle.example.com at EXAMPLE.COM
>    1 06/09/2014 13:36:45 test1/grindle.example.com at EXAMPLE.COM
>    1 06/09/2014 13:36:51 test1/grindle.example.com at EXAMPLE.COM
>    1 06/09/2014 13:36:51 test1/grindle.example.com at EXAMPLE.COM
>    1 06/09/2014 13:36:51 test1/grindle.example.com at EXAMPLE.COM
> 
> The krbPrincipalKey value remained constant.

Why should it change ?
You are asking to get the exact same key back, and that's what it is
giving to you.

New patches attached.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-keytabs-Modularize-setkeytab-operation.patch
Type: text/x-patch
Size: 16951 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140609/52f59069/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-keytabs-Expose-and-modify-key-encoding-function.patch
Type: text/x-patch
Size: 5298 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140609/52f59069/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-keytab-Add-new-extended-operation-to-get-a-keytab.patch
Type: text/x-patch
Size: 23879 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140609/52f59069/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-ipa-getkeytab-Modularize-ldap_set_keytab-function.patch
Type: text/x-patch
Size: 11261 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140609/52f59069/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-ipa-getkeytab-Add-support-for-get_keytab-extop.patch
Type: text/x-patch
Size: 15927 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140609/52f59069/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-man-Add-r-option-to-ipa-getkeytab.1.patch
Type: text/x-patch
Size: 2017 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140609/52f59069/attachment-0005.bin>