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

[Simo Sorce]

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.

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

The main DS code afaik.

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

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

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

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

Inherited from old code, see ipa_setkeytab()

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

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

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

> Has the new OID been registered?

Yup.

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

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

be more specific please.

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

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

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

> 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

> 0006
> 
> ACK


I'll fix the minor issue after I get replies to my questions.

Simo.

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