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

Fix auth routing logic #4266

Merged
merged 7 commits into from
Oct 19, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
6 changes: 6 additions & 0 deletions changelog/unreleased/auth-fixes.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Enhancement: improve authentication routing logic

Provides a safer approach to route requests, both in HTTP and gRPC land
when authentication is needed.

https://github.com/cs3org/reva/pull/4266
29 changes: 2 additions & 27 deletions internal/grpc/interceptors/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,23 +98,10 @@ func NewUnary(m map[string]interface{}, unprotected []string) (grpc.UnaryServerI

if utils.Skip(info.FullMethod, unprotected) {
log.Debug().Str("method", info.FullMethod).Msg("skipping auth")

// If a token is present, set it anyway, as we might need the user info
// to decide the storage provider.
tkn, ok := appctx.ContextGetToken(ctx)
if ok {
u, scopes, err := dismantleToken(ctx, tkn, req, tokenManager, conf.GatewayAddr, true)
if err == nil {
if blockedUsers.IsBlocked(u.Username) {
return nil, status.Errorf(codes.PermissionDenied, "user %s blocked", u.Username)
}
ctx = appctx.ContextSetUser(ctx, u)
ctx = appctx.ContextSetScopes(ctx, scopes)
}
}
return handler(ctx, req)
}

// from this point on, the method must be authenticated
tkn, ok := appctx.ContextGetToken(ctx)

if !ok || tkn == "" {
Expand All @@ -137,6 +124,7 @@ func NewUnary(m map[string]interface{}, unprotected []string) (grpc.UnaryServerI
ctx = appctx.ContextSetScopes(ctx, scopes)
return handler(ctx, req)
}

return interceptor, nil
}

Expand Down Expand Up @@ -171,19 +159,6 @@ func NewStream(m map[string]interface{}, unprotected []string) (grpc.StreamServe

if utils.Skip(info.FullMethod, unprotected) {
log.Debug().Str("method", info.FullMethod).Msg("skipping auth")

// If a token is present, set it anyway, as we might need the user info
// to decide the storage provider.
tkn, ok := appctx.ContextGetToken(ctx)
if ok {
u, scopes, err := dismantleToken(ctx, tkn, ss, tokenManager, conf.GatewayAddr, true)
if err == nil {
ctx = appctx.ContextSetUser(ctx, u)
ctx = appctx.ContextSetScopes(ctx, scopes)
ss = newWrappedServerStream(ctx, ss)
}
}

return handler(srv, ss)
}

Expand Down
37 changes: 36 additions & 1 deletion internal/grpc/services/gateway/gateway.go
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,42 @@ func (s *svc) Close() error {
}

func (s *svc) UnprotectedEndpoints() []string {
return []string{"/cs3.gateway.v1beta1.GatewayAPI"}
return []string{
"/cs3.gateway.v1beta1.GatewayAPI/ListShare",
"/cs3.gateway.v1beta1.GatewayAPI/GetAppPassword",
"/cs3.gateway.v1beta1.GatewayAPI/AddAppProvider",
"/cs3.gateway.v1beta1.GatewayAPI/ListSupportedMimeTypes",
"/cs3.gateway.v1beta1.GatewayAPI/Authenticate",
"/cs3.gateway.v1beta1.GatewayAPI/GetAuthProvider",
"/cs3.gateway.v1beta1.GatewayAPI/ListAuthProviders",
"/cs3.gateway.v1beta1.GatewayAPI/CreateOCMCoreShare",
"/cs3.gateway.v1beta1.GatewayAPI/AcceptInvite",
"/cs3.gateway.v1beta1.GatewayAPI/GetAcceptedUser",
"/cs3.gateway.v1beta1.GatewayAPI/IsProviderAllowed",
"/cs3.gateway.v1beta1.GatewayAPI/ListAllProviders",
"/cs3.gateway.v1beta1.GatewayAPI/GetOCMShareByToken",
"/cs3.gateway.v1beta1.GatewayAPI/GetPublicShareByToken",
"/cs3.gateway.v1beta1.GatewayAPI/GetUser",
"/cs3.gateway.v1beta1.GatewayAPI/GetUserByClaim",
"/cs3.gateway.v1beta1.GatewayAPI/GetUserGroups",

"/cs3.auth.applications.v1beta1.ApplicationsAPI/GetAppPassword",
"/cs3.app.registry.v1beta1.RegistryAPI/AddAppProvider",
"/cs3.app.registry.v1beta1.RegistryAPI/ListSupportedMimeTypes",
"/cs3.auth.provider.v1beta1.ProviderAPI/Authenticate",
"/cs3.auth.registry.v1beta1.RegistryAPI/GetAuthProvider",
"/cs3.auth.registry.v1beta1.RegistryAPI/ListAuthProviders",
"/cs3.ocm.core.v1beta1.OcmCoreAPI/CreateOCMCoreShare",
"/cs3.ocm.invite.v1beta1.InviteAPI/AcceptInvite",
"/cs3.ocm.invite.v1beta1.InviteAPI/GetAcceptedUser",
"/cs3.ocm.provider.v1beta1.ProviderAPI/IsProviderAllowed",
"/cs3.ocm.provider.v1beta1.ProviderAPI/ListAllProviders",
"/cs3.sharing.ocm.v1beta1.OcmAPI/GetOCMShareByToken",
"/cs3.sharing.link.v1beta1.LinkAPI/GetPublicShareByToken",
"/cs3.identity.user.v1beta1.UserAPI/GetUser",
"/cs3.identity.user.v1beta1.UserAPI/GetUserByClaim",
"/cs3.identity.user.v1beta1.UserAPI/GetUserGroups",
}
}

func getTokenManager(manager string, m map[string]map[string]interface{}) (token.Manager, error) {
Expand Down
4 changes: 1 addition & 3 deletions internal/http/interceptors/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,10 +168,8 @@ func New(m map[string]interface{}, unprotected []string) (global.Middleware, err

log := appctx.GetLogger(r.Context())

// For unprotected URLs, we try to authenticate the request in case some service needs it,
// but don't return any errors if it fails.
if utils.Skip(r.URL.Path, unprotected) {
log.Info().Msg("skipping auth check for: " + r.URL.Path)
log.Info().Interface("unprotected", unprotected).Msg("skipping auth check for: " + r.URL.Path)
} else {
ctx, err := authenticateUser(w, r, conf, tokenStrategyChain, tokenManager, tokenWriter, credChain, false)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion pkg/utils/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ var (
// i.e: /a/b/c/d/e contains prefix /a/b/c.
func Skip(source string, prefixes []string) bool {
for i := range prefixes {
if strings.HasPrefix(source, prefixes[i]) {
if strings.HasPrefix(path.Join(source, "/"), path.Join(prefixes[i], "/")) {
return true
}
}
Expand Down