Skip to content

Commit

Permalink
feat(idtoken-auth): add auth validator using google idtoken (#349)
Browse files Browse the repository at this point in the history 
* feat(idtoken-auth): add auth validator using google idtoken

* docs: add idtoken validation on sample config.yaml

* feat: add auth config to replace idtoken validator config, and refactor default auth header key

* refactor: move auth interceptor to pkg/auth

* chore: delete idtoken validator mock

* feat: keep config.AuthenticatedUserHeaderKey for backward-compatibility

* chore: re-order import

* refactor: change sample config.yaml for auth config

* chore: make auth user header key sample and default config consistent

* chore: re-order import on oidc test

* fix: change oidc initialism

* fix: oidc validator mocks renaming

* fix: avoid using params with pointer, as it will lead to panic if params is nil

* fix: detect old auth user header key using empty string instead of default tag

* refactor: move OIDCAuth to pkg/auth

* refactor: move oidc validator mocks into pkg/auth/mocks

* chore: add deprecation notes on AuthenticatedUserHeaderKey

* refactor: make default auth email context key back to unexported, use different context key for oidc email

* refactor: do not use default header key on oidc auth, use its own header (it's only used for logrus)

* refactor: move logrus context custom fields to new interceptor and retrieve its value from context

* refactor: auth email context key mapping for default and oidc used for grpc server

* test: change ways of getting user email from request header to context

* refactor: use auth.OIDCAuth instead of OIDCValidatorParams
  • Loading branch information
@swallowstalker
swallowstalker authored Feb 16, 2023
1 parent 575dfc7 commit a4b0c5d
Show file tree
Hide file tree
Showing 12 changed files with 448 additions and 113 deletions.
42 changes: 7 additions & 35 deletions api/handler/v1beta1/appeal_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,7 @@ func (s *GrpcHandlersSuite) TestListUserAppeals() {
ResourceUrns: []string{"test-resource-urn"},
OrderBy: []string{"test-order"},
}
ctx := context.Background()
md := metadata.New(map[string]string{
s.authenticatedUserHeaderKey: expectedUser,
})
ctx = metadata.NewIncomingContext(ctx, md)
ctx := context.WithValue(context.Background(), authEmailTestContextKey{}, expectedUser)
res, err := s.grpcServer.ListUserAppeals(ctx, req)

s.NoError(err)
Expand Down Expand Up @@ -162,11 +158,7 @@ func (s *GrpcHandlersSuite) TestListUserAppeals() {
Return(nil, expectedError).Once()

req := &guardianv1beta1.ListUserAppealsRequest{}
ctx := context.Background()
md := metadata.New(map[string]string{
s.authenticatedUserHeaderKey: "test-user",
})
ctx = metadata.NewIncomingContext(ctx, md)
ctx := context.WithValue(context.Background(), authEmailTestContextKey{}, "test-user")
res, err := s.grpcServer.ListUserAppeals(ctx, req)

s.Equal(codes.Internal, status.Code(err))
Expand All @@ -188,11 +180,7 @@ func (s *GrpcHandlersSuite) TestListUserAppeals() {
Return(invalidAppeals, nil).Once()

req := &guardianv1beta1.ListUserAppealsRequest{}
ctx := context.Background()
md := metadata.New(map[string]string{
s.authenticatedUserHeaderKey: "test-user",
})
ctx = metadata.NewIncomingContext(ctx, md)
ctx := context.WithValue(context.Background(), authEmailTestContextKey{}, "test-user")
res, err := s.grpcServer.ListUserAppeals(ctx, req)

s.Equal(codes.Internal, status.Code(err))
Expand Down Expand Up @@ -484,11 +472,7 @@ func (s *GrpcHandlersSuite) TestCreateAppeal() {
},
Description: "The answer is 42",
}
ctx := context.Background()
md := metadata.New(map[string]string{
s.authenticatedUserHeaderKey: expectedUser,
})
ctx = metadata.NewIncomingContext(ctx, md)
ctx := context.WithValue(context.Background(), authEmailTestContextKey{}, expectedUser)
res, err := s.grpcServer.CreateAppeal(ctx, req)

s.NoError(err)
Expand Down Expand Up @@ -520,11 +504,7 @@ func (s *GrpcHandlersSuite) TestCreateAppeal() {
s.appealService.EXPECT().Create(mock.AnythingOfType("*context.valueCtx"), mock.Anything).Return(appeal.ErrAppealDuplicate).Once()

req := &guardianv1beta1.CreateAppealRequest{}
ctx := context.Background()
md := metadata.New(map[string]string{
s.authenticatedUserHeaderKey: "[email protected]",
})
ctx = metadata.NewIncomingContext(ctx, md)
ctx := context.WithValue(context.Background(), authEmailTestContextKey{}, "[email protected]")
res, err := s.grpcServer.CreateAppeal(ctx, req)

s.Equal(codes.AlreadyExists, status.Code(err))
Expand All @@ -539,11 +519,7 @@ func (s *GrpcHandlersSuite) TestCreateAppeal() {
s.appealService.EXPECT().Create(mock.AnythingOfType("*context.valueCtx"), mock.Anything).Return(expectedError).Once()

req := &guardianv1beta1.CreateAppealRequest{}
ctx := context.Background()
md := metadata.New(map[string]string{
s.authenticatedUserHeaderKey: "[email protected]",
})
ctx = metadata.NewIncomingContext(ctx, md)
ctx := context.WithValue(context.Background(), authEmailTestContextKey{}, "[email protected]")
res, err := s.grpcServer.CreateAppeal(ctx, req)

s.Equal(codes.Internal, status.Code(err))
Expand All @@ -567,11 +543,7 @@ func (s *GrpcHandlersSuite) TestCreateAppeal() {
Return(nil).Once()

req := &guardianv1beta1.CreateAppealRequest{Resources: make([]*guardianv1beta1.CreateAppealRequest_Resource, 1)}
ctx := context.Background()
md := metadata.New(map[string]string{
s.authenticatedUserHeaderKey: "[email protected]",
})
ctx = metadata.NewIncomingContext(ctx, md)
ctx := context.WithValue(context.Background(), authEmailTestContextKey{}, "[email protected]")
res, err := s.grpcServer.CreateAppeal(ctx, req)

s.Equal(codes.Internal, status.Code(err))
Expand Down
36 changes: 6 additions & 30 deletions api/handler/v1beta1/approval_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,11 +116,7 @@ func (s *GrpcHandlersSuite) TestListUserApprovals() {
Statuses: []string{"active", "pending"},
OrderBy: []string{"test-order"},
}
ctx := context.Background()
md := metadata.New(map[string]string{
s.authenticatedUserHeaderKey: expectedUser,
})
ctx = metadata.NewIncomingContext(ctx, md)
ctx := context.WithValue(context.Background(), authEmailTestContextKey{}, expectedUser)
res, err := s.grpcServer.ListUserApprovals(ctx, req)

s.NoError(err)
Expand Down Expand Up @@ -150,11 +146,7 @@ func (s *GrpcHandlersSuite) TestListUserApprovals() {
Return(nil, expectedError).Once()

req := &guardianv1beta1.ListUserApprovalsRequest{}
ctx := context.Background()
md := metadata.New(map[string]string{
s.authenticatedUserHeaderKey: "test-user",
})
ctx = metadata.NewIncomingContext(ctx, md)
ctx := context.WithValue(context.Background(), authEmailTestContextKey{}, "test-user")
res, err := s.grpcServer.ListUserApprovals(ctx, req)

s.Equal(codes.Internal, status.Code(err))
Expand All @@ -178,11 +170,7 @@ func (s *GrpcHandlersSuite) TestListUserApprovals() {
Return(invalidApprovals, nil).Once()

req := &guardianv1beta1.ListUserApprovalsRequest{}
ctx := context.Background()
md := metadata.New(map[string]string{
s.authenticatedUserHeaderKey: "test-user",
})
ctx = metadata.NewIncomingContext(ctx, md)
ctx := context.WithValue(context.Background(), authEmailTestContextKey{}, "test-user")
res, err := s.grpcServer.ListUserApprovals(ctx, req)

s.Equal(codes.Internal, status.Code(err))
Expand Down Expand Up @@ -443,11 +431,7 @@ func (s *GrpcHandlersSuite) TestUpdateApproval() {
Reason: expectedReason,
},
}
ctx := context.Background()
md := metadata.New(map[string]string{
s.authenticatedUserHeaderKey: expectedUser,
})
ctx = metadata.NewIncomingContext(ctx, md)
ctx := context.WithValue(context.Background(), authEmailTestContextKey{}, expectedUser)
res, err := s.grpcServer.UpdateApproval(ctx, req)

s.NoError(err)
Expand Down Expand Up @@ -553,11 +537,7 @@ func (s *GrpcHandlersSuite) TestUpdateApproval() {
Return(nil, tc.expectedError).Once()

req := &guardianv1beta1.UpdateApprovalRequest{}
ctx := context.Background()
md := metadata.New(map[string]string{
s.authenticatedUserHeaderKey: expectedUser,
})
ctx = metadata.NewIncomingContext(ctx, md)
ctx := context.WithValue(context.Background(), authEmailTestContextKey{}, expectedUser)
res, err := s.grpcServer.UpdateApproval(ctx, req)

s.Equal(tc.expectedStatusCode, status.Code(err))
Expand All @@ -579,11 +559,7 @@ func (s *GrpcHandlersSuite) TestUpdateApproval() {
Return(invalidAppeal, nil).Once()

req := &guardianv1beta1.UpdateApprovalRequest{}
ctx := context.Background()
md := metadata.New(map[string]string{
s.authenticatedUserHeaderKey: "[email protected]",
})
ctx = metadata.NewIncomingContext(ctx, md)
ctx := context.WithValue(context.Background(), authEmailTestContextKey{}, "[email protected]")
res, err := s.grpcServer.UpdateApproval(ctx, req)

s.Equal(codes.Internal, status.Code(err))
Expand Down
39 changes: 19 additions & 20 deletions api/handler/v1beta1/grpc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ package v1beta1

import (
"context"
"errors"
"strings"

"github.com/odpf/guardian/core/appeal"
"github.com/odpf/guardian/core/grant"

guardianv1beta1 "github.com/odpf/guardian/api/proto/odpf/guardian/v1beta1"
"github.com/odpf/guardian/domain"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
)

type ProtoAdapter interface {
Expand Down Expand Up @@ -116,7 +117,7 @@ type GRPCServer struct {
grantService grantService
adapter ProtoAdapter

authenticatedUserHeaderKey string
authenticatedUserContextKey interface{}

guardianv1beta1.UnimplementedGuardianServiceServer
}
Expand All @@ -130,32 +131,30 @@ func NewGRPCServer(
approvalService approvalService,
grantService grantService,
adapter ProtoAdapter,
authenticatedUserHeaderKey string,
authenticatedUserContextKey interface{},
) *GRPCServer {
return &GRPCServer{
resourceService: resourceService,
activityService: activityService,
providerService: providerService,
policyService: policyService,
appealService: appealService,
approvalService: approvalService,
grantService: grantService,
adapter: adapter,
authenticatedUserHeaderKey: authenticatedUserHeaderKey,
resourceService: resourceService,
activityService: activityService,
providerService: providerService,
policyService: policyService,
appealService: appealService,
approvalService: approvalService,
grantService: grantService,
adapter: adapter,
authenticatedUserContextKey: authenticatedUserContextKey,
}
}

func (s *GRPCServer) getUser(ctx context.Context) (string, error) {
md, ok := metadata.FromIncomingContext(ctx)
authenticatedEmail, ok := ctx.Value(s.authenticatedUserContextKey).(string)
if !ok {
return "", errors.New("unable to retrieve metadata from context")
return "", status.Error(codes.Unauthenticated, "unable to get authenticated user from context")
}

users := md.Get(s.authenticatedUserHeaderKey)
if len(users) == 0 {
return "", errors.New("user email not found")
if strings.TrimSpace(authenticatedEmail) == "" {
return "", status.Error(codes.Unauthenticated, "unable to get authenticated user from context")
}

currentUser := users[0]
return currentUser, nil
return authenticatedEmail, nil
}
7 changes: 3 additions & 4 deletions api/handler/v1beta1/grpc_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"github.com/stretchr/testify/suite"
)

type authEmailTestContextKey struct{}

type GrpcHandlersSuite struct {
suite.Suite

Expand All @@ -19,8 +21,6 @@ type GrpcHandlersSuite struct {
approvalService *mocks.ApprovalService
grantService *mocks.GrantService
grpcServer *v1beta1.GRPCServer

authenticatedUserHeaderKey string
}

func TestGrpcHandler(t *testing.T) {
Expand All @@ -35,7 +35,6 @@ func (s *GrpcHandlersSuite) setup() {
s.appealService = new(mocks.AppealService)
s.approvalService = new(mocks.ApprovalService)
s.grantService = new(mocks.GrantService)
s.authenticatedUserHeaderKey = "test-header-key"
s.grpcServer = v1beta1.NewGRPCServer(
s.resourceService,
s.activityService,
Expand All @@ -45,6 +44,6 @@ func (s *GrpcHandlersSuite) setup() {
s.approvalService,
s.grantService,
v1beta1.NewAdapter(),
s.authenticatedUserHeaderKey,
authEmailTestContextKey{},
)
}
18 changes: 14 additions & 4 deletions internal/server/auth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,29 @@ import (

type authenticatedUserEmailContextKey struct{}

var logrusActorKey = "actor"

func withAuthenticatedUserEmail(headerKey string) grpc.UnaryServerInterceptor {
return func(ctx context.Context, req interface{}, _ *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
if md, ok := metadata.FromIncomingContext(ctx); ok {
if v := md.Get(headerKey); len(v) > 0 {
userEmail := v[0]
ctx = context.WithValue(ctx, authenticatedUserEmailContextKey{}, userEmail)

ctx_logrus.AddFields(ctx, logrus.Fields{
headerKey: userEmail,
})
}
}

return handler(ctx, req)
}
}

func withLogrusContext() grpc.UnaryServerInterceptor {
return func(ctx context.Context, req interface{}, _ *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
if userEmail, ok := ctx.Value(authenticatedUserEmailContextKey{}).(string); ok {
ctx_logrus.AddFields(ctx, logrus.Fields{
logrusActorKey: userEmail,
})
}

return handler(ctx, req)
}
}
37 changes: 28 additions & 9 deletions internal/server/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"

"github.com/odpf/guardian/internal/store"
"github.com/odpf/guardian/pkg/auth"
"github.com/odpf/guardian/pkg/tracing"
"github.com/odpf/guardian/plugins/notifiers"
"github.com/odpf/salt/config"
Expand Down Expand Up @@ -33,16 +34,28 @@ type Jobs struct {
ExpiringAccessNotification JobConfig `mapstructure:"expiring_access_notification"`
}

type DefaultAuth struct {
HeaderKey string `mapstructure:"header_key" default:"X-Auth-Email"`
}

type Auth struct {
Provider string `mapstructure:"provider" default:"default"`
Default DefaultAuth `mapstructure:"default"`
OIDC auth.OIDCAuth `mapstructure:"oidc"`
}

type Config struct {
Port int `mapstructure:"port" default:"8080"`
EncryptionSecretKeyKey string `mapstructure:"encryption_secret_key"`
Notifier notifiers.Config `mapstructure:"notifier"`
LogLevel string `mapstructure:"log_level" default:"info"`
DB store.Config `mapstructure:"db"`
AuthenticatedUserHeaderKey string `mapstructure:"authenticated_user_header_key"`
AuditLogTraceIDHeaderKey string `mapstructure:"audit_log_trace_id_header_key" default:"X-Trace-Id"`
Jobs Jobs `mapstructure:"jobs"`
Telemetry tracing.Config `mapstructure:"telemetry"`
Port int `mapstructure:"port" default:"8080"`
EncryptionSecretKeyKey string `mapstructure:"encryption_secret_key"`
Notifier notifiers.Config `mapstructure:"notifier"`
LogLevel string `mapstructure:"log_level" default:"info"`
DB store.Config `mapstructure:"db"`
// Deprecated: use Auth.Default.HeaderKey instead note on the AuthenticatedUserHeaderKey
AuthenticatedUserHeaderKey string `mapstructure:"authenticated_user_header_key"`
AuditLogTraceIDHeaderKey string `mapstructure:"audit_log_trace_id_header_key" default:"X-Trace-Id"`
Jobs Jobs `mapstructure:"jobs"`
Telemetry tracing.Config `mapstructure:"telemetry"`
Auth Auth `mapstructure:"auth"`
}

func LoadConfig(configFile string) (Config, error) {
Expand All @@ -56,5 +69,11 @@ func LoadConfig(configFile string) (Config, error) {
}
return Config{}, err
}

// keep for backward-compatibility
if cfg.AuthenticatedUserHeaderKey != "" {
cfg.Auth.Default.HeaderKey = cfg.AuthenticatedUserHeaderKey
}

return cfg, nil
}
11 changes: 9 additions & 2 deletions internal/server/config.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

PORT: 3000
ENCRYPTION_SECRET_KEY:
AUTHENTICATED_USER_HEADER_KEY: X-User-Email
AUTHENTICATED_USER_HEADER_KEY: X-Auth-Email
LOG:
LEVEL: info
DB:
Expand Down Expand Up @@ -42,4 +42,11 @@ TELEMETRY:
OTLP:
HEADERS:
api-key: <YOUR-LICENSE-KEY>
ENDPOINT: "otlp.nr-data.net:4317"
ENDPOINT: "otlp.nr-data.net:4317"
AUTH:
PROVIDER: default # can be "default" or "oidc"
DEFAULT:
HEADER_KEY: X-Auth-Email # AUTHENTICATED_USER_HEADER_KEY takes priority for backward-compatibility
OIDC:
AUDIENCE: "some-kind-of-audience.com"
ELIGIBLE_EMAIL_DOMAINS: "emaildomain1.com,emaildomain2.com"
Loading

0 comments on commit a4b0c5d

Please sign in to comment.