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

[Nathaniel McCallum]

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.

I'm a bit confused by the memory management in get_realm_backend() and
its callers. Who owns 'be'?

> > 0002
> > 
> > ACK

One small nitpick, then I too say ACK. In the commit message, the second
sentence doesn't need a line break.

> > 0003

Same as patch 002: unnecessary line breaks in the commit message.

I think ipapwd_getkeytab() is unnecessarily long. Several sections of it
could be broken out into functions and would make it much more readable.
Nearly forty lines of variable declarations is a bit much. :) You could
break apart BER encoding/decoding, key writing, etc.

The ipapwd_getkeytab() function variable declarations contain a mix of
camel case and underscore styles.

The comment containing the ASN.1 code contains invalid syntax.

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

This has inconsistent indents:
+        svals = ipapwd_encrypt_encode_key(krbcfg, &data,
+                                          kenctypes ? num_kenctypes :
+                                            krbcfg->num_pref_encsalts,
+                                          kenctypes ? kenctypes :
+                                                krbcfg->pref_encsalts,
+                                          &errMesg);

Has the new OID been registered?

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

> > 0004

More occasional indentation issues (tab vs space).

Local loop iterators preferred (for example: get_control_data()).

I'm not a fan of the output variable name for ipa_ldap_bind().

Other than that, pretty close to ACK on this one.

> > 0005

Unnecessary line breaks in git commit message.

ASN.1 syntax errors. Also, this comment has some rogue tab indents.

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

Line 308 ("int retrieve = 0;") has an 8 space indent. This was likely to
match the tab indents of the surrounding code.

0006

ACK

Nathaniel