Add options to write lightweight CA cert or chain to file #177

frasertweedale · 2016-10-21T08:08:23Z

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

HonzaCholasta · 2016-10-24T06:45:13Z

The original review thread is available at:

https://www.redhat.com/archives/freeipa-devel/2016-October/msg00578.html

frasertweedale · 2016-11-09T10:56:37Z

Bump for review

tiran · 2016-11-17T09:37:21Z

pylint fails:

Pylint is running, please wait ...
************* Module ipalib.x509
ipalib/x509.py:161: [E0602(undefined-variable), pkcs7_to_pems] Undefined variable 'paths')
make: *** [pylint] Error 2
Makefile:1040: recipe for target 'pylint' failed

tiran · 2016-11-17T09:38:26Z

ipalib/x509.py

+
+    """
+    cmd = [
+        paths.OPENSSL, "pkcs7", "-print_certs",


you forgot to add from ipaplatform.paths import paths.

thanks, the import was removed in a refactoring commit and didn't notice because patch rebased cleanly :) fixed in latest update.

The import was removed for a reason, putting it back will break the ipalib package on PyPI.

Now that we require pyasn1_modules, we can use PyASN1 to do the parsing instead:

if datatype == PEM: match = re.search(r'-----BEGIN PKCS7-----(.*?)-----END PKCS7-----', data, re.DOTALL) if not match: raise ValueError() data = base64.b64decode(match.group(1)) ci, _rest = decoder.decode(data, rfc2315.ContentInfo()) if ci['contentType'] != rfc2315.ContentType('1.2.840.113549.1.7.2'): raise ValueError() data = str(ci['content']) sd, _rest = decoder.decode(data, rfc2315.SignedData()) certs = [encoder.encode(c['certificate']) for c in sd['certificates']] return certs

tiran · 2016-11-17T09:41:50Z

ipapython/certdb.py

@@ -290,7 +285,7 @@ def import_files(self, files, db_password_filename, import_keys=False,
                                    filename, line, e)
                            continue
                        else:
-                            extracted_certs += result.output + '\n'
+                            extracted_certs += '\n'.join(certs) + '\n'


The line is ugly. Please convert extracted_certs to a list, append/extend the list and perform '\n\.join(extracted_certs) at the end of the function.

I made a separate commit for this refactor.

tiran · 2016-11-17T09:46:02Z

ipaserver/install/cainstance.py

-                    subid += 1
+        for cert in certlist:
+            try:
+                (chain_fd, chain_name) = tempfile.mkstemp()


Minor style nit-pick: The parenthesis are unnecessary.

Fixed in latest update.

tkrizek · 2016-11-24T16:35:43Z

Please update the xmlrpc tests to reflect the extra certificate attributes (~12 failed tests in test_xmlrpc/test_ca_plugin.py, test_caacl_plugin.py and test_caacl_profile_enforcement.py).

There are also a couple tests failing with ACIError:

ACIError: Insufficient access: Principal 'srv/santest-host-1...' is not permitted to use CA 'default-profile-subca' with profile 'caIPAserviceCert' for certificate issuance.

I also found the --certificate-out option a bit confusing. At first I thought I should provide the certificate name to be exported. Perhaps the help text could be improved to make it clear the used should provide a file name?

frasertweedale · 2016-11-25T03:11:13Z

@tomaskrizek thanks for reviewing. Updated tests and change the --certificate-out metavar to FILE.

HonzaCholasta · 2016-11-25T09:37:32Z

To continue the discussion from the mailing list:

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.

What I actually meant is that cert-show should also have a chain option and certificate_chain param in the future, which should work the same as in ca-show. Adding everything from BaseCertObject is an overkill IMHO, and out of the scope of ticket 6178.

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.

This does not scale well - if a new unrelated attribute is added to the CA LDAP entry, or if a new param is added to the CA object, --chain will imply retrieving them, which is not something we want. It should really be the other way around and --all should imply --chain, which also means --chain has to be defined on the server side.

Generator expressions are generally preferred over map():
data = '\n'.join(to_pem(der) for der in ders)
Preferred by whom? ;)

Pythonistas, I believe :)

frasertweedale · 2016-11-30T07:49:00Z

@jcholast thanks for review. PR updated. No longer inheriting BaseCertObject. --chain now defined
server-side and no longer implies --all.

frasertweedale · 2016-11-30T07:52:41Z

Never mind... my --chain option disappeared... not quite there yet >_<

frasertweedale · 2016-11-30T08:14:01Z

@jcholast OK there we go. I'd forgotten to remove the include='cli' when converting to server-side option.

HonzaCholasta · 2016-12-07T08:16:46Z

ipaserver/plugins/ca.py

+            'chain',
+            default=False,
+            doc=_('Include certificate chain in output'),
+        ),


Could we get rid of this copy-pasta? Maybe like this:

_chain_flag = Flag( 'chain', doc=_('Include certificate chain in output'), ) ... takes_options = LDAPCreate.takes_options + ( _chain_flag, )

(Also note that all flags implicitly have default value of False.)

meet you halfway: yes to de-dup; no to deferring to default value. Explicit is better than implicit :)

HonzaCholasta · 2016-12-07T08:28:48Z

Could you make ca-find return the cert/chain as well if (and only if) --all is specified? Do not add the --chain and --certificate-out options to it though. This is for consistency with cert-find, host-find, service-find, etc. Not a blocker.

Also see inline comments.

frasertweedale · 2016-12-07T10:13:16Z

@jcholast returning cert and chain in ca_find when --all is given will incur n * 2 additional round-trips to Dogtag where n = number of IPA-managed CAs. I am hesitant to do it unless/until Dogtag provides a better way. Let's open tickets.

HonzaCholasta · 2016-12-07T11:10:14Z

@frasertweedale, yep, I'm aware of that - cert-find does the same. Not a big deal IMO since it has to be explicitly requested by the user. But tickets are certainly a good idea.

frasertweedale · 2016-12-08T08:43:49Z

@jcholast updated PR to include certificate and certificate_chain in ca_find output when --all is specified.

HonzaCholasta · 2016-12-12T06:36:26Z

@frasertweedale, thanks. What about this?

frasertweedale · 2016-12-12T07:12:15Z

On Sun, Dec 11, 2016 at 10:36:27PM -0800, Jan Cholasta wrote: @frasertweedale, thanks. What about [this](https://github.com/freeipa/freeipa/pull/177/files#r91243228)?

It is a worthwhile change (thank you for reminding me). Let's address any remaining issues for this feature and get it merged. The proposed PKCS #7 refactoring can be tacked separately. I filed a ticket https://fedorahosted.org/freeipa/ticket/6550 Thanks.

…

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #177 (comment)

HonzaCholasta · 2016-12-12T07:22:50Z

@frasertweedale, I'm afraid we can't do that. As I said in the comment, you cannot unconditionally import from ipaplatform to ipalib anymore, so you either have to make the change to PyASN1, or make the import conditional:

try:
    from ipaplatform.paths import paths
except ImportError:
    OPENSSL = '/usr/bin/openssl'
else:
    OPENSSL = paths.OPENSSL

Add a single function for extracting X.509 certs in PEM format from a PKCS freeipa#7 object. Refactor sites that execute ``openssl pkcs7`` to use the new function. Part of: https://fedorahosted.org/freeipa/ticket/6178

certdb.NSSDatabase.import_files currently accumulates certificates extracted from input files as a string, which is ugly. Accumulate a list of PEMs instead, and join() them just in time for PKCS freeipa#12 creation. Part of: https://fedorahosted.org/freeipa/ticket/6178

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). Add the `--chain' flag for including the chain in the result (chain is also included with `--all'). Add the `--certificate-out' option for writing the certificate to a file (or the chain, if `--chain' was given). Fixes: https://fedorahosted.org/freeipa/ticket/6178

frasertweedale · 2016-12-12T11:04:28Z

@jcholast right you are. PR updated with conditional import.

Thanks.

HonzaCholasta · 2016-12-12T11:58:50Z

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/c7ea56c049ec8ab1a5500852eca6faf750b1479f
https://fedorahosted.org/freeipa/changeset/cc5b88e5d4ac1171374be9ae8e6e60730243dd3d
https://fedorahosted.org/freeipa/changeset/32b1743e5fb318b226a602ec8d9a4b6ef2a25c9d

martbab · 2016-12-12T12:05:50Z

@jcholast @frasertweedale I hope you did notice those failures in Travis CI before acking/pushing...

frasertweedale force-pushed the feature/6178-ca-cert-chain-out branch from 0804716 to 6c938c6 Compare October 22, 2016 04:18

frasertweedale force-pushed the feature/6178-ca-cert-chain-out branch from 6c938c6 to 0706cef Compare November 10, 2016 11:54

tiran reviewed Nov 17, 2016

View reviewed changes

frasertweedale force-pushed the feature/6178-ca-cert-chain-out branch from 0706cef to ea4b283 Compare November 18, 2016 00:14

tkrizek self-assigned this Nov 24, 2016

frasertweedale force-pushed the feature/6178-ca-cert-chain-out branch from ea4b283 to e574e55 Compare November 25, 2016 03:11

frasertweedale force-pushed the feature/6178-ca-cert-chain-out branch from e574e55 to f049751 Compare November 30, 2016 07:47

frasertweedale force-pushed the feature/6178-ca-cert-chain-out branch from f049751 to 104e7f3 Compare November 30, 2016 08:11

frasertweedale force-pushed the feature/6178-ca-cert-chain-out branch from 104e7f3 to 38973fc Compare November 30, 2016 10:46

HonzaCholasta reviewed Dec 7, 2016

View reviewed changes

frasertweedale force-pushed the feature/6178-ca-cert-chain-out branch from 38973fc to fad6ebe Compare December 7, 2016 10:11

frasertweedale force-pushed the feature/6178-ca-cert-chain-out branch from fad6ebe to eb4a4b1 Compare December 8, 2016 08:44

frasertweedale added 2 commits December 12, 2016 21:02

Add function for extracting PEM certs from PKCS freeipa#7

9fc9910

Add a single function for extracting X.509 certs in PEM format from a PKCS freeipa#7 object. Refactor sites that execute ``openssl pkcs7`` to use the new function. Part of: https://fedorahosted.org/freeipa/ticket/6178

frasertweedale force-pushed the feature/6178-ca-cert-chain-out branch from eb4a4b1 to 9364b5d Compare December 12, 2016 11:03

HonzaCholasta added ack Pull Request approved, can be merged pushed Pull Request has already been pushed labels Dec 12, 2016

HonzaCholasta closed this Dec 12, 2016

frasertweedale deleted the feature/6178-ca-cert-chain-out branch December 12, 2016 12:35

Add options to write lightweight CA cert or chain to file #177

Add options to write lightweight CA cert or chain to file #177

frasertweedale commented Oct 21, 2016 •

edited by HonzaCholasta

HonzaCholasta commented Oct 24, 2016

frasertweedale commented Nov 9, 2016

tiran commented Nov 17, 2016

tiran Nov 17, 2016

frasertweedale Nov 18, 2016

HonzaCholasta Dec 7, 2016

tiran Nov 17, 2016

frasertweedale Nov 18, 2016

tiran Nov 17, 2016

frasertweedale Nov 18, 2016

tkrizek commented Nov 24, 2016

frasertweedale commented Nov 25, 2016

HonzaCholasta commented Nov 25, 2016

frasertweedale commented Nov 30, 2016

frasertweedale commented Nov 30, 2016

frasertweedale commented Nov 30, 2016

HonzaCholasta Dec 7, 2016

frasertweedale Dec 7, 2016

HonzaCholasta commented Dec 7, 2016 •

edited

frasertweedale commented Dec 7, 2016 •

edited

HonzaCholasta commented Dec 7, 2016

frasertweedale commented Dec 8, 2016

HonzaCholasta commented Dec 12, 2016

frasertweedale commented Dec 12, 2016 via email

HonzaCholasta commented Dec 12, 2016

frasertweedale commented Dec 12, 2016

HonzaCholasta commented Dec 12, 2016

martbab commented Dec 12, 2016

Add options to write lightweight CA cert or chain to file #177

Add options to write lightweight CA cert or chain to file #177

Conversation

frasertweedale commented Oct 21, 2016 • edited by HonzaCholasta

HonzaCholasta commented Oct 24, 2016

frasertweedale commented Nov 9, 2016

tiran commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkrizek commented Nov 24, 2016

frasertweedale commented Nov 25, 2016

HonzaCholasta commented Nov 25, 2016

frasertweedale commented Nov 30, 2016

frasertweedale commented Nov 30, 2016

frasertweedale commented Nov 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HonzaCholasta commented Dec 7, 2016 • edited

frasertweedale commented Dec 7, 2016 • edited

HonzaCholasta commented Dec 7, 2016

frasertweedale commented Dec 8, 2016

HonzaCholasta commented Dec 12, 2016

frasertweedale commented Dec 12, 2016 via email

HonzaCholasta commented Dec 12, 2016

frasertweedale commented Dec 12, 2016

HonzaCholasta commented Dec 12, 2016

martbab commented Dec 12, 2016

frasertweedale commented Oct 21, 2016 •

edited by HonzaCholasta

HonzaCholasta commented Dec 7, 2016 •

edited

frasertweedale commented Dec 7, 2016 •

edited