On 28/05/15 11:17, Fraser Tweedale wrote:
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.

Ok then, leave there len() > 0

but to catch these kind of mistakes, we should have asserts everywhere to verify type.

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



--
Martin Basti

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to