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

[Rob Crittenden]

Simo Sorce wrote:
> On Tue, 2014-06-10 at 14:27 -0400, Nathaniel McCallum wrote:
>> On Tue, 2014-06-10 at 12:02 -0400, Simo Sorce wrote:
>>> On Mon, 2014-06-09 at 21:49 -0400, Nathaniel McCallum wrote:
>>>> On Mon, 2014-06-09 at 20:58 -0400, Simo Sorce wrote:
>>>>> On Mon, 2014-06-09 at 17:53 -0400, Nathaniel McCallum wrote:
>>>>>> On Mon, 2014-06-09 at 15:02 -0400, Simo Sorce wrote:
>>>>>>> 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
>>>>>>>> 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.
>>>>>>
>>>>>> The logic here is correct, it just reads awkwardly. It is probably fine
>>>>>> as is.
>>>>>>
>>>>>> There appear to be indiscriminate tab indents throughout the patch.
>>>>>> Please changes these to spaces.
>>>>>
>>>>> There are because the coed is old, I do not change the style of a piece
>>>>> of code if we are just changing a few lines.
>>>>> You need to read the patch in the context of the code to seen it.
>>>>
>>>> If it were just the problem you alluded to, I wouldn't have called it
>>>> out. I'm referring to tabs in the middle of new code that uses spaces.
>>>> This is most likely the result of copy/paste. Either write all the new
>>>> code to use tabs or match the copy/pasted lines to the surrounding new
>>>> code (my preference).
>>
>> Nearly all the new code in ipapwd_setkeytab() uses space indents where
>> the surrounding code uses tab indents.
> 
> Yes although it looks a bit ugly a long time ago we decided that we'd
> move to space indenting for big blocks of code, or keep tab indent only
> for minor modification. In the hope of eventually converting the
> remaining tabs.
> 
> While we are here I decided to split setkeytab along the lines of
> getkeytab too, HTH.
> 
>> I'm not sure the block comment in is_allowed_to_access_attr() belongs
>> there.
> 
> Uhhmm now that we check an arbitrary attribute I need to change it. 
> 
>> Also, a utility function like is_allowed_to_access_attr() would probably
>> be handy in upstream 389ds. I might use this in an upcoming token sync
>> patch. This is not a requirement of ACK'ing the patch.
> 
> The generic 389ds function is slapi_access_allowed() which is normally
> sufficient, is_allowed_to_access_attr() is just a wrapper around some
> additional boilerplate that is not normally needed.
> Anyway feel free to open a bug in 389ds's trac.
> 
>>>>>>>> 0002
>>>>>>>>
>>>>>>>> ACK
>>>>>>
>>>>>> One small nitpick, then I too say ACK. In the commit message, the second
>>>>>> sentence doesn't need a line break.
>>>>>
>>>>> I try to keep comments within 72 chars (though sometimes I forget and go
>>>>> past till 80), which is why there is a line break there.
>>>>
>>>> Yeah, it just looks bad when sent over email, which is why I noticed it.
>>
>> This didn't get fixed. "Add" should follow "patch." on the same line.
> 
> Well, kind of arbitrary, but ok
> 
>>>>>>>> 0003
>>>>>>
>>>>>> Same as patch 002: unnecessary line breaks in the commit message.
>>>>>
>>>>> See above.
>>
>> Also not fixed. "The new set of keys" should follow "existing ones." on
>> the same line.
> 
> ok
> 
>>>>>> I think ipapwd_getkeytab() is unnecessarily long. Several sections of it
>>>>>> could be broken out into functions and would make it much more readable.
>>>>>
>>>>> That has already been done :-)
>>>>> You should see the original ipa_setkeytab() ...
>>>>
>>>> I'm not talking about ipapwd_setkeytab(). I'm talking about
>>>> ipapwd_getkeytab(). This is entirely new code. There are very clear
>>>> spots where it could be broken up. I consider this the biggest issue in
>>>> this set of patches for two important reasons:
>>>> 1. It makes the function really hard to review.
>>>> 2. It is one of the most security/policy sensitive part of the code.
>>>>
>>>> These two are a bad combo.
>>
>> Much better! I was a bit disappointed that
>> decode_getkeytab_request()/encode_getkeytab_reply() don't output/input a
>> single request/response struct, but I'm not going to hold up the review
>> on that nitpick.
> 
> Good, because I would not use a struct anyway :)
> 
>>>>>>>> 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 ? :)
>>>>>>
>>>>>> I agree with rcrit. Fix this. :)
>>>>>
>>>>> Fixing how ? There is nothing wrong with this code, note that if
>>>>> (getnew) would require to change the order of the 2 blocks and I wanted
>>>>> the short one first intentionally, so I would have to use if (!getnew),
>>>>> is this ok ?
>>>>
>>>> That is how I would write it.
>>
>> There is one dangling reference to "(getnew == 0)" on line 172.
> 
> Nope, there is an intentional reference to it, getnew is an integer type
> so it gets compared to an integer to get your boolean.
> 
>>>>>>>> 0004
>>>>>>
>>>>>> More occasional indentation issues (tab vs space).
>>
>> These still need to be fixed. See line 289 of the patch and following.
>> All the new additions after this line in ldap_set_keytab() are using
>> spaces, but the surrounding code uses tabs.
> 
> Right, and it is intentional, although ugly to see we want to slowly
> transition all code to space and 4 chars indent. We just didn't want to
> arbitrarily mass re-indent and clobber git annotate.
> 
> So the idea is to slowly convert all isolated blocks when new code is
> added or a substantial part is changed.
> 
>>>>>>>> 0005
>>>>>>
>>>>>> Unnecessary line breaks in git commit message.
>>>>>
>>>>> As above
>>
>> Still needs to be fixed. "The new method" should follow "fails." on the
>> same line.
> 
> ok
> 
>>>>>> Line 308 ("int retrieve = 0;") has an 8 space indent. This was likely to
>>>>>> match the tab indents of the surrounding code.
>>>>>
>>>>> ah nice catch
>>
>> This still needs to be fixed. Also, there is indentation mismatches
>> below this (starting on line 331 and following.
> 
> ah forgot about this one, let's see if I got them all this time.
> 
> Still upgrading my server, so still untested, but again just to catch
> style issues, I'll post news once I can test the changes do not break
> functionality.
> 
> Simo.

0001

When is_allowed_to_access_attr() fails it should include the value of
access in the error log for debugging.

Nit: Coluld not fetch REALM backend

There are still a ton of "ber_scanf failed" duplicated fatal errors. I'm
fine keeping a common err_msg but the fatal error should be unique.

This breaks normal host delegation. If you add a host to another host's
managedby, getting the keytab will fail. This is due to:

[11/Jun/2014:16:56:45 -0400] NSACLPlugin - conn=4 op=3 (main): Deny
write on
entry(fqdn=client2.example.com,cn=computers,cn=accounts,dc=example,dc=com).attr(ipaProtectedOperation;write_keys)
to fqdn=client1.example.com,cn=computers,cn=accounts,dc=example,dc=com:
no aci matched the subject by aci(97): aciname= "Groups allowed to
create keytab keys", acidn="cn=accounts,dc=example,dc=com"

0003

In ipapwd_getkeytab() there is an odd comment, /* ok access allowed */,
before access control is checked.

I tested a RHEL-5 client and it seems to work ok.

rob