Client-side CSR autogeneration #10

LiptonB · 2016-08-22T17:19:08Z

Adds a library that builds scripts that builds CSRs. Adds a CLI command, 'cert-get-requestdata', that uses this library and builds the script for a given principal. Adds rules for the caIPAserviceCert profile, as well as a new userCert profile, stored in json files in /usr/share/ipa/csr. The rule provider is a separate class so that it can be replaced easily if we ever want to move rules to the server side.

https://fedorahosted.org/freeipa/ticket/4899

abbra · 2016-08-26T15:27:58Z

freeipa.spec.in

@@ -503,6 +503,7 @@ Requires: python-custodia
 Requires: python-dns >= 1.11.1
 Requires: python-netifaces >= 0.10.4
 Requires: pyusb
+Requires: python-jinja2


What about BuildRequires? pylint would be failing on the system where python-jinja2 is not installed.

I had that before, but removed it when I couldn't get pylint to actually complain about the missing import. But maybe that was just because "import-error" is disabled (twice?) in pylintrc. Anyway, added as a fixup commit.

Edit: force pushed to fix conflicts with master

LiptonB · 2016-08-31T15:58:28Z

As discussed elsewhere, this script generation is a fairly low-level operation; you have to specify the helper and know how to run the script. Most users will probably want a command that just takes in a private key location and a profile and requests the cert for them. In case you'd like to look at it now, I have an implementation of that in a separate branch, which I'll create a pull request for after this one is complete: master...LiptonB:local-cert-build

LiptonB · 2016-09-06T21:21:16Z

I've added a commit (Use data_sources option to define which fields are rendered) that simplifies the way we avoid rendering rules whose source data are missing, as discussed here: https://www.redhat.com/archives/freeipa-devel/2016-September/msg00051.html. I prefer this approach to the macros in the original implementation, but I'm leaving it as a separate commit in case you would like to compare them.

LiptonB · 2016-09-14T22:17:03Z

Some tests for the CSR generation functionality have been added to the pull request.

HonzaCholasta · 2016-09-15T05:26:16Z

ipaclient/plugins/certmapping.py

+        )
+    )
+
+    def forward(self, *args, **options):


This should be execute, the command is not forwarded to the server.

I used forward because the default run method only calls execute if you're running in the server context. Overriding forward is what other client-only commands like vault-add do. Should I override run instead?

I forgot to mention that you should also inherit from Local. (Sorry.)

Ah, I had tried that before but abandoned it because the rpc client wasn't connected automatically with Local. But now I see that vault-add connects it explicitly, so I've updated my code to do the same.

HonzaCholasta · 2016-09-15T05:31:13Z

ipalib/certmapping.py

+stored mapping rules.
+""")
+
+CSR_DATA_DIR = '/usr/share/ipa/csr'


File paths should be defined in ipaplatform.paths.

HonzaCholasta · 2016-09-15T05:32:41Z

ipalib/certmapping.py

+
+from ipalib import errors
+from ipalib.text import _
+from ipapython.ipa_log_manager import root_logger


Do not log into the root logger, use a module-specific logger:

from ipapython.ipa_log_manager import log_mgr logger = log_mgr.get_logger(__name__)

Thank you! I thought this might not be right but I didn't know what the right way was.

Right, this is not used often in our code, but we try to follow common best practices in new code, and the best practice for logging seems to be to use module-specific loggers.

HonzaCholasta · 2016-09-15T05:36:21Z

ipalib/certmapping.py

+import collections
+import jinja2
+import jinja2.ext
+import jinja2.sandbox


The order of imports should be:

standard

3rd party

ipa

Done. It seems to be a convention that six is the last import so I've left it that way. Is that right?

No, six should be imported together with other 3rd party imports.

Ok, moved six to the proper place.

HonzaCholasta · 2016-09-15T05:55:23Z

ipalib/certmapping.py

+
+            try:
+                rule = [r for r in ruleset['rules']
+                        if r['helper'] == helper][0]


The [0] does not look right. Either use the complete list, or fail if there is more than one item.

There should only be one, so I'll raise an error otherwise.

HonzaCholasta · 2016-09-15T05:58:40Z

ipalib/certmapping.py

+            except IOError:
+                raise errors.CertificateMappingError(
+                    reason=_('Ruleset %(ruleset)s is configured in profile but'
+                             ' does not exist.') % {'ruleset': rule_name})


This looks rather like errors.NotFound.

Also, this method has no knowledge of profiles, so how do you know the ruleset is configured in profile?

Good points.

HonzaCholasta · 2016-09-15T06:00:51Z

ipalib/certmapping.py

+                raise errors.CertificateMappingError(
+                    reason=_('No transformation in "%(ruleset)s" rule supports'
+                             ' helper "%(helper)s"') %
+                    {'ruleset': rule_name, 'helper': helper})


This looks rather like errors.EmptyResult.

HonzaCholasta · 2016-09-15T06:01:11Z

ipalib/certmapping.py

+        except IOError:
+            raise errors.CertificateMappingError(
+                reason=_('No certificate mappings are defined for profile'
+                         ' %(profile_id)s') % {'profile_id': profile_id})


This also looks rather like errors.NotFound.

HonzaCholasta · 2016-09-15T06:03:12Z

ipalib/errors.py

@@ -1697,6 +1697,15 @@ class CertificateFormatError(CertificateError):
    format = _('Certificate format error: %(error)s')


+class CertificateMappingError(CertificateError):


Could we do without adding yet another god exception? See also my other exception related comments.

You're right, the meaning of this exception drifted a lot. After making the changes you suggest above, the remaining cases are:

A mapping rule contains more than one stanza that matches the given helper. This is very close to SingleMatchExpected, but the format string for that exception doesn't really make sense for this case.

The jinja2 render failed (probably means the template was written incorrectly). I can't find any errors that seem appropriate for this at all.

Nothing was generated for a rule that's marked as "required". RequirementError looks almost right, but the error message ("ERROR: 'syntaxSubject' is required") could be confusing. Admittedly, the real problem with this error message isn't just that it's generic but that it doesn't tell you what the actual missing data are, and that's trickier to solve.

Do you have any advice for what to do about these remaining cases?

You can subclass SingleMatchExpected with your own format string.

I would use a new ExecutionError subclass for this.

RequirementError is not a good choice, as it is a validation error, but what you are describing is an execution error.

What do you think of this version (0e999cf)? I subclassed SingleMatchExpected for the case of multiple matches for the helper, and merged the other two into CSRTemplateError with different descriptions.

HonzaCholasta · 2016-09-15T06:12:00Z

In addition to my inline comments above:

"Certificate mapping" does not really evoke "certificate request templating" to me, and is also used in the context of mapping identities to certificates. Could we use a more suitable name to avoid confusion?
The ipalib.certmapping module is used only in ipaclient, so that's where it should be located. It can be moved to ipalib later if necessary.
I don't think IPAExtension deserves it's own module, at least not now.

LiptonB

Thanks for the comments! I've fixed the simple ones and replied to the rest. Regarding your comments about file organization:

I quite agree that certmapping isn't a good name for what this turned out to be. With the convention of naming modules after the objects they model, perhaps a good name would be certrequest or csr? The command could be renamed to something like certrequest-get-data (or certrequest-get-script).
Just to confirm, you're suggesting just moving these classes to the ipaclient.plugins.<whatever we end up calling it> module?
Seems reasonable, I've moved it into the ipalib module for now. It will go wherever the contents of that module end up.

Logistical stuff:

Now that this is under review I won't add any more content. Are you ok with the two commits about testing being part of this review or should I remove them?
If you run rebase --autosquash with the latest commit it doesn't actually apply cleanly, but I'm trying not to change history while it's being reviewed, so I'll do the rebase later on if that's ok?

LiptonB · 2016-09-15T11:32:02Z

ipaclient/plugins/certmapping.py

+        )
+    )
+
+    def forward(self, *args, **options):


I used forward because the default run method only calls execute if you're running in the server context. Overriding forward is what other client-only commands like vault-add do. Should I override run instead?

LiptonB · 2016-09-15T18:43:47Z

ipalib/certmapping.py

+import collections
+import jinja2
+import jinja2.ext
+import jinja2.sandbox


Done. It seems to be a convention that six is the last import so I've left it that way. Is that right?

LiptonB · 2016-09-15T18:47:57Z

ipalib/certmapping.py

+
+from ipalib import errors
+from ipalib.text import _
+from ipapython.ipa_log_manager import root_logger


Thank you! I thought this might not be right but I didn't know what the right way was.

LiptonB · 2016-09-15T18:55:04Z

ipalib/certmapping.py

+stored mapping rules.
+""")
+
+CSR_DATA_DIR = '/usr/share/ipa/csr'


LiptonB · 2016-09-15T18:59:01Z

ipalib/certmapping.py

+            except IOError:
+                raise errors.CertificateMappingError(
+                    reason=_('Ruleset %(ruleset)s is configured in profile but'
+                             ' does not exist.') % {'ruleset': rule_name})


Good points.

LiptonB · 2016-09-15T19:08:38Z

ipalib/certmapping.py

+
+            try:
+                rule = [r for r in ruleset['rules']
+                        if r['helper'] == helper][0]


There should only be one, so I'll raise an error otherwise.

LiptonB · 2016-09-15T19:33:53Z

ipalib/errors.py

@@ -1697,6 +1697,15 @@ class CertificateFormatError(CertificateError):
    format = _('Certificate format error: %(error)s')


+class CertificateMappingError(CertificateError):


You're right, the meaning of this exception drifted a lot. After making the changes you suggest above, the remaining cases are:

A mapping rule contains more than one stanza that matches the given helper. This is very close to SingleMatchExpected, but the format string for that exception doesn't really make sense for this case.

The jinja2 render failed (probably means the template was written incorrectly). I can't find any errors that seem appropriate for this at all.

Nothing was generated for a rule that's marked as "required". RequirementError looks almost right, but the error message ("ERROR: 'syntaxSubject' is required") could be confusing. Admittedly, the real problem with this error message isn't just that it's generic but that it doesn't tell you what the actual missing data are, and that's trickier to solve.

Do you have any advice for what to do about these remaining cases?

HonzaCholasta · 2016-09-19T07:56:05Z

I'm afraid certrequest (actually certreq) is already taken. What about csrgen?
I would be perfectly happy with ipaclient.<whatever>.
OK.

Logistical stuff:

I'm fine with the testing commits being part of this review. I would also be fine with new content if it was added after this part of review is done.
I'm fine with later rebase.

LiptonB · 2016-09-19T12:53:12Z

csrgen sounds good to me. The new modules have now been moved to ipaclient.plugins.csrgen, ipaclient.csrgen, and ipatests.test_ipaclient.test_csrgen.

FYI: I force pushed a few minutes after adding these commits to fix a pep8 error.

LiptonB · 2016-10-03T15:01:53Z

@jcholast, when you get a chance, could you take another look at this and let me know what else is needed?

HonzaCholasta · 2016-10-10T06:12:44Z

freeipa.spec.in

+%dir %{_usr}/share/ipa/csr/profiles
+%{_usr}/share/ipa/csr/profiles/*.json
+%dir %{_usr}/share/ipa/csr/rules
+%{_usr}/share/ipa/csr/rules/*.json


Now that we call the feature csrgen, I guess we should put the files into /usr/share/ipa/csrgen rather than /usr/share/ipa/csr.

Ah, true. Done.

HonzaCholasta · 2016-10-10T06:32:17Z

@LiptonB, I have added some inline comments.

Also, have you seen my latest reply in the design thread?

LiptonB · 2016-10-10T16:48:13Z

Thanks, I've updated the code based on your comments (force pushed to fix conflicts with master). And thanks for pointing out that email! I don't know how I missed it, but I will respond shortly.

In the dogtag-ipa-ca-renew-agent-submit certmonger renewal helper, we currently use our hand-rolled PKCS freeipa#10 pyasn1 specification to parse the friendlyName out of CSRs generated by certmonger (it contains the NSSDB nickname of the cert). Use other information from the renewal helper process environment to determine the nickname and remove our PKCS freeipa#10 pyasn1 spec. Part of: https://fedorahosted.org/freeipa/ticket/6398

LiptonB · 2016-10-20T20:21:35Z

Updated to fix conflicts with master again. I'm not sure what's up with Travis, it seems to be checking out PR #109 instead of this one for the pep8 check:

$ cd freeipa/freeipa
$ git fetch origin +refs/pull/109/merge:
remote: Counting objects: 78053, done.
remote: Compressing objects: 100% (14671/14671), done.
remote: Total 78053 (delta 64375), reused 76890 (delta 63219), pack-reused 0
Receiving objects: 100% (78053/78053), 28.18 MiB | 18.03 MiB/s, done.
Resolving deltas: 100% (64375/64375), completed with 841 local objects.
From https://github.com/freeipa/freeipa
 * branch            refs/pull/109/merge -> FETCH_HEAD
$ git checkout -qf FETCH_HEAD

In the dogtag-ipa-ca-renew-agent-submit certmonger renewal helper, we currently use our hand-rolled PKCS freeipa#10 pyasn1 specification to parse the friendlyName out of CSRs generated by certmonger (it contains the NSSDB nickname of the cert). Use other information from the renewal helper process environment to determine the nickname and remove our PKCS freeipa#10 pyasn1 spec. Part of: https://fedorahosted.org/freeipa/ticket/6398

In the dogtag-ipa-ca-renew-agent-submit certmonger renewal helper, we currently use our hand-rolled PKCS #10 pyasn1 specification to parse the friendlyName out of CSRs generated by certmonger (it contains the NSSDB nickname of the cert). Use other information from the renewal helper process environment to determine the nickname and remove our PKCS #10 pyasn1 spec. Part of: https://fedorahosted.org/freeipa/ticket/6398 Reviewed-By: Jan Cholasta <jcholast@redhat.com> Reviewed-By: Florence Blanc-Renaud <frenaud@redhat.com>

Adds a library that uses jinja2 to format a script that, when run, will build a CSR. Also adds a CLI command, 'cert-get-requestdata', that uses this library and builds the script for a given principal. The rules are read from json files in /usr/share/ipa/csr, but the rule provider is a separate class so that it can be replaced easily. https://fedorahosted.org/freeipa/ticket/4899

https://fedorahosted.org/freeipa/ticket/4899

This removes the ipa.syntaxrule and ipa.datarule macros in favor of simple 'if' statements based on the data referenced in the rules. The 'if' statement for a syntax rule is generated based on the data rules it contains. The Subject DN should not be generated unless all data rules are in place, so the ability to override the logical operator that combines data_sources (from 'or' to 'and') is added.

Some tests are failing and will be fixed in the next commit. This patch also contains some code changes to make the code easier to test.

abbra reviewed Aug 26, 2016
View reviewed changes

LiptonB force-pushed the local-mapping-review branch from fa15aeb to 207eb51 Compare August 26, 2016 22:20

HonzaCholasta reviewed Sep 15, 2016

View reviewed changes

LiptonB commented Sep 15, 2016

View reviewed changes

LiptonB force-pushed the local-mapping-review branch from 05ce914 to b1e0968 Compare September 19, 2016 12:47

HonzaCholasta reviewed Oct 10, 2016

View reviewed changes

LiptonB force-pushed the local-mapping-review branch from 046c6b5 to f03a945 Compare October 10, 2016 16:47

LiptonB force-pushed the local-mapping-review branch from f03a945 to 054179c Compare October 20, 2016 19:57

frasertweedale mentioned this pull request Nov 10, 2016

change certificate processing code to use python-cryptography #217

Closed

LiptonB added 21 commits December 13, 2016 17:41

Add certificate mappings for caIPAserviceCert

6240e0e

https://fedorahosted.org/freeipa/ticket/4899

Add a new cert profile for users

9fb8430

https://fedorahosted.org/freeipa/ticket/4899

fixup! Add code to generate scripts that generate CSRs

9f25ebc

fixup! Add code to generate scripts that generate CSRs

31f671e

fixup! Add code to generate scripts that generate CSRs

f9da65e

fixup! Use data_sources option to define which fields are rendered

ddf609b

Add tests for CSR autogeneration

d8ab019

Some tests are failing and will be fixed in the next commit. This patch also contains some code changes to make the code easier to test.

Fix broken tests in CSR autogeneration

2555074

fixup! Add code to generate scripts that generate CSRs

7ebfce5

fixup! Add code to generate scripts that generate CSRs

d270441

fixup! Add code to generate scripts that generate CSRs

645273b

fixup! Add tests for CSR autogeneration

634f31c

fixup! Add code to generate scripts that generate CSRs

6233297

fixup! Add certificate mappings for caIPAserviceCert

9983684

fixup! Add a new cert profile for users

fcc0fa0

fixup! Use data_sources option to define which fields are rendered

23192ee

fixup! Add code to generate scripts that generate CSRs

163d62f

fixup! Add code to generate scripts that generate CSRs

1eaf0d6

fixup! Add tests for CSR autogeneration

fd40f55

LiptonB force-pushed the local-mapping-review branch from 054179c to fd40f55 Compare December 13, 2016 22:57

LiptonB mentioned this pull request Dec 14, 2016

Client-side CSR autogeneration (take 2) #337

Closed

LiptonB closed this Jan 24, 2017

frasertweedale mentioned this pull request Feb 21, 2017

Hide request_type doc string in cert-request help #480

Closed

		@@ -1697,6 +1697,15 @@ class CertificateFormatError(CertificateError):
		format = _('Certificate format error: %(error)s')


		class CertificateMappingError(CertificateError):

Client-side CSR autogeneration #10

Client-side CSR autogeneration #10

Conversation

LiptonB commented Aug 22, 2016 • edited

Choose a reason for hiding this comment

LiptonB Aug 26, 2016 • edited

Choose a reason for hiding this comment

LiptonB commented Aug 31, 2016

LiptonB commented Sep 6, 2016

LiptonB commented Sep 14, 2016

HonzaCholasta Sep 15, 2016 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LiptonB Sep 15, 2016 • edited

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

HonzaCholasta Sep 15, 2016 • edited

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

LiptonB Oct 10, 2016 • edited

Choose a reason for hiding this comment

HonzaCholasta commented Sep 15, 2016

LiptonB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LiptonB Sep 15, 2016 • edited

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

HonzaCholasta commented Sep 19, 2016

LiptonB commented Sep 19, 2016

LiptonB commented Oct 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HonzaCholasta commented Oct 10, 2016

LiptonB commented Oct 10, 2016

LiptonB commented Oct 20, 2016

LiptonB commented Aug 22, 2016 •

edited

LiptonB Aug 26, 2016 •

edited

HonzaCholasta Sep 15, 2016 •

edited

LiptonB Sep 15, 2016 •

edited

HonzaCholasta Sep 15, 2016 •

edited

LiptonB Oct 10, 2016 •

edited

LiptonB Sep 15, 2016 •

edited