Patches have been reborn as https://github.com/freeipa/freeipa/pull/177.
Brief commentary inline. If any further issues, let us continue discussion at GitHub. Thanks, Fraser On Thu, Oct 06, 2016 at 10:02:55AM +0200, Jan Cholasta wrote: > On 23.9.2016 05:29, Fraser Tweedale wrote: > > Bump for review. > > > > Rebased patches attached (there was a trivial conflict in imports). > > > > Thanks, > > Fraser > > > > On Tue, Sep 06, 2016 at 02:05:06AM +1000, Fraser Tweedale wrote: > > > On Fri, Aug 26, 2016 at 10:28:58AM +0200, Jan Cholasta wrote: > > > > On 19.8.2016 13:11, Fraser Tweedale wrote: > > > > > Bump for review. > > > > > > > > > > On Wed, Aug 17, 2016 at 12:09:39AM +1000, Fraser Tweedale wrote: > > > > > > On Tue, Aug 16, 2016 at 08:10:08AM +0200, Jan Cholasta wrote: > > > > > > > On 16.8.2016 07:24, Fraser Tweedale wrote: > > > > > > > > On Mon, Aug 15, 2016 at 08:19:33AM +0200, Jan Cholasta wrote: > > > > > > > > > On 9.8.2016 16:47, Fraser Tweedale wrote: > > > > > > > > > > On Mon, Aug 08, 2016 at 10:49:27AM +0200, Jan Cholasta > > > > > > > > > > wrote: > > > > > > > > > > > On 8.8.2016 09:06, Fraser Tweedale wrote: > > > > > > > > > > > > On Mon, Aug 08, 2016 at 08:54:05AM +0200, Jan Cholasta > > > > > > > > > > > > wrote: > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > > > > > On 8.8.2016 06:34, Fraser Tweedale wrote: > > > > > > > > > > > > > > Please review the attached patch with adds > > > > > > > > > > > > > > --certificate-out and > > > > > > > > > > > > > > --certificate-chain-out options to `ca-show' > > > > > > > > > > > > > > command. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Note that --certificate-chain-out currently writes > > > > > > > > > > > > > > a bogus file due > > > > > > > > > > > > > > to a bug in Dogtag that will be fixed in this > > > > > > > > > > > > > > week's build. > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://fedorahosted.org/freeipa/ticket/6178 > > > > > > > > > > > > > > > > > > > > > > > > > > 1) The client-side *-out options should be defined on > > > > > > > > > > > > > the client side, not > > > > > > > > > > > > > on the server side. > > > > > > > > > > > > > > > > > > > > > > > > > Will option defined on client side be propagated to, > > > > > > > > > > > > and observable > > > > > > > > > > > > in the ipaserver plugin? The ipaserver plugin needs to > > > > > > > > > > > > observe that > > > > > > > > > > > > *-out has been requested and executes additional > > > > > > > > > > > > command(s) on that > > > > > > > > > > > > basis. > > > > > > > > > > > > > > > > > > > > > > Is there a reason not to *always* return the certs? > > > > > > > > > > > > > > > > > > > > > We hit Dogtag to retrieve them. > > > > > > > > > > > > > > > > > > I don't think that's an issue in a -show command. > > > > > > > > > > > > > > > > > cert_show is invoked by other commands (cert_find*, cert_show, > > > > > > > > cert_request, cert_status, ca_del) but these all hit Dogtag > > > > > > > > anyway > > > > > > > > so I suppose that's fine. I'll return the cert *and* the chain > > > > > > > > in > > > > > > > > separate attributes, unconditionally. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2) I don't think there should be additional > > > > > > > > > > > > > information included in summary > > > > > > > > > > > > > (and it definitely should not be multi-line). I would > > > > > > > > > > > > > rather inform the user > > > > > > > > > > > > > via an error message when unable to write the files. > > > > > > > > > > > > > > > > > > > > > > > > > I was just following the pattern of other commands that > > > > > > > > > > > > write certs, > > > > > > > > > > > > profile config, etc. Apart from consistency with other > > > > > > > > > > > > commands I > > > > > > > > > > > > agree that there is no need to have it. So I will > > > > > > > > > > > > remove it. > > > > > > > > > > > > > > > > > > > > > > > > > If you think there is an actual value in informing > > > > > > > > > > > > > the user about > > > > > > > > > > > > > successfully writing the files, please use > > > > > > > > > > > > > ipalib.messages for the job. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 3) IMO a better format for the certificate chain than > > > > > > > > > > > > > PKCS#7 would be > > > > > > > > > > > > > concatenated PEM, as that's the most commonly used > > > > > > > > > > > > > format in IPA (in > > > > > > > > > > > > > installers, there are no cert chains in API commands > > > > > > > > > > > > > ATM). > > > > > > > > > > > > > > > > > > > > > > > > > Sure, but the main use case isn't IPA. Other apps > > > > > > > > > > > > require PKCS #7 > > > > > > > > > > > > or concatenated PEMs, but sometimes they must be > > > > > > > > > > > > concatenated > > > > > > > > > > > > forward, and othertimes backwards. There is no one > > > > > > > > > > > > size fits all. > > > > > > > > > > > > > > > > > > > > > > True, which is exactly why I think we should at least be > > > > > > > > > > > self-consistent and > > > > > > > > > > > use concatenated PEM (and multi-value DER over the wire). > > > > > > > > > > > > > > > > > > > > > Dogtag returns a PKCS7 (either DER or PEM, according to > > > > > > > > > > HTTP Accept > > > > > > > > > > header). > > > > > > > > > > > > > > > > > > > > If we want list-of-PEMs between server and client we have > > > > > > > > > > to convert > > > > > > > > > > on the server. Do we have a good way of doing this without > > > > > > > > > > exec'ing > > > > > > > > > > `openssl pkcs7' on the server? Is it acceptable to exec > > > > > > > > > > 'openssl' > > > > > > > > > > to do the conversion on the server? python-nss does not > > > > > > > > > > have PKCS7 > > > > > > > > > > functions and I am not keen on adding a pyasn1 PKCS7 parser > > > > > > > > > > just for > > > > > > > > > > the sake of pushing bits as list-of-PEMs. > > > > > > > > > > > > > > > > > > I'm afraid we can't avoid conversion to/from PKCS#7 one way > > > > > > > > > or the other. > > > > > > > > > For example, if we added a call to retrieve external CA chain > > > > > > > > > using certs > > > > > > > > > from cn=certificates,cn=ipa,cn=etc, we would have to convert > > > > > > > > > the result to > > > > > > > > > PKCS#7 if it was our cert chain format of choice. > > > > > > > > > > > > > > > > > > What we can avoid though is executing "openssl pkcs7" to do > > > > > > > > > the conversion - > > > > > > > > > we can use an approach similar to our DNSSEC code and use > > > > > > > > > python-cffi to > > > > > > > > > call libcrypto's PKCS#7 conversion routines instead. > > > > > > > > > > > > > > > > > I had a look at the OpenSSL API for parsing PKCS #7; now I > > > > > > > > prefer to > > > > > > > > exec `openssl' to do the job :) > > > > > > > > > > > > > > > > I will transmit DER-encoded PKCS #7 object on the wire; we > > > > > > > > cannot > > > > > > > > used multi-valued DER attribute because order is important. > > > > > > > > Client > > > > > > > > will convert to PEMs. > > > > > > > > > > > > > > Well, my point was not to send PKCS#7 over the wire, so that > > > > > > > clients > > > > > > > (including 3rd party clients) do not have to convert from PKCS#7 > > > > > > > themselves. > > > > > > > > > > > > > > In fact we can use multi-valued DER - whatever you send over the > > > > > > > wire from > > > > > > > the server will be received in the exact same order by the > > > > > > > client. Even if > > > > > > > it wasn't, you can easily restore the order by matching issuer > > > > > > > and subject > > > > > > > names of the certificates. > > > > > > > > > > > > > > > > > > > > > > > Should have new patch on list this afternoon. > > > > > > > > > > > > > > > > Thanks, > > > > > > > > Fraser > > > > > > > > > > > > > > > > > > > > > > > > > > > > FWIW, man pages and code suggest that PKCS #7 is accepted in > > > > > > > > > > installer, etc. > > > > > > > > > > > > > > > > > > True, but that's a relatively new feature (since 4.1) and the > > > > > > > > > installer > > > > > > > > > internally executes "openssl pkcs7" to convert PKCS #7 to > > > > > > > > > list of certs :-) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > We can add an option to control the format later, but > > > > > > > > > > > > for now, > > > > > > > > > > > > Dogtag returns a PKCS #7 (PEM or DER) so let's go with > > > > > > > > > > > > that. Worst > > > > > > > > > > > > case is an admin has to invoke `openssl pkcs7' and > > > > > > > > > > > > concat the certs > > > > > > > > > > > > themselves. > > > > > > > > > > > > > > > > > > > > > > AFAIK none of NSS, OpenSSL or p11-kit can use PKCS#7 cert > > > > > > > > > > > chains directly, > > > > > > > > > > > so I'm afraid the worst case would happen virtually > > > > > > > > > > > always. > > > > > > > > > > > > > > > > > > > > > If you're OK with invoking OpenSSL on the client to convert > > > > > > > > > > PKCS #7 > > > > > > > > > > to list-of-PEMs (similar to what is done in > > > > > > > > > > ipapython.certdb.NSSDatabase) then we can have the client > > > > > > > > > > perform > > > > > > > > > > the conversion. > > > > > > > > > > > > > > > > > > See above. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 4) Over the wire, the certs should be DER-formatted, > > > > > > > > > > > > > as that's the most > > > > > > > > > > > > > common wire format in other API commands. > > > > > > > > > > > > > > > > > > > > > > > > > OK. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 5) What is the benefit in having the CA cert and the > > > > > > > > > > > > > rest of the chain > > > > > > > > > > > > > separate? For end-entity certs it makes sense to > > > > > > > > > > > > > separate the cert from the > > > > > > > > > > > > > CA chain, but for CA certs, you usually want the full > > > > > > > > > > > > > chain, no? > > > > > > > > > > > > > > > > > > > > > > > > > If you want to anchor trust directly at a subca (e.g. > > > > > > > > > > > > restrict VPN > > > > > > > > > > > > login to certs issued by VPN sub-CA) then you often > > > > > > > > > > > > just want the > > > > > > > > > > > > cert. The chain option does subsume it, at cost of > > > > > > > > > > > > more work for > > > > > > > > > > > > administrators with this use case. I think it makes > > > > > > > > > > > > sense to keep > > > > > > > > > > > > both options. > > > > > > > > > > > > > > > > > > > > > > Does it? From what you described above, you either want > > > > > > > > > > > just the sub-CA > > > > > > > > > > > cert, or the full chain including the sub-CA cert, in > > > > > > > > > > > which case it might > > > > > > > > > > > make more sense to have a single --out option and a > > > > > > > > > > > --chain flag. > > > > > > > > > > > > > > > > > > > > > How about --certificate-out which defaults to single cert, > > > > > > > > > > but does > > > > > > > > > > chain (as list-of-PEMs) when --chain flag given. > > > > > > > > > > > > > > > > > > > > Per https://fedorahosted.org/freeipa/ticket/5166 let's not > > > > > > > > > > add more > > > > > > > > > > `--out' options. > > > > > > > > > > > > > > > > > > +1 > > > > > > > > > > > > > > > Updated patch 0097-2 attached, and new patch 0099 which must be > > > > > > applied first. > > > > > > > > > > > > I have implemented the suggested changes, except for cffi (I execute > > > > > > `openssl pkcs7' instead). > > > > > > > > I don't like it, but OK. Another alternative would be to use pyasn1. > > > > > > > I don't like it either, but neither did I like the idea of > > > reimplementing the wheel with pyasn1. Now is not the time for > > > busywork :) > > > > > > > > > > > > > > > There are two new output attributes on the wire, 'certificate' > > > > > > (single-value DER X.509), and 'certificate_chain' (ordered > > > > > > multi-value DER X.509). They are always returned. The first cert > > > > > > in the chain is always the same as 'certificate'; obviously this is > > > > > > redunant but I have left it this way because I think usage is > > > > > > clearer. > > > > > > > > I don't have a strong feeling about this one way or the other, but the > > > > same > > > > scheme should be used for cert-show in the future. Does it make sense > > > > to do > > > > it this way for cert-show? > > > > > > > > I'm not sure about always returning the chain in cert-show. Now that we > > > > have > > > > a --chain flag rather than two out options, maybe we should go back to > > > > returning the chain only if --chain is specified. What do you think? > > > > > > > I think we should go for consistency and always include both over > > > the wire. > > My point exactly - ca-show output should be equivalent to cert-show on the > CA certificate, as far as the certificate and chain are concerned. > I reused `BaseCertObject.takes_params' and `BaseCertObject._parse' to define the params and do most of the work. There is some overlap with what `BaseCertObject' defines and fields of the `ca' LDAP attribute so these are ignored/removed. > > If we want to hide cert or chain or both at the `ipa' CLI > > > depending on options, I also don't feel strongly either way. For > > > now they're both displayed. > > I think I would prefer if the certificate was always returned by the server, > but the chain only if --chain (or --all) is specified. > > Additionally, ca-add should also get the new options and do all of this. > I've implemented this. `--chain' implies `--all' but otherwise remains a client-side only param. A single MethodOverride provides the desired functionality and is inherited by both `ca_add' and `ca_show' on the client side. > > > > > > > > > > > Patch 0099: > > > > > > > > 1) Please fix this: > > > > > > > > $ git show -U0 | pep8 --diff > > > > ./ipalib/x509.py:59:80: E501 line too long (93 > 79 characters) > > > > > > > Done. > > > > > > > > > > > Patch 0097: > > > > > > > > 1) `certificate` and `certificate_chain` are actually attributes of the > > > > ca > > > > object, so they should be defined in ca.takes_params rather than > > > > ca_show.has_output_params. > > > > > > > Done. Out of interest, now that they are part of ca_takes_params is > > > there a way to hide them by default in CLI output, and only show > > > them when `--all' is given? > > > > > > > > > > > 2) Please fix these: > > > > > > > > $ git show -U0 | pep8 --diff > > > > ./ipaclient/plugins/ca.py:21:9: E124 closing bracket does not match > > > > visual > > > > indentation > > > > ./ipaclient/plugins/ca.py:23:13: E128 continuation line under-indented > > > > for > > > > visual indent > > > > ./ipaclient/plugins/ca.py:24:13: E128 continuation line under-indented > > > > for > > > > visual indent > > > > ./ipaclient/plugins/ca.py:25:13: E128 continuation line under-indented > > > > for > > > > visual indent > > > > ./ipaclient/plugins/ca.py:26:9: E124 closing bracket does not match > > > > visual > > > > indentation > > > > ./ipaclient/plugins/ca.py:38:13: E731 do not assign a lambda > > > > expression, use > > > > a def > > > > > > > Done. Updated patches attached. > > > > > > Thanks, > > > Fraser > > > > > From 046b3dd078c4ccc3732a0106786bae4c01d30a89 Mon Sep 17 00:00:00 2001 > > > From: Fraser Tweedale <ftwee...@redhat.com> > > > Date: Tue, 16 Aug 2016 13:16:58 +1000 > > > Subject: [PATCH] Add function for extracting PEM certs from PKCS #7 > > > > > > Add a single function for extracting X.509 certs in PEM format from > > > a PKCS #7 object. Refactor sites that execute ``openssl pkcs7`` to > > > use the new function. > > > > > > Part of: https://fedorahosted.org/freeipa/ticket/6178 > > > --- > > > ipalib/x509.py | 23 +++++++++++++++++- > > > ipapython/certdb.py | 14 ++++------- > > > ipaserver/install/cainstance.py | 52 > > > +++++++++++++++-------------------------- > > > 3 files changed, 45 insertions(+), 44 deletions(-) > > > > > > diff --git a/ipalib/x509.py b/ipalib/x509.py > > > index > > > e986a97a58aafd3aeab08765a397edbf67c7841a..0461553a73e3862c85f1ffcfe4432cabf4fdf7a1 > > > 100644 > > > --- a/ipalib/x509.py > > > +++ b/ipalib/x509.py > > > @@ -51,11 +51,14 @@ from ipalib import util > > > from ipalib import errors > > > from ipaplatform.paths import paths > > > from ipapython.dn import DN > > > +from ipapython import ipautil > > > > > > PEM = 0 > > > DER = 1 > > > > > > -PEM_REGEX = re.compile(r'(?<=-----BEGIN CERTIFICATE-----).*?(?=-----END > > > CERTIFICATE-----)', re.DOTALL) > > > +PEM_REGEX = re.compile( > > > + r'-----BEGIN CERTIFICATE-----.*?-----END CERTIFICATE-----', > > > + re.DOTALL) > > > > > > EKU_SERVER_AUTH = '1.3.6.1.5.5.7.3.1' > > > EKU_CLIENT_AUTH = '1.3.6.1.5.5.7.3.2' > > > @@ -148,6 +151,24 @@ def load_certificate_list(data, dbdir=None): > > > certs = [load_certificate(cert, PEM, dbdir) for cert in certs] > > > return certs > > > > > > + > > > +def pkcs7_to_pems(data, datatype=PEM): > > > + """ > > > + Extract certificates from a PKCS #7 object. > > > + > > > + Return a ``list`` of X.509 PEM strings. > > > + > > > + May throw ``ipautil.CalledProcessError`` on invalid data. > > > + > > > + """ > > > + cmd = [ > > > + paths.OPENSSL, "pkcs7", "-print_certs", > > > + "-inform", "PEM" if datatype == PEM else "DER", > > > + ] > > > + result = ipautil.run(cmd, stdin=data, capture_output=True) > > > + return PEM_REGEX.findall(result.output) > > > + > > > + > > > def load_certificate_list_from_file(filename, dbdir=None): > > > """ > > > Load a certificate list from a PEM file. > > > diff --git a/ipapython/certdb.py b/ipapython/certdb.py > > > index > > > e19f712d82f160ebc5de9c5b8d6627cb941c2cef..fd18023794a2daace60efd97aff54180b8409bbd > > > 100644 > > > --- a/ipapython/certdb.py > > > +++ b/ipapython/certdb.py > > > @@ -270,13 +270,11 @@ class NSSDatabase(object): > > > continue > > > > > > if label in ('PKCS7', 'PKCS #7 SIGNED DATA', > > > 'CERTIFICATE'): > > > - args = [ > > > - paths.OPENSSL, 'pkcs7', > > > - '-print_certs', > > > - ] > > > try: > > > - result = ipautil.run( > > > - args, stdin=body, capture_output=True) > > > + certs = x509.pkcs7_to_pems(body) > > > + extracted_certs += '\n'.join(certs) + '\n' > > > + loaded = True > > > + continue > > > except ipautil.CalledProcessError as e: > > > if label == 'CERTIFICATE': > > > root_logger.warning( > > > @@ -287,10 +285,6 @@ class NSSDatabase(object): > > > "Skipping PKCS#7 in %s at line %s: > > > %s", > > > filename, line, e) > > > continue > > > - else: > > > - extracted_certs += result.output + '\n' > > > - loaded = True > > > - continue > > Moving this to the try block is a completely unnecessary change. > Moved it back :) > > > > > > if label in ('PRIVATE KEY', 'ENCRYPTED PRIVATE KEY', > > > 'RSA PRIVATE KEY', 'DSA PRIVATE KEY', > > > diff --git a/ipaserver/install/cainstance.py > > > b/ipaserver/install/cainstance.py > > > index > > > c4b8e9ae326fb7ebda9e927cd4d0b5bad9743db4..f57c724b0273a275f8146f0d6055e2ee2e51192c > > > 100644 > > > --- a/ipaserver/install/cainstance.py > > > +++ b/ipaserver/install/cainstance.py > > > @@ -844,44 +844,30 @@ class CAInstance(DogtagInstance): > > > # makes openssl throw up. > > > data = base64.b64decode(chain) > > > > > > - result = ipautil.run( > > > - [paths.OPENSSL, > > > - "pkcs7", > > > - "-inform", > > > - "DER", > > > - "-print_certs", > > > - ], stdin=data, capture_output=True) > > > - certlist = result.output > > > + certlist = x509.pkcs7_to_pems(data, x509.DER) > > > > > > # Ok, now we have all the certificates in certs, walk through it > > > # and pull out each certificate and add it to our database > > > > > > - st = 1 > > > - en = 0 > > > - subid = 0 > > > ca_dn = DN(('CN','Certificate Authority'), self.subject_base) > > > - while st > 0: > > > - st = certlist.find('-----BEGIN', en) > > > - en = certlist.find('-----END', en+1) > > > - if st > 0: > > > - try: > > > - (chain_fd, chain_name) = tempfile.mkstemp() > > > - os.write(chain_fd, certlist[st:en+25]) > > > - os.close(chain_fd) > > > - (_rdn, subject_dn) = > > > certs.get_cert_nickname(certlist[st:en+25]) > > > - if subject_dn == ca_dn: > > > - nick = get_ca_nickname(self.realm) > > > - trust_flags = 'CT,C,C' > > > - else: > > > - nick = str(subject_dn) > > > - trust_flags = ',,' > > > - self.__run_certutil( > > > - ['-A', '-t', trust_flags, '-n', nick, '-a', > > > - '-i', chain_name] > > > - ) > > > - finally: > > > - os.remove(chain_name) > > > - subid += 1 > > > + for cert in certlist: > > > + try: > > > + (chain_fd, chain_name) = tempfile.mkstemp() > > > + os.write(chain_fd, cert) > > > + os.close(chain_fd) > > > + (_rdn, subject_dn) = certs.get_cert_nickname(cert) > > > + if subject_dn == ca_dn: > > > + nick = get_ca_nickname(self.realm) > > > + trust_flags = 'CT,C,C' > > > + else: > > > + nick = str(subject_dn) > > > + trust_flags = ',,' > > > + self.__run_certutil( > > > + ['-A', '-t', trust_flags, '-n', nick, '-a', > > > + '-i', chain_name] > > > + ) > > > + finally: > > > + os.remove(chain_name) > > > > > > def __request_ra_certificate(self): > > > # Create a noise file for generating our private key > > > -- > > > 2.5.5 > > > > > > > > From fba36bd2b86c2aee1d77e05aa563ced4633ab182 Mon Sep 17 00:00:00 2001 > > > From: Fraser Tweedale <ftwee...@redhat.com> > > > Date: Mon, 8 Aug 2016 14:27:20 +1000 > > > Subject: [PATCH] Add options to write lightweight CA cert or chain to file > > > > > > Administrators need a way to retrieve the certificate or certificate > > > chain of an IPA-managed lightweight CA. Add params to the `ca' > > > object for carrying the CA certificate and chain (as multiple DER > > > values), and add the `--certificate-out' option and `--chain' flag > > > as client-side options for writing one or the other to a file. > > > > > > Fixes: https://fedorahosted.org/freeipa/ticket/6178 > > > --- > > > ipaclient/plugins/ca.py | 50 > > > +++++++++++++++++++++++++++++++++++++++++++++ > > > ipaserver/plugins/ca.py | 31 ++++++++++++++++++++++++---- > > > ipaserver/plugins/dogtag.py | 12 +++++++++++ > > > 3 files changed, 89 insertions(+), 4 deletions(-) > > > create mode 100644 ipaclient/plugins/ca.py > > > > > > diff --git a/ipaclient/plugins/ca.py b/ipaclient/plugins/ca.py > > > new file mode 100644 > > > index > > > 0000000000000000000000000000000000000000..f7e55dec196495f820ebf745eb49e8ddce6b3ee7 > > > --- /dev/null > > > +++ b/ipaclient/plugins/ca.py > > > @@ -0,0 +1,50 @@ > > > +# > > > +# Copyright (C) 2016 FreeIPA Contributors see COPYING for license > > > +# > > > + > > > +import base64 > > > +from ipaclient.frontend import MethodOverride > > > +from ipalib import util, x509, Flag, Str > > > +from ipalib.plugable import Registry > > > +from ipalib.text import _ > > > + > > > +register = Registry() > > > + > > > + > > > +@register(override=True, no_fail=True) > > > +class ca_show(MethodOverride): > > > + > > > + takes_options = ( > > > + Str( > > > + 'certificate_out?', > > > + doc=_('Write certificate to file'), > > > + include='cli', > > > + ), > > > + Flag( > > > + 'chain', > > > + default=False, > > > + doc=_('Write certificate chain instead of single > > > certificate'), > > > + include='cli', > > > + ), > > > + ) > > > + > > > + def forward(self, *keys, **options): > > > + filename = None > > > + if 'certificate_out' in options: > > > + filename = options.pop('certificate_out') > > > + util.check_writable_file(filename) > > > + chain = options.pop('chain', False) > > > + > > > + result = super(ca_show, self).forward(*keys, **options) > > > + if filename: > > > + def to_pem(x): > > > + return x509.make_pem(base64.b64encode(x)) > > > + if chain: > > > + ders = result['result']['certificate_chain'] > > > + data = '\n'.join(map(to_pem, ders)) > > Generator expressions are generally preferred over map(): > > data = '\n'.join(to_pem(der) for der in ders) > Preferred by whom? ;) I changed it to gen expr. > > > + else: > > > + data = to_pem(result['result']['certificate']) > > > + with open(filename, 'wb') as f: > > > + f.write(data) > > > + > > > + return result > > > diff --git a/ipaserver/plugins/ca.py b/ipaserver/plugins/ca.py > > > index > > > 966ae2b1bdb4bb0207dfa58f0e9c951bc930f766..0684ddaed0ebfcab8910c1ea356550b504af15e2 > > > 100644 > > > --- a/ipaserver/plugins/ca.py > > > +++ b/ipaserver/plugins/ca.py > > > @@ -2,14 +2,14 @@ > > > # Copyright (C) 2016 FreeIPA Contributors see COPYING for license > > > # > > > > > > -from ipalib import api, errors, DNParam, Str > > > +from ipalib import api, errors, Bytes, DNParam, Str > > > from ipalib.constants import IPA_CA_CN > > > from ipalib.plugable import Registry > > > from ipaserver.plugins.baseldap import ( > > > LDAPObject, LDAPSearch, LDAPCreate, LDAPDelete, > > > LDAPUpdate, LDAPRetrieve) > > > from ipaserver.plugins.cert import ca_enabled_check > > > -from ipalib import _, ngettext > > > +from ipalib import _, ngettext, x509 > > > > > > > > > __doc__ = _(""" > > > @@ -79,6 +79,18 @@ class ca(LDAPObject): > > > doc=_('Issuer Distinguished Name'), > > > flags=['no_create', 'no_update'], > > > ), > > > + Bytes( > > > + 'certificate', > > > + label=_("Certificate"), > > > + doc=_("X.509 certificate"), > > > + flags={'no_create', 'no_update', 'no_search', 'no_display'}, > > Note that the no_display flag currently does nothing. > Removed the flag. > > > > + ), > > > + Bytes( > > > + 'certificate_chain*', > > > + label=_("Certificate chain"), > > > + doc=_("PKCS #7 certificate chain"), > > Looks like you forgot to update the doc string. > Well spotted; updated. > > > + flags={'no_create', 'no_update', 'no_search', 'no_display'}, > > > + ), > > > ) > > > > > > permission_filter_objectclasses = ['ipaca'] > > > @@ -140,9 +152,20 @@ class ca_find(LDAPSearch): > > > class ca_show(LDAPRetrieve): > > > __doc__ = _("Display the properties of a CA.") > > > > > > - def execute(self, *args, **kwargs): > > > + def execute(self, *keys, **options): > > > ca_enabled_check() > > > - return super(ca_show, self).execute(*args, **kwargs) > > > + result = super(ca_show, self).execute(*keys, **options) > > > + > > > + ca_id = result['result']['ipacaid'][0] > > > + with self.api.Backend.ra_lightweight_ca as ca_api: > > > + result['result']['certificate'] = ca_api.read_ca_cert(ca_id) > > > + > > > + pkcs7_der = ca_api.read_ca_chain(ca_id) > > > + pems = x509.pkcs7_to_pems(pkcs7_der, x509.DER) > > > + ders = (x509.normalize_certificate(pem) for pem in pems) > > > + result['result']['certificate_chain'] = list(ders) > > I would think list comprehension is the obvious choice over generator > expression when generating a list: > > ders = [x509.normalize_certificate(pem) for pem in pems] > result['result']['certificate_chain'] = ders > Changed, thanks. > > > + > > > + return result > > > > > > > > > @register() > > > diff --git a/ipaserver/plugins/dogtag.py b/ipaserver/plugins/dogtag.py > > > index > > > aef1e888eb1b6c273c1fd12cbf4912407f8f8132..1fd3106e0ae723eb30dbe32c61e637790f6085d2 > > > 100644 > > > --- a/ipaserver/plugins/dogtag.py > > > +++ b/ipaserver/plugins/dogtag.py > > > @@ -2205,6 +2205,18 @@ class ra_lightweight_ca(RestClient): > > > except: > > > raise errors.RemoteRetrieveError(reason=_("Response from CA > > > was not valid JSON")) > > > > > > + def read_ca_cert(self, ca_id): > > > + status, resp_headers, resp_body = self._ssldo( > > > + 'GET', '{}/cert'.format(ca_id), > > > + headers={'Accept': 'application/pkix-cert'}) > > > + return resp_body > > > + > > > + def read_ca_chain(self, ca_id): > > > + status, resp_headers, resp_body = self._ssldo( > > > + 'GET', '{}/chain'.format(ca_id), > > > + headers={'Accept': 'application/pkcs7-mime'}) > > > + return resp_body > > > + > > > def disable_ca(self, ca_id): > > > self._ssldo( > > > 'POST', ca_id + '/disable', > > > -- > > > 2.5.5 > > > > > -- > Jan Cholasta -- 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