CLOUDSTACK-10424:Potential sensitive information disclosure

[lujiefsi]

Description

This PR fixing CLOUDSTACK-10424

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

[yadvr]

@blueorangutan package

[blueorangutan]

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2487

[yadvr]

@blueorangutan test

[blueorangutan]

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

[DaanHoogland]

I don't agree.

[blueorangutan]

Trillian test result (tid-3344)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31001 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4537-t3344-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Smoke tests completed. 86 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

[lujiefsi]

I don't agree.

Hum, I give this patch for two reasons:

• this log just indicate that "Unable to find a wakeup dispatcher from the joined job". I have check the code, it finds the dispatcher by jobId. so there's no need to print all detail of the job in this case, just jobID is ok, as other TRACE level log in this file,
• if we enable TRACE level log for some reason, the password can be logged into files which can then be propagated to other log systems, i.e Splunk where we can't make sure the log file are protected.

I know current change is bad for debugging, but i still think the sensitive information is more important, and in this case, password is not usefull to debug. In order to make the log be useful to debug, i will change the patch as:
s_logger.trace("Unable to find a wakeup dispatcher from the joined job:" + StringUtils.cleanString(job.toString()));

this change will give more details about the job.
Hope for more discussion!

[yadvr]

When trace logs are enabled, you'll even see SQL queries in the logs. Trace is supposed to show sensitive information and used for investigation/debugging when normal logs have failed and isn't enabled by default. I agree with Daan, -1.

[lujiefsi]

When trace logs are enabled, you'll even see SQL queries in the logs. Trace is supposed to show sensitive information and used for investigation/debugging when normal logs have failed and isn't enabled by default. I agree with Daan, -1.

OK, but I still think we should document this to tell sysadmin that sensitive information are logged and need carefully handle.

[DaanHoogland]

@lujiefsi I thank you for your diligence and I can live with sanitised logs and I agree that a password is only useful when debugging the authentication. There are many things wrong with our logging and passwords being logged for the wrong reasons is one of them. If this change suits your need, i wont block it. cc @rhtyd

[yadvr]

I still don't agree, we don't enable trace logs in production unless we're investigating something. It may be a useful property to see unmasked/unsanitised data. Instead if there's any specific VO or job or command parameter that shouldn't be logged, it should be fixed there than a general fix; for example explicit log off annoation for a command can be done like this: https://github.com/apache/cloudstack/blob/master/core/src/main/java/org/apache/cloudstack/ca/SetupKeyStoreCommand.java#L31

Let's revisit this towards 4.16.

[lujiefsi]

I still don't agree, we don't enable trace logs in production unless we're investigating something. It may be a useful property to see unmasked/unsanitised data. Instead if there's any specific VO or job or command parameter that shouldn't be logged, it should be fixed there than a general fix; for example explicit log off annoation for a command can be done like this: https://github.com/apache/cloudstack/blob/master/core/src/main/java/org/apache/cloudstack/ca/SetupKeyStoreCommand.java#L31

Let's revisit this towards 4.16.

It looks like a good solution!

[yadvr]

Hi, the PR got auto-closed due to no known source repository or some other Github-specific issue during the recent renaming of the default branch to main. If you wish kindly submit a new PR.