[Freeipa-devel] [freeipa PR#542][opened] Implementation independent interface for CSR generation

[LiptonB]

URL: https://github.com/freeipa/freeipa/pull/542
Author: LiptonB
Title: #542: Implementation independent interface for CSR generation
Action: opened

PR body:
"""
@HonzaCholasta and everyone, here is where I am so far on the [CertificationRequestInfo-based interface for CSR generation](https://www.redhat.com/archives/freeipa-devel/2017-February/msg00104.html).

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](https://pagure.io/certmonger/blob/master/f/src/csrgen-o.c#_193-223) 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?
"""

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/542/head:pr542
git checkout pr542

[LiptonB]

URL: https://github.com/freeipa/freeipa/pull/542
Author: LiptonB
Title: #542: Implementation independent interface for CSR generation
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/542/head:pr542
git checkout pr542

[HonzaCholasta]

URL: https://github.com/freeipa/freeipa/pull/542
Title: #542: Implementation independent interface for CSR generation

HonzaCholasta commented:
"""
* 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.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/542#issuecomment-284647475

[LiptonB]

URL: https://github.com/freeipa/freeipa/pull/542
Title: #542: Implementation independent interface for CSR generation

LiptonB commented:
"""
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).
"""

See the full comment at https://github.com/freeipa/freeipa/pull/542#issuecomment-284727415

[HonzaCholasta]

URL: https://github.com/freeipa/freeipa/pull/542
Title: #542: Implementation independent interface for CSR generation

HonzaCholasta commented:
"""
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.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/542#issuecomment-284995622

[tiran]

URL: https://github.com/freeipa/freeipa/pull/542
Title: #542: Implementation independent interface for CSR generation

tiran commented:
"""
@LiptonB needs rebase
"""

See the full comment at https://github.com/freeipa/freeipa/pull/542#issuecomment-286452115

[LiptonB]

URL: https://github.com/freeipa/freeipa/pull/542
Title: #542: Implementation independent interface for CSR generation

LiptonB commented:
"""

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.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/542#issuecomment-286819264

[MartinBasti]

URL: https://github.com/freeipa/freeipa/pull/542
Title: #542: Implementation independent interface for CSR generation

MartinBasti commented:
"""
I meant this:
```diff
- 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
"""

See the full comment at https://github.com/freeipa/freeipa/pull/542#issuecomment-286829945

[HonzaCholasta]

URL: https://github.com/freeipa/freeipa/pull/542
Title: #542: Implementation independent interface for CSR generation

HonzaCholasta commented:
"""
@MartinBasti, it is an internal, user invisible API. @LiptonB, it is OK to change it.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/542#issuecomment-286966192

[LiptonB]

URL: https://github.com/freeipa/freeipa/pull/542
Author: LiptonB
Title: #542: Implementation independent interface for CSR generation
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/542/head:pr542
git checkout pr542

[LiptonB]

URL: https://github.com/freeipa/freeipa/pull/542
Title: #542: Implementation independent interface for CSR generation

LiptonB commented:
"""
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.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/542#issuecomment-288569409

[LiptonB]

URL: https://github.com/freeipa/freeipa/pull/542
Title: #542: Implementation independent interface for CSR generation

LiptonB commented:
"""
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.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/542#issuecomment-288569409

[HonzaCholasta]

URL: https://github.com/freeipa/freeipa/pull/542
Title: #542: Implementation independent interface for CSR generation

HonzaCholasta commented:
"""
@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.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/542#issuecomment-291070970

[HonzaCholasta]

URL: https://github.com/freeipa/freeipa/pull/542
Title: #542: Implementation independent interface for CSR generation

Label: +ack

[HonzaCholasta]

URL: https://github.com/freeipa/freeipa/pull/542
Title: #542: Implementation independent interface for CSR generation

HonzaCholasta commented:
"""
master:

* 5420e9cfbe7803808b6e26d2dae64f2a6a50149a csrgen: Remove helper abstraction
* 136c6c3e2a4f77a27f435efd4a1cd95c9e089314 csrgen: Change to pure openssl config format (no script)
* e7588ab2dc73e7f66ebc6cdcfb99470540e37731 csrgen: Modify cert_get_requestdata to return a CertificationRequestInfo
* a53e17830c3d4fd59a62248d4447491675c6a80e csrgen: Beginnings of NSS database support


"""

See the full comment at https://github.com/freeipa/freeipa/pull/542#issuecomment-291071297

[HonzaCholasta]

URL: https://github.com/freeipa/freeipa/pull/542
Title: #542: Implementation independent interface for CSR generation

Label: +pushed

[HonzaCholasta]

URL: https://github.com/freeipa/freeipa/pull/542
Author: LiptonB
Title: #542: Implementation independent interface for CSR generation
Action: closed

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/542/head:pr542
git checkout pr542

[LiptonB]

URL: https://github.com/freeipa/freeipa/pull/542
Title: #542: Implementation independent interface for CSR generation

LiptonB commented:
"""
@HonzaCholasta, thanks! I have an attempt at NSS support in progress. It
might take me a few more days to get it ready to send out, but I think it's
close.

@LiptonB <https://github.com/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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/freeipa/freeipa/pull/542#issuecomment-291070970>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAcj_faOJ9GWrYO3aT4sxV7gx6_fBfUIks5rsKOggaJpZM4MUefo>
.

"""

See the full comment at https://github.com/freeipa/freeipa/pull/542#issuecomment-291172628