Skip to content

Redact S3 credentials from logs #10811

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jerome079
Copy link

Description

This PR addresses a security issue where S3 credentials used for Secondary Storage were being logged in plain text in CloudStack logs (access.log and management-server.log). Even when debug logging is enabled, secret credentials such as accessKey and secretKey should never appear in logs.

Fix details:

  • Redacts the accessKey and secretKey from the S3TO object before logging DownloadCommand in NfsSecondaryStorageResource.java.
  • Adds a unit test in NfsSecondaryStorageResourceTest.java to verify that credentials are redacted.

Steps to reproduce the issue:

  1. Deploy CloudStack 4.20.0.0 with KVM and Ceph RGW S3 as Secondary Storage.
  2. Create a Secondary Storage using S3 credentials.
  3. Observe logs in /var/log/cloudstack/management/access.log or management-server.log — credentials will be printed.

Fixes: #10339

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • test (unit or integration test code)

Bug Severity

  • Major

How Has This Been Tested?

  • Added a unit test that mocks S3TO and verifies that setAccessKey("***REDACTED***") and setSecretKey("***REDACTED***") are called during executeRequest.

Copy link

boring-cyborg bot commented May 4, 2025

Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

Comment on lines +288 to +296
DataStoreTO store = safeCmd.getDataStore();
if (store instanceof S3TO) {
((S3TO) store).setAccessKey("***REDACTED***");
((S3TO) store).setSecretKey("***REDACTED***");
}
logger.debug(LogUtils.logGsonWithoutException("Executing command %s [%s].", safeCmd.getClass().getSimpleName(), safeCmd));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks good, but it seems this should be in LogUtils. There is other obfuscation code also scattered across the code base, so definately not a 👎 but a mere suggestion.

@sureshanaparti sureshanaparti added this to the 4.20.2 milestone Jun 5, 2025
@DaanHoogland DaanHoogland force-pushed the fix/s3-credential-log-leak branch from 0dedb70 to a43d34c Compare July 10, 2025 10:27
…udstack/storage/resource/NfsSecondaryStorageResourceTest.java
@DaanHoogland
Copy link
Contributor

@blueorangutan LLpackage

@blueorangutan
Copy link

@DaanHoogland a [LL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

Copy link

codecov bot commented Jul 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 3.91%. Comparing base (8e4fe1c) to head (ff6a437).

❗ There is a different number of reports uploaded between BASE (8e4fe1c) and HEAD (ff6a437). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (8e4fe1c) HEAD (ff6a437)
unittests 1 0
Additional details and impacted files
@@              Coverage Diff              @@
##               main   #10811       +/-   ##
=============================================
- Coverage     16.57%    3.91%   -12.67%     
=============================================
  Files          5745      415     -5330     
  Lines        510847    33793   -477054     
  Branches      62140     6078    -56062     
=============================================
- Hits          84696     1322    -83374     
+ Misses       416677    32313   -384364     
+ Partials       9474      158     -9316     
Flag Coverage Δ
uitests 3.91% <ø> (ø)
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@blueorangutan
Copy link

Packaging result [LL]: ✖️ el7 ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 6219

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 14149

@DaanHoogland
Copy link
Contributor

@jerome079 can you have a look at this PR?

20:43:58 [ERROR] /jenkins/workspace/acs-centos8-pkg-builder/dist/rpmbuild/BUILD/cloudstack-4.21.0.0-SNAPSHOT/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java:299: Line has trailing spaces. [RegexpSingleline]

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

Successfully merging this pull request may close these issues.

S3 credentials leak in log files
4 participants