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

Wed Jun 11 00:13:02 UTC 2014

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.

Yes although it looks a bit ugly a long time ago we decided that we'd
move to space indenting for big blocks of code, or keep tab indent only
for minor modification. In the hope of eventually converting the
remaining tabs.

While we are here I decided to split setkeytab along the lines of
getkeytab too, HTH.

> I'm not sure the block comment in is_allowed_to_access_attr() belongs
> there.

Uhhmm now that we check an arbitrary attribute I need to change it. 

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

The generic 389ds function is slapi_access_allowed() which is normally
sufficient, is_allowed_to_access_attr() is just a wrapper around some
additional boilerplate that is not normally needed.
Anyway feel free to open a bug in 389ds's trac.

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

Well, kind of arbitrary, but ok

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

ok

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

Good, because I would not use a struct anyway :)

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

Nope, there is an intentional reference to it, getnew is an integer type
so it gets compared to an integer to get your boolean.

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

Right, and it is intentional, although ugly to see we want to slowly
transition all code to space and 4 chars indent. We just didn't want to
arbitrarily mass re-indent and clobber git annotate.

So the idea is to slowly convert all isolated blocks when new code is
added or a substantial part is changed.

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

ok

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

ah forgot about this one, let's see if I got them all this time.

Still upgrading my server, so still untested, but again just to catch
style issues, I'll post news once I can test the changes do not break
functionality.

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: 36681 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/2528fe23/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/2528fe23/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: 27330 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/2528fe23/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: 11295 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/2528fe23/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: 15976 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/2528fe23/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/2528fe23/attachment-0005.bin>