Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementation independent interface for CSR generation #542

Closed
wants to merge 4 commits into from

Conversation

LiptonB
Copy link
Contributor

@LiptonB LiptonB commented Mar 6, 2017

@HonzaCholasta and everyone, here is where I am so far on the CertificationRequestInfo-based interface for CSR generation.

As I see it, there are a few rough edges still, so I'd like to get your opinion, especially about these things:

  • For feeding to build_requestinfo we want a config file, not a script, so I needed to add another formatter/helper that omits the bash code that's there for other helpers.
  • While openssl has a library function for creating cert extensions from the config file format, the logic for creating the subject name from the config format is implemented within the openssl req command rather than the library. In build_requestinfo I copied the code from certmonger that creates the subject name, which takes a simpler format. So the new formatter is called "certmonger" and uses that format.
  • I'm not sure where in the freeipa project the code for build_requestinfo should go, how to work it into the build process, and where it should be installed. Right now I just have a TODO to do so. Or did you mean for that code to be run via CFFI as well?

@HonzaCholasta
Copy link
Contributor

  • Maybe I'm missing something, but the intent behind the CertificationRequestInfo-based interface was to replace all of the different helpers with a single way of generating CSRs, so it seems a bit strange to me that you are adding another helper for it.
  • I would rather avoid creating new, similar but slightly incompatible configuration format. If you can copy code from certmonger, you can copy code from openssl req too, no?
  • The idea was indeed to implement this in Python using CFFI to call into OpenSSL library functions.

@LiptonB
Copy link
Contributor Author

LiptonB commented Mar 7, 2017

Thanks for the feedback. I will put together a new version using CFFI and the openssl req format for subject names.

Regarding helpers, this code has all CSR generation go through the CertificationRequestInfo-based flow, so the other helpers can't actually be accessed. Maybe we should remove the helper/formatter abstraction entirely, and have the new format (raw openssl config) be the only jinja template available. This makes things simpler but will remove all support for NSS databases until we add it to the new flow. What do you think? (An alternative would be to remove only the openssl helper, and add a CertificationRequestInfoFormatter in its place).

@HonzaCholasta
Copy link
Contributor

I would rather make things simple and remove the abstraction.

We can support NSS databases by PKCS#12 export/import until we have first-class support:

  1. generate private key and temporary cert in the NSS database:
    certutil -S ...
  2. export the private key from the NSS database into a temporary PKCS#12 file:
    pk12util -o key.p12 ...
  3. delete the temporary cert from the NSS database:
    certutil -D ...
  4. extract the private key from the temporary PKCS#12 file into a temporary PKCS#8 file:
    openssl pkcs12 -in key.p12 -nocerts -out key.pem ...
  5. delete the temporary PKCS#12 file
  6. request a certificate using the OpenSSL workflow on the temporary PKCS#8 file
  7. import the certificate into the NSS database

Granted, this won't work with HSMs, but I think that's OK, given it is only a temporary solution.

@tiran
Copy link
Member

tiran commented Mar 14, 2017

@LiptonB needs rebase

@LiptonB
Copy link
Contributor Author

LiptonB commented Mar 15, 2017

Regarding this comment from @MartinBasti in #590:

For the record, #542 removes the helper parameter of cert-get-requestdata, and will be modified to remove the concept of different helpers entirely, though I haven't had a chance to make that change yet.

today is 4.5 release so you have to keep some level of backward compatibility in that PR

What level of backward compatibility is required? Is it not ok to remove helpers? I thought the purpose of making cert-get-requestdata an internal, client-side API was that it would be ok to change the parameters as we figured out how it should work.

@MartinBasti
Copy link
Contributor

I meant this:

-        Str(
-            'helper',
-            label=_('Name of CSR generation tool'),
-            doc=_('Name of tool (e.g. openssl, certutil) that will be used to'
-                  ' create CSR'),

AFAIK this is user API

@HonzaCholasta
Copy link
Contributor

@MartinBasti, it is an internal, user invisible API. @LiptonB, it is OK to change it.

All requests now use the OpenSSL formatter. However, we keep Formatter
a separate class so that it can be changed out for tests.

https://pagure.io/freeipa/issue/4899
Also modify cert_request to use this new format. Note, only PEM private
keys are supported for now. NSS databases are not.

https://pagure.io/freeipa/issue/4899
@LiptonB
Copy link
Contributor Author

LiptonB commented Mar 22, 2017

Thanks for the clarification, @HonzaCholasta. (And for the timely intervention in #579 to make it actually invisible).

A new version is pushed, which uses CFFI and the unmodified openssl config format, and removes the helper abstraction as requested. NSS support is still broken for now, I haven't had a chance to look into the change you suggested. Let me know what you think otherwise.

@HonzaCholasta
Copy link
Contributor

@LiptonB, superb, thank you!

Have you made any progress with NSS support? If not, I can add it in a subsequent PR, if you agree.

@HonzaCholasta HonzaCholasta added the ack Pull Request approved, can be merged label Apr 3, 2017
@HonzaCholasta
Copy link
Contributor

master:

  • 5420e9c csrgen: Remove helper abstraction
  • 136c6c3 csrgen: Change to pure openssl config format (no script)
  • e7588ab csrgen: Modify cert_get_requestdata to return a CertificationRequestInfo
  • a53e178 csrgen: Beginnings of NSS database support

@HonzaCholasta HonzaCholasta added the pushed Pull Request has already been pushed label Apr 3, 2017
@LiptonB
Copy link
Contributor Author

LiptonB commented Apr 3, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
4 participants