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

[Simo Sorce]

On Wed, 2014-06-11 at 17:03 -0400, Rob Crittenden wrote:
> 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"

Did this ever work ? What API is it using ?
I am not aware of a rule that would allow it, what did I miss ?

> 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.

Great.

-- 
Simo Sorce * Red Hat, Inc * New York