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

[Fraser Tweedale]

On Thu, May 28, 2015 at 10:40:22AM +0200, Martin Basti wrote:
> On 28/05/15 10:13, Fraser Tweedale wrote:
> >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.
> I still think it should be in PEP8 style.
> 
> We should avoid these mistakes, not create the code which will detect it.
> 
How do you know if you made a mistake?  This is not just about me
making this change now.  If someone in the future comes along and
for whatever reasons changes the list to a generator, the diff in
isolation may look like a good change, and the reviewer might not
notice that the change leads to undesired behaviour elsewhere
(outside the diff context).

If you have good test coverage, tests might catch the bug.  If not,
hopefully QE notices the incorrect behaviour before a customer does.
The best tool is a type system so you don't get a program if you
make a mistake like this.  Well, Python doesn't give us that tool
but let us not ignore what it *can* do to help us for the sake of
adhereing to PEP-8 or saving a few characters.

> And, I forgot to write, please use
> except Exception as e:  instead except Exception, e
> to be compatible with python3
>
Sure, no problem.

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