[Freeipa-devel] [PATCHES 0001-0011 v3] Profile management

[Fraser Tweedale]

On Thu, May 21, 2015 at 05:33:11PM +0200, Martin Basti wrote:
> On 20/05/15 16:41, Fraser Tweedale wrote:
> >Hi Honza, Martin et al,
> >
> >Latest patches attached.  On top of previous patches (most review
> >matters addressed**) patches 0008..0011 add support for profiles and
> >user certificates to `ipa cert-request'.
> >
> >** those that were not are being tracked at [1]; please add anything
> >    I missed.
> >
> >Some points to note:
> >
> >- usercertificate is not yet a multi-valued attribute for users,
> >   hosts and services.
> >
> >   QUESTION - we do want to allow multiple certificates for all
> >   principal types, not just users?  Or have I got that wrong.
> Changing schema can cause issues in future, we already burn ourselves
> several times.
> If you plan to have multi valued attribute in close future, could be better
> to have mutltivalued schema now, instead of make this change in future?
> 
The 'usercertificate' attribute is multi-valued in schema.  Only how
we treat it in IPA framework would change.

Martin Kosek clarified that services and hosts are also supporting
multiple certs so I will make that change for next patch set as
well.

> >
> >- "DN and SAN match principal" checks are not implemented for users
> >   yet.
> >
> >- ACL was added to allow user principals to request their own
> >   certificates, however, this will be further subject to CA/profile
> >   ACLs which are to come.
> >
> >- Pursuant to [2] revocation logic was removed from `cert-request'
> >
> >[1] http://idm.etherpad.corp.redhat.com/rhel72-cert-mgmt-progress
> >[2] http://www.freeipa.org/page/V4/User_Certificates#Revocation_of_the_Certificates
> >
> >Thanks,
> >Fraser
> Thanks for updated patches:
> 
> Please update required pki version in freeipa.spec.in
> 
> Patch 0001:
> Again, will be this change done after upgrade, instead of new installation?
> config.set("CA", "pki_profiles_in_ldap", "True")
> 
> I blame method 'ca_enable_ldap_profile_subsystem' in upgrade to do this, but
> I'm not sure.
> 
That's correct.  The `config.set' is setting the value in the
(ephemeral) pkispawn(8) config file.

> 
> Patch 0005:
> 1)
> just nitpick:
> profile_id_pattern = re.compile('^[a-zA-Z]\w*$')
> PROFILE_ID_PATTERN = re.compile('^profileId=(\w+)', re.MULTILINE)
> 
> In PROFILE_ID_PATTERN should be '^profileId=([a-zA-Z]\w*)' to be consistent
> 
Good point; is fixed for next patchset.

> Patch 0007:
> There are still modifications in ipa-server-isntall
> 
It is necessary to import the included profiles into Dogtag after
LDAP updates are applied to IPA DIT, so that step is inevitable.

I would like to avoid the import hackery but haven't found the
better way yet.  IMO it shouldn't be a blocker, but something to
revisit once I finish the caacl plugin and move to bugfixing /
polishing.  (I suspect the best way forward is to explicitly pass
more arguments around where they are needed and reduce the amount of
shared state that is implicitly relied on in the module.)

> Patch 0010:
> 1)
> +aci: (targetattr = "usercertificate")(version 3.0;acl "selfservice:Users
> can manage their own X.509 certificates";allow (write) userdn =
> "ldap:///self";)
>  This is not in any upgrade file
> 
> 2)
> +# User certificates
> +dn: $SUFFIX
> +add:aci:(targetattr = "ipasshpubkey")(version 3.0;acl "selfservice:Users
> can manage their own X.509 certificates";allow (write) userdn =
> "ldap:///self";)
> Is this right? (ipasshpubkey) This ACI with different name is already there.
> 
Whups, this is copy-pasta error :)  Changing it to 'usercertificate'
will fix 1) and 2).  This is done for the next patchset.

> 3)
> IMO this should not be there.
> 
>              'replaces': [
> -                '(targetattr = "givenname || sn || cn || displayname ||
> title || initials || loginshell || gecos || homephone || mobile || pager ||
> facsimiletelephonenumber || telephonenumber || street || roomnumber || l ||
> st || postalcode || manager || secretary || description || carlicense ||
> labeleduri || inetuserhttpurl || seealso || employeetype || businesscategory
> || ou || mepmanagedentry || objectclass")(target =
> "ldap:///uid=*,cn=users,cn=accounts,$SUFFIX")(version 3.0;acl
> "permission:Modify Users";allow (write) groupdn = "ldap:///cn=Modify
> Users,cn=permissions,cn=pbac,$SUFFIX";)',
> +                '(targetattr = "givenname || sn || cn || displayname ||
> title || initials || loginshell || gecos || homephone || mobile || pager ||
> facsimiletelephonenumber || telephonenumber || street || roomnumber || l ||
> st || postalcode || manager || secretary || description || carlicense ||
> labeleduri || inetuserhttpurl || seealso || employeetype || businesscategory
> || usercertificate || ou || mepmanagedentry || objectclass")(target =
> "ldap:///uid=*,cn=users,cn=accounts,$SUFFIX")(version 3.0;acl
> "permission:Modify Users";allow (write) groupdn = "ldap:///cn=Modify
> Users,cn=permissions,cn=pbac,$SUFFIX";)',
>              ],
> 
> this replace replaces permissions written in old style. New permission are
> updated automaticaly (AFAIK)
> 
OK, thanks.  Is removed in next patchset.

> PAtch 0011:
> Please dont use '_' in code, this is for  ugettext and may cause an issue in
> future
> +        bind_service, _, _ = bind_principal
> 
Oh yeah!  I am used to using "_" for "don't care".  Thanks for
pointing this out.  I have given them proper names for the next
patchset.

Thanks!
Fraser

> Martin^2
> 
> -- 
> Martin Basti
>