release-25.2: importer: sanitize cloud storage URIs in error messages#167948
release-25.2: importer: sanitize cloud storage URIs in error messages#167948trunk-io[bot] merged 1 commit intorelease-25.2from
Conversation
Previously, when an import job failed, error messages in
`readInputFiles` included the full cloud storage URI which could
contain credentials (e.g., `CREDENTIALS=<secret>`). The code already
acknowledged this risk with a comment ("Don't log dataFile here because
it could leak auth information") but the error return paths were not
similarly protected.
Use `cloud.SanitizeExternalStorageURI` to strip credentials from the
data file URI before including it in error messages, following the same
pattern as the backup code's `RedactURIForErrorMessage`. This covers
the three error paths in `readInputFiles` that explicitly embed the
file path: the `pgerror.Newf` for "too many parsing errors" and both
`errors.Wrapf` calls that annotate errors with the file path.
Resolves: #151884
Release note (bug fix): Fixed a bug where IMPORT error messages
could include unredacted cloud storage credentials from the source
URI. Credentials are now stripped from URIs before they appear in
error messages.
Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
16a08b2 to
f536523
Compare
|
Thanks for opening a backport. Before merging, please confirm that it falls into one of the following categories (select one):
Add a brief release justification to the PR description explaining your selection. Also, confirm that the change does not break backward compatibility and complies with all aspects of the backport policy. All backports must be reviewed by the TL and EM for the owning area. |
|
😎 Merged directly without going through the merge queue, as the queue was empty and the PR was up to date with the target branch - details. |
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
✅ PR #167948 is compliant with backport policy Confidence: high The changed file 'pkg/sql/importer/read_import_base.go' is a production file; however, critical security fixes do not require feature flag gating. The fixes improve the security posture of the system without breaking backward compatibility since they only alter the behavior of error message handling, ensuring credentials are not exposed. There is no evidence of removed version gates or incompatible changes, ensuring it preserves existing functionality. The changes are well-documented in the PR description, providing a high level of confidence in the assessment. ✅ ENGREQ Check Passed: No ENGREQ required (non-production code or serious issues). 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Backport 1/1 commits from #164881 on behalf of @mw5h.
Summary
readInputFileserror messages usingcloud.SanitizeExternalStorageURIto strip credentials before includingthem in user-visible errors.
errors.Wrapfcalls that annotate errors with the file path.Resolves: #151884
Release note (bug fix): Fixed a bug where IMPORT error messages
could include unredacted cloud storage credentials from the source
URI. Credentials are now stripped from URIs before they appear in
error messages.
Release justification: Important security fix for an issue encountered by a customer.