Skip to content
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

log: add mini log library #365

Merged
merged 4 commits into from
Feb 14, 2024
Merged

log: add mini log library #365

merged 4 commits into from
Feb 14, 2024

Conversation

lbajolet-hashicorp
Copy link
Contributor

The Azure log library mimicks what the standard log library allows (with fewer functions exposed), and uses what the Packer SDK exposes for filtering out sensitive strings.

We have to do this here for censoring potential sensitive data separately from Packer itself, as the secrets produced/consumed by the plugin itself aren't necessarily known by Packer, so it cannot censor them from its output, and we have no way for a plugin to declare what is sensitive in its logs.

Therefore, we do it at log-time, in the plugin process, so Packer receives pre-censored logs, which it can render normally through its pipeline.

Then we adopt it for all our logging uses in the plugin, which should censor all the strings we want in the plugin process.

Closes: #358

Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

Nice approach on wrapping the logger any reason why there couldn't be a Set method on the common log that wrap packer.LogSecretFilter.Set(secret)?

This is a question not a blocker to merging

@lbajolet-hashicorp
Copy link
Contributor Author

Honestly that is a good question, I have not considered it since we have access to the LogSecretFilter already, and there were some use of that in the code (the adminPassword was already registering through this path).
This could be a good idea to add this just for better visibility.

The Azure log library mimicks what the standard log library allows (with
fewer functions exposed), and uses what the Packer SDK exposes for
filtering out sensitive strings.

We have to do this here for censoring potential sensitive data
separately from Packer itself, as the secrets produced/consumed by the
plugin itself aren't necessarily known by Packer, so it cannot censor
them from its output, and we have no way for a plugin to declare what is
sensitive in its logs.

Therefore, we do it at log-time, in the plugin process, so Packer
receives pre-censored logs, which it can render normally through its
pipeline.
Since there's some data we want to censor from the output of the plugin,
we start using the plugin's wrapper over stdlib's log which uses the
SDK's filtering methods, and replace all the log statements of the
plugin by this.
The Azure plugin had used the SecretFilter from the Packer SDK prior to
introducing the log wrapper, however, because the process that produces
the logs is separate from Packer, the filter was not propagated to the
main Packer process, and thus this assignation was ineffective when it
came to protecting secrets.

The previous commits has introduced a local wrapper for the log printer,
and this commit only fixes/introduces a comment to explain the call to
SecretFilter.Set in both the ARM and DTL builders.
Since the secret key contains a P12 blob, it is sensitive and should be
redacted from the output.
@lbajolet-hashicorp lbajolet-hashicorp merged commit 8a4454a into main Feb 14, 2024
12 checks passed
@lbajolet-hashicorp lbajolet-hashicorp deleted the censor_sensitive_values branch February 14, 2024 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Packer logs contain adminPassword in plain text
2 participants