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

Add support for per route authorization #2011

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add support for per route authorization #2011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f931662
Add support for per route authorization
Feb 11, 2023
113f95e
Merge branch 'master' into route_auth
Apr 19, 2023
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
4 changes: 2 additions & 2 deletions oauthproxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -862,7 +862,7 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {
}

// set cookie, or deny
authorized, err := p.provider.Authorize(req.Context(), session)
authorized, err := p.provider.Authorize(req.Context(), session, req.URL.Path)
if err != nil {
logger.Errorf("Error with authorization: %v", err)
}
Expand Down Expand Up @@ -1050,7 +1050,7 @@ func (p *OAuthProxy) getAuthenticatedSession(rw http.ResponseWriter, req *http.R
}

invalidEmail := session.Email != "" && !p.Validator(session.Email)
authorized, err := p.provider.Authorize(req.Context(), session)
authorized, err := p.provider.Authorize(req.Context(), session, req.URL.Path)
if err != nil {
logger.Errorf("Error with authorization: %v", err)
}
Expand Down
6 changes: 4 additions & 2 deletions oauthproxy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -836,9 +836,11 @@ func NewProcessCookieTest(opts ProcessCookieTestOpts, modifiers ...OptionsModifi
}

groups := pcTest.opts.Providers[0].AllowedGroups
testProvider.ProviderData.AllowedGroups = make(map[string]struct{}, len(groups))
testProvider.ProviderData.AllowedGroups = map[string]map[string]struct{}{
"/": make(map[string]struct{}, len(groups)),
}
for _, group := range groups {
testProvider.ProviderData.AllowedGroups[group] = struct{}{}
testProvider.ProviderData.AllowedGroups["/"][group] = struct{}{}
}
pcTest.proxy.provider = testProvider

Expand Down
8 changes: 6 additions & 2 deletions providers/gitlab.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,21 @@ func NewGitLabProvider(p *ProviderData, opts options.GitLabOptions) (*GitLabProv
// setAllowedProjects adds Gitlab projects to the AllowedGroups list
// and tracks them to do a project API lookup during `EnrichSession`.
func (p *GitLabProvider) setAllowedProjects(projects []string) error {
for _, project := range projects {
groups := make([]string, len(projects))
for i, project := range projects {
gp, err := newGitlabProject(project)
if err != nil {
return err
}
p.allowedProjects = append(p.allowedProjects, gp)
p.AllowedGroups[formatProject(gp)] = struct{}{}
groups[i] = formatProject(gp)
}
p.addAllowedGroups(groups)

if len(p.allowedProjects) > 0 {
p.setProjectScope()
}

return nil
}

Expand Down
2 changes: 1 addition & 1 deletion providers/gitlab_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ var _ = Describe("Gitlab Provider Tests", func() {
Expect(err).To(BeNil())
Expect(session.Groups).To(Equal(in.expectedGroups))

authorized, err := p.Authorize(context.Background(), session)
authorized, err := p.Authorize(context.Background(), session, "/")
Expect(err).To(BeNil())
Expect(authorized).To(Equal(in.expectedAuthz))
},
Expand Down
16 changes: 11 additions & 5 deletions providers/keycloak_oidc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package providers
import (
"context"
"fmt"
"strings"

"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options"
"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions"
Expand Down Expand Up @@ -37,12 +38,17 @@ var _ Provider = (*KeycloakOIDCProvider)(nil)
// Assumes `SetAllowedGroups` is already called on groups and appends to that
// with `role:` prefixed roles.
func (p *KeycloakOIDCProvider) addAllowedRoles(roles []string) {
if p.AllowedGroups == nil {
p.AllowedGroups = make(map[string]struct{})
}
for _, role := range roles {
p.AllowedGroups[formatRole(role)] = struct{}{}
groups := make([]string, len(roles))
for i, r := range roles {
path, role, hasPath := strings.Cut(r, "|")
if !hasPath {
role = path
path = "/"
}

groups[i] = path + "|" + formatRole(role)
}
p.addAllowedGroups(groups)
}

// CreateSessionFromToken converts Bearer IDTokens into sessions
Expand Down
9 changes: 5 additions & 4 deletions providers/keycloak_oidc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,12 +102,13 @@ var _ = Describe("Keycloak OIDC Provider Tests", func() {
})

Context("Allowed Roles", func() {
It("should prefix allowed roles and add them to groups", func() {
It("should prefix allowed roles and add them to groups for the correct path", func() {
p := newKeycloakOIDCProvider(nil, options.KeycloakOptions{
Roles: []string{"admin", "editor"},
Roles: []string{"admin", "editor", "/api/|api"},
})
Expect(p.AllowedGroups).To(HaveKey("role:admin"))
Expect(p.AllowedGroups).To(HaveKey("role:editor"))
Expect(p.AllowedGroups["/"]).To(HaveKey("role:admin"))
Expect(p.AllowedGroups["/"]).To(HaveKey("role:editor"))
Expect(p.AllowedGroups["/api/"]).To(HaveKey("role:api"))
})
})

Expand Down
28 changes: 25 additions & 3 deletions providers/provider_data.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ type ProviderData struct {

// Universal Group authorization data structure
// any provider can set to consume
AllowedGroups map[string]struct{}
AllowedGroups map[string]map[string]struct{}

getAuthorizationHeaderFunc func(string) http.Header
loginURLParameterDefaults url.Values
Expand Down Expand Up @@ -172,9 +172,31 @@ func regexpForRule(rule options.URLParameterRule) string {
// setAllowedGroups organizes a group list into the AllowedGroups map
// to be consumed by Authorize implementations
func (p *ProviderData) setAllowedGroups(groups []string) {
p.AllowedGroups = make(map[string]struct{}, len(groups))
p.AllowedGroups = nil
p.addAllowedGroups(groups)
}

// addAllowedGroups appends a group list into the AllowedGroups map
func (p *ProviderData) addAllowedGroups(groups []string) {
if p.AllowedGroups == nil {
p.AllowedGroups = make(map[string]map[string]struct{})
}

for _, group := range groups {
p.AllowedGroups[group] = struct{}{}
path, group, hasPath := strings.Cut(group, "|")
if !hasPath {
group = path
path = "/"
}

logger.Printf("Add allowed group %q for path %q", group, path)

pathGroups := p.AllowedGroups[path]
if pathGroups == nil {
pathGroups = make(map[string]struct{})
}
pathGroups[group] = struct{}{}
p.AllowedGroups[path] = pathGroups
}
}

Expand Down
20 changes: 16 additions & 4 deletions providers/provider_default.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"errors"
"fmt"
"net/url"
"strings"

"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware"
"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions"
Expand Down Expand Up @@ -114,14 +115,25 @@ func (p *ProviderData) EnrichSession(_ context.Context, _ *sessions.SessionState
}

// Authorize performs global authorization on an authenticated session.
// This is not used for fine-grained per route authorization rules.
func (p *ProviderData) Authorize(_ context.Context, s *sessions.SessionState) (bool, error) {
if len(p.AllowedGroups) == 0 {
// This can be used for fine-grained per route authorization rules.
func (p *ProviderData) Authorize(_ context.Context, s *sessions.SessionState, path string) (bool, error) {
allowedGroups := p.AllowedGroups["/"]
var matchPathLen int
for groupsPath, groups := range p.AllowedGroups {
if strings.HasPrefix(path, groupsPath) {
if len(groupsPath) > matchPathLen {
matchPathLen = len(groupsPath)
allowedGroups = groups
}
}
}

if len(allowedGroups) == 0 {
return true, nil
}

for _, group := range s.Groups {
if _, ok := p.AllowedGroups[group]; ok {
if _, ok := allowedGroups[group]; ok {
return true, nil
}
}
Expand Down
56 changes: 55 additions & 1 deletion providers/provider_default_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,32 +75,86 @@ func TestProviderDataAuthorize(t *testing.T) {
name string
allowedGroups []string
groups []string
url *url.URL
expectedAuthZ bool
}{
{
name: "NoAllowedGroups",
allowedGroups: []string{},
groups: []string{},
url: &url.URL{
Scheme: "http",
Host: "my.test.idp",
Path: "/",
},
expectedAuthZ: true,
},
{
name: "NoAllowedGroupsUserHasGroups",
allowedGroups: []string{},
groups: []string{"foo", "bar"},
url: &url.URL{
Scheme: "http",
Host: "my.test.idp",
Path: "/",
},
expectedAuthZ: true,
},
{
name: "UserInAllowedGroup",
allowedGroups: []string{"foo"},
groups: []string{"foo", "bar"},
url: &url.URL{
Scheme: "http",
Host: "my.test.idp",
Path: "/",
},
expectedAuthZ: true,
},
{
name: "UserNotInAllowedGroup",
allowedGroups: []string{"bar"},
groups: []string{"baz", "foo"},
url: &url.URL{
Scheme: "http",
Host: "my.test.idp",
Path: "/",
},
expectedAuthZ: false,
},
{
name: "NoAllowedGroupsMatchingPath",
allowedGroups: []string{"bar", "/path|doo"},
groups: []string{"foo"},
url: &url.URL{
Scheme: "http",
Host: "my.test.idp",
Path: "/path",
},
expectedAuthZ: false,
},
{
name: "AllowedGroupsMatchingPath",
allowedGroups: []string{"bar", "/path|foo"},
groups: []string{"foo"},
url: &url.URL{
Scheme: "http",
Host: "my.test.idp",
Path: "/path",
},
expectedAuthZ: true,
},
{
name: "MoreSpecificPathMatches",
Copy link

Choose a reason for hiding this comment

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

Equating "more specific" to "longer prefix match" is ambiguous and potentially misleading?

Which is "more specific" - /administration or /admin/a/b? I think most people would say the latter, because it is 'deeper'. But given a rule for /administration and a rule for /admin/a/b, your code will pick the former as being "more specific" and hence the rule that is used.

An alternative to consider would be to use the first match in the list?

(I'm not associated with the project, this is a drive-by comment!)

Copy link
Author

Choose a reason for hiding this comment

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

This makes sense indeed. Maybe we should allow globing or regex. It’s more cognitive load on the user but at the same time the contract is clear.

Copy link

Choose a reason for hiding this comment

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

I can imagine some use cases might benefit from regex/glob matching. But I think it's an orthogonal question - you could still have multiple matches and need a way to pick one, i.e. first wins.

Copy link
Author

Choose a reason for hiding this comment

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

I see what you mean. Probably first wins is more intuitive.

allowedGroups: []string{"bar", "/p|bar", "/pa|bar", "/pat|bar", "/path|foo"},
groups: []string{"foo"},
url: &url.URL{
Scheme: "http",
Host: "my.test.idp",
Path: "/path",
},
expectedAuthZ: true,
},
}

for _, tc := range testCases {
Expand All @@ -113,7 +167,7 @@ func TestProviderDataAuthorize(t *testing.T) {
p := &ProviderData{}
p.setAllowedGroups(tc.allowedGroups)

authorized, err := p.Authorize(context.Background(), session)
authorized, err := p.Authorize(context.Background(), session, tc.url.Path)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(authorized).To(Equal(tc.expectedAuthZ))
})
Expand Down
2 changes: 1 addition & 1 deletion providers/providers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ type Provider interface {
// Deprecated: Migrate to EnrichSession
GetEmailAddress(ctx context.Context, s *sessions.SessionState) (string, error)
EnrichSession(ctx context.Context, s *sessions.SessionState) error
Authorize(ctx context.Context, s *sessions.SessionState) (bool, error)
Authorize(ctx context.Context, s *sessions.SessionState, path string) (bool, error)
ValidateSession(ctx context.Context, s *sessions.SessionState) bool
RefreshSession(ctx context.Context, s *sessions.SessionState) (bool, error)
CreateSessionFromToken(ctx context.Context, token string) (*sessions.SessionState, error)
Expand Down