[Freeipa-devel] [PATCHES] [RFC] New getkeytab operation

Tue Mar 11 14:28:36 UTC 2014

On Mon, 2014-03-10 at 17:19 -0400, Rob Crittenden wrote:
> Simo Sorce wrote:
> > Hello,
> > this is a patchset I have been brewing for quite some time.
> >
> > It addresses primarily ticket #3859 however the implementation
> > implicitly also addresses tickets #232 (effective only if we change
> > permissions and break the old interface so only potentially but not
> > immediately) and #233.
> >
> > The patchset is marked [RFC] because it involves the clever use of ACIs
> > to introduce a new ipaPermittedOperations attribute that is used to
> > allow to define a 'virtual' operation as a subtype. This clever use of
> > ACIs is also what stalled this patchset because of 389DS bugs #47569 and
> > #47571 which have since been fixed and I was finally able to verify.
> >
> > Also another blocker for this patchset is that we need to wait for 4.0
> > when we change the Permission model and stop allowing anyone to read all
> > attributes.
> >
> > Another reason this is still RFC is that the admin user apparently is
> > allowed to retrieve any keytab with the current code and default ACIs as
> > augmented by the 3rd patch. It is not entirely clear to me why that
> > happens, I think it maybe due to the broad permissions granted to the
> > admins group. This is *not* something we want to allow in the default
> > case so help to figure out how to avoid it will go a great way into
> > allowing this patchset to be acceptable.
> >
> > However due to the various changes I want to post it to the list for
> > feedback, to see if someone can poke holes in the general architecture
> > of the patches.
> >
> > Thanks for reading this far :-)
> 
> patch 1 looks ok, though I didn't test it. It seems like fairly 
> straightforward refactoring.

Yes, it's pure refactoring so that common functions can be reused later.

> In patch 2 shouldn't pwd be initialized with:
> 
>      krb5_data pwd =  { 0 , 0, NULL};

Not needed, in C99 it is allowed to use a single 0 as a shorthand to
say: 'all zeros', it is quite handy :-)

> In patch 3 I'd prefer that each ber failure have a distinct error 
> message so we can more clearly see where the request failed. There is 
> also some inconsistency when LOG_FATAL is called and when it isn't.

Uhmm it seem like we do have individual errors in ipapwd_setkeytab
indeed, must have been changed after my initial implementation, I had to
adjust a bunch of things in my patches as a patch touching log strings
wouldn't apply anymore.

LOG_FATAL is generally applied only when an internal fatal error
happens, if it is a user generated error (unknown principal in the
request for example, then we do not log a fatal error).

Fixed messages so that different failures have different messages.

> Is this the sort of thing where seeing the value from the request would 
> be helpful in error messages? For example, where the principalname is 
> not found, should that be included in the error?

Well the service name is the name of the principal you are trying to get
a keytab for, it is known to the client as it sent it, don't think it is
worth changing errMesg from const to allocated and all additional code
to handle memory management to return to the client information it
already knows.

> For the ACIs it would be helpful to have owner and group in the ACI 
> comment. Right now they are all the same for each type.

Changed the descriptions.

> Patch 4, do we want a knob to tune the timeout?

No I do not think we need to for now.

> Similar duplicated error messages.
> 
> Typo in "Incopatible options ...."

Ok these are in patch 5 and fixed.

Any comments on the actual mechanism used with the ACIs ?

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-simo-508-2-keytabs-Modularize-setkeytab-operation.patch
Type: text/x-patch
Size: 16797 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140311/494ac41b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-simo-509-2-keytabs-Expose-and-modify-key-encoding-function.patch
Type: text/x-patch
Size: 5300 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140311/494ac41b/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-simo-510-2-keytab-Add-new-extended-operation-to-get-a-keytab.patch
Type: text/x-patch
Size: 23121 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140311/494ac41b/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-simo-511-2-ipa-getkeytab-Modularize-ldap_set_keytab-function.patch
Type: text/x-patch
Size: 11208 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140311/494ac41b/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-simo-512-2-ipa-getkeytab-Add-support-for-get_keytab-extop.patch
Type: text/x-patch
Size: 15929 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140311/494ac41b/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-simo-513-2-man-Add-r-option-to-ipa-getkeytab.1.patch
Type: text/x-patch
Size: 2019 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140311/494ac41b/attachment-0005.bin>