never log secrets #171

pohly · 2018-11-21T10:46:49Z

When running at glog level >= 5, external-provisioner logged 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.

Ultimately this new package should live in a "csi-common" repo and
also include other utility code, like logGRPC itself.

Fixes: #82, #167

When running at glog level >= 5, external-provisioner logged 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. Ultimately this new package should live in a "csi-common" repo and also include other utility code, like logGRPC itself. Fixes: kubernetes-csi#82, kubernetes-csi#167

pohly · 2018-11-21T10:47:03Z

/assign @saad-ali

sbezverk · 2018-11-21T18:38:32Z

@pohly is it really required to go through marshal/unmarshal? Would it not be sufficient just to strip secret and convert it into a string for printing it in the log?
Also giving package csigrpc name imho is a bit misleading, I think this function belongs more to utils/tools package.

pohly · 2018-11-21T19:45:11Z

Serguei Bezverkhi <[email protected]> writes:

@pohly is it really required to go through marshal/unmarshal?

Are you worried about performance? The code doesn't run often.

Would it not be sufficient just to strip secret and convert it into a string for printing it in the log?

You mean, converting the entire message to a string, then convert the secret and do a string replacement of the secret string in the complete string? That would work, but I am a bit worried that this might be too aggressive (as in: replacing a harmless substring also elsewhere). There's also going to be a problem with finding the instance of the secret field in the original message - that code isn't going to be nice.

Also giving package `csigrpc` name imho is a bit misleading, I think this function belongs more to utils/tools package.

The long-term plan was to have it under kubernetes-csi/csi-common/pkg/csigrpc, together with logGRPC. Does the name make more sense from that perspective? A package called "utils" or "tools" IMHO is too generic.

msau42 · 2018-11-21T21:22:42Z

Gopkg.lock

@@ -102,6 +102,7 @@
  digest = "1:3dd078fda7500c341bc26cfbc6c6a34614f295a2457149fc1045cab767cbcf18"


Did Gopkg.toml also need to be modified?

It gets updated by "dep" when the set of input files changes.

Looks like we now have a conflict because of that. I can rebase if this approach is how we want to continue.

pkg/csigrpc/csigrpc.go

saad-ali · 2018-11-21T21:38:41Z

pkg/csigrpc/csigrpc.go

+			ex, err := proto.GetExtension(field.Options, csi.E_CsiSecret)
+			if err == nil && ex != nil && *ex.(*bool) {
+				parsedFields[field.GetName()] = "***stripped***"
+			} else if field.GetType() == protobuf.FieldDescriptorProto_TYPE_MESSAGE {


if err != nil do we still want to do this?

I haven't checked in detail which errors might be returned by proto.GetExtension. My assumption here is that we won't get an error for a field that has the csi_secret option set and that therefore getting an error indicates that it isn't a secret field. Under that assumption it is okay and actually desirable to continue filtering that field recursively despite an error.

Ok, sounds good.

It is indeed normal to get errors. For the "name" field:

(dlv) p err error(*errors.errorString) *{ s: "proto: nil *descriptor.FieldOptions is not extendable",} (dlv) p field *github.com/kubernetes-csi/external-provisioner/vendor/github.com/golang/protobuf/protoc-gen-go/descriptor.FieldDescriptorProto { Name: *"name", Number: *1, ...

For "name" we don't need to recurse, but the same error also occurs for "capacity_range", and in that case we must ignore the error and filter CapacityRange.

sbezverk · 2018-11-22T01:01:36Z

@pohly I would like to propose an alternative solution which I think is less complex and should be more performant. I hope you do not mind.

pohly · 2018-11-22T07:39:44Z

Serguei Bezverkhi <[email protected]> writes:

@pohly I would like to propose an alternative solution which I think is less complex and should be more performant. I hope you do not mind.

Of course not, please go ahead.

pohly · 2018-11-26T14:51:57Z

Serguei Bezverkhi <[email protected]> writes:

@pohly I would like to propose an alternative solution which I think is less complex and should be more performant.

I assume you are still working on this? Do you have an estimate how long it might take? Perhaps we can discuss this further in the CSI WG meeting today?

sbezverk · 2018-11-26T14:58:33Z

@pohly it is ready for review, it is just a matter of creating repo csi-utils as per @saad-ali suggestion.

pohly · 2018-11-26T15:28:37Z

Serguei Bezverkhi <[email protected]> writes:

@pohly it is ready for review, it is just a matter of creating repo csi-utils as per @saad-ali suggestion.

Can you push it somewhere else in the meantime?

sbezverk · 2018-11-26T15:30:58Z

@pohly here you go. kubernetes/kubernetes#71336

pohly · 2018-11-26T16:05:25Z

Serguei Bezverkhi <[email protected]> writes:

@pohly here you go. kubernetes/kubernetes#71336

You solution is less general than mine, for example it only works for secret fields which are of type map[string]string and it doesn't recurse into messages. But perhaps that's good enough. What worries me more is that it also modifies the message in place, doesn't it? If this was done in the logGRPC messages of the sidecar containers, they would never pass the actual secrets to the underlying driver. Were you planning to do a deep copy of the message before passing it to SanitizeMsg()? How? Any package for that then also becomes a dependency for the sidecar containers. The other question I have is about delayed conversion. In glog.V(5).Infof("GRPC response: %s", SanitizeMsg(reply)), the conversion to string always happens, whether it is needed or not. Would it be worthwhile to delay the conversion to string until really needed?

pohly · 2018-11-26T17:46:02Z

Serguei Bezverkhi <[email protected]> writes:

@pohly it does not alter the original message.

It does: ``` 50: for _, test := range tests { 51: result := SanitizeMsg(test.msg) => 52: if c := strings.Compare(test.expected, result); c != 0 { 53: t.Fatalf("Test %s failed, expected: %s got: %s, c: %d", test.name, test.expected, result, c) 54: } 55: } 56: } (dlv) p test.msg *k8s.io/kubernetes/vendor/github.com/container-storage-interface/spec/lib/go/csi.CreateVolumeRequest { Name: "", CapacityRange: *k8s.io/kubernetes/vendor/github.com/container-storage-interface/spec/lib/go/csi.CapacityRange nil, VolumeCapabilities: []*k8s.io/kubernetes/vendor/github.com/container-storage-interface/spec/lib/go/csi.VolumeCapability len: 0, cap: 0, nil, Parameters: map[string]string nil, Secrets: map[string]string [ "secret1": "* * * Sanitized * * *", "secret2": "* * * Sanitized * * *", ], VolumeContentSource: *k8s.io/kubernetes/vendor/github.com/container-storage-interface/spec/lib/go/csi.VolumeContentSource nil, AccessibilityRequirements: *k8s.io/kubernetes/vendor/github.com/container-storage-interface/spec/lib/go/csi.TopologyRequirement nil, XXX_NoUnkeyedLiteral: struct {} {}, XXX_unrecognized: []uint8 len: 0, cap: 0, nil, XXX_sizecache: 0,} ```

jsafrane · 2018-11-27T09:24:16Z

pkg/controller/controller.go

@@ -119,9 +120,9 @@ var (
 //TODO consolidate ane librarize
 func logGRPC(ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error {
 	glog.V(5).Infof("GRPC call: %s", method)
-	glog.V(5).Infof("GRPC request: %+v", req)
+	glog.V(5).Infof("GRPC request: %s", csigrpc.StripSecrets(req))


for performance:

if glog.V(5) { glog.Infof("GRPC request: %s", csigrpc.StripSecrets(req)) }

This isn't necessary, csigrpc.StripSecrets was explicitly designed to not run the expensive string conversion at the time when Infof gets called.

jsafrane · 2018-11-27T09:27:39Z

pkg/csigrpc/csigrpc_test.go

+	"testing"
+
+	"github.com/container-storage-interface/spec/lib/go/csi"
+	"github.com/stretchr/testify/assert"


I'm not sure it's worth adding new dependency for few asserts below.

I agree, for the external-attacher repo a simpler test would be better. But the code was meant to land in its own repo and now it looks like we'll do that right away (see kubernetes/org#268) and there I think it makes sense to use good testing infrastructure. The output of assert.Equal is much more readable than just printing the strings in manually-crafted failure messages.

liggitt · 2018-11-28T16:50:51Z

pkg/csigrpc/csigrpc_test.go

+	// The secret is hidden because StripSecrets is a struct referencing it.
+	dump := fmt.Sprintf("%#v", StripSecrets(createVolume))
+	assert.NotContains(t, secretName, dump)
+	assert.NotContains(t, secretValue, dump)


these assertions are backwards... should be assert.NotContains(t, dump, secretName)

liggitt · 2018-11-28T17:55:08Z

pkg/csigrpc/csigrpc.go

+	}
+
+	// Walk through all fields and replace those with ***stripped*** that
+	// are marked as secret. This relies on protobuf adding "json:" tags


relying on the json/proto correlation seems particularly fragile... is there a reason not to copy the proto message and walk it instead?

pohly · 2018-11-28T18:05:10Z

Jordan Liggitt <[email protected]> writes:

liggitt commented on this pull request. > + // manually that it recurses properly for single and + // repeated values. One-of might require further work. + } + + for _, c := range cases { + original := c.original + stripped := StripSecrets(original) + assert.Equal(t, c.stripped, fmt.Sprintf("%s", stripped), "unexpected result for fmt s") + assert.Equal(t, c.stripped, fmt.Sprintf("%v", stripped), "unexpected result for fmt v") + assert.Equal(t, original, c.original, "original value modified") + } + + // The secret is hidden because StripSecrets is a struct referencing it. + dump := fmt.Sprintf("%#v", StripSecrets(createVolume)) + assert.NotContains(t, secretName, dump) + assert.NotContains(t, secretValue, dump) these assertions are backwards... should be `assert.NotContains(t, dump, secretName)`

Well spotted. FWIW, I have an updated version which also handles oneof and has tests for various cases that don't occur in the current spec. But as we want the code to land in a new repo (kubernetes-csi/csi-lib-utils), I haven't pushed that code yet.

pohly · 2018-11-28T18:42:34Z

Jordan Liggitt <[email protected]> writes:

+ // Walk through all fields and replace those with ***stripped*** that + // are marked as secret. This relies on protobuf adding "json:" tags relying on the json/proto correlation seems particularly fragile...

I consider it part of the stable protobuf API because changing that could break code which uses marshalling to or from JSON. But yes, documentation about it is a bit sparse.

is there a reason not to copy the proto message and walk it instead?

Then one has to implement the mapping from protobuf name to Golang name. Doable, but more code. Ultimately that approach leads to https://github.com/golang/protobuf/blob/master/proto/text.go. If we want to use the same approach, then we should either fork that code and add field filtering to it or work with upstream gRPC to get such a feature into the official gRPC TextMarshaler.

msau42 · 2018-11-28T21:26:25Z

pkg/csigrpc/csigrpc_test.go

+	secretValue := "123"
+	createVolume := &csi.CreateVolumeRequest{
+		Name: "foo",
+		VolumeCapabilities: []*csi.VolumeCapability{


Can you also add topology into your unit test? It's one of the more complicated structures in the csi proto.

Done, see kubernetes-csi/csi-lib-utils@0f60cbc

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.

pohly · 2018-11-29T11:16:16Z

/hold

kubernetes-csi/csi-lib-utils#1 needs to be merged first, then we can vendor that code into this PR.

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.

pohly · 2018-12-03T12:54:34Z

The code was moved to kubernetes-csi/csi-lib-utils#1 and later merged into external-provisioner in #179.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 21, 2018

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

This was referenced Nov 21, 2018

Take advantage of csi_secret in CSI 1.0 kubernetes-csi/external-attacher#94

Closed

Take advantage of csi_secret in CSI 1.0 kubernetes-csi/driver-registrar#72

Closed

Take advantage of csi_secret in CSI 1.0 kubernetes-csi/external-snapshotter#62

Closed

msau42 reviewed Nov 21, 2018

View reviewed changes

saad-ali reviewed Nov 21, 2018

View reviewed changes

jsafrane reviewed Nov 27, 2018

View reviewed changes

liggitt reviewed Nov 28, 2018

View reviewed changes

msau42 reviewed Nov 28, 2018

View reviewed changes

pohly mentioned this pull request Nov 29, 2018

never log secrets kubernetes-csi/csi-lib-utils#1

Merged

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 29, 2018

pohly closed this Dec 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

never log secrets #171

never log secrets #171

pohly commented Nov 21, 2018

pohly commented Nov 21, 2018

sbezverk commented Nov 21, 2018

pohly commented Nov 21, 2018 via email

msau42 Nov 21, 2018

pohly Nov 21, 2018

pohly Nov 21, 2018

saad-ali Nov 21, 2018

pohly Nov 21, 2018

saad-ali Nov 21, 2018

pohly Nov 22, 2018

sbezverk commented Nov 22, 2018

pohly commented Nov 22, 2018 via email

pohly commented Nov 26, 2018 via email

sbezverk commented Nov 26, 2018

pohly commented Nov 26, 2018 via email

sbezverk commented Nov 26, 2018

pohly commented Nov 26, 2018 via email

pohly commented Nov 26, 2018 via email

jsafrane Nov 27, 2018 •

edited

Loading

pohly Nov 27, 2018

jsafrane Nov 27, 2018

pohly Nov 27, 2018

liggitt Nov 28, 2018

liggitt Nov 28, 2018

pohly commented Nov 28, 2018 via email

pohly commented Nov 28, 2018 via email

msau42 Nov 28, 2018

pohly Nov 29, 2018

pohly commented Nov 29, 2018

pohly commented Dec 3, 2018

		@@ -102,6 +102,7 @@
		digest = "1:3dd078fda7500c341bc26cfbc6c6a34614f295a2457149fc1045cab767cbcf18"

never log secrets #171

never log secrets #171

Conversation

pohly commented Nov 21, 2018

pohly commented Nov 21, 2018

sbezverk commented Nov 21, 2018

pohly commented Nov 21, 2018 via email

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

sbezverk commented Nov 22, 2018

pohly commented Nov 22, 2018 via email

pohly commented Nov 26, 2018 via email

sbezverk commented Nov 26, 2018

pohly commented Nov 26, 2018 via email

sbezverk commented Nov 26, 2018

pohly commented Nov 26, 2018 via email

pohly commented Nov 26, 2018 via email

jsafrane Nov 27, 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

pohly commented Nov 28, 2018 via email

pohly commented Nov 28, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented Nov 29, 2018

pohly commented Dec 3, 2018

jsafrane Nov 27, 2018 •

edited

Loading