add pun_pre_hook #535

johrstrom · 2020-06-17T16:21:09Z

Partial fix for #496.

Adds a pun pre_hook. --user $USER is given as a command line argument to the hook and OIDC_ACCESS_TOKEN is set as an environment variable if it exists in the parent process (the lua scripts in ood_mod_proxy).

I can provide a script that exchanges OIDC tokens and sets the kuberenetes config in this PR too. We'll eventually need to put it somewhere to share, I'm just not sure where. The k8s exchange script relies on environment specific things like which IDP to look at for example (OSC's dev/test/prod) and it's unclear where these should be source from (/etc/ood/profile?)

mod_ood_proxy/lib/ood/nginx_stage.lua

nginx_stage/lib/nginx_stage/generators/pun_config_generator.rb

ericfranz · 2020-06-18T16:05:24Z

What does the Apache config change look like in this case? It would seem that we would want to include the appropriate ood-portal-generator modifications to support that change as well.

Is there is a way to pass all claim headers to stdin instead of cherry-picking OIDC access token?

Is there a way to do this without being OIDC specific?

johrstrom · 2020-06-18T19:20:01Z

What does the Apache config change look like in this case?

Same. We hold our oidc configs separately from what we distribute anyway. Mod auth openidc defaults to passing these items as both header and environment variables, so we'd need to document that you need at least the environment variable for this to work.

Is there is a way to pass all claim headers to stdin instead of cherry-picking OIDC access token?

Maybe? We could loop through r:subprocess_env_table() and export anything matching oidc_*. We just don't need any more yet.

Is there a way to do this without being OIDC specific?

Sure, but our use case (along with SDSC's) is to use the access token. I guess we could blindly export the entire subprocess_env_table?

treydock · 2020-06-18T20:49:23Z

What does the Apache config change look like in this case?

Same. We hold our oidc configs separately from what we distribute anyway. Mod auth openidc defaults to passing these items as both header and environment variables, so we'd need to document that you need at least the environment variable for this to work.

Once we support Dex the OIDC settings will be exposed via ood-portal-generator. That was part of #474 . Part of that PR was to make it possible to define OIDC settings as part of ood-portal.conf because they are needed by Dex.

ericfranz · 2020-06-19T00:38:52Z

blindly export the entire subprocess_env_table

Yeah, especially if you don't modify the nginx_stage process's environment (see inline comment above). I think that is the best solution. Other Apache modules set env vars that would end up in this table and this should provide us access to all the OIDC claim headers. So it would be a win-win - our hook scripts get access all the claim headers and it is no longer OIDC specific.

One question though: who is this hook script running as? root or the user? We may want to have two hooks: a root hook script and a per user hook script, unless both ours and SDSC can be handled by a per user hook script.

johrstrom · 2020-06-19T14:30:43Z

One question though: who is this hook script running as? root or the user?

This runs as root. In my k8s script I sudo -u "$UNPRIV_USER" kubectl config. Indeed we don't even need root (SDSC may not either). There could be some way to use passenger hooks, but we'd need to pass the right environment to it.

johrstrom · 2020-06-22T14:31:00Z

It's not immediately clear how to copy the environment. I've got to find docs on how to iterate through an Apr.Table (the type returned by r.subprocess_env). Apparently there's a read-only version available in 2.5.x but, that's years away. So there's some work to be done trying to iterate through this table.

johrstrom · 2020-06-22T19:17:22Z

After some digging I'm fairly certain we can't export the r.subprocess_env table until this commit on the httpd source becomes available to us. Even in that commit message is said 'only get and set APIs are available' which I believe are defined through this c function that exposes them, thought it's not 100% clear to me how C code translates to LUA APIs.

So unless these things are reachable from some other mechanism, I'm not sure if there is a way to do this generically yet.

ericfranz · 2020-06-26T15:54:59Z

Darn. Maybe then we make configurable somehow (comma delimited list) the list of vars in r.subprocess_env to export? Then we can specify all the oidc claim headers we want to pass

johrstrom · 2020-07-02T20:34:21Z

Updated with a configuration. Unfortunately it's in the ood portal generator so there's one config in nginx_stage for the command to run and one config in the ood_portal.yml for what variables to export to the it. I'm not really sure there's a way around it because that's where the environment comes from.

If this strategy seems OK I can work on some test cases for both the ood portal and the nginx stage.

mod_ood_proxy/lib/ood/nginx_stage.lua

nginx_stage/lib/nginx_stage/generators/pun_config_generator.rb

ericfranz · 2020-07-10T14:20:09Z

We configure exports in Apache, and lua sets stdin with this when calling nginx_stage
We then configure nginx_stage configuration to tell nginx_stage to execute a prehook, converting this stdin to env for the prehook

Instead of utilizing nginx_stage configuration to tell what prehook to run, what if we just added support for more arguments to nginx stage - for example --pre-hook=/path/to/pre/hook.sh? Then we could handle configuration for the exports and the prehook in ood-portal-generator config.

johrstrom · 2020-07-10T14:33:42Z

That seems fine to me. Do we want to change the name to pun_root_pre_hook. I can't find a spot for this to run as a user.

(also I see the apache tests failing now, so I'll get on that).

ericfranz · 2020-07-18T17:12:34Z

Hold off on the per-user one till this is merged - we can revisit. Maybe the root/user is appended to the end of the name? Instead of --pre-hook it is --pre-hook-root?

ericfranz · 2020-08-07T17:47:33Z

mod_ood_proxy/lib/ood/nginx_stage.lua

+  Give a command and some standard in, get a string for merged stdout and stderr
+--]]
+function capture2e_with_stdin(cmd, stdin)
+  local output_file = os.tmpname()


Since we are back to using temporary filenames, and every single PUN launch will now use this, should we do io.tmpfile() instead? Also, this file we should ensure the permissions to be 600 before writing sensitive data to this.

When I was testing this, I saw it was creating 600 files, though that could be platform/configuration dependent. You think a os.execute(chmod ... ) is enough?

Looks like I was using os.tmpname because it's actually a string, so we don't have to cast it.

> t = os.tmpname() > os.execute('chmod 600 ' .. t .. ' >/dev/null 2>&1') true exit 0 > f = io.tmpfile() > os.execute('chmod 600 ' .. f .. ' >/dev/null 2>&1') stdin:1: attempt to concatenate a FILE* value (global 'f') stack traceback: stdin:1: in main chunk [C]: in ?

Then I'm not sure how to cast it to a string.

> tostring(f) file (0x56056fb7ea90) > tostring(t) /tmp/lua_8GVcnl

Maybe we can rely on io.tmpfile() to create 600 files. It is either that or introduce a tiny race condition with the use of os.tmpname.

ericfranz · 2020-08-07T17:51:22Z

mod_ood_proxy/lib/ood/nginx_stage.lua

  if app_init_url then
    cmd = cmd .. " -a '" .. r:escape(app_init_url) .. "'"
  end

-  local err = capture2e(cmd)


Can we take advantage of this STDIN any other way besides a prehook? Are we comfortable with changing every Execution of nginx_stage by Apache by providing STDIN like we are doing below.

If it could only ever be used for the pre_hook maybe we should consider maintaining the default local err = capture2e(cmd) and only passing stdin when a pre_hook_root_cmd is provided reduce the scope of this change. I have mixed feelings on this, though.

ericfranz · 2020-08-07T17:52:02Z

mod_ood_proxy/lib/ood/nginx_stage.lua

+
+  os.remove(output_file)
+
+  return output


we will need to document that the pre hook when executed by Apache should not output any data to stdout or stderr since we report both as an error.

ericfranz · 2020-09-22T14:19:54Z

In #496 we discussed being able to add luaposix. Would that be useful here at all?

johrstrom · 2020-10-15T18:19:11Z

In #496 we discussed being able to add luaposix. Would that be useful here at all?

It would help us pass the environment variables directly into the pre hook instead of parsing them from std in. So yes it would be helpful, but I'm not sure if the package management is worth it. We'd have to build and distribute a version for centos/RHEL 7 to say nothing about other platforms.

treydock · 2020-10-16T15:47:25Z

PowerTools for CentOS 8 is very common to have to enable and so is EPEL for EL7. I see no problem with depending on those repos. For CentOS they are very simple to get enabled, for RHEL it's a bit more involved but still common to have enabled.

johrstrom · 2020-10-26T20:49:14Z

Rebased off of current main and added syslog to nginx_stage. We should be good with this now, although it's been so long it's worth looking over it again.

ood-portal-generator/templates/ood-portal.conf.erb

treydock · 2020-10-27T13:16:56Z

@johrstrom Were you going to try and use lua posix to handle environment variables?

johrstrom · 2020-10-27T14:36:55Z

I could try, and I'm sure it would work, but I'm hesitant to add the dependency.

Of course if folks feel strongly about it, I can change this PR, but my vote would be to defer it at least until we know we want it also for some other use case (like the regex exploration in #710). The user/admin interface would look the same even if the internals change.

pun_pre_hook runs the configured executable before the PUN starts. This addition also enables logic to pass the OIDC_ACCESS_TOKEN if it exists into the nginx_stage script's standard in (in the form key=value).

to the nginx stage pre hook.

move pun_pre_hook configuration from the nginx_stage to the ood-portal-generator to minimize edit locations for this feature. now we pass in the script to the 'nginx_stage pun' command with the -P option.

06b7ebc.

most installations will continue to use the same code path as before.

ericfranz · 2020-11-02T18:40:39Z

I'd like to see how the code would change/possibly simplify if we used the lua posix module. If it is less code for us to maintain then I'd say go for it.

ericfranz reviewed Jun 18, 2020

View reviewed changes

mod_ood_proxy/lib/ood/nginx_stage.lua Show resolved Hide resolved

ericfranz reviewed Jun 18, 2020

View reviewed changes

nginx_stage/lib/nginx_stage/generators/pun_config_generator.rb Outdated Show resolved Hide resolved

johrstrom added the needs work label Jun 19, 2020

ericfranz added this to the OOD1.8 milestone Jul 2, 2020

johrstrom commented Jul 10, 2020

View reviewed changes

mod_ood_proxy/lib/ood/nginx_stage.lua Outdated Show resolved Hide resolved

johrstrom commented Jul 10, 2020

View reviewed changes

nginx_stage/lib/nginx_stage/generators/pun_config_generator.rb Outdated Show resolved Hide resolved

ericfranz linked an issue Aug 5, 2020 that may be closed by this pull request

Find and document a solution for before PUN start and after PUN stop hook scripts #496

Closed

johrstrom added ready and removed needs work labels Aug 6, 2020

ericfranz reviewed Aug 7, 2020

View reviewed changes

ericfranz modified the milestones: OOD1.8, OOD1.8-patch Aug 10, 2020

ericfranz modified the milestones: OOD1.8-patch, OOD2.0 Sep 22, 2020

johrstrom removed the ready label Oct 21, 2020

johrstrom force-pushed the nginx_stage_prehook branch from 5af0688 to a8ef0d3 Compare October 26, 2020 20:47

treydock reviewed Oct 27, 2020

View reviewed changes

ood-portal-generator/templates/ood-portal.conf.erb Outdated Show resolved Hide resolved

treydock reviewed Oct 27, 2020

View reviewed changes

ood-portal-generator/templates/ood-portal.conf.erb Show resolved Hide resolved

johrstrom added 15 commits November 2, 2020 10:42

add pun_pre_hook

025ba89

pun_pre_hook runs the configured executable before the PUN starts. This addition also enables logic to pass the OIDC_ACCESS_TOKEN if it exists into the nginx_stage script's standard in (in the form key=value).

use a named fifo instead of redirecting into a file.

4689f3d

read stdin in the exec_pre_hook instead of modifying the programs env

559f040

Use a configuration to specify which environment variables to export

158e341

to the nginx stage pre hook.

wrong comparison of key values

3e73942

move pun_pre_hook configuration

d60af25

move pun_pre_hook configuration from the nginx_stage to the ood-portal-generator to minimize edit locations for this feature. now we pass in the script to the 'nginx_stage pun' command with the -P option.

use a file instead of fifo to get io.tmpfile. this reverts

aa59329

06b7ebc.

fix ood-portal-generator tests with new default

328aa4e

add test case for all configs to ood-portal-generator

94d22bf

add rspec tests to nginx_stage

8b64da9

only use capture2e_with_stdin if pre_hook_root_cmd is supplied so

731f622

most installations will continue to use the same code path as before.

merge with current main

6580a39

add syslog to nginx_stage and use it if the pun_pre_hook fails

6abdae5

wrong variables in if block

a054f28

fix ood-portal.conf.all test

328568e

johrstrom force-pushed the nginx_stage_prehook branch from b74eaac to 328568e Compare November 2, 2020 15:55

stub Etc.getgrgid

d8a126c

johrstrom mentioned this pull request Nov 4, 2020

use lua-posix to pass environment variables to nginx_stage #726

Closed

ericfranz approved these changes Nov 30, 2020

View reviewed changes

johrstrom merged commit 28da29d into master Nov 30, 2020

johrstrom deleted the nginx_stage_prehook branch October 12, 2021 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add pun_pre_hook #535

add pun_pre_hook #535

johrstrom commented Jun 17, 2020

ericfranz commented Jun 18, 2020

johrstrom commented Jun 18, 2020

treydock commented Jun 18, 2020

ericfranz commented Jun 19, 2020

johrstrom commented Jun 19, 2020

johrstrom commented Jun 22, 2020

johrstrom commented Jun 22, 2020

ericfranz commented Jun 26, 2020

johrstrom commented Jul 2, 2020

ericfranz commented Jul 10, 2020

johrstrom commented Jul 10, 2020

ericfranz commented Jul 18, 2020

ericfranz Aug 7, 2020

johrstrom Aug 10, 2020 •

edited

Loading

ericfranz Aug 10, 2020

ericfranz Aug 7, 2020

ericfranz Aug 7, 2020

ericfranz commented Sep 22, 2020

johrstrom commented Oct 15, 2020

treydock commented Oct 16, 2020

johrstrom commented Oct 26, 2020

treydock commented Oct 27, 2020

johrstrom commented Oct 27, 2020

ericfranz commented Nov 2, 2020

add pun_pre_hook #535

add pun_pre_hook #535

Conversation

johrstrom commented Jun 17, 2020

ericfranz commented Jun 18, 2020

johrstrom commented Jun 18, 2020

treydock commented Jun 18, 2020

ericfranz commented Jun 19, 2020

johrstrom commented Jun 19, 2020

johrstrom commented Jun 22, 2020

johrstrom commented Jun 22, 2020

ericfranz commented Jun 26, 2020

johrstrom commented Jul 2, 2020

ericfranz commented Jul 10, 2020

johrstrom commented Jul 10, 2020

ericfranz commented Jul 18, 2020

ericfranz Aug 7, 2020

Choose a reason for hiding this comment

johrstrom Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

ericfranz Aug 10, 2020

Choose a reason for hiding this comment

ericfranz Aug 7, 2020

Choose a reason for hiding this comment

ericfranz Aug 7, 2020

Choose a reason for hiding this comment

ericfranz commented Sep 22, 2020

johrstrom commented Oct 15, 2020

treydock commented Oct 16, 2020

johrstrom commented Oct 26, 2020

treydock commented Oct 27, 2020

johrstrom commented Oct 27, 2020

ericfranz commented Nov 2, 2020

johrstrom Aug 10, 2020 •

edited

Loading