Race condition between signalling pid file and certificate reading

[ringerc]

There's a race between the process in pid_file_name being restarted and spiffe-helper signalling it to reload its certificates.

The problem

If the process being controlled is restarted, and it reads its certs before its pid file gets updated, spiffe-helper might replace the certs after the process has read the old copies of the certs. But spiffe-helper might fail to signal the process to reload the certificates because the pid file hasn't been updated yet.

This is likely to occur in cases where the pid file is managed by a separate supervisor rather than the process itself, e.g. a shell script that is managing both spiffe-helper and the process that uses the certs, e.g.

#!/bin/bash
set -e -u

# spiffe_helper.hcl also contains "pid_file_name=mycmd.pid"
pid_file_name="mycmd.pid"

spiffe-helper -c spiffe_helper.hcl &
spiffe_helper_pid=$!

while :
do
  mycmd &
  mycmd_pid=$!
  echo >"${mycmd.pid"} "$!"
  wait -p mycmd_exit_status "${mycmd_pid}"
done

here, if mycmd exits just as spiffe-helper renews a cert, the new mycmd might read the old cert and spiffe-helper might signal the old pid in pid_file_name before the script echos the new pid into mycmd.pid.

This wouldn't be an issue if mycmd did its own pid-file management and wrote its pid file before reading its certs, but neither can be broadly guaranteed for all commands.

Potential solutions

This race can be almost completely closed by having spiffe-helper retry if signalling the pid in pid_file_name fails due to the file being missing or the pid it points to not existing.

That would also help the issue with "normal errors" on startup in #251

It's not a completely perfect fix because in theory the old pid in pid_file_name could be re-used by a new process after the old one exits, before the pid file contents are rewritten to point to the new pid. Signalling it would thus not cause an error, but not deliver a signal to the right process. That's exceedingly unlikely to the point of not being worth caring about unless it could somehow be an exploit, and I don't see how it could be, so it can IMO be ignored.

[kfox1111]

Can flip around your script I think and get there without changes to spiffe-helper

Fork off a subshell at the beginning of the script, and write out that bash processes pid to the pid file (variable $$). then exec the real command it so the process has the same pid as the bash script used to execute it.

[ringerc]

Good point @kfox1111 , since exec re-uses the pid that'd work.

Sensible workaround.

For others, here's what they suggested doing:

#!/bin/bash
set -e -u

# spiffe_helper.hcl also contains "pid_file_name=mycmd.pid"
pid_file_name="mycmd.pid"

spiffe-helper -c spiffe_helper.hcl &
spiffe_helper_pid=$!

while :
do
  # spawn a subshell, write the subshell's pid to the pid file, and exec the real process to control
  # so it re-uses the pid of the subshell
  ( echo >"${mycmd.pid"} "$$" && exec mycmd ) &
  wait -p mycmd_exit_status "${mycmd_pid}"
done

... or the non-sh equivalent where your wrapper would fork() without exec(), write its pid file, and only then exec() the real process.

[ringerc]

#252 proposes to fix this, once the dependent PRs are merged and I can rebase it

[kfox1111]

Does reordering the script eliminate the need for signal retries?

[ringerc]

@kfox1111 Using the modified script with a subshell will avoid the need for retrying signal delivery, yes. It's not exactly user friendly and most users will probably just have race-prone code without realising, so I'd still suggest adopting retries per the PR I raised. But yes, your suggestion provides a usable workaround, thankyou.