doc/todo/Let_plugins_influence_what_environment_variables_a_wrapper_will_preserve.mdwn

   1 [[!template id=gitbranch branch=jcflack/config-envsave
   2 author="[[Chapman Flack|jcflack]]"
   3 browse=https://github.com/joeyh/ikiwiki/pull/14]]
   4
   5 I created this [[!taglink patch]] in advance of writing the [[plugins/contrib/signinview]] plugin. This patch does nothing `signinview`-specific, but simply refactors wrapper generation a bit so that plugins have some influence over what environment variables a wrapper will preserve.
   6
   7 For example, Wrapper.pm previously hardcoded not only (some of) the RFC 3875 variables needed for a CGI wrapper (and hardcoded its own test for _whether it was generating_ a CGI wrapper), but also the Apache ErrorDocument-specific variables needed by the [[plugins/404]] plugin. Given that `signinview`, as a `403` handler, would have similar requirements to `404`, and it seemed possible other wrappers for other purposes could rely on other environment variables too, it seemed to make sense to move the preserved-variable list out of Wrapper.pm hardcoding, and closer to the plugins or other code relying on the variables.
   8
   9 ----
  10 I was asked by [[Joey]] to create a page here for purposes of review. I'm still trying to figure out the preferred workflow for this project ... I'm assuming the github link is ok for looking over the comments and code changes, since they're already pushed to my git fork and (to me, anyway) reviewing changes in a decent repository browser is so much nicer than a `diff -u` pasted into a page between `<pre>` tags.
  11
  12 That seemed to be ok for reviewing [[bugs/CGI wrapper doesn't store PERL5LIB environment variable]], so I hope it's ok for this one too. If another way would be preferable, please let me know.
  13
  14 -- [[jcflack]]
  15
  16 > This is less about what plugins need, and more about what is safe.
  17 > If an environment variable is unsafe (in the sense of "can make a
  18 > setuid executable change its behaviour in dangerous ways") then we
  19 > must not pass it through, however desirable it might be.
  20 >
  21 > Because the only safe thing we can do is a whitelist, the list
  22 > is secondarily about what plugins need: if nothing needs a variable,
  23 > we don't pass it through.
  24 >
  25 > However, if a particular variable is safe, then it's always safe;
  26 > so if any plugin needs something, we might as well just put it in
  27 > the big list of things to keep. (In other words, any change to this
  28 > list is already security-sensitive.)
  29 >
  30 > As such, and because importing CGI into Setup pulls in a bunch
  31 > of extra code that is normally only imported when we are actually
  32 > running as a CGI, it might make more sense to have the "master list"
  33 > stay in Wrapper.
  34 >
  35 > What variables would `signinview` need? Can we just add them to
  36 > the list and skip the complexity of per-plugin configurability?
  37 >
  38 > Sorting the list makes sense to me, and so does adding the RFC 3875 set.
  39 >
  40 > [[!format txt """
  41 This change does seem to have exposed a thing where various plugins that
  42 call checksessionexpiry() (in CGI.pm) have been supplying one more argument
  43 than its prototype allows ... for years ...
  44 """]]
  45 >
  46 > I fixed that in ikiwiki 3.20141016. Please don't add the extra ignored
  47 > parameter to the prototype.
  48 >
  49 > [[!format diff """
  50 + if ( $config{needenvkeys} ) {
  51 """]]
  52 >
  53 > If this is needed at all, you should include this in the master list of
  54 > setup keys in IkiWiki.pm so it's documentable. Please mention setuid
  55 > in the description: "environment variables that are safe to pass through
  56 > a setuid wrapper" or something.
  57 >
  58 > I think it's `safe => 0, advanced => 1`.
  59 >
  60 > `preserve_env` or `env_keep` (or without the underscore, as you prefer)
  61 > might be better names for it (terminology stolen from `debuild` and `sudo`
  62 > respectively).
  63 >
  64 > --[[smcv]]