Don't log authentication configuration when connecting to MQTT broker

[Bravo555]

PR #2233 introduced some info! logging statements to MQTT connection code, so that the user could see what's happening.

However, this code printed out the contents of the AuthenticationConfig struct which contained root certificate store and a client private key if client authentication option was used.

The intent was to print out the certificate paths, but what was printed was the contents, potentially including secrets.

Fortunately it was caught quickly by @reubenmiller.

I think the biggest factor that contributed to this mistake was keeping secrets together with other safe-to-print fields, without using any marker type to denote that ClientAuthConfig.key is a secret, disabling it's Debug implementation so that it can't be accidentally printed out in the first place. Crates like secrecy can be used to replace Display and Debug impls so that sensitive information cannot be printed.

Proposed changes

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
• Documentation Update (if none of the other choices apply)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

• I have read the CONTRIBUTING doc
• I have signed the CLA (in all commits with git commit -s)
• I ran cargo fmt as mentioned in CODING_GUIDELINES
• I used cargo clippy as mentioned in CODING_GUIDELINES
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)

Further comments

[codecov]

Codecov Report

Merging #2249 (f1b610b) into main (e560182) will decrease coverage by 0.1%.
Report is 15 commits behind head on main.
The diff coverage is 0.0%.

Additional details and impacted files

Files Changed	Coverage Δ
crates/common/mqtt_channel/src/config.rs	44.5% <0.0%> (-4.6%)	⬇️
crates/common/mqtt_channel/src/connection.rs	54.6% <0.0%> (+0.2%)	⬆️

... and 3 files with indirect coverage changes

[github-actions]

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
272	0	5	272	100	51m26.841s

[didier-wenzek]

To avoid repeating this mistake, I would remove the culprit derive(Debug).

-#[derive(Debug, Clone)]
+#[derive(Clone)]
 struct ClientAuthConfig {
     cert_chain: Vec<Certificate>,
     key: PrivateKey,
 }
 
+impl Debug for ClientAuthConfig {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        // Do not print the private key!
+        self.cert_chain.fmt(f)
+    }
+}

[Bravo555]

To avoid repeating this mistake, I would remove the culprit derive(Debug).

fixed in b6a13ab.

[didier-wenzek]

Approved. Cargo.lock has to be updated before merge.

[reubenmiller]

Approved

The log entries look much better now:

Sep 13 06:47:08 ad0bf762eaeb c8y-firmware-plugin[969]: 2023-09-13T06:47:08.91651Z  INFO mqtt_channel::connection: MQTT connecting to broker: host=localhost:8883, session_name=Some("c8y-firmware-plugin")