[Freeipa-devel] [PATCH 0004-0012] Automatic CSR generation

Wed Aug 10 12:52:29 UTC 2016

The pull request at https://github.com/LiptonB/freeipa/pull/4/commits 
has been brought up to date (with a force push), and also includes 3 
more patches, described below.

The patchset is also attached. To make sure that everything applies, I 
just regenerated the whole set, though there may not be meaningful changes.

On 08/03/2016 09:21 AM, Ben Lipton wrote:
> On 08/01/2016 11:57 PM, Fraser Tweedale wrote:
>> On Fri, Jul 29, 2016 at 11:13:16AM -0400, Ben Lipton wrote:
>>> On 07/29/2016 09:39 AM, Petr Spacek wrote:
>>>> On 27.7.2016 19:06, Ben Lipton wrote:
>>>>> Hi all,
>>>>>
>>>>> I think the automatic CSR generation feature
>>>>> (https://fedorahosted.org/freeipa/ticket/4899,
>>>>> http://www.freeipa.org/page/V4/Automatic_Certificate_Request_Generation) 
>>>>> is
>>>>> stable enough to review now. The following are summaries of the 
>>>>> attached patches:
>>>>> 0004: LDAP schema changes for the new feature
>>>>> 0005: Basic API for new objects and CSR generation
>>>>> 0006: Update install automation to create some default mapping rules
>>>>> 0007: Implement the lookups and text processing that generates the 
>>>>> CSR config
>>>>> 0008 and 0009: Implement some actual transformation rules so that 
>>>>> the feature
>>>>> is usable
>>>>> 0010: Add a new cert profile for user certs, with mappings
>>>>> 0011: Implement import/export of cert profiles with mappings
>>>>> 0012: Tests for profile import/export

0013: Add support for generating a full script that makes a CSR, rather 
than just a config, and use that support to automate the full flow from 
script generation through cert issuance
Usage note: the UI for this could probably use work. I currently have 
the --helper-args param that allows additional data to be passed to the 
helper. Commonly this would be something like:
Certutil: --helper-args '-d /path/to/nss/db' (precreated with certutil 
-N -d /path/to/nss/db)
Openssl: --helper-args 'd /path/to/keyfile' (precreated with openssl 
genrsa -out /path/to/keyfile)
See the commit message for a full command line.

0014: Allow the feature to be used by non-admin users

0015: Improve error handling by reporting a nice message if the mapping 
rules are broken, or if the data required to generate the subject DN is 
missing
>>>>>
>>>>> Generally speaking, later patches depend on earlier ones, but I don't
>>>>> anticipate any problems from committing earlier patches without 
>>>>> later ones.
>>>>>
>>>>> If you prefer, you can also comment on the pull request version:
>>>>> https://github.com/LiptonB/freeipa/pull/4. Note that I may force 
>>>>> push on this
>>>>> branch.
>>>>>
>>>>> Allocation of OIDs for schema change also needs review:
>>>>> https://code.engineering.redhat.com/gerrit/#/c/80061/
>>>>>
>>>>> Known issues:
>>>>> - When the requested principal does not have some of the requested 
>>>>> data,
>>>>> produces funny-looking configs with extra commas, empty sections, 
>>>>> etc. They
>>>>> are still accepted by my copies of openssl and certutil, but they 
>>>>> look ugly.
>>>>> - The new objects don't have any ACIs, so for the moment only 
>>>>> admin can run
>>>>> the new commands.
Fixed by patch 14
>>>>> - Does not yet have support for prompting user for field values, 
>>>>> so currently
>>>>> all data must come from the database.
>>>>> - All processing happens on the server side. As discussed in a 
>>>>> previous
>>>>> thread, it would be desirable to break this out into a library so 
>>>>> it could be
>>>>> used client-side.
>>>>>
>>>>> Very excited to hear your thoughts!
>>>> Hi Ben,
>>>>
>>>> I wanted to give it a try from user's point of view first, before 
>>>> diving into
>>>> implementation details. Here are some observations:
>>> Thanks for giving it a try! This is great feedback.
>>>> 0. Design pages are using term "helper" and it is used even as 
>>>> option in the
>>>> example with smartcards. Please fix either design page or the code 
>>>> so they are
>>>> consistent.
>>> Good point. In a previous discussion, Alexander remarked that --helper
>>> sounded too low-level, but I find that --use sounds very generic and
>>> --format doesn't make a lot of sense for ipa cert-request, which never
>>> actually gives you the config that's generated. So if they're all 
>>> going to
>>> be the same word, which is probably a good idea, I might be leaning 
>>> towards
>>> --helper, but I'm interested to hear opinions on this.
This is called --helper everywhere now.
>>>> 1. The "ipa cert-request" command is missing options --autofill and 
>>>> --use (aka
>>>> helper aka format :-) which are mentioned in the design pages.
>>> Yeah, I haven't managed to implement much of the UI niceness 
>>> suggested by
>>> the design page. I probably should have mentioned that in the email 
>>> - all
>>> that I expect to be working at this point is 'ipa 
>>> cert-get-requestdata' and
>>> CRUD for the mapping rules/profiles themselves.
This is implemented as "ipa cert-build", described above.
>>>> 2. "ipa cert_get_requestdata" command passes even without 
>>>> --profile-id and
>>>> generates empty config. I think that this is not expected :-)
>>> More expected than you might think :) I'm guessing what's happening 
>>> is that
>>> you're passing a user principal and it's defaulting to the 
>>> caIPAserviceCert
>>> profile, then failing to fill out any of the fields because the data it
>>> needs is not there. I agree this isn't great. I was planning to 
>>> address this
>>> by having it throw an error if it can't generate at least the 
>>> subject of the
>>> request, and maybe suggest using a different profile.
>>>
>>> I chose to have it default to caIPAserviceCert because that's what ipa
>>> cert-request does, but maybe that's not as predictable as I thought.
>>>
>> In general use I think that 'caIPAserviceCert' is unlikely to be
>> used a majory of the time, and it is a new command so there are no
>> compatibility issues; therefore why not make the profile option
>> mandatory?
> Fair enough. Ok, a modified patch that changes this (and fixes some 
> label errors I noticed) is attached.
>>
>>>> 3. "ipa cert_get_requestdata --format=openssl" prints the text to 
>>>> stdout
>>>> including label "Configuration file contents:". This is hard to use 
>>>> because
>>>> simple redirection like "ipa cert_get_requestdata --format=openssl 
>>>> > config"
>>>> will not give you valid OpenSSL config file - it needs hand-editing.
>>>>
>>>> It would be good to add --out parameter you envisioned in the 
>>>> design page.
>>>> Please ask jcholast for proper name and semantics :-) With --out 
>>>> option the
>>>> command can be used to generate valid config (or script if certutil 
>>>> was selected).
>>> Agreed. Another example of the UI not being quite right yet. I've been
>>> unsure how to handle this, because of certutil taking a command line 
>>> and
>>> openssl a config file. It actually gets even more complicated 
>>> because, as
>>> you point out in the next item, openssl also needs a command in 
>>> addition to
>>> the config file. I'm interested in thinking about how to handle this 
>>> cleanly
>>> from a user perspective. Generating a script, or providing the 
>>> command lines
>>> as hints, might be ways to get around these concerns.
As of patch 13, the --out parameter writes a script to a local file, so 
you no longer have to manually edit the output, or guess about how to 
invoke certutil/openssl. The scripts take parameters such as where to 
find the private key, though.
>>>> 4. "ipa cert_get_requestdata --format=openssl" could print hint 
>>>> what OpenSSL
>>>> command should be executed on the generated config file. For 
>>>> testing I've used
>>>> command "openssl req -new -out csr -text -config config" (stolen 
>>>> and modified
>>>> from smart card example). Also, as a second hint, it could print 
>>>> the IPA
>>>> command which needs to be used to sign the CSR generated by the 
>>>> helper.
>>> Also agreed, the framework should be able to generate and (for 
>>> purposes of
>>> 'ipa cert-request --autofill') even execute the command required to 
>>> make the
>>> CSR.
Fixed-ish. See previous comment.
>>>
>>>> 5. My naive attempt to get userCert for admin failed:
>>>> $ ipa cert_get_requestdata --format=openssl --principal=admin
>>>> --profile-id=userCert > usercert.conf
>>>> # remove the trailing label
>>>> $ openssl req -new -out csr -text -config usercert.conf
>>>> $ ipa cert-request --principal=admin --profile-id=userCert 
>>>> usercert.csr
>>>> ipa: ERROR: invalid 'csr': No Common Name was found in subject of 
>>>> request.
>>>>
>>>> I'm attaching files generated by the commands above. I did not 
>>>> modify anything
>>>> in the templates or profiles, just tried to use the new profile 
>>>> added by
>>>> freeipa-blipton-0010-Add-a-new-cert-profile-for-users.patch .
>>> Hah! This is what I get for thinking I know what the output has to look
>>> like, and not testing all the way through to requesting the cert. I'll
>>> change the profile to generate a subject with CN= instead of UID=. 
>>> Updated
>>> patch is attached. Unfortunately these rules are only updated at
>>> ipa-server-install time, so if you'd like to fix it without 
>>> reinstalling:
>>>
>> (Tangential commentary...) Yeah, currently cert-request demands the
>> CN.  There is a design to relax the requirement to handle empty
>> subject names (look at SAN only).  IMO it would make sense to accept
>> other "obvious" mappings in Subject DN like accepting UID instead of
>> CN for user subjects, but that would be a separate RFE.  Noone has
>> actually asked for it yet :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-blipton-0004-2-Add-schema-to-support-automatic-CSR-generation.patch
Type: text/x-patch
Size: 6057 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160810/7ac284c9/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-blipton-0005-3-Add-plugin-for-CSR-generation.patch
Type: text/x-patch
Size: 20505 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160810/7ac284c9/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-blipton-0006-2-Add-generation-rules-to-the-default-cert-profile.patch
Type: text/x-patch
Size: 8004 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160810/7ac284c9/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-blipton-0007-2-Add-code-to-support-generating-configs-using-mapping.patch
Type: text/x-patch
Size: 10875 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160810/7ac284c9/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-blipton-0008-2-Add-jinja2-templates-and-macros-to-support-generatin.patch
Type: text/x-patch
Size: 7034 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160810/7ac284c9/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-blipton-0009-2-Add-jinja2-transformation-rules-for-caIPAserviceCert.patch
Type: text/x-patch
Size: 6224 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160810/7ac284c9/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-blipton-0010-3-Add-a-new-cert-profile-for-users.patch
Type: text/x-patch
Size: 9460 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160810/7ac284c9/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-blipton-0011-2-Add-ability-to-import-export-mappings-with-profile.patch
Type: text/x-patch
Size: 15517 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160810/7ac284c9/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-blipton-0012-2-Add-tests-for-mapping-rules-import-export.patch
Type: text/x-patch
Size: 18907 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160810/7ac284c9/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-blipton-0013-2-Automate-full-cert-request-flow.patch
Type: text/x-patch
Size: 11578 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160810/7ac284c9/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-blipton-0014-2-Add-ACIs-for-mapping-rules.patch
Type: text/x-patch
Size: 10693 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160810/7ac284c9/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-blipton-0015-3-Improve-error-handling-for-certificate-mapping.patch
Type: text/x-patch
Size: 5513 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160810/7ac284c9/attachment-0011.bin>