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.

Sign up for GitHub

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

ATLAS-10930: log which RBAC check (rbac/entitlement) failed authz #12

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ATLAS-10930: log which RBAC check (rbac/entitlement) failed authz #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 37 additions & 3 deletions grpc_opa/authorizer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ const (
TypeKey = ABACKey("ABACType")
VerbKey = ABACKey("ABACVerb")
ObKey = ObligationKey("obligations")

// These Rego obligations constants are hard-coded in
// https://github.com/Infoblox-CTO/ngp.authz/blob/develop/helm/authz/files/rego/authz.rego
RBACObKey = "authz.rbac.rbac"
EntitlementObKey = "authz.rbac.entitlement"
)

// Override to set your servicename
Expand All @@ -35,9 +40,9 @@ var (
)

var (
ErrForbidden = status.Errorf(codes.PermissionDenied, "Request forbidden: not authorized")
ErrUnknown = status.Errorf(codes.Unknown, "Unknown error")
ErrInvalidArg = status.Errorf(codes.InvalidArgument, "Invalid argument")
ErrForbidden = status.Errorf(codes.PermissionDenied, "Request forbidden: not authorized")
ErrUnknown = status.Errorf(codes.Unknown, "Unknown error")
ErrInvalidArg = status.Errorf(codes.InvalidArgument, "Invalid argument")
)

// DecisionInput is app/service-specific data supplied by app/service ABAC requests
Expand Down Expand Up @@ -160,6 +165,7 @@ func (a *DefaultAuthorizer) Evaluate(ctx context.Context, fullMethod string, grp

logger := ctxlogrus.Extract(ctx).WithFields(log.Fields{
"application": a.application,
"fullMethod": fullMethod,
})

bearer, newBearer := athena_claims.AuthBearersFromCtx(ctx)
Expand Down Expand Up @@ -252,6 +258,8 @@ func (a *DefaultAuthorizer) Evaluate(ctx context.Context, fullMethod string, grp
}

if !opaResp.Allow() {
forbidReason := opaResp.rbacForbiddenReason()
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we first check that an obligation was expected? This is going to be triggered and have no message for 100% of authz failures today.

logger.Infof("Request forbidden because these RBAC checks failed:%s", forbidReason)
Copy link
Contributor

Choose a reason for hiding this comment

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

this will make it easier to find in kibana

Suggested change
logger.Infof("Request forbidden because these RBAC checks failed:%s", forbidReason)
logger.WithField("msg", forbidReason).Info("obligation_forbidden msg")

return false, ctx, ErrForbidden
Copy link
Contributor

Choose a reason for hiding this comment

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

Another way to surface error messages is to wrap them in the error being returned.

Suggested change
return false, ctx, ErrForbidden
return false, ctx, fmt.Errorf("%w: %s", ErrForbidden, forbidReason)

Caller verifies type of error with errors.Is(err, grpc_opa.ErrForbidden)

}

Expand Down Expand Up @@ -353,6 +361,32 @@ func (o OPAResponse) Allow() bool {
return allow
}

// If reason is available, returns reason RBAC was forbidden
func (o OPAResponse) rbacForbiddenReason() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the signature needs work here. We should distinguish lack of obligations from a specific obligation denial. You could also do that by emitting an error instead of a string. err == nil nothing about obligations to report.

Also, how do services turn these logs off if they don't want them? Perhaps, don't log at all and return wrapped errors is a better way to enable implementation to choose behavior.

unavailReason := ""

obRaw, obFound := o[string(ObKey)]
if !obFound {
return unavailReason
}

obMap, isMap := obRaw.(map[string]interface{})
if !isMap {
return unavailReason
}

var availReason strings.Builder
if _, ok := obMap[RBACObKey]; !ok {
availReason.WriteByte(' ')
availReason.WriteString(RBACObKey)
}
if _, ok := obMap[EntitlementObKey]; !ok {
availReason.WriteByte(' ')
availReason.WriteString(EntitlementObKey)
}
return availReason.String()
}

// Obligations parses the returned obligations and returns them in standard format
func (o OPAResponse) Obligations() (*ObligationsNode, error) {
if obIfc, ok := o[string(ObKey)]; ok {
Expand Down
76 changes: 62 additions & 14 deletions grpc_opa/authorizer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,13 @@ import (
"encoding/json"
"errors"
"fmt"
"strings"
"reflect"
"strings"
"testing"

"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
logrus "github.com/sirupsen/logrus"
logrustesthook "github.com/sirupsen/logrus/hooks/test"
)

func TestRedactJWT(t *testing.T) {
Expand All @@ -30,6 +34,7 @@ func Test_parseEndpoint(t *testing.T) {
func Test_addObligations(t *testing.T) {
for idx, tst := range obligationsNodeTests {
ctx := context.Background()
ctx = ctxlogrus.ToContext(ctx, logrus.NewEntry(logrus.StandardLogger()))
var opaResp OPAResponse

err := json.Unmarshal([]byte(tst.regoRespJSON), &opaResp)
Expand All @@ -47,7 +52,7 @@ func Test_addObligations(t *testing.T) {
idx, tst.expectedErr, actualErr)
}

actualVal, _ := newCtx.Value(ObKey).(*ObligationsNode)
actualVal, _ := newCtx.Value(ObKey).(*ObligationsNode)
if actualVal != nil {
t.Logf("tst#%d: before DeepSort: %s", idx, actualVal)
actualVal.DeepSort()
Expand Down Expand Up @@ -99,30 +104,54 @@ func TestAffirmAuthorization(t *testing.T) {
claimsVerifier = nullClaimsVerifier

testMap := []struct {
name string
opaEvaltor OpaEvaluator
expectCtx bool
expectedErr error
name string
opaEvaltor OpaEvaluator
expectCtx bool
expectedErr error
forbiddenLog string
}{
{
name: "authz permitted, nil opa error",
opaEvaltor: func(ctx context.Context, decisionDocument string, opaReq, opaResp interface{}) error {
respJSON := fmt.Sprintf(`{"allow": %s}`, "true")
respJSON := fmt.Sprintf(`{"allow": %s, "obligations": {}}`, "true")
json.Unmarshal([]byte(respJSON), opaResp)
return nil
},
expectCtx: true,
expectedErr: nil,
},
{
name: "authz denied, nil opa error",
name: "authz denied, nil opa error, both rbac checks failed",
opaEvaltor: func(ctx context.Context, decisionDocument string, opaReq, opaResp interface{}) error {
respJSON := fmt.Sprintf(`{"allow": %s}`, "false")
respJSON := fmt.Sprintf(`{"allow": %s, "obligations": {}}`, "false")
json.Unmarshal([]byte(respJSON), opaResp)
return nil
},
expectCtx: false,
expectedErr: ErrForbidden,
expectCtx: false,
expectedErr: ErrForbidden,
forbiddenLog: `Request forbidden because these RBAC checks failed: authz.rbac.rbac authz.rbac.entitlement`,
},
{
name: "authz denied, nil opa error, rbac.rbac check ok",
opaEvaltor: func(ctx context.Context, decisionDocument string, opaReq, opaResp interface{}) error {
respJSON := fmt.Sprintf(`{"allow": %s, "obligations": {"authz.rbac.rbac": {}}}`, "false")
json.Unmarshal([]byte(respJSON), opaResp)
return nil
},
expectCtx: false,
expectedErr: ErrForbidden,
forbiddenLog: `Request forbidden because these RBAC checks failed: authz.rbac.entitlement`,
},
{
name: "authz denied, nil opa error, rbac.entitlement check ok",
opaEvaltor: func(ctx context.Context, decisionDocument string, opaReq, opaResp interface{}) error {
respJSON := fmt.Sprintf(`{"allow": %s, "obligations": {"authz.rbac.entitlement": {}}}`, "false")
json.Unmarshal([]byte(respJSON), opaResp)
return nil
},
expectCtx: false,
expectedErr: ErrForbidden,
forbiddenLog: `Request forbidden because these RBAC checks failed: authz.rbac.rbac`,
},
{
name: "bogus opa response, nil opa error",
Expand All @@ -131,13 +160,14 @@ func TestAffirmAuthorization(t *testing.T) {
json.Unmarshal([]byte(respJSON), opaResp)
return nil
},
expectCtx: false,
expectedErr: ErrForbidden,
expectCtx: false,
expectedErr: ErrForbidden,
forbiddenLog: `Request forbidden because these RBAC checks failed:`,
},
{
name: "opa error",
opaEvaltor: func(ctx context.Context, decisionDocument string, opaReq, opaResp interface{}) error {
respJSON := fmt.Sprintf(`{"allow": %s}`, "true")
respJSON := fmt.Sprintf(`{"allow": %s, "obligations": {}}`, "true")
json.Unmarshal([]byte(respJSON), opaResp)
return ErrBoom
},
Expand All @@ -146,12 +176,16 @@ func TestAffirmAuthorization(t *testing.T) {
},
}

loggertesthook := logrustesthook.NewGlobal()
ctx := context.WithValue(context.Background(), TestingTContextKey, t)
ctx = ctxlogrus.ToContext(ctx, logrus.NewEntry(logrus.StandardLogger()))

for nth, tm := range testMap {
auther := NewDefaultAuthorizer("app", WithOpaEvaluator(tm.opaEvaltor))

loggertesthook.Reset()
resultCtx, resultErr := auther.AffirmAuthorization(ctx, "FakeMethod", nil)

if resultErr != tm.expectedErr {
t.Errorf("%d: %q: got error: %s, wanted error: %s", nth, tm.name, resultErr, tm.expectedErr)
}
Expand All @@ -161,5 +195,19 @@ func TestAffirmAuthorization(t *testing.T) {
if resultErr != nil && resultCtx != nil {
t.Errorf("%d: %q: returned ctx should be nil if err returned", nth, tm.name)
}

if resultErr == tm.expectedErr && tm.expectedErr == ErrForbidden {
gotExpectedForbiddenLogMsg := false
for _, entry := range loggertesthook.AllEntries() {
t.Logf("%d: logrus.Entry.Message: %s", nth, entry.Message)
if entry.Message == tm.forbiddenLog {
gotExpectedForbiddenLogMsg = true
break
}
}
if !gotExpectedForbiddenLogMsg {
t.Errorf("%d: Did not get logrus.Entry.Message: `%s`", nth, tm.forbiddenLog)
}
}
}
}