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

Client-side CSR autogeneration (take 2) #337

Closed
wants to merge 5 commits into from

Conversation

LiptonB
Copy link
Contributor

@LiptonB LiptonB commented Dec 14, 2016

Clone of PR #10 to see if CI picks up the right commits this time

@martbab
Copy link
Contributor

martbab commented Dec 14, 2016

From Travis CI logs it looks like a correct branch was fetched this time.

@@ -147,6 +147,7 @@ BuildRequires: python-sssdconfig
BuildRequires: python-nose
BuildRequires: python-paste
BuildRequires: systemd-python
BuildRequires: python2-jinja2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you should also add python3-jinja2 build dependency below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I also realized that I had the jinja2 dependency on the ipalib package, but it's actually the ipaclient package that needs it now. So I added it to both the python2 and python3 ipaclient packages.

@tiran
Copy link
Member

tiran commented Jan 18, 2017

@LiptonB thanks a lot for resuming your work!

Please add jinja2 to ipaclient/setup.py, too.

@LiptonB
Copy link
Contributor Author

LiptonB commented Jan 19, 2017

@tiran Thanks to the team for resuming the review, too! Added the dependency, does that look right?

@tiran
Copy link
Member

tiran commented Jan 19, 2017

@LiptonB yes, it's correct.

@HonzaCholasta
Copy link
Contributor

@LiptonB, there's still one issue which I'd like to be resolved in this PR, and that's that currently CSR templates are tied to certificate profiles. IMO this needs to be changed, as certificate profiles in IPA are Dogtag-specific, but Dogtag is not required to generate CSRs with this feature, and it should be possible to use this feature even in CA-less mode when Dogtag is not installed and certificate profiles are not available. Luckily this PR has no hard dependency on certificate profiles, with the exception of the validate_profile_id() call and the inclusion of the userCert profile, both of which I would like to be removed before the PR is merged.

@LiptonB
Copy link
Contributor Author

LiptonB commented Jan 24, 2017

@HonzaCholasta, I think I see what you mean about these templates not being dependent on dogtag, and I'm fine with removing the userCert dogtag profile from this PR if you don't think it's relevant. Is it ok to leave the userCert CSR generation profile, as an example of what the tool can do?

So, do you mean we should no longer consider CSR generation profiles to be associated with IPA profiles? In https://github.com/LiptonB/freeipa/tree/local-cert-build I have code that allows you to run ipa cert-request --autogenerate --principal someserver --profile-id caIPAserviceCert and get a cert for the server back in one step. It uses the caIPAserviceCert CSR profile to make a CSR that works with the caIPAserviceCert IPA profile. So it seems to me that having the profiles linked makes the cert generation experience simpler, and that was the original way this feature was proposed to me. But, if you'd rather have them not be linked, should I modify this command so the CSR profile is specified with a separate flag from the IPA one?

@HonzaCholasta
Copy link
Contributor

@LiptonB, I think certificate profiles and CSR generation profiles / templates should be associated, but not by sharing the same logical certprofile object, as it creates an unwarranted dependency on Dogtag. Instead CSR templates should be represented by their own dedicated objects separate from certprofile objects, which can contain a reference to the default CSR template object. This way it will be possible to extend cert-request as you described, but it will also be possible to generate a CSR and submit it to an external CA, even in CA-less IPA deployment.

As for userCert, removing just the dogtag profile but keeping the CSR template is exactly what I meant.

@LiptonB
Copy link
Contributor Author

LiptonB commented Jan 24, 2017

@HonzaCholasta, I think we're on the same page, then. I removed the dogtag profile and the validation from the profile_id parameter, and rebased the PR against master.

For the cert-request --autogenerate functionality, I will think about where in the CSR profile to store a link to the IPA profile to use.

@HonzaCholasta
Copy link
Contributor

@LiptonB, I meant it the other way around - certprofile should have an (optional) attribute which points to the associated CSR template.

@HonzaCholasta HonzaCholasta added ack Pull Request approved, can be merged and removed ack Pull Request approved, can be merged labels Jan 30, 2017
@HonzaCholasta
Copy link
Contributor

Before I push this, could you please:

  • squash "Fix broken tests in CSR autogeneration" into "Add tests for CSR autogeneration",
  • add module prefix to commit subjects ("csrgen:", "tests:", etc.),
  • make the terminology used in commit messages consistent ("CSR generation" vs. "CSR autogeneration", "certificate mapping" vs. "cert profile" vs. "CSR generation profile" vs. "CSR template" vs. ... - FTR my favorites are "CSR generation" and "CSR template")

?

Adds a library that uses jinja2 to format a script that, when run, will
build a CSR. Also adds a CLI command, 'cert-get-requestdata', that uses
this library and builds the script for a given principal. The rules are
read from json files in /usr/share/ipa/csr, but the rule provider is a
separate class so that it can be replaced easily.

https://fedorahosted.org/freeipa/ticket/4899
This removes the ipa.syntaxrule and ipa.datarule macros in favor of
simple 'if' statements based on the data referenced in the rules. The
'if' statement for a syntax rule is generated based on the data rules it
contains.

The Subject DN should not be generated unless all data rules are in
place, so the ability to override the logical operator that combines
data_sources (from 'or' to 'and') is added.

https://fedorahosted.org/freeipa/ticket/4899
This patch also contains some code changes to make the code easier to
test and to make the tests pass.

https://fedorahosted.org/freeipa/ticket/4899
@LiptonB
Copy link
Contributor Author

LiptonB commented Jan 30, 2017

@HonzaCholasta, updated, please take a look. I standardized on "CSR generation profile" because the names of the objects in the code and the directory that stores them are already "profiles," and because "template" is what the code calls the Jinja2 syntax that's built from the profile and the rules. But if you strongly prefer "template" to mean the collection of rules to use for generation I'm ok with changing it, I'd just want to change the code and filenames to be consistent as well.

Thanks for clarifying about the reference from certprofile to the CSR profile rather than the other way around. That seems fine to me too, especially if it's considered a default CSR profile rather than the only allowable one. Should I add that field to certprofile when I make the PR that adds cert-request --autogenerate?

@HonzaCholasta
Copy link
Contributor

@LiptonB, "CSR generation profile" works for me. Once the new design is implemented though, "CSR template" will be more fitting IMO.

I would not add the certprofile field before CSR generation is moved to server. For now it should be sufficient to assume that associated certificate profile and CSR generation profile have the same name (i.e. what this PR does).

@HonzaCholasta HonzaCholasta added the ack Pull Request approved, can be merged label Jan 31, 2017
@HonzaCholasta HonzaCholasta added the pushed Pull Request has already been pushed label Jan 31, 2017
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