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

Tue Aug 2 03:57:46 UTC 2016

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

> > 
> > 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.
> > 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.
>
> > 
> > 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 :)

Cheers,
Fraser

> ipa certtransformationrule-mod dataUsernameCN dataUsernameCertutil
> --template 'CN={{ipa.datafield(subject.uid.0)|quote}},{{ipa.datafield(config.ipacertificatesubjectbase.0)|quote}}'
> ipa certtransformationrule-mod dataUsernameCN dataUsernameOpenssl --template
> '{{ipa.datafield(config.ipacertificatesubjectbase.0)}}
> CN={{ipa.datafield(subject.uid.0)}}'
> Yes, that must be an actual linebreak between the subjectbase and the CN in
> the openssl one. I know :-/
> 
> > 
> > Hopefully I will get to other things soon (but not this week).
> > 
> > 
> > Anyway, all this seems like (expected) initial problems. In general the first
> > touch with user interface seems reasonable and needs only small improvements.
> > 
> > Good work!
> >