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

[Nathaniel McCallum]

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

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

> > > > 0003
> > 
> > Same as patch 002: unnecessary line breaks in the commit message.
> 
> See above.
> 
> > 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.

> > Nearly forty lines of variable declarations is a bit much. :) You could
> > break apart BER encoding/decoding, key writing, etc.
> 
> Perhaps, but wouldn't change the amount of code, unless you say it is
> necessary in order to do a better review I will skip doing that for now.

See above.

> > The ipapwd_getkeytab() function variable declarations contain a mix of
> > camel case and underscore styles.
> 
> Inherited from old code, see ipa_setkeytab()

Sure, but it is also a brand new section. Doesn't your editor have a
variable renamer? Mine does. ;)

> > The comment containing the ASN.1 code contains invalid syntax.
> 
> Please be more specific ?
> That pseudo code is not meant to be compiled, so it is a bit liberal.

::= is, I think, the only issue.

> > I find code like this very hard to read:
> > 	if (rtag == (ctag | 2)) {
> > Some named constants would be helpful here, or maybe a descriptively
> > named macro function.
> > We have C99 now, so I'd prefer local scope iterators in for loops:
> >   for (int i = 0; ...) -- rather than -- for (i = 0; ...)
> 
> We still declare everything upfront in freeipa code, not going to change
> style with these patches.

All of the OTP code *in this same exact plugin* uses the new style. I
consider us to have migrated at least in this portion of the code.

> > This has inconsistent indents:
> > +        svals = ipapwd_encrypt_encode_key(krbcfg, &data,
> > +                                          kenctypes ? num_kenctypes :
> > +                                            krbcfg->num_pref_encsalts,
> > +                                          kenctypes ? kenctypes :
> > +                                                krbcfg->pref_encsalts,
> > +                                          &errMesg);
> 
> Yes these indents are not the best but allow to keep the code within 80
> chars.

I'm not worried about the problem with keeping them under 80 lines. No
problem there. The problem is that the lines are inconsistently
indented. They should be the same (compare the two lines that start with
"krbcfg->").

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

> > > > 0004
> > 
> > More occasional indentation issues (tab vs space).
> > 
> > Local loop iterators preferred (for example: get_control_data()).
> 
> be more specific please.

for (int i = 0; ...)

> > I'm not a fan of the output variable name for ipa_ldap_bind().
> 
> That's a convention we use both in freeipa and sssd code.

I haven't seen that before. But I'll take your word on it.

> > Other than that, pretty close to ACK on this one.
> > 
> > > > 0005
> > 
> > Unnecessary line breaks in git commit message.
> 
> As above
> 
> > ASN.1 syntax errors. Also, this comment has some rogue tab indents.
> 
> I'l fix the ::= issues, anything else ?

I think that is it.

> > In ldap_get_keytab(), can't the big while loop be a for loop with a
> > local scope iterator? Same with the for loop in
> > ipa_string_to_enctypes().
> 
> No, we never use local scope iterators in freeipa code, I do not see why
> we should introduce that now.

Again, we have started using them in this plugin.

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

Nathaniel