csrgen: Automate full cert request flow #434

LiptonB · 2017-02-04T15:38:02Z

Adds --autogenerate flag to ipa cert-request command. It no longer
requires a CSR passed on the command line, instead it creates a config
(bash script) with cert-get-requestdata, then runs it to build a CSR,
and submits that CSR.

Example usage (NSS database):
$ ipa cert-request --autogenerate --principal blipton --profile-id userCert --database /tmp/certs

Example usage (PEM private key file):
$ ipa cert-request --autogenerate --principal blipton --profile-id userCert --private-key /tmp/key.pem

MartinBasti · 2017-02-07T17:59:27Z

pylint:

************* Module ipaclient.plugins.cert
ipaclient/plugins/cert.py:102: [W1612(unicode-builtin), cert_request.forward] unicode built-in referenced)
ipaclient/plugins/cert.py:127: [W1612(unicode-builtin), cert_request.forward] unicode built-in referenced)
ipaclient/plugins/cert.py:99: [W0612(unused-variable), cert_request.forward] Unused variable 'requestdata')

for unicode you can use etiher six.string_type() or

if six.PY3:
    unicode = str

pep8 errors
failing test expects DN object instead of String

HonzaCholasta · 2017-02-08T09:42:34Z

ipaclient/plugins/cert.py

            yield arg

+    def forward(self, *keys, **options):
+        autogenerate = options.pop('autogenerate', False)


I don't think autogenerate needs to (or should) be a separate option:

def forward(self, csr=None, **options): autogenerate = csr is None ...

HonzaCholasta · 2017-02-08T09:43:42Z

ipaclient/plugins/cert.py

+            'private_key?',
+            label=_('Path to private key file'),
+            doc=_('Path to PEM file containing a private key'),
+        ),


You will most certainly need a password (file) as well for the NSS database or private key.

HonzaCholasta · 2017-02-08T09:45:30Z

ipaclient/plugins/cert.py

+                helper_args = [private_key]
+            else:
+                raise errors.InvocationError(
+                    format="One of 'database' or 'private_key' is required")


You should use message= here.

HonzaCholasta · 2017-02-08T09:47:38Z

ipaclient/plugins/cert.py

+            scriptfile.close()
+            csrfile = tempfile.NamedTemporaryFile(delete=False)
+            csrfile.close()
+            csrfilename = csrfile.name


It would be better to put these into a with statement so that they are deleted automatically after use:

with NamedTemporaryFile() as scriptfile, NamedTemporaryFile() as csrfile: ...

HonzaCholasta · 2017-02-08T09:54:32Z

ipaclient/plugins/cert.py

+            # necessary
+            profile_id = options.get('profile_id')
+            if profile_id is None:
+                profile_id = self.get_default_of('profile_id')


I know I suggested this to you, but I didn't realize it would require hard-coding the default profile name into profile_id in server-side cert_request. I would rather not do that and instead use dogtag.DEFAULT_PROFILE as the default value of profile_id in cert_get_requestdata. I'm aware that this goes against the idea of having the server decide what is the default profile, but cert_get_requestdata will be moved to the server, so this is only temporary.

HonzaCholasta · 2017-02-08T09:56:16Z

ipaclient/plugins/cert.py

+                raise errors.CertificateOperationError(
+                    error=(
+                        _('Error running "%(cmd)s" to generate CSR') %
+                        {'cmd': ' '.join(helper_cmd)}))


It would be nice to at least log the original error, or use it as part of the new error message. Ditto for the IOError below.

HonzaCholasta · 2017-02-08T09:58:48Z

ipaclient/plugins/cert.py

+            return rv
+        else:
+            if database is not None or private_key is not None:
+                raise errors.OptionError(format=_(


This should be a MutuallyExclusiveError.

LiptonB · 2017-02-09T14:02:00Z

Thanks for the comments, and sorry about submitting this with lint errors. I think I've followed all of your suggestions, let me know what you think.

HonzaCholasta · 2017-02-22T13:58:10Z

ipaclient/plugins/cert.py

+                    message=u"One of 'database' or 'private_key' is required")
+
+            with tempfile.NamedTemporaryFile(
+                    ) as scriptfile, tempfile.NamedTemporaryFile() as csrfile:


Nitpick: if you from tempfile import NamedTemporaryFile, the you might be able to fit this into a single line.

Had to shorten the name more to keep the descriptive variable names, is this ok?

HonzaCholasta · 2017-02-22T14:03:03Z

ipaclient/plugins/cert.py

+                raise errors.MutuallyExclusiveError(reason=_(
+                    "Options 'database' and 'private_key' are not compatible"
+                    " with 'csr'"))
+            return super(cert_request, self).forward(csr, **options)


Nitpick: if you move this call one intendation level to the left, you will be able to remove the same call above.

HonzaCholasta · 2017-02-22T14:10:06Z

Thank you. LGTM, but please squash the fixup commit.

Allows the `ipa cert-request` command to generate its own CSR. It no longer requires a CSR passed on the command line, instead it creates a config (bash script) with `cert-get-requestdata`, then runs it to build a CSR, and submits that CSR. Example usage (NSS database): $ ipa cert-request --principal host/test.example.com --profile-id caIPAserviceCert --database /tmp/certs Example usage (PEM private key file): $ ipa cert-request --principal host/test.example.com --profile-id caIPAserviceCert --private-key /tmp/key.pem https://fedorahosted.org/freeipa/ticket/4899

In case users want multiple CSR generation profiles that work with the same dogtag profile, or in case the profiles are not named the same, this flag allows specifying an alternative CSR generation profile. https://fedorahosted.org/freeipa/ticket/4899

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

LiptonB · 2017-02-28T03:29:01Z

@HonzaCholasta thanks, updated!

HonzaCholasta · 2017-02-28T08:56:52Z

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/39a5d9c5aae77687f67d9be02457733bdfb99ead
https://fedorahosted.org/freeipa/changeset/4350dcdea22fd2284836315d0ae7d38733a7620e
https://fedorahosted.org/freeipa/changeset/ada91c20588046bb147fc701718d3da4d2c080ca

HonzaCholasta suggested changes Feb 8, 2017

View reviewed changes

LiptonB force-pushed the local-cert-build branch from 5e1ab64 to 79786f2 Compare February 9, 2017 02:53

HonzaCholasta reviewed Feb 22, 2017

View reviewed changes

LiptonB added 3 commits February 27, 2017 21:30

csrgen: Support encrypted private keys

83f4e89

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

LiptonB force-pushed the local-cert-build branch from 5f34122 to 83f4e89 Compare February 28, 2017 02:36

HonzaCholasta added the ack Pull Request approved, can be merged label Feb 28, 2017

HonzaCholasta approved these changes Feb 28, 2017

View reviewed changes

HonzaCholasta added the pushed Pull Request has already been pushed label Feb 28, 2017

HonzaCholasta closed this Feb 28, 2017

csrgen: Automate full cert request flow #434

csrgen: Automate full cert request flow #434

LiptonB commented Feb 4, 2017 •

edited by HonzaCholasta

MartinBasti commented Feb 7, 2017

HonzaCholasta Feb 8, 2017

HonzaCholasta Feb 8, 2017

HonzaCholasta Feb 8, 2017

HonzaCholasta Feb 8, 2017

HonzaCholasta Feb 8, 2017

HonzaCholasta Feb 8, 2017

HonzaCholasta Feb 8, 2017

LiptonB commented Feb 9, 2017

HonzaCholasta Feb 22, 2017 •

edited

LiptonB Feb 28, 2017

HonzaCholasta Feb 22, 2017 •

edited

HonzaCholasta commented Feb 22, 2017

LiptonB commented Feb 28, 2017

HonzaCholasta commented Feb 28, 2017

csrgen: Automate full cert request flow #434

csrgen: Automate full cert request flow #434

Conversation

LiptonB commented Feb 4, 2017 • edited by HonzaCholasta

MartinBasti commented Feb 7, 2017

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 commented Feb 9, 2017

HonzaCholasta Feb 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HonzaCholasta Feb 22, 2017 • edited

Choose a reason for hiding this comment

HonzaCholasta commented Feb 22, 2017

LiptonB commented Feb 28, 2017

HonzaCholasta commented Feb 28, 2017

LiptonB commented Feb 4, 2017 •

edited by HonzaCholasta

HonzaCholasta Feb 22, 2017 •

edited

HonzaCholasta Feb 22, 2017 •

edited