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.
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.
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.
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)
+ 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.
+ ),
+ Bytes(
+ 'certificate_chain*',
+ label=_("Certificate chain"),
+ doc=_("PKCS #7 certificate chain"),
Looks like you forgot to update the doc string.
+ 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
+
+ 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
