Skip to content

Commit

Permalink
Fix JSON logging of RPCAuthInput (#151)
Browse files Browse the repository at this point in the history 
We can't just json marshal and then stringify it into the logs. It gets escaped then which makes it not workable if you assume the logs are json.

To get the 2 fields which are already marshaled we'd like to read those as logr will print slices as [byte, byte, ...] which isn't useful.

So maintain a mirror type of RPCAuthInput where the 2 marshal'd fields are strings and then copy everything over and return that struct for logging purposes.

It does mean input.message is escaped still but that's easier than a byte stream and the rest of the log is fully parseable JSON now.
  • Loading branch information
@sfc-gh-jchacon
sfc-gh-jchacon authored Jul 18, 2022
1 parent d5aba5a commit 94b3fd0
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 24 deletions.
4 changes: 2 additions & 2 deletions auth/opa/rpcauth/hooks.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func (c *conditionalHook) Hook(ctx context.Context, input *RPCAuthInput) error {

// HostNetHook returns an RPCAuthzHook that sets host networking information.
func HostNetHook(addr net.Addr) RPCAuthzHook {
return RPCAuthzHookFunc(func(ctx context.Context, input *RPCAuthInput) error {
return RPCAuthzHookFunc(func(_ context.Context, input *RPCAuthInput) error {
if input.Host == nil {
input.Host = &HostAuthInput{}
}
Expand All @@ -82,7 +82,7 @@ var (
// that validates if justification was included. If it is required and passes the optional validation function
// it will return nil, otherwise an error.
func JustificationHook(justificationFunc func(string) error) RPCAuthzHook {
return RPCAuthzHookFunc(func(ctx context.Context, input *RPCAuthInput) error {
return RPCAuthzHookFunc(func(_ context.Context, input *RPCAuthInput) error {
// See if we got any metadata and if it contains the justification
var j string
v := input.Metadata[ReqJustKey]
Expand Down
54 changes: 54 additions & 0 deletions auth/opa/rpcauth/input.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ import (
"context"
"crypto/x509/pkix"
"encoding/json"
"fmt"
"net"
"reflect"

"google.golang.org/grpc/codes"
"google.golang.org/grpc/credentials"
Expand All @@ -32,6 +34,7 @@ import (
)

// RPCAuthInput is used as policy input to validate Sansshell RPCs
// NOTE: RPCAuthInputForLogging must be updated when this changes.
type RPCAuthInput struct {
// The GRPC method name, as '/Package.Service/Method'
Method string `json:"method"`
Expand All @@ -55,6 +58,57 @@ type RPCAuthInput struct {
Extensions json.RawMessage `json:"extensions"`
}

// RPCAuthInputForLogging is a mirror struct for RPCAuthInput which
// translates the json.RawMessage fields into strings type wise.
// This allows logging as human readable values vs [xx, yy, zz] style
// NOTE: This must be updated when RPCAuthInput changes.
type RPCAuthInputForLogging struct {
Method string `json:"method"`
Message string `json:"message"`
MessageType string `json:"type"`
Metadata metadata.MD `json:"metadata"`
Peer *PeerAuthInput `json:"peer"`
Host *HostAuthInput `json:"host"`
Extensions string `json:"extensions,string"`
}

func (r RPCAuthInput) MarshalLog() interface{} {
// Transform for logging by forcing the types
// for raw JSON into string. Otherwise logr
// will print them as strings of bytes. If we
// cast to string for the whole object then
// we end up with string escaping.
return RPCAuthInputForLogging{
Method: r.Method,
Message: string(r.Message),
MessageType: r.MessageType,
Metadata: r.Metadata,
Peer: r.Peer,
Host: r.Host,
Extensions: string(r.Extensions),
}
}

func init() {
// Sanity check we didn't get the 2 structs out of sync.
r := reflect.TypeOf(RPCAuthInput{})
rl := reflect.TypeOf(RPCAuthInputForLogging{})

rFields := reflect.VisibleFields(r)
rlFields := reflect.VisibleFields(rl)
m := make(map[string]bool)
ml := make(map[string]bool)
for _, f := range rFields {
m[f.Name] = true
}
for _, f := range rlFields {
ml[f.Name] = true
}
if !reflect.DeepEqual(m, ml) {
panic(fmt.Sprintf("fields from RPCAuthInput (%v) do not match fields from RPCAuthInputForLogging (%v)", m, ml))
}
}

// PeerAuthInput contains policy-relevant information about an RPC peer.
type PeerAuthInput struct {
// Network information about the peer
Expand Down
15 changes: 2 additions & 13 deletions auth/opa/rpcauth/rpcauth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ package rpcauth

import (
"context"
"encoding/json"

"github.com/go-logr/logr"
"google.golang.org/grpc"
Expand Down Expand Up @@ -81,12 +80,7 @@ func (g *Authorizer) Eval(ctx context.Context, input *RPCAuthInput) error {
logger := logr.FromContextOrDiscard(ctx)
if input != nil {
if logger.V(2).Enabled() {
b, err := json.Marshal(input)
if err != nil {
logger.V(2).Info("marshal", "can't marshal input", err)
} else {
logger.V(2).Info("evaluating authz policy", "input", string(b))
}
logger.V(2).Info("evaluating authz policy", "input", input)
}
}
if input == nil {
Expand All @@ -102,12 +96,7 @@ func (g *Authorizer) Eval(ctx context.Context, input *RPCAuthInput) error {
}
}
if logger.V(1).Enabled() {
b, err := json.Marshal(input)
if err != nil {
logger.V(1).Info("marshal", "can't marshal input", err)
} else {
logger.V(1).Info("evaluating authz policy post hooks", "input", string(b))
}
logger.V(1).Info("evaluating authz policy post hooks", "input", input)
}
allowed, err := g.policy.Eval(ctx, input)
if err != nil {
Expand Down
18 changes: 9 additions & 9 deletions auth/opa/rpcauth/rpcauth_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ func TestAuthzHook(t *testing.T) {
name: "single hook, create allow",
input: &RPCAuthInput{},
hooks: []RPCAuthzHook{
RPCAuthzHookFunc(func(ctx context.Context, input *RPCAuthInput) error {
RPCAuthzHookFunc(func(_ context.Context, input *RPCAuthInput) error {
input.Method = "/Foo.Bar/Baz"
input.MessageType = "Foo.BazRequest"
return nil
Expand All @@ -153,7 +153,7 @@ func TestAuthzHook(t *testing.T) {
name: "extension hook",
input: &RPCAuthInput{},
hooks: []RPCAuthzHook{
RPCAuthzHookFunc(func(ctx context.Context, input *RPCAuthInput) error {
RPCAuthzHookFunc(func(_ context.Context, input *RPCAuthInput) error {
input.Extensions = extensions
return nil
}),
Expand All @@ -164,11 +164,11 @@ func TestAuthzHook(t *testing.T) {
name: "multiple hooks, create allow",
input: &RPCAuthInput{},
hooks: []RPCAuthzHook{
RPCAuthzHookFunc(func(ctx context.Context, input *RPCAuthInput) error {
RPCAuthzHookFunc(func(_ context.Context, input *RPCAuthInput) error {
input.Method = "/Foo.Bar/Baz"
return nil
}),
RPCAuthzHookFunc(func(ctx context.Context, input *RPCAuthInput) error {
RPCAuthzHookFunc(func(_ context.Context, input *RPCAuthInput) error {
input.MessageType = "Foo.BazRequest"
return nil
}),
Expand Down Expand Up @@ -226,12 +226,12 @@ func TestAuthzHook(t *testing.T) {
name: "hook ordering",
input: &RPCAuthInput{},
hooks: []RPCAuthzHook{
RPCAuthzHookFunc(func(ctx context.Context, input *RPCAuthInput) error {
RPCAuthzHookFunc(func(_ context.Context, input *RPCAuthInput) error {
input.Method = "/Foo.Bar/Baz"
input.MessageType = "Foo.BarRequest"
return nil
}),
RPCAuthzHookFunc(func(ctx context.Context, input *RPCAuthInput) error {
RPCAuthzHookFunc(func(_ context.Context, input *RPCAuthInput) error {
input.MessageType = "Foo.BazRequest"
return nil
}),
Expand All @@ -242,7 +242,7 @@ func TestAuthzHook(t *testing.T) {
name: "synthesize data, allow",
input: &RPCAuthInput{Method: "/Foo.Bar/Foo"},
hooks: []RPCAuthzHook{
RPCAuthzHookFunc(func(ctx context.Context, input *RPCAuthInput) error {
RPCAuthzHookFunc(func(_ context.Context, input *RPCAuthInput) error {
if input.Peer == nil {
input.Peer = &PeerAuthInput{
Principal: &PrincipalAuthInput{
Expand Down Expand Up @@ -321,7 +321,7 @@ func TestAuthzHook(t *testing.T) {
input: &RPCAuthInput{Method: "/Some.Random/Method"},
// Set principal to admin if method = "/Some.Random/Method"
hooks: []RPCAuthzHook{
HookIf(RPCAuthzHookFunc(func(ctx context.Context, input *RPCAuthInput) error {
HookIf(RPCAuthzHookFunc(func(_ context.Context, input *RPCAuthInput) error {
input.Peer = &PeerAuthInput{
Principal: &PrincipalAuthInput{
ID: "admin@foo",
Expand All @@ -339,7 +339,7 @@ func TestAuthzHook(t *testing.T) {
input: &RPCAuthInput{Method: "/Some.Other/Method"},
// Set principal to admin if method = "/Some.Random/Method"
hooks: []RPCAuthzHook{
HookIf(RPCAuthzHookFunc(func(ctx context.Context, input *RPCAuthInput) error {
HookIf(RPCAuthzHookFunc(func(_ context.Context, input *RPCAuthInput) error {
input.Peer = &PeerAuthInput{
Principal: &PrincipalAuthInput{
ID: "admin@foo",
Expand Down

0 comments on commit 94b3fd0

Please sign in to comment.