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

CSI: fix redaction of volume status mount flags #12150

Merged
merged 1 commit into from
Mar 1, 2022

Conversation

tgross
Copy link
Member

@tgross tgross commented Feb 28, 2022

Fixes #9825

The volume status command and associated API redacts the entire
mount options instead of just the MountFlags field that can contain
sensitive data. Return a redacted value so that the return value makes
sense to operators who have set this field.

@tgross tgross changed the title csi: fix redaction of volume status mount flags CSI: fix redaction of volume status mount flags Feb 28, 2022
The `volume status` command and associated API redacts the entire
mount options instead of just the `MountFlags` field that can contain
sensitive data. Return a redacted value so that the return value makes
sense to operators who have set this field.
Copy link
Member

@shoenig shoenig left a comment

Choose a reason for hiding this comment

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

LGTM; without doing any investigation, are the explicit brackets result in output like [[REDACTED]]?

@tgross
Copy link
Member Author

tgross commented Mar 1, 2022

LGTM; without doing any investigation, are the explicit brackets result in output like [[REDACTED]]?

The MountOptions gets flattened into a map on the k/v formatter like this:

$ nomad volume status csi-volume-nfs0
ID                   = csi-volume-nfs0
Name                 = csi-volume-nfs0
External ID          = csi-volume-nfs0
Plugin ID            = org.democratic-csi.nfs
Provider             = org.democratic-csi.nfs
Version              = 1.4.3
Schedulable          = false
Controllers Healthy  = 0
Controllers Expected = 2
Nodes Healthy        = 0
Nodes Expected       = 0
Access Mode          = <none>
Attachment Mode      = <none>
Mount Options        = flags: [REDACTED]
Namespace            = default

Allocations
No allocations placed

If there were a filesystem type, it would appear on the same line like:

Mount Options        = flags: [REDACTED] fstype: ext4 

@tgross tgross merged commit 8ccb9a3 into main Mar 1, 2022
@tgross tgross deleted the csi-cli-mount-flags-redaction branch March 1, 2022 13:34
tgross added a commit that referenced this pull request Apr 15, 2022
The CSI HTTP API has to transform the CSI volume to redact secrets,
remove the claims fields, and to consolidate the allocation stubs into
a single slice of alloc stubs. This was done manually in #8590 but
this is a large amount of code and has proven both very bug prone
(see #8659, #8666, #8699, #8735, and #12150) and requires updating
lots of code every time we add a field to volumes or plugins.

In #10202 we introduce encoding improvements for the `Node` struct
that allow a more minimal transformation. Apply this same approach to
serializing `structs.CSIVolume` to API responses.

Also, the original reasoning behind #8590 for plugins no longer holds
because the counts are now denormalized within the state store, so we
can simply remove this transformation entirely.
tgross added a commit that referenced this pull request Apr 15, 2022
The CSI HTTP API has to transform the CSI volume to redact secrets,
remove the claims fields, and to consolidate the allocation stubs into
a single slice of alloc stubs. This was done manually in #8590 but
this is a large amount of code and has proven both very bug prone
(see #8659, #8666, #8699, #8735, and #12150) and requires updating
lots of code every time we add a field to volumes or plugins.

In #10202 we introduce encoding improvements for the `Node` struct
that allow a more minimal transformation. Apply this same approach to
serializing `structs.CSIVolume` to API responses.

Also, the original reasoning behind #8590 for plugins no longer holds
because the counts are now denormalized within the state store, so we
can simply remove this transformation entirely.
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'nomad volume status' mount_options redaction is confusing
2 participants