Skip to content

[bug fix] pass docker login token via stdin#1395

Open
marchrius wants to merge 2 commits into
moghtech:mainfrom
marchrius:feature/password-special-chars
Open

[bug fix] pass docker login token via stdin#1395
marchrius wants to merge 2 commits into
moghtech:mainfrom
marchrius:feature/password-special-chars

Conversation

@marchrius
Copy link
Copy Markdown

Ref. #1377

Avoid interpolating registry tokens into a shell command during docker login. This preserves passwords containing shell-special characters such as quotes or dollar signs.

Summary

Fix Docker registry login handling in Periphery by passing the registry token directly to docker login --password-stdin instead of interpolating it into a shell command.

Why

Registry passwords or tokens containing shell-special characters like ' or $ could break login because they were evaluated by the shell before reaching Docker.

Changes

  • Replace echo {token} | docker login ... with direct tokio::process::Command execution.

Changes

  • Replace echo {token} | docker login ... with direct tokio::process::Command execution.
  • Write the registry token to the Docker process stdin.
  • Preserve existing Docker login error handling behavior.

Verification

  • cargo fmt --check
  • git diff --check
  • cargo check -p komodo_periphery
  • VSCode Init task equivalent completed

Avoid interpolating registry tokens into a shell command during docker login. This preserves passwords containing shell-special characters such as quotes or dollar signs.
@bytedream
Copy link
Copy Markdown
Contributor

bytedream commented Apr 27, 2026

There are multiple occurrences where this can happen, run_shell_command is also internally called by run_komodo_shell_command, which is used in different places with string interpolation. Rather than fixing this one specific command, the whole shell execution model should move away from sh -c IMO, as it's inherently insecure (breaking the command as reported in the issue is only the best case, this might also lead to remote-code execution).

Add direct command execution helpers that pass arguments separately and support piped stdin.

Use the safe execution path for Docker login, log searches, Docker secret/config creation, container stop/remove, Docker build/run/service create, compose operations without wrappers, stack deploy without env prefixes, and Core network route commands.

Keep shell execution only for explicit user-provided scripts, compose command wrappers, and env-prefixed swarm commands where shell behavior is intentional.
@marchrius
Copy link
Copy Markdown
Author

I tried to address this by centralizing safe command execution in lib/command.
Internal commands now pass the program and arguments separately instead of interpolating through a shell.
I migrated the riskier paths: Docker login, log search, secret/config creation, stop/remove, and internal Docker/Compose commands.
Shell execution remains only where intentional: user scripts, configured wrappers, and cases requiring explicit shell behavior.
This will definitely need review from a maintainer who knows the app better than I do; I’m trying to help with the support of AI.

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.

2 participants