[Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
Simo Sorce
simo at redhat.com
Tue Jun 10 16:02:29 UTC 2014
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).
>
> > > > > 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
>
I should have addressed all concerns, and the patchset compiles, but I
haven't tested it yet (should be able to in the aft if my master server
survives an upgrade).
Let me know if there are any more concerns wrt style (or substance :).
Simo.
--
Simo Sorce * Red Hat, Inc * New York
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-keytabs-Modularize-setkeytab-operation.patch
Type: text/x-patch
Size: 17056 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/bfe6b805/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-keytabs-Expose-and-modify-key-encoding-function.patch
Type: text/x-patch
Size: 5298 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/bfe6b805/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-keytab-Add-new-extended-operation-to-get-a-keytab.patch
Type: text/x-patch
Size: 29028 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/bfe6b805/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-ipa-getkeytab-Modularize-ldap_set_keytab-function.patch
Type: text/x-patch
Size: 11261 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/bfe6b805/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-ipa-getkeytab-Add-support-for-get_keytab-extop.patch
Type: text/x-patch
Size: 15986 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/bfe6b805/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-man-Add-r-option-to-ipa-getkeytab.1.patch
Type: text/x-patch
Size: 2017 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/bfe6b805/attachment-0005.bin>
More information about the Freeipa-devel
mailing list