Repository navigation
feat(run): mask fetched values by default; --no-mask turns it off - #171
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #170
nodkey runmasks fetched values by default.--no-maskturns it off for one run.Rule for terminals
When both stdout and stderr of
nodkey runare terminals, the defaultkeeps the old
execvepath: the command owns the terminal, so editors,ssh, prompts and colour work, and its output is not masked. In every othercase (a pipe or a file on stdout or stderr, which is how an agent reads
output) the command runs as a child and both streams are masked.
--maskforces the mask also on a terminal.--no-maskalways usesexecve.Why this rule and not "always mask": the issue allows it, and a person in
a terminal gets a working
ssh, editor or prompt without a new flag. Thesecurity review noted the risk: an agent that runs its commands in a
pseudo-terminal of its own gets the values unmasked. THREAT-MODEL.md T15
says this, and
--maskmasks also on a terminal. A config setting or aline in the phone message about masking can come later, as its own issue.
Not changed from the review: the ioctl check is now in
internal/clientand in
cmd/nodkey(fornodkey version). They take different inputs; ashared package for 5 lines is not worth it now.
Why not a pseudo-terminal: it needs much more code (raw mode, window size,
job control, SIGWINCH) and new risk, for the case where a person, not an
agent, reads the output. The leaner rule covers the agent case.
Progress
nodkey runwithout flags masks values in stdout and stderr (canary tests)nodkey run --no-maskbehaves as before PR feat(run): --mask hides fetched values in the command's output #169 (execve, no child); test--no-mask(changelog)