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

Tue Jun 10 21:20:17 UTC 2014

On Tue, 2014-06-10 at 16:24 -0400, Nathaniel McCallum 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.
> > 
> > I'm not sure the block comment in is_allowed_to_access_attr() belongs
> > there.
> > 
> > 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.
> > 
> > > > > > > > 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.
> > 
> > > > > > > > 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.
> > 
> > > > > > 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.
> > 
> > > > > > > > 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.
> > 
> > > > > > > > 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.
> > 
> > > > > > > > 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.
> > 
> > > > > > 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.
> 
> Also, this doesn't compile because of:
> +    while (int i = 0; rtag == LBER_SEQUENCE; i++) {

Uhmm this is odd, I wonder why my make step didn't catch this ... I'll
make sure the next batch doesn't fail to compile at least, sorry about
that.

Simo.

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