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

[Fraser Tweedale]

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.
> 
Actually, yeah, we can just write the PEMs to a single file,
sequentially.  I don't know why I didn't think of that... ¯\_(ツ)_/¯

> 
> 1)
> +            if len(certs) > 0:
> 
> please use only,
> if certs:
> 
I have strong feels about this.  ``if len(certs) > 0:`` admits fewer
bugs than ``if certs:`` , e.g. if 'certs' were bound to a
non-length-able object by mistake.  Even though we have already
iterated ``certs`` at this point in the function, this failure mode
is still possible, e.g. if ``certs`` is a generator.  Using ``if
certs:`` will not fail for a generator, but it will be a silent bug!

PEP-8 is wrong on this one.  It is better to use the construction
that admits fewer errors.  Unless it causes lint failure (on f21 it
does not) I prefer not to change it.

> 2)
> You need to re-generate API/ACI.txt in this patch
> 
Good pickup, thanks.

> 3)
> syntax error:
> +        for dercert in certs_der
> 
Geez... dunno how I let that one past -_-  My bad.

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

Yeah... I was a bit confused about how --addattr=... interacts with
options.  I understand it now, and yes I think we should revoke
certificates that get removed this way.

cert-request will use addattr= so no existing certificates will get
removed (or revoked).  New patch addressing this and other points
will arrive on list today.

Thanks for reviewing!
Fraser

> -- 
> Martin Basti
>