On 09/02/2016 05:04 AM, Petr Spacek wrote:
On 2.9.2016 04:19, Ben Lipton wrote:
On 07/27/2016 02:42 PM, Ben Lipton wrote:
On 07/21/2016 11:43 AM, Petr Spacek wrote:
Besides this nit,

sounds reasonable. I like how it prevents bad data from template-injection.
That's what I like about it, too. It does turn out to make things a little
tricky when it comes to writing rules that won't render if the data they
depend on is unavailable. (Because instead of rendering individual rules
which we can drop if they're missing data, we build one big template that
has to handle missing data correctly on its own.) I think it's probably
still worth it, though. I added this to the "Alternatives considered"
section of the above document.
By the way, I just wrote a followup blog post on this subject: describing the
challenges I've had with suppressing rules when the data isn't available, and
wondering if it's worth it. The post is here:
http://blog.benjaminlipton.com/2016/09/01/rule-suppression.html. It might be a
bit of a dense read, but I wanted to have the considerations documented at
least. As always, please let me know if there's anything I can clarify. And if
you do happen to read it and it makes you prefer one solution over the others,
I'd love to hear your opinion.
Hello Ben,

my comments are in-line (text copied from the blog post):


The current implementation is working ok, but the “Declaring data dependencies” 
solution is also appealing. Recording in data rules what data they depend on is 
only slightly more involved than wrapping that reference in ipa.datafield(), 
and could also be useful for other purposes.
I agree that syntax with explicit "if"s is little bit more elaborate. On the
other hand, the explicit condition is easier to read (for me) because I can
see what it is doing directly - I do not remember meaning of magical IPA macros.

I.e. I like version with explicit "if"s more.

Plus, it would get rid of the empty sections in openssl configs, as well as
some of the complex macros.


The extra templating and new tags required to get rid of extra commas and 
newlines don’t seem worth it to me, unless we discover a version of openssl or 
certutil that can’t consume the current output.
I definitely agree.

Finally, I think the number of hoops needing to be jumped through to fine-tune 
the output format hint at this “template interpolation” approach being less 
successful than originally expected. While it was expected that inserting data 
rule templates into syntax rule templates and rendering the whole thing would 
produce similar results to rendering data rules first and inserting the output 
into syntax rules, that is not turning out to be the case. It might be wise to 
reconsider the simpler option - it may be easier to implement reliable jinja2 
template markup escaping than to build templates smart enough to handle any 
combination of data that’s available.
This is certainly something to think about. Personally I think that version
with explicit "if"s is easy to understandad, write, and also it has no risk of
data injection (AFAIK).

Explicit escaping is usually very error prone... but I'm not in position to
judge how user-friendly it would be when compared with other solutions. After
all, goal of the feature is to make life of an average admin easier :-)

I hope this brain-dump will help you somehow :-) Have a nice day!
It very much does, thanks for reading! I was leaning towards the "explicit ifs" version but didn't want to add more changes unless the difference was meaningful. I will finish up the patch to convert to the "explicit ifs" version and make it available in the PR for discussion.

I also agree that implementing escaping for a format we don't control makes me nervous. One thing we could do is change the format of the data that goes into the final formatter step. http://blog.benjaminlipton.com/2016/07/19/csr-generation-templating.html#two-pass-data-interpolation assumed that would also be jinja2 (hence the {% section %} tag), but it wouldn't have to be. We could create/choose a different format with well-defined escaping routines. But it would need to be a structured format, so that the openssl formatter can figure out what things go into sections and where. And if we're implementing a structured format for defining CSR contents, maybe we should skip the openssl format entirely and go straight to templating the actual CSR structure, as discussed in this thread https://www.redhat.com/archives/freeipa-devel/2016-August/msg00652.html (towards the bottom of the email). These are just some ideas for the future; for now I'm going to set this aside because template interpolation seems to be working well enough.

Manage your subscription for the Freeipa-devel mailing list:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to