[Freeipa-devel] [PATCH 0014 v3] Support multiple user and host certificates

[Fraser Tweedale]

On Wed, Jun 03, 2015 at 01:55:47PM +0200, Milan Kubik wrote:
> On 06/03/2015 01:17 PM, Martin Basti wrote:
> >On 02/06/15 16:03, Jan Cholasta wrote:
> >>Dne 2.6.2015 v 12:36 Martin Basti napsal(a):
> >>>On 02/06/15 11:42, Fraser Tweedale wrote:
> >>>>On Mon, Jun 01, 2015 at 02:50:45PM +0200, Martin Basti wrote:
> >>>>>On 01/06/15 06:40, Fraser Tweedale wrote:
> >>>>>>New version of patch; ``{host,service}-show --out=FILE`` now writes
> >>>>>>all certs to FILE.  Rebased on latest master.
> >>>>>>
> >>>>>>Thanks,
> >>>>>>Fraser
> >>>>>>
> >>>>>>On Thu, May 28, 2015 at 09:18:04PM +1000, Fraser Tweedale wrote:
> >>>>>>>Updated patch attached.  Notably restores/adds revocation behaviour
> >>>>>>>to host-mod and service-mod.
> >>>>>>>
> >>>>>>>Thanks,
> >>>>>>>Fraser
> >>>>>>>
> >>>>>>>On Wed, May 27, 2015 at 06:12:50PM +0200, Martin Basti wrote:
> >>>>>>>>On 27/05/15 15:53, Fraser Tweedale wrote:
> >>>>>>>>>This patch adds supports for multiple user / host
> >>>>>>>>>certificates.  No
> >>>>>>>>>schema change is needed ('usercertificate' attribute is already
> >>>>>>>>>multi-value).  The revoke-previous-cert behaviour of host-mod and
> >>>>>>>>>user-mod has been removed but revocation behaviour of -del and
> >>>>>>>>>-disable is preserved.
> >>>>>>>>>
> >>>>>>>>>The latest profiles/caacl patchset (0001..0013 v5) depends
> >>>>>>>>>on this
> >>>>>>>>>patch for correct cert-request behaviour.
> >>>>>>>>>
> >>>>>>>>>There is one design question (or maybe more, let me know): the
> >>>>>>>>>`--out=FILENAME' option to {host,service} show saves ONE
> >>>>>>>>>certificate
> >>>>>>>>>to the named file.  I propose to either:
> >>>>>>>>>
> >>>>>>>>>a) write all certs, suffixing suggested filename with either a
> >>>>>>>>>    sequential numerical index, e.g. "cert.pem" becomes
> >>>>>>>>>    "cert.pem.1", "cert.pem.2", and so on; or
> >>>>>>>>>
> >>>>>>>>>b) as above, but suffix with serial number and, if there are
> >>>>>>>>>    different issues, some issuer-identifying information.
> >>>>>>>>>
> >>>>>>>>>Let me know your thoughts.
> >>>>>>>>>
> >>>>>>>>>Thanks,
> >>>>>>>>>Fraser
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>Is there a possible way how to store certificates into one file?
> >>>>>>>>I read about possibilities to have multiple certs in one .pem
> >>>>>>>>file, but I'm
> >>>>>>>>not cert guru :)
> >>>>>>>>
> >>>>>>>>I personally vote for serial number in case there are multiple
> >>>>>>>>certificates,
> >>>>>>>>if ^ is no possible.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>1)
> >>>>>>>>+            if len(certs) > 0:
> >>>>>>>>
> >>>>>>>>please use only,
> >>>>>>>>if certs:
> >>>>>>>>
> >>>>>>>>2)
> >>>>>>>>You need to re-generate API/ACI.txt in this patch
> >>>>>>>>
> >>>>>>>>3)
> >>>>>>>>syntax error:
> >>>>>>>>+        for dercert in certs_der
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>4)
> >>>>>>>>command
> >>>>>>>>ipa user-mod ca_user --certificate=<ceritifcate>
> >>>>>>>>
> >>>>>>>>removes the current certificate from the LDAP, by design.
> >>>>>>>>Should be the old certificate(s) revoked? You removed that part in
> >>>>>>>>the code.
> >>>>>>>>
> >>>>>>>>only the --addattr='usercertificate=<cert>' appends new
> >>>>>>>>value there
> >>>>>>>>
> >>>>>>>>-- 
> >>>>>>>>Martin Basti
> >>>>>>>>
> >>>>>My objections/proposed solutions in attached patch.
> >>>>>
> >>>>>* VERSION
> >>>>>* In the previous version normalized values was stored in LDAP, so I
> >>>>>added
> >>>>>it back.  (I dont know why there is no normalization in param
> >>>>>settings, but
> >>>>>normalization for every certificate is done in callback. I will
> >>>>>file a
> >>>>>ticket for this)
> >>>>>* IMO only normalized certificates should be compared in the old
> >>>>>certificates detection
> >>>>>
> >>>>I incorporated your suggested changes in new patch (attached).
> >>>>
> >>>>There were no proposed changes to the other patchset (0001..0013)
> >>>>since rebase.
> >>>>
> >>>>Thanks,
> >>>>Fraser
> >>>Thank you,
> >>>ACK
> >>>Martin^2
> >>>
> >>
> >>Pushed to master: 7f7c247bb5a4b0030d531f4f14c156162e808212
> >>
> >Regression found.
> >
> >Patch to fix the issue is attached.
> >
> The fix works, thanks.
> 
> Milan

Thanks for finding, fixing and testing!  ACK from me.

I also present a fix of my own.  It fixes a problem where
service-mod deleted all certificates when
'--addattr usercertificate=XXX' was used instead of
'--usercertificate=XXX' options.

Cheers,
Fraser
-------------- next part --------------
From 5816c655b75a516068301186b20ddc36b966073c Mon Sep 17 00:00:00 2001
From: Fraser Tweedale <ftweedal at redhat.com>
Date: Wed, 3 Jun 2015 02:49:28 -0400
Subject: [PATCH] Fix certificate management with service-mod

Adding or removing certificates from a service via --addattr or
--delattr is broken.  Get certificates from entry_attrs instead of
options.
---
 ipalib/plugins/service.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipalib/plugins/service.py b/ipalib/plugins/service.py
index c290344cf6c14155ec1b103525ff8642a7a8e2af..369321d76a7b8e4e0a0d572fa1d26145cca010f4 100644
--- a/ipalib/plugins/service.py
+++ b/ipalib/plugins/service.py
@@ -598,7 +598,7 @@ class service_mod(LDAPUpdate):
         (service, hostname, realm) = split_principal(keys[-1])
 
         # verify certificates
-        certs = options.get('usercertificate') or []
+        certs = entry_attrs.get('usercertificate') or []
         certs_der = map(x509.normalize_certificate, certs)
         for dercert in certs_der:
             x509.verify_cert_subject(ldap, hostname, dercert)
-- 
2.1.0