Logging of Secret data

[shawkins]

When the operator is creating a secret it will log at a debug level something like:

2023-07-27 18:31:50,503 DEBUG [io.jav.ope.pro.dep.AbstractDependentResource] (pool-18-thread-1) Creating dependent Secret(apiVersion=v1, data={username=YWRtaW4=, password=OWI0NDliZjM5MTI1NGNlMzg3OWFkNDBkOGYzM2E5MjE=}, immutable=null, kind=Secret, metadata=ObjectMeta(annotations={}, creationTimestamp=null, deletionGracePeriodSeconds=null, deletionTimestamp=null, ...

Should the data / stringData be redacted?

cc @vmuzikar

[vmuzikar]

Definitely. Seems almost like a weakness in JOSDK, IMHO secrets should never be leaked in a log.

[csviri]

Yep, not on debug level, will issue a fix.

[vmuzikar]

Hmmmm, isn't leaking credentials at any log level considered an antipattern? ;)

[csviri]

Well, depends, don't have strong opinion on this, historically framework logs for example requests/response pairs (in terms of WS) on trace level. While doing this generally, req/resp might contain any sensitive data. To my knowledge the approach to log such sensitive data on trace level is quite widely accepted. It's true that secret is not just any object but specifically there to hold sensitive data. Not sure if we should make an exception for that. But don't have strong opinion :) fine to have not logging the resource, just this is very handy sometimes.

[vmuzikar]

As far as I can tell, it's a good practice any data that's explicitly considered as credentials to be redacted in logs (AFAIK e.g. Jenkins does it). Unless explicitly printed on purpose (initial admin e.g.).

[csviri]

Jenkins replaces the credetial occurences in all logs (with some mask), this makes sense since it is hard to prevent logging the credential into the logs in first place (given the nature how these build scripts work on Jenkins). What I'm saying that for dev purposes logging k8s objects is handy on these cases, and trace level logging should never be turned on in production.
But I see the point, that since k8s gives this object this explicit semantics is not nice to have it logged anyway. I'm kinda also leaning toward the approach that credentials should not be there, especially in this case when we can nicely identify the case, so the solution is bulletproof.

[csviri]

removed the log, just the simplified without the actual data will be logged on debug level

[shawkins]

@csviri SSABasedGenericKubernetesResourceMatcher has the same issue:

java-operator-sdk/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java

Line 93 in f936a51

log.debug("Pruned actual: \n {} \n desired: \n {} ", prunedActual, desiredMap);

and it looks like a couple of trace logs as well.

[csviri]

ahh, ok but it is really needed there, can't we agree that we will log those on trace levels?

[csviri]

#2002

[csviri]

maybe something like this as alternative: #2003

[metacosm]

More generally speaking, maybe we should consider reworking the logging in JOSDK for v5 to align better with observability requirements?