Skip to content

Allow customisation of OIDC scopes #3234

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

Merged
merged 1 commit into from
Jan 6, 2023
Merged

Allow customisation of OIDC scopes #3234

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
1 change: 1 addition & 0 deletions cmd/gitops-server/cmd/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ func NewCommand() *cobra.Command {
cmd.Flags().DurationVar(&options.OIDC.TokenDuration, "oidc-token-duration", time.Hour, "The duration of the ID token. It should be set in the format: number + time unit (s,m,h) e.g., 20m")
cmd.Flags().StringVar(&options.OIDC.ClaimsConfig.Username, "oidc-username-claim", auth.ClaimUsername, "JWT claim to use as the user name. By default email, which is expected to be a unique identifier of the end user. Admins can choose other claims, such as sub or name, depending on their provider")
cmd.Flags().StringVar(&options.OIDC.ClaimsConfig.Groups, "oidc-groups-claim", auth.ClaimGroups, "JWT claim to use as the user's group. If the claim is present it must be an array of strings")
cmd.Flags().StringSliceVar(&options.OIDC.Scopes, "custom-oidc-scopes", auth.DefaultScopes, "Customise the requested scopes for then OIDC authentication flow - openid will always be requested")
Copy link
Contributor

Choose a reason for hiding this comment

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

This flag doesn't seem to be connected right now, maybe we remove it for now or?

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 think it is connected, through a long line of bits.

It is passed here

weave-gitops/cmd/gitops-server/cmd/cmd.go

Line 175 in 2deffd8

authServer, err := auth.InitAuthServer(cmd.Context(), log, rawClient, options.OIDC, options.OIDCSecret, namespace, options.AuthMethods)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh you're right. I was testing with mixed secret/cli config and then missed this

weave-gitops/pkg/server/auth/init.go

Lines 40 to 41 in 2deffd8

log.V(logger.LogLevelWarn).Info("OIDC client configured by both CLI and secret. CLI values will be overridden.")
}

LGTM

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 find it a bit confusing that the secret overrides the cmd-line, but that's the existing behaviour for other options, consistency wins out :-)

// Metrics
cmd.Flags().BoolVar(&options.EnableMetrics, "enable-metrics", false, "Starts the metrics listener")
cmd.Flags().StringVar(&options.MetricsAddress, "metrics-address", ":2112", "If the metrics listener is enabled, bind to this address")
Expand Down
12 changes: 6 additions & 6 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

56 changes: 55 additions & 1 deletion pkg/server/auth/auth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,59 @@ func TestWithAPIAuthOnlyUsesValidMethods(t *testing.T) {
g.Expect(res).To(HaveHTTPStatus(http.StatusOK))
}

func TestOauth2FlowRedirectsToOIDCIssuerWithCustomScopes(t *testing.T) {
g := NewGomegaWithT(t)

m, err := mockoidc.Run()
g.Expect(err).NotTo(HaveOccurred())

t.Cleanup(func() {
_ = m.Shutdown()
})

fake := m.Config()
mux := http.NewServeMux()
fakeKubernetesClient := ctrlclient.NewClientBuilder().Build()

tokenSignerVerifier, err := auth.NewHMACTokenSignerVerifier(5 * time.Minute)
g.Expect(err).NotTo(HaveOccurred())

oidcCfg := auth.OIDCConfig{
ClientID: fake.ClientID,
ClientSecret: fake.ClientSecret,
IssuerURL: fake.Issuer,
ClaimsConfig: &auth.ClaimsConfig{Username: "email", Groups: "groups"},
Scopes: []string{"test1", "test2"},
}

authMethods := map[auth.AuthMethod]bool{auth.OIDC: true}

authCfg, err := auth.NewAuthServerConfig(logr.Discard(), oidcCfg, fakeKubernetesClient, tokenSignerVerifier, testNamespace, authMethods)
g.Expect(err).NotTo(HaveOccurred())

srv, err := auth.NewAuthServer(context.Background(), authCfg)
g.Expect(err).NotTo(HaveOccurred())

g.Expect(auth.RegisterAuthServer(mux, "/oauth2", srv, 1)).To(Succeed())

s := httptest.NewServer(mux)

t.Cleanup(s.Close)

// Set the correct redirect URL now that we have a server running
redirectURL := s.URL + "/oauth2/callback"
srv.SetRedirectURL(redirectURL)

res := httptest.NewRecorder()
req := httptest.NewRequest(http.MethodGet, s.URL, nil)
srv.OAuth2Flow().ServeHTTP(res, req)

g.Expect(res).To(HaveHTTPStatus(http.StatusSeeOther))

authCodeURL := fmt.Sprintf("%s?client_id=%s&redirect_uri=%s&response_type=code&scope=%s", m.AuthorizationEndpoint(), fake.ClientID, url.QueryEscape(redirectURL), strings.Join([]string{oidc.ScopeOpenID, "test1", "test2"}, "+"))
g.Expect(res.Result().Header.Get("Location")).To(ContainSubstring(authCodeURL))
}

func TestOauth2FlowRedirectsToOIDCIssuerForUnauthenticatedRequests(t *testing.T) {
g := NewGomegaWithT(t)

Expand All @@ -181,6 +234,7 @@ func TestOauth2FlowRedirectsToOIDCIssuerForUnauthenticatedRequests(t *testing.T)
ClientSecret: fake.ClientSecret,
IssuerURL: fake.Issuer,
ClaimsConfig: &auth.ClaimsConfig{Username: "email", Groups: "groups"},
Scopes: auth.DefaultScopes,
}

authMethods := map[auth.AuthMethod]bool{auth.OIDC: true}
Expand Down Expand Up @@ -209,7 +263,7 @@ func TestOauth2FlowRedirectsToOIDCIssuerForUnauthenticatedRequests(t *testing.T)

g.Expect(res).To(HaveHTTPStatus(http.StatusSeeOther))

authCodeURL := fmt.Sprintf("%s?client_id=%s&redirect_uri=%s&response_type=code&scope=%s", m.AuthorizationEndpoint(), fake.ClientID, url.QueryEscape(redirectURL), strings.Join([]string{auth.ScopeProfile, oidc.ScopeOpenID, auth.ScopeEmail, auth.ScopeGroups}, "+"))
authCodeURL := fmt.Sprintf("%s?client_id=%s&redirect_uri=%s&response_type=code&scope=%s", m.AuthorizationEndpoint(), fake.ClientID, url.QueryEscape(redirectURL), strings.Join([]string{oidc.ScopeOpenID, auth.ScopeProfile, auth.ScopeEmail, auth.ScopeGroups}, "+"))
g.Expect(res.Result().Header.Get("Location")).To(ContainSubstring(authCodeURL))
}

Expand Down
48 changes: 35 additions & 13 deletions pkg/server/auth/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"fmt"
"net/http"
"net/url"
"strings"
"time"

"github.com/coreos/go-oidc/v3/oidc"
Expand Down Expand Up @@ -37,6 +38,14 @@ const (
ClaimGroups string = "groups"
)

// DefaultScopes is the set of scopes that we require.
var DefaultScopes = []string{
oidc.ScopeOpenID,
ScopeProfile,
ScopeEmail,
ScopeGroups,
}

// OIDCConfig is used to configure an AuthServer to interact with
// an OIDC issuer.
type OIDCConfig struct {
Expand All @@ -45,6 +54,7 @@ type OIDCConfig struct {
ClientSecret string
RedirectURL string
TokenDuration time.Duration
Scopes []string
ClaimsConfig *ClaimsConfig
}

Expand Down Expand Up @@ -110,9 +120,29 @@ func NewOIDCConfigFromSecret(secret corev1.Secret) OIDCConfig {

cfg.TokenDuration = tokenDuration

scopes := splitAndTrim(string(secret.Data["customScopes"]))
if len(scopes) == 0 {
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 I may have been doing the odd if slice == nil in other places, will keep the distinction in mind..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, result := []string{} will return a slice of zero length.

var result []string would mean that result could be nil.

Taking the length of a nil slice yields 0 any which way.

scopes = []string{oidc.ScopeOpenID, ScopeEmail, ScopeGroups}
}

cfg.Scopes = scopes

return cfg
}

func splitAndTrim(s string) []string {
result := []string{}
splits := strings.Split(s, ",")

for _, s := range splits {
if v := strings.TrimSpace(s); v != "" {
result = append(result, v)
}
}

return result
}

func claimsConfigFromSecret(secret corev1.Secret) *ClaimsConfig {
claimUsername, ok := secret.Data["claimUsername"]
if !ok {
Expand Down Expand Up @@ -214,27 +244,20 @@ func (s *AuthServer) verifier() *oidc.IDTokenVerifier {
}

func (s *AuthServer) oauth2Config(scopes []string) *oauth2.Config {
requestScopes := []string{}
// Ensure "openid" scope is always present.
if !contains(scopes, oidc.ScopeOpenID) {
scopes = append(scopes, oidc.ScopeOpenID)
requestScopes = append(requestScopes, oidc.ScopeOpenID)
}

// Request "email" scope to get user's email address.
if !contains(scopes, ScopeEmail) {
scopes = append(scopes, ScopeEmail)
}

// Request "groups" scope to get user's groups.
if !contains(scopes, ScopeGroups) {
scopes = append(scopes, ScopeGroups)
}
requestScopes = append(requestScopes, scopes...)

return &oauth2.Config{
ClientID: s.OIDCConfig.ClientID,
ClientSecret: s.OIDCConfig.ClientSecret,
RedirectURL: s.OIDCConfig.RedirectURL,
Endpoint: s.provider.Endpoint(),
Scopes: scopes,
Scopes: requestScopes,
}
}

Expand Down Expand Up @@ -502,8 +525,7 @@ func (s *AuthServer) startAuthFlow(rw http.ResponseWriter, r *http.Request) {

state := base64.StdEncoding.EncodeToString(b)

scopes := []string{ScopeProfile}
authCodeURL := s.oauth2Config(scopes).AuthCodeURL(state)
authCodeURL := s.oauth2Config(s.OIDCConfig.Scopes).AuthCodeURL(state)

// Issue state cookie
http.SetCookie(rw, s.createCookie(StateCookieName, state))
Expand Down
18 changes: 18 additions & 0 deletions pkg/server/auth/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"testing"
"time"

"github.com/coreos/go-oidc/v3/oidc"
"github.com/go-logr/logr"
"github.com/google/go-cmp/cmp"
"github.com/oauth2-proxy/mockoidc"
Expand Down Expand Up @@ -905,6 +906,7 @@ func TestNewOIDCConfigFromSecret(t *testing.T) {
RedirectURL: "https://example.com/redirect",
TokenDuration: time.Minute * 10,
ClaimsConfig: &auth.ClaimsConfig{Username: "email", Groups: "groups"},
Scopes: []string{oidc.ScopeOpenID, auth.ScopeEmail, auth.ScopeGroups},
},
},
{
Expand All @@ -915,6 +917,7 @@ func TestNewOIDCConfigFromSecret(t *testing.T) {
want: auth.OIDCConfig{
TokenDuration: time.Hour * 1,
ClaimsConfig: &auth.ClaimsConfig{Username: "email", Groups: "groups"},
Scopes: []string{oidc.ScopeOpenID, auth.ScopeEmail, auth.ScopeGroups},
},
},
{
Expand All @@ -925,11 +928,26 @@ func TestNewOIDCConfigFromSecret(t *testing.T) {
},
want: auth.OIDCConfig{
TokenDuration: time.Hour * 1,
Scopes: []string{oidc.ScopeOpenID, auth.ScopeEmail, auth.ScopeGroups},
ClaimsConfig: &auth.ClaimsConfig{
Username: "test-user", Groups: "test-groups",
},
},
},
{
name: "overridden scopes",
data: map[string][]byte{
"claimUsername": []byte("test-user"),
"customScopes": []byte("other-groups,new-user-id"),
},
want: auth.OIDCConfig{
TokenDuration: time.Hour * 1,
Scopes: []string{"other-groups", "new-user-id"},
ClaimsConfig: &auth.ClaimsConfig{
Username: "test-user", Groups: "groups",
},
},
},
}

for _, tt := range configTests {
Expand Down