never log secrets #1

pohly · 2018-11-29T10:16:40Z

This is the utility code that can be used to avoid logging secrets at loglevel >= 5 in the following apps:

This is the first code being added to the repo and therefore the PR also adds a Makefile.

It was previously located and discussed in kubernetes-csi/external-provisioner#171, which also shows how the function would be used.

Just adds the usual all/clean/test targets for whatever packages are going to be added later.

pohly · 2018-11-29T10:17:10Z

/assign @saad-ali
/cc @msau42 @sbezverk

pohly · 2018-11-29T10:23:34Z

Note that I have added support for maps and oneof fields in another commit, see the full branch here:
https://github.com/pohly/csi-lib-utils/commits/secrets

But my proposal is to use the version that has been reviewed already, or at least mostly: 111cbba is new and of course the whole integration into this repo is also different.

The best solution would be to extend the text marshaller in https://github.com/golang/protobuf/blob/master/proto/text.go, ideally upstream, such that it injects the "stripped" message for fields that the caller wants to filter out. I can work on that next.

pohly · 2018-11-29T10:42:28Z

The best solution would be to extend the text marshaller in https://github.com/golang/protobuf/blob/master/proto/text.go, ideally upstream, such that it injects the "stripped" message for fields that the caller wants to filter out. I can work on that next.

Let me take this back. I thought that code already used information from protobuf and that therefore adding filtering via the csi_secret option wouldn't be hard. But when having another look, I realized that it's a text marshaller that walks the data structure via reflect and just relies on some know naming conventions for protobuf specific behavior, without actually doing anything with protobuf meta data.

Grafting field filtering onto that code won't be easy. Neither is it any easier to handle all cases correctly when starting from scratch (the approach that @sbezverk has taken in his revised PR kubernetes/kubernetes#71336). Therefore I don't intend to work on this further. If we want a truly generic, future-proof solution, then let's take also the last commit from https://github.com/pohly/csi-lib-utils/commits/secrets.

The output can be made similar (or even identical) to the original gRPC text marshaller without much work. That's the biggest drawback that I see with using json.Marshall, not the performance overhead.

pohly · 2018-11-29T15:52:57Z

@msau42, @saad-ali: please let me know what you want in this PR:
[ ] JSON marshaled output or gRPC style (not included, implemented in "nicer marshaling of stripped messages" from https://github.com/pohly/csi-lib-utils/commits/secrets)
[ ] simpler, reviewed implementation or the complete one ("handle secrets in oneof and maps" in https://github.com/pohly/csi-lib-utils/commits/secrets)

saad-ali · 2018-11-29T22:58:31Z

test/pkg/csi-spec/csi.proto

@@ -0,0 +1,1203 @@
+// Code generated by make; DO NOT EDIT.


Is this file required?

Yes, it is need for the csitest.proto -> csitest.pb.go creation because csitest.proto has to import it.

saad-ali · 2018-11-29T22:59:27Z

test/pkg/csi-spec/csitest/csitest.pb.go

@@ -0,0 +1,5326 @@
+// Code generated by protoc-gen-go. DO NOT EDIT.


Do you want to include a Makefile for generating csitest.pb.go from cistest.proto file?

I think I did? ;-)

csi-lib-utils/test/pkg/csi-spec/Makefile

Lines 85 to 89 in c276171

CSI_PROTO := ./csitest.proto

CSI_PKG_ROOT := github.com/kubernetes-csi/csi-lib-utils/test/pkg/csi-spec

CSI_PKG_SUB := $(shell cat $(CSI_PROTO) | sed -n -e 's/^package.$[^;]*$.v[0-9]\+;$$/\1/p'|tr '.' '/')

CSI_BUILD := $(CSI_PKG_SUB)/.build

CSI_GO := $(CSI_PKG_SUB)/csitest.pb.go

saad-ali · 2018-11-29T23:03:08Z

pkg/csigrpc/doc.go

@@ -0,0 +1,19 @@
+/*


Instead of putting this library under pkg/csigrpc/... please move this to the top under protosanitizer/... (similar to packages in https://github.com/kubernetes/utils).

Since this is a util only repo, it does not need cmd, pkg, etc directories. It will contain 1 or more go packages that other consumers will import and build in to binaries to ship.

Having it at the top make sense.

I wanted to grow the content of the package over time and also include other gRPC-related functions (hence the current package name), but I can't really come up with an argument why that has to be in the same package.

So protosanitizer it is.

saad-ali · 2018-11-29T23:05:11Z

test/pkg/csi-spec/.gitignore

@@ -0,0 +1,5 @@
+/protoc


let's move all the contents of test/* to protosanitizer/test/...

saad-ali · 2018-11-29T23:10:51Z

@msau42, @saad-ali: please let me know what you want in this PR:
[ ] JSON marshaled output or gRPC style (not included, implemented in "nicer marshaling of stripped messages" from https://github.com/pohly/csi-lib-utils/commits/secrets)
[ ] simpler, reviewed implementation or the complete one ("handle secrets in oneof and maps" in https://github.com/pohly/csi-lib-utils/commits/secrets)

This is a good starting point. Left a few minor comments.

We want to finish this before end of week. Let's get it merged, create a release-0.1 branch, and 0.1.0 tag. And update the sidecars to use 0.1.0 release of this. Then we can cut the official sidecar releases.

After all that is done, we can come back and refactor this: ideally remove JSON marshaling/unmarshalling, and any other simplifications. @sbezverk can hopefully help with that.

sbezverk · 2018-11-30T00:40:14Z

@saad-ali I am working on a version which is not using Marshal/Unmarshal. Once complete I will push pr against new repo.

sbezverk

Marshal/Unmarhal does not work for structs which have OneOf proto elements, could you please add to unit test that at least one of test cases has this struct:

v := csipb.VolumeCapability{
		AccessType: &csipb.VolumeCapability_Mount{
			Mount: &csipb.VolumeCapability_MountVolume{
				FsType:     "ext4",
				MountFlags: []string{"flag1", "flag2", "flag3"},
			},
		},
		AccessMode: &csipb.VolumeCapability_AccessMode{
			Mode: csipb.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER,
		},
	}

	var msg = csipb.CreateVolumeRequest{
		Name: "test-volume",
		CapacityRange: &csipb.CapacityRange{
			RequiredBytes: int64(1024),
			LimitBytes:    int64(1024),
		},
		VolumeCapabilities:        []*csipb.VolumeCapability{ &v },
		Secrets:                   map[string]string{"secret1": "secret1", "secret2": "secret2"},
		Parameters:                map[string]string{"param1": "param1", "param2": "param2"},
		VolumeContentSource:       &csipb.VolumeContentSource{},
		AccessibilityRequirements: &csipb.TopologyRequirement{},
	}

I doubt it will be successfully unmarshaled.

When running at glog level >= 5, CSI sidecar apps log the full CreateVolumeRequest, including the secrets. Secrets should never be logged at any level to avoid accidentally exposing them. We need to filter out the secrets. With older CSI versions, that could have been done based on the field name, which is still an option should this get backported. With CSI 1.0, a custom field option marks fields as secret. Using that option has the advantage that the code will continue to work also when new secret fields get added in the future. For the sake of simplicity, JSON is now used as representation of the string instead of the former compact text format from gRPC. That makes it possible to strip values from a map with generic types, instead of having to copy and manipulate the real generated structures. Another option would have been to copy https://github.com/golang/protobuf/blob/master/proto/text.go and modify it so that it skips secret fields, but that's over 800 lines of code. This version of the code is identical to the one reviewed in kubernetes-csi/external-provisioner#171 with the backwards parameters in assert.NotContains (kubernetes-csi/external-provisioner#171 (review)) fixed.

No code changes required because json handles those types for us, but it's still good to have a test case for it.

During PR review there were concerns about support for oneof fields, with this example suggested as test case.

The previous version was adding "secrets: *** strippped ***" even when the "secrets" field was unset.

This is a copy of the CSI 1.0.0 spec and build rules with more fields added that test various special cases, in particular the addition of secrets in nested data structures (simple structs, lists, maps).

pohly · 2018-11-30T09:41:10Z

@sbezverk wrote:

Marshal/Unmarhal does not work for structs which have OneOf proto elements, could you please add to unit test that at least one of test cases has this struct:
csipb.VolumeCapability{
AccessType: &csipb.VolumeCapability_Mount{
...

The existing test already has AccessType, a OneOf proto element. Your example doesn't seem to be that different from it, but for the sake of expedience I have added it anyway.

That works fine, no changes in the code needed. Why did you expect it to fail?

pohly · 2018-11-30T10:00:33Z

@saad-ali force-pushed, PTAL

sbezverk · 2018-11-30T12:59:10Z

@pohly Interesting, sometime ago on another project we hit the issue that since proto OneOf was generated as an interface, Unmarshalling process was failing as it could not determine which struct to use. OneOf has two possibilities, 2 structs. Maybe we were using different json package, not sure..

pohly · 2018-11-30T14:23:56Z

Serguei Bezverkhi <[email protected]> writes:

@pohly Interesting, sometime ago on another project we hit the issue that since proto `OneOf` was generated as an interface, Unmarshalling process was failing as it could not determine which struct to use. OneOf has two possibilities, 2 structs. Maybe we were using different json package, not sure..

I can see that this happens when trying to unmarshal into a gRPC message. But that's not what this code uses JSON for: the whole point is to convert into generic types because that's then easier to iterate over.

saad-ali

/lgtm
/approve

k8s-ci-robot · 2018-11-30T19:12:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pohly, saad-ali

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [saad-ali]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

saad-ali · 2018-11-30T19:42:42Z

I merged this, created a new release-0.1 branch, and cut a v0.1.0 tag.

Please remember to cherry pick any future bug fixes to the release-0.1 branch.

msau42 · 2019-01-15T20:28:19Z

xref: kubernetes-csi/external-provisioner#82

initial content

connection: add tests

Makefile: initial version for packages

a761933

Just adds the usual all/clean/test targets for whatever packages are going to be added later.

k8s-ci-robot requested a review from saad-ali November 29, 2018 10:16

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 29, 2018

k8s-ci-robot assigned saad-ali Nov 29, 2018

k8s-ci-robot requested review from msau42 and sbezverk November 29, 2018 10:17

pohly force-pushed the secrets-pr branch from a0d0f84 to 0168560 Compare November 29, 2018 11:05

pohly mentioned this pull request Nov 29, 2018

never log secrets kubernetes-csi/external-provisioner#171

Closed

pohly force-pushed the secrets-pr branch from 0168560 to c276171 Compare November 29, 2018 15:48

saad-ali requested changes Nov 29, 2018

View reviewed changes

sbezverk suggested changes Nov 30, 2018

View reviewed changes

pohly added 5 commits November 30, 2018 09:47

protosanitizer: test marshaling of arrays and maps

43143fa

No code changes required because json handles those types for us, but it's still good to have a test case for it.

protosanitizer: another test for oneof

af584b9

During PR review there were concerns about support for oneof fields, with this example suggested as test case.

protosanitizer: only overwrite secret fields if already set

663368c

The previous version was adding "secrets: *** strippped ***" even when the "secrets" field was unset.

protosanitizer: test with updated CSI spec

2333c94

This is a copy of the CSI 1.0.0 spec and build rules with more fields added that test various special cases, in particular the addition of secrets in nested data structures (simple structs, lists, maps).

pohly force-pushed the secrets-pr branch from c276171 to 2333c94 Compare November 30, 2018 09:41

saad-ali approved these changes Nov 30, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2018

k8s-ci-robot merged commit 1628ab5 into kubernetes-csi:master Nov 30, 2018

pohly pushed a commit to pohly/csi-lib-utils that referenced this pull request Jan 24, 2019

Merge pull request kubernetes-csi#1 from pohly/master

b32dd01

initial content

pohly pushed a commit to pohly/csi-lib-utils that referenced this pull request Feb 4, 2019

Merge pull request kubernetes-csi#1 from pohly/connection-tests

23d286e

connection: add tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

never log secrets #1

never log secrets #1

pohly commented Nov 29, 2018

pohly commented Nov 29, 2018

pohly commented Nov 29, 2018

pohly commented Nov 29, 2018

pohly commented Nov 29, 2018

saad-ali Nov 29, 2018

pohly Nov 30, 2018

saad-ali Nov 30, 2018

saad-ali Nov 29, 2018 •

edited

Loading

pohly Nov 30, 2018

saad-ali Nov 30, 2018

saad-ali Nov 29, 2018

pohly Nov 30, 2018

saad-ali Nov 29, 2018

pohly Nov 30, 2018

saad-ali commented Nov 29, 2018

sbezverk commented Nov 30, 2018

sbezverk left a comment

pohly commented Nov 30, 2018 •

edited

Loading

pohly commented Nov 30, 2018

sbezverk commented Nov 30, 2018

pohly commented Nov 30, 2018 via email

saad-ali left a comment

k8s-ci-robot commented Nov 30, 2018

saad-ali commented Nov 30, 2018

msau42 commented Jan 15, 2019

		@@ -0,0 +1,5326 @@
		// Code generated by protoc-gen-go. DO NOT EDIT.

	CSI_PROTO := ./csitest.proto
	CSI_PKG_ROOT := github.com/kubernetes-csi/csi-lib-utils/test/pkg/csi-spec
	CSI_PKG_SUB := $(shell cat $(CSI_PROTO) \| sed -n -e 's/^package.\([^;]*\).v[0-9]\+;$$/\1/p'\|tr '.' '/')
	CSI_BUILD := $(CSI_PKG_SUB)/.build
	CSI_GO := $(CSI_PKG_SUB)/csitest.pb.go

never log secrets #1

never log secrets #1

Conversation

pohly commented Nov 29, 2018

pohly commented Nov 29, 2018

pohly commented Nov 29, 2018

pohly commented Nov 29, 2018

pohly commented Nov 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali commented Nov 29, 2018

sbezverk commented Nov 30, 2018

sbezverk left a comment

Choose a reason for hiding this comment

pohly commented Nov 30, 2018 • edited Loading

pohly commented Nov 30, 2018

sbezverk commented Nov 30, 2018

pohly commented Nov 30, 2018 via email

saad-ali left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 30, 2018

saad-ali commented Nov 30, 2018

msau42 commented Jan 15, 2019

saad-ali Nov 29, 2018 •

edited

Loading

pohly commented Nov 30, 2018 •

edited

Loading