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.

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

proposal: add banneduser middleware #455

Merged
merged 21 commits into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
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
1 change: 1 addition & 0 deletions pkg/application/service/services.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ type InformerService interface {
ListSpaceBindings(reqs ...labels.Requirement) ([]toolchainv1alpha1.SpaceBinding, error)
GetProxyPluginConfig(name string) (*toolchainv1alpha1.ProxyPlugin, error)
GetNSTemplateTier(name string) (*toolchainv1alpha1.NSTemplateTier, error)
ListBannedUsersByEmail(email string) ([]toolchainv1alpha1.BannedUser, error)
}

type SignupService interface {
Expand Down
24 changes: 17 additions & 7 deletions pkg/informers/informers.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package informers
import (
"fmt"

toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
"github.com/codeready-toolchain/registration-service/pkg/configuration"
"github.com/codeready-toolchain/registration-service/pkg/kubeclient/resources"
"github.com/codeready-toolchain/registration-service/pkg/log"
Expand All @@ -23,9 +24,12 @@ type Informer struct {
UserSignup cache.GenericLister
ProxyPluginConfig cache.GenericLister
NSTemplateTier cache.GenericLister
BannedUsers cache.GenericLister
}

func StartInformer(cfg *rest.Config) (*Informer, chan struct{}, error) {
group := toolchainv1alpha1.GroupVersion.Group
version := toolchainv1alpha1.GroupVersion.Version

informer := &Informer{}
dynamicClient, err := dynamic.NewForConfig(cfg)
Expand All @@ -36,40 +40,45 @@ func StartInformer(cfg *rest.Config) (*Informer, chan struct{}, error) {
factory := dynamicinformer.NewFilteredDynamicSharedInformerFactory(dynamicClient, 0, configuration.Namespace(), nil)

// MasterUserRecords
genericMasterUserRecordInformer := factory.ForResource(schema.GroupVersionResource{Group: "toolchain.dev.openshift.com", Version: "v1alpha1", Resource: resources.MurResourcePlural})
genericMasterUserRecordInformer := factory.ForResource(schema.GroupVersionResource{Group: group, Version: version, Resource: resources.MurResourcePlural})
informer.Masteruserrecord = genericMasterUserRecordInformer.Lister()
masterUserRecordInformer := genericMasterUserRecordInformer.Informer()

// Space
genericSpaceInformer := factory.ForResource(schema.GroupVersionResource{Group: "toolchain.dev.openshift.com", Version: "v1alpha1", Resource: resources.SpaceResourcePlural})
genericSpaceInformer := factory.ForResource(schema.GroupVersionResource{Group: group, Version: version, Resource: resources.SpaceResourcePlural})
informer.Space = genericSpaceInformer.Lister()
spaceInformer := genericSpaceInformer.Informer()

// SpaceBinding
genericSpaceBindingInformer := factory.ForResource(schema.GroupVersionResource{Group: "toolchain.dev.openshift.com", Version: "v1alpha1", Resource: resources.SpaceBindingResourcePlural})
genericSpaceBindingInformer := factory.ForResource(schema.GroupVersionResource{Group: group, Version: version, Resource: resources.SpaceBindingResourcePlural})
informer.SpaceBinding = genericSpaceBindingInformer.Lister()
spaceBindingInformer := genericSpaceBindingInformer.Informer()

// ToolchainStatus
genericToolchainStatusInformer := factory.ForResource(schema.GroupVersionResource{Group: "toolchain.dev.openshift.com", Version: "v1alpha1", Resource: resources.ToolchainStatusPlural})
genericToolchainStatusInformer := factory.ForResource(schema.GroupVersionResource{Group: group, Version: version, Resource: resources.ToolchainStatusPlural})
informer.ToolchainStatus = genericToolchainStatusInformer.Lister()
toolchainstatusInformer := genericToolchainStatusInformer.Informer()

// UserSignups
genericUserSignupInformer := factory.ForResource(schema.GroupVersionResource{Group: "toolchain.dev.openshift.com", Version: "v1alpha1", Resource: resources.UserSignupResourcePlural})
genericUserSignupInformer := factory.ForResource(schema.GroupVersionResource{Group: group, Version: version, Resource: resources.UserSignupResourcePlural})
informer.UserSignup = genericUserSignupInformer.Lister()
userSignupInformer := genericUserSignupInformer.Informer()

// Proxy plugins
proxyPluginInformer := factory.ForResource(schema.GroupVersionResource{Group: "toolchain.dev.openshift.com", Version: "v1alpha1", Resource: resources.ProxyPluginsPlural})
proxyPluginInformer := factory.ForResource(schema.GroupVersionResource{Group: group, Version: version, Resource: resources.ProxyPluginsPlural})
informer.ProxyPluginConfig = proxyPluginInformer.Lister()
proxyPluginConfigInformer := proxyPluginInformer.Informer()

// NSTemplateTier plugins
genericNSTemplateTierInformer := factory.ForResource(schema.GroupVersionResource{Group: "toolchain.dev.openshift.com", Version: "v1alpha1", Resource: resources.NSTemplateTierPlural})
genericNSTemplateTierInformer := factory.ForResource(schema.GroupVersionResource{Group: group, Version: version, Resource: resources.NSTemplateTierPlural})
informer.NSTemplateTier = genericNSTemplateTierInformer.Lister()
nsTemplateTierInformer := genericNSTemplateTierInformer.Informer()

// BannedUsers
genericBannedUsersInformer := factory.ForResource(schema.GroupVersionResource{Group: group, Version: version, Resource: resources.BannedUserResourcePlural})
informer.BannedUsers = genericBannedUsersInformer.Lister()
bannedUsersInformer := genericBannedUsersInformer.Informer()

stopper := make(chan struct{})

log.Info(nil, "Starting proxy cache informers")
Expand All @@ -83,6 +92,7 @@ func StartInformer(cfg *rest.Config) (*Informer, chan struct{}, error) {
userSignupInformer.HasSynced,
proxyPluginConfigInformer.HasSynced,
nsTemplateTierInformer.HasSynced,
bannedUsersInformer.HasSynced,
) {
err := fmt.Errorf("timed out waiting for caches to sync")
log.Error(nil, err, "Failed to create informers")
Expand Down
32 changes: 32 additions & 0 deletions pkg/informers/service/informer_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@
"github.com/codeready-toolchain/registration-service/pkg/informers"
"github.com/codeready-toolchain/registration-service/pkg/kubeclient/resources"
"github.com/codeready-toolchain/registration-service/pkg/log"
"github.com/codeready-toolchain/toolchain-common/pkg/hash"

"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/selection"
)

type Option func(f *ServiceImpl)
Expand Down Expand Up @@ -145,3 +147,33 @@
}
return tier, err
}

func (s *ServiceImpl) ListBannedUsersByEmail(email string) ([]toolchainv1alpha1.BannedUser, error) {
hashedEmail := hash.EncodeString(email)
r, err := labels.NewRequirement(toolchainv1alpha1.BannedUserEmailHashLabelKey, selection.Equals, []string{hashedEmail})
if err != nil {
return nil, err

Check warning on line 155 in pkg/informers/service/informer_service.go

View check run for this annotation

Codecov / codecov/patch

pkg/informers/service/informer_service.go#L155

Added line #L155 was not covered by tests
}

ls := labels.NewSelector().Add(*r)
objs, err := s.informer.BannedUsers.ByNamespace(configuration.Namespace()).List(ls)
if err != nil {
return nil, err

Check warning on line 161 in pkg/informers/service/informer_service.go

View check run for this annotation

Codecov / codecov/patch

pkg/informers/service/informer_service.go#L161

Added line #L161 was not covered by tests
}

bb := []toolchainv1alpha1.BannedUser{}
for _, obj := range objs {
unobj := obj.(*unstructured.Unstructured)
Copy link
Contributor

@rsoaresd rsoaresd Sep 11, 2024

Choose a reason for hiding this comment

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

Should we check here the type assertion?

unobj, ok := obj.(*unstructured.Unstructured)
if !ok { 
     .... 
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this can happen. If I'm not mistaken it's stored internally as *unstructured.Unstructured (cf. https://github.com/kubernetes/client-go/blob/2666bd29867c96168c5e9429fd74fd9bfbedac8b/dynamic/dynamicinformer/informer.go#L138). Also I can not immagine how to write a test for covering this check.

The other functions in this file are using an unchecked casting like this too. I'd say that -if we find out it to be a problem- we can create a PR for fixing this in all the functions. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood! Thank you :)

bu := &toolchainv1alpha1.BannedUser{}
if err := runtime.DefaultUnstructuredConverter.FromUnstructured(unobj.UnstructuredContent(), bu); err != nil {
log.Errorf(nil, err, "failed to list BannedUsers")
return nil, err

Check warning on line 170 in pkg/informers/service/informer_service.go

View check run for this annotation

Codecov / codecov/patch

pkg/informers/service/informer_service.go#L169-L170

Added lines #L169 - L170 were not covered by tests
}

// check in case of hash collision
if bu.Spec.Email == email {
bb = append(bb, *bu)
}
}
return bb, nil
}
alexeykazakov marked this conversation as resolved.
Show resolved Hide resolved
116 changes: 114 additions & 2 deletions pkg/informers/service/informer_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,18 @@ import (

toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
appservice "github.com/codeready-toolchain/registration-service/pkg/application/service"
"github.com/codeready-toolchain/registration-service/pkg/configuration"
"github.com/codeready-toolchain/registration-service/pkg/informers"
"github.com/codeready-toolchain/registration-service/pkg/informers/service"
"github.com/codeready-toolchain/registration-service/pkg/kubeclient"
"github.com/codeready-toolchain/registration-service/test"
"github.com/codeready-toolchain/toolchain-common/pkg/hash"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -386,6 +389,108 @@ func (s *TestInformerServiceSuite) TestInformerService() {
})
})

s.Run("bannedusers", func() {
// given
bb := map[string]toolchainv1alpha1.BannedUser{
"alice": {
ObjectMeta: metav1.ObjectMeta{
Name: "alice",
Namespace: configuration.Namespace(),
Labels: map[string]string{
toolchainv1alpha1.BannedUserEmailHashLabelKey: hash.EncodeString("alice@redhat.com"),
},
},
Spec: toolchainv1alpha1.BannedUserSpec{
Email: "alice@redhat.com",
},
},
"bob": {
ObjectMeta: metav1.ObjectMeta{
Name: "bob",
Namespace: configuration.Namespace(),
Labels: map[string]string{
toolchainv1alpha1.BannedUserEmailHashLabelKey: hash.EncodeString("bob@redhat.com"),
},
},
Spec: toolchainv1alpha1.BannedUserSpec{
Email: "bob@redhat.com",
},
},
"bob-dup": {
ObjectMeta: metav1.ObjectMeta{
Name: "bob-dup",
Namespace: configuration.Namespace(),
Labels: map[string]string{
toolchainv1alpha1.BannedUserEmailHashLabelKey: hash.EncodeString("bob@redhat.com"),
},
},
Spec: toolchainv1alpha1.BannedUserSpec{
Email: "bob@redhat.com",
},
},
}

// convert to unstructured.Unstructured for fakeLister
bbu := make(map[string]*unstructured.Unstructured, len(bb))
for k, b := range bb {
bu, err := runtime.DefaultUnstructuredConverter.ToUnstructured(b.DeepCopy())
require.NoError(s.T(), err)
bbu[k] = &unstructured.Unstructured{Object: bu}
}

// setup InformerService with fakeLister
inf := informers.Informer{BannedUsers: fakeLister{objs: bbu}}
svc := service.NewInformerService(fakeInformerServiceContext{
Svcs: s.Application,
informer: inf,
})

s.Run("not found", func() {
// when
rbb, err := svc.ListBannedUsersByEmail("unknown@unknown.com")

// then
require.NoError(s.T(), err)
require.Empty(s.T(), rbb)
})

s.Run("invalid email", func() {
// when
rbb, err := svc.ListBannedUsersByEmail("not-an-email")

// then
require.NoError(s.T(), err)
require.Empty(s.T(), rbb)
})

s.Run("found one", func() {
// given
expected := bb["alice"]

// when
rbb, err := svc.ListBannedUsersByEmail(expected.Spec.Email)

// then
require.NotNil(s.T(), rbb)
require.NoError(s.T(), err)
require.Len(s.T(), rbb, 1, "expected 1 result for email %s", expected.Spec.Email)
require.Equal(s.T(), expected, rbb[0])
})

s.Run("found multiple", func() {
// given
expected := []toolchainv1alpha1.BannedUser{bb["bob"], bb["bob-dup"]}

// when
rbb, err := svc.ListBannedUsersByEmail(expected[0].Spec.Email)

// then
require.NotNil(s.T(), rbb)
require.NoError(s.T(), err)
require.Len(s.T(), rbb, 2, "expected 2 results for email %s", expected[0].Spec.Email)
require.Equal(s.T(), expected, rbb)
})
})
}

type fakeInformerServiceContext struct {
Expand All @@ -410,8 +515,15 @@ type fakeLister struct {
}

// List will return all objects across namespaces
func (l fakeLister) List(_ labels.Selector) (ret []runtime.Object, err error) {
return nil, nil
func (l fakeLister) List(ls labels.Selector) (ret []runtime.Object, err error) {
objs := []runtime.Object{}
for _, o := range l.objs {
ll := labels.Set(o.GetLabels())
if ls.Matches(ll) {
objs = append(objs, o)
}
}
return objs, nil
}

// Get will attempt to retrieve assuming that name==key
Expand Down
1 change: 1 addition & 0 deletions pkg/kubeclient/resources/names.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,5 @@ const (
ToolchainStatusName = "toolchain-status"
ProxyPluginsPlural = "proxyplugins"
NSTemplateTierPlural = "nstemplatetiers"
BannedUserResourcePlural = "bannedusers"
)
51 changes: 43 additions & 8 deletions pkg/proxy/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@
return next(ctx)
}
},
p.ensureUserIsNotBanned(),
)

// middleware after routing
Expand Down Expand Up @@ -391,18 +392,52 @@
return next(ctx)
}

userID, username, err := p.extractUserID(ctx.Request())
token, err := p.extractUserToken(ctx.Request())
if err != nil {
return crterrors.NewUnauthorizedError("invalid bearer token", err.Error())
}
ctx.Set(context.SubKey, userID)
ctx.Set(context.UsernameKey, username)
ctx.Set(context.SubKey, token.Subject)
ctx.Set(context.UsernameKey, token.PreferredUsername)
ctx.Set(context.EmailKey, token.Email)

return next(ctx)
}
}
}

// ensureUserIsNotBanned rejects the request if the user is banned.
// This Middleware requires the context to contain the email of the user,
// so it needs to be executed after the `addUserContext` Middleware.
func (p *Proxy) ensureUserIsNotBanned() echo.MiddlewareFunc {
return func(next echo.HandlerFunc) echo.HandlerFunc {
return func(ctx echo.Context) error {
if unsecured(ctx) { // skip only for unsecured endpoints
return next(ctx)
}

email := ctx.Get(context.EmailKey).(string)
alexeykazakov marked this conversation as resolved.
Show resolved Hide resolved
if email == "" {
return crterrors.NewUnauthorizedError("unauthenticated request", "invalid email in token")

Check warning on line 420 in pkg/proxy/proxy.go

View check run for this annotation

Codecov / codecov/patch

pkg/proxy/proxy.go#L420

Added line #L420 was not covered by tests
}

// retrieve banned users
uu, err := p.app.InformerService().ListBannedUsersByEmail(email)
if err != nil {
ctx.Logger().Errorf("error retrieving the list of banned users with email address %s: %v", email, err)
return crterrors.NewInternalError(errs.New("user access could not be verified"), "could not define user access")
}

// if a matching Banned user is found, then user is banned
if len(uu) > 0 {
return crterrors.NewForbiddenError("user access is forbidden", "user access is forbidden")
}

// user is not banned
return next(ctx)
}
}
}
alexeykazakov marked this conversation as resolved.
Show resolved Hide resolved

func (p *Proxy) stripInvalidHeaders() echo.MiddlewareFunc {
return func(next echo.HandlerFunc) echo.HandlerFunc {
return func(ctx echo.Context) error {
Expand Down Expand Up @@ -430,26 +465,26 @@
}
}

func (p *Proxy) extractUserID(req *http.Request) (string, string, error) {
func (p *Proxy) extractUserToken(req *http.Request) (*auth.TokenClaims, error) {
userToken := ""
var err error
if wsstream.IsWebSocketRequest(req) {
userToken, err = extractTokenFromWebsocketRequest(req)
if err != nil {
return "", "", err
return nil, err
}
} else {
userToken, err = extractUserToken(req)
if err != nil {
return "", "", err
return nil, err
}
}

token, err := p.tokenParser.FromString(userToken)
if err != nil {
return "", "", crterrors.NewUnauthorizedError("unable to extract userID from token", err.Error())
return nil, crterrors.NewUnauthorizedError("unable to extract userID from token", err.Error())
}
return token.Subject, token.PreferredUsername, nil
return token, nil
}

func extractUserToken(req *http.Request) (string, error) {
Expand Down
Loading
Loading