-
Notifications
You must be signed in to change notification settings - Fork 338
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
never log secrets #171
never log secrets #171
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,135 @@ | ||
/* | ||
Copyright 2018 The Kubernetes Authors. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
// Package csigrpc contains common utility code for CSI drivers and sidecar | ||
// apps that helps with operations related to gRPC. | ||
package csigrpc | ||
|
||
import ( | ||
"encoding/json" | ||
"fmt" | ||
"reflect" | ||
"strings" | ||
|
||
"github.com/container-storage-interface/spec/lib/go/csi" | ||
"github.com/golang/protobuf/descriptor" | ||
"github.com/golang/protobuf/proto" | ||
protobuf "github.com/golang/protobuf/protoc-gen-go/descriptor" | ||
) | ||
|
||
// StripSecrets returns a wrapper around the original CSI gRPC message | ||
// which has a Stringer implementation that serializes the message | ||
// as one-line JSON, but without including secret information. | ||
// Instead of the secret value(s), the string "***stripped***" is | ||
// included in the result. | ||
// | ||
// StripSecrets itself is fast and therefore it is cheap to pass the | ||
// result to logging functions which may or may not end up serializing | ||
// the parameter depending on the current log level. | ||
func StripSecrets(msg interface{}) fmt.Stringer { | ||
return &stripSecrets{msg} | ||
} | ||
|
||
type stripSecrets struct { | ||
msg interface{} | ||
} | ||
|
||
func (s *stripSecrets) String() string { | ||
// First convert to a generic representation. That's less efficient | ||
// than using reflect directly, but easier to work with. | ||
var parsed interface{} | ||
b, err := json.Marshal(s.msg) | ||
if err != nil { | ||
return fmt.Sprintf("<<json.Marshal %T: %s>>", s.msg, err) | ||
msau42 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
if err := json.Unmarshal(b, &parsed); err != nil { | ||
return fmt.Sprintf("<<json.Unmarshal %T: %s>>", s.msg, err) | ||
msau42 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
// Now remove secrets from the generic representation of the message. | ||
strip(parsed, s.msg) | ||
|
||
// Re-encoded the stripped representation and return that. | ||
b, err = json.Marshal(parsed) | ||
if err != nil { | ||
return fmt.Sprintf("<<json.Marshal %T: %s>>", s.msg, err) | ||
msau42 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
return string(b) | ||
} | ||
|
||
func strip(parsed interface{}, msg interface{}) { | ||
protobufMsg, ok := msg.(descriptor.Message) | ||
if !ok { | ||
// Not a protobuf message, so we are done. | ||
return | ||
} | ||
|
||
// The corresponding map in the parsed JSON representation. | ||
parsedFields, ok := parsed.(map[string]interface{}) | ||
if !ok { | ||
// Probably nil. | ||
return | ||
} | ||
|
||
// Walk through all fields and replace those with ***stripped*** that | ||
// are marked as secret. This relies on protobuf adding "json:" tags | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. relying on the json/proto correlation seems particularly fragile... is there a reason not to copy the proto message and walk it instead? |
||
// on each field where the name matches the field name in the protobuf | ||
// spec (like volume_capabilities). The field.GetJsonName() method returns | ||
// a different name (volumeCapabilities) which we don't use. | ||
_, md := descriptor.ForMessage(protobufMsg) | ||
fields := md.GetField() | ||
if fields != nil { | ||
for _, field := range fields { | ||
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, sounds good. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is indeed normal to get errors. For the "name" field:
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. |
||
// When we get here, | ||
// the type name is something like ".csi.v1.CapacityRange" (leading dot!) | ||
// and looking up "csi.v1.CapacityRange" | ||
// returns the type of a pointer to a pointer | ||
// to CapacityRange. We need a pointer to such | ||
// a value for recursive stripping. | ||
typeName := field.GetTypeName() | ||
if strings.HasPrefix(typeName, ".") { | ||
typeName = typeName[1:] | ||
} | ||
t := proto.MessageType(typeName) | ||
if t == nil || t.Kind() != reflect.Ptr { | ||
// Shouldn't happen, but | ||
// better check anyway instead | ||
// of panicking. | ||
continue | ||
} | ||
v := reflect.New(t.Elem()) | ||
|
||
// Recursively strip the message(s) that | ||
// the field contains. | ||
i := v.Interface() | ||
entry := parsedFields[field.GetName()] | ||
if slice, ok := entry.([]interface{}); ok { | ||
// Array of values, like VolumeCapabilities in CreateVolumeRequest. | ||
for _, entry := range slice { | ||
strip(entry, i) | ||
} | ||
} else { | ||
// Single value. | ||
strip(entry, i) | ||
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
/* | ||
Copyright 2018 The Kubernetes Authors. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package csigrpc | ||
|
||
import ( | ||
"fmt" | ||
"testing" | ||
|
||
"github.com/container-storage-interface/spec/lib/go/csi" | ||
"github.com/stretchr/testify/assert" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure it's worth adding new dependency for few asserts below. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
) | ||
|
||
func TestStripSecrets(t *testing.T) { | ||
secretName := "secret-abc" | ||
secretValue := "123" | ||
createVolume := &csi.CreateVolumeRequest{ | ||
Name: "foo", | ||
VolumeCapabilities: []*csi.VolumeCapability{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also add topology into your unit test? It's one of the more complicated structures in the csi proto. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, see kubernetes-csi/csi-lib-utils@0f60cbc |
||
&csi.VolumeCapability{ | ||
AccessType: &csi.VolumeCapability_Mount{ | ||
Mount: &csi.VolumeCapability_MountVolume{ | ||
FsType: "ext4", | ||
}, | ||
}, | ||
}, | ||
}, | ||
CapacityRange: &csi.CapacityRange{ | ||
RequiredBytes: 1024, | ||
}, | ||
Secrets: map[string]string{ | ||
secretName: secretValue, | ||
"secret-xyz": "987", | ||
}, | ||
} | ||
|
||
cases := []struct { | ||
original, stripped interface{} | ||
}{ | ||
{nil, "null"}, | ||
{1, "1"}, | ||
{"hello world", `"hello world"`}, | ||
{true, "true"}, | ||
{false, "false"}, | ||
{createVolume, `{"capacity_range":{"required_bytes":1024},"name":"foo","secrets":"***stripped***","volume_capabilities":[{"AccessType":{"Mount":{"fs_type":"ext4"}}}]}`}, | ||
|
||
// There is currently no test case that can verify | ||
// that recursive stripping works, because there is no | ||
// message where that is necessary. The code | ||
// nevertheless implements it and it has been verified | ||
// 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these assertions are backwards... should be |
||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for performance:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't necessary, csigrpc.StripSecrets was explicitly designed to not run the expensive string conversion at the time when Infof gets called.