fix: keep database and redis passwords out of the process output - #757
Open
solracsf wants to merge 3 commits into
Open
fix: keep database and redis passwords out of the process output#757solracsf wants to merge 3 commits into
solracsf wants to merge 3 commits into
Conversation
`Config` derives `Debug`, and neither of the two things it holds redacts its password. `AnyConnectOptions` stores the connection url in a public `Url` field, and `RedisConnectionInfo` has a plain `password: Option<String>`, so the derived output prints both in full. That reaches two places. `--dump-config` is the natural thing to attach to a bug report, and the `log::trace!` in `run()` means an instance started at trace level writes both passwords into the journal, where they stay. Redact the database url through `to_url_lossy` and the redis password behind a marker, keeping the host, user and database name that make the dump useful. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
Redacting `Config`'s `Debug` closes `--dump-config` and the startup trace, but
not the earliest place a password can escape. Clap puts the raw argument into
its own message, so a typo an admin can easily make prints the whole url:
$ notify_push --database-url "postgres://user:hunter2@host:not_a_port/db"
error: invalid value 'postgres://user:hunter2@host:not_a_port/db' for
'--database-url <DATABASE_URL>': error with configuration: invalid port number
For a systemd unit that lands in the journal. Take both url options as an
opaque `ConnectionUrl` whose `FromStr` cannot fail, so clap never formats them,
and validate in `PartialConfig::from_opt` where the message is ours. The error
names the option and carries the underlying parse error, neither of which
repeats the value.
`ConnectionUrl` redacts its own `Debug`, and `PartialConfig` now gets the same
treatment `Config` did, so the pattern is not left sitting one struct away for
the next person who adds a debug print. The redaction marker is plain
`REDACTED` rather than `<redacted>`, which `Url::set_password` percent-encoded
into `%3Credacted%3E`, and a failed `set_password` no longer falls through to
printing the url it could not redact.
Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
`Config` is the only thing ever formatted: `--dump-config` and the startup trace both print it, and its `Debug` has to keep showing host, user and database name. That impl stays. `Opt` and `PartialConfig` are never printed at all, so redacting them was guarding nothing. Not deriving `Debug` for them is both smaller and stronger: adding a debug print now fails to compile instead of quietly depending on the redaction being right. That removes the reason `ConnectionUrl` existed. It only wrapped a `String` to give `Opt` a safe `Debug`; a plain `String` already has the infallible `FromStr` that keeps clap from ever formatting the raw argument. Its two parse methods collapse into one generic helper. 103 lines out, 15 in. The binary behaves identically: a malformed url still reports without echoing itself, and `--dump-config` still redacts. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
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
1 participant
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.
Two paths put a password in the process output.
ConfigderivesDebug, and bothAnyConnectOptionsand the redis connection infoprint their password as a plain field, so
--dump-configand thelog::trace!inrun()emit it verbatim.Clap is the earlier path and the less obvious one, because it happens before any of
this code runs:
For a systemd unit both end up in the journal.
OptandPartialConfigdeliberately lose theirDebugderives rather than gainingredacting ones. Neither is ever printed, and a compile error is a better guard than
redaction code that has to stay correct.
Verified on the built binary: a malformed url on either option is now reported without
echoing itself, and
--dump-configcontains neither secret.