Skip to content

fix(pluggable): enforce executable timeout and kill the helper on expiry - #607

Open
wtn wants to merge 1 commit into
googleapis:mainfrom
wtn:timeout
Open

wtn wants to merge 1 commit into
googleapis:mainfrom
wtn:timeout

Conversation

@wtn

@wtn wtn commented Oct 27, 2026

Copy link
Copy Markdown

PluggableAuthCredentials#retrieve_subject_token! reads timeout_millis from the executable credential source, defaults it to 30000, checks it against the documented 5000 to 120000 bounds, and then passes it unchanged to subprocess_with_timeout, which hands it to Timeout.timeout. Timeout.timeout takes seconds, so the default allows the helper to run for 30000 seconds, over eight hours, and the bounds check never constrains anything at run time.

Converting the units is not enough on its own. subprocess_with_timeout wraps Open3.capture3, which runs inside Open3.popen_run, whose ensure closes the pipes and joins the child. When Timeout raises inside the block, that ensure waits for the helper to exit, so the error surfaces only after the helper finishes by itself, and each expiry prints two IOError: stream closed in another thread backtraces from the reader threads:

creds = Google::Auth::ExternalAccount::PluggableAuthCredentials.new(
  audience: "a", subject_token_type: "t", token_url: "https://x",
  credential_source: { executable: { command: "sleep 3" } },
)
creds.send(:subprocess_with_timeout, {}, "sleep 3", 0.5)
# => Timeout::Error after 3.01s

subprocess_with_timeout now uses Open3.popen3 with pgroup: true on POSIX, reads stdout and stderr on threads with report_on_exception off, and computes one monotonic deadline from timeout_millis / 1000.0. The process wait and both reader joins are bounded by the time remaining on that deadline, so a background child that inherits stdout cannot extend the wait past it. If any of them runs out, a Google::Auth::CredentialsError is raised, as the method's @raise already promised. An ensure on the popen3 block sends SIGKILL to the process group whenever the block leaves before the helper has been reaped. That covers the deadline, and it also covers an Interrupt in the caller, which the helper would otherwise outlive because its own process group no longer receives the terminal's Ctrl-C. On Windows Process.kill reaches only the direct process, so no process group is requested there.

The new specs fail on main and pass with this change.

PluggableAuthCredentials passed the configured timeout_millis value
straight to Timeout.timeout, which takes seconds. The default 30000 ms
timeout therefore allowed the helper executable to run for over eight
hours before being interrupted, and the documented 5-120 second bounds
were never enforced at run time.

Correcting the units alone was not enough. Open3.capture3 joins the
child in an ensure block, so Timeout::Error only surfaced once the
helper exited on its own, and each expiry printed reader-thread
backtraces to stderr.

Run the helper with Open3.popen3 in its own process group and apply a
single deadline to the process wait and to reading stdout and stderr,
so a background child that inherits the pipes cannot extend the wait.
Raise CredentialsError when the deadline passes, matching the
documented contract of retrieve_subject_token!, and send SIGKILL to the
group from an ensure whenever the block exits before the helper has
been reaped. That covers the deadline and also an interrupted caller,
which would otherwise wait on a helper that no longer shares its
terminal's process group. On Windows only the direct process is killed.

Specs cover the millisecond-to-second conversion, a real process that
outlives a sub-second deadline, a background child holding stdout open
after the executable exits, and an Interrupt raised in the caller
before the deadline.

Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
@google-cla

google-cla Bot commented Oct 27, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@wtn

wtn commented Oct 27, 2026

Copy link
Copy Markdown
Author

@googlebot I signed it!

@wtn
wtn marked this pull request as ready for review October 27, 2026 19:36
@wtn
wtn requested a review from a team as a code owner October 27, 2026 19:36
@quartzmo quartzmo self-assigned this Oct 29, 2026
@quartzmo quartzmo added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 29, 2026
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 29, 2026
@quartzmo

quartzmo commented Oct 29, 2026 •

Copy link
Copy Markdown
Member

@wtn The CLA check is failing for noreply@anthropic.com.

It's in your commit message:


Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants