Skip to content

Commit

Permalink
support multiple subjects in oidc ping
Browse files Browse the repository at this point in the history
Validate the subject in an oidc ping against a list of logged in subjects.

This resolves the issue that multiple connected FRP clients with different
OIDC clients result in a failing ping. The ping would fail because the
subject in memory would be the value of the last logged in FRPC.

This change also changes the constructor of OidcAuthVerifier to take
a TokenVerifier interface. This will not change production behavior, but makes
testing easier because we can inject a mock verifier during testing.

Resolves: fatedier#4466
  • Loading branch information
Rob Kenis committed Oct 11, 2024
1 parent fe4ca1b commit d082976
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 9 deletions.
3 changes: 2 additions & 1 deletion pkg/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ func NewAuthVerifier(cfg v1.AuthServerConfig) (authVerifier Verifier) {
case v1.AuthMethodToken:
authVerifier = NewTokenAuth(cfg.AdditionalScopes, cfg.Token)
case v1.AuthMethodOIDC:
authVerifier = NewOidcAuthVerifier(cfg.AdditionalScopes, cfg.OIDC)
tokenVerifier := NewTokenVerifier(cfg.OIDC)
authVerifier = NewOidcAuthVerifier(cfg.AdditionalScopes, tokenVerifier)
}
return authVerifier
}
27 changes: 19 additions & 8 deletions pkg/auth/oidc.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,18 @@ func (auth *OidcAuthProvider) SetNewWorkConn(newWorkConnMsg *msg.NewWorkConn) (e
return err
}

type TokenVerifier interface {
Verify(context.Context, string) (*oidc.IDToken, error)
}

type OidcAuthConsumer struct {
additionalAuthScopes []v1.AuthScope

verifier *oidc.IDTokenVerifier
subjectFromLogin string
verifier TokenVerifier
subjectsFromLogin []string
}

func NewOidcAuthVerifier(additionalAuthScopes []v1.AuthScope, cfg v1.AuthOIDCServerConfig) *OidcAuthConsumer {
func NewTokenVerifier(cfg v1.AuthOIDCServerConfig) TokenVerifier {
provider, err := oidc.NewProvider(context.Background(), cfg.Issuer)
if err != nil {
panic(err)
Expand All @@ -105,9 +109,14 @@ func NewOidcAuthVerifier(additionalAuthScopes []v1.AuthScope, cfg v1.AuthOIDCSer
SkipExpiryCheck: cfg.SkipExpiryCheck,
SkipIssuerCheck: cfg.SkipIssuerCheck,
}
return provider.Verifier(&verifierConf)
}

func NewOidcAuthVerifier(additionalAuthScopes []v1.AuthScope, verifier TokenVerifier) *OidcAuthConsumer {
return &OidcAuthConsumer{
additionalAuthScopes: additionalAuthScopes,
verifier: provider.Verifier(&verifierConf),
verifier: verifier,
subjectsFromLogin: []string{},
}
}

Expand All @@ -116,7 +125,9 @@ func (auth *OidcAuthConsumer) VerifyLogin(loginMsg *msg.Login) (err error) {
if err != nil {
return fmt.Errorf("invalid OIDC token in login: %v", err)
}
auth.subjectFromLogin = token.Subject
if !slices.Contains(auth.subjectsFromLogin, token.Subject) {
auth.subjectsFromLogin = append(auth.subjectsFromLogin, token.Subject)
}
return nil
}

Expand All @@ -125,11 +136,11 @@ func (auth *OidcAuthConsumer) verifyPostLoginToken(privilegeKey string) (err err
if err != nil {
return fmt.Errorf("invalid OIDC token in ping: %v", err)
}
if token.Subject != auth.subjectFromLogin {
if !slices.Contains(auth.subjectsFromLogin, token.Subject) {
return fmt.Errorf("received different OIDC subject in login and ping. "+
"original subject: %s, "+
"original subjects: %s, "+
"new subject: %s",
auth.subjectFromLogin, token.Subject)
auth.subjectsFromLogin, token.Subject)
}
return nil
}
Expand Down
64 changes: 64 additions & 0 deletions pkg/auth/oidc_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package auth_test

import (
"context"
"testing"
"time"

"github.com/coreos/go-oidc/v3/oidc"
"github.com/stretchr/testify/require"

"github.com/fatedier/frp/pkg/auth"
v1 "github.com/fatedier/frp/pkg/config/v1"
"github.com/fatedier/frp/pkg/msg"
)

type mockTokenVerifier struct{}

func (m *mockTokenVerifier) Verify(ctx context.Context, subject string) (*oidc.IDToken, error) {
return &oidc.IDToken{
Subject: subject,
}, nil
}

func TestPingWithEmptySubjectFromLoginFails(t *testing.T) {
r := require.New(t)
consumer := auth.NewOidcAuthVerifier([]v1.AuthScope{v1.AuthScopeHeartBeats}, &mockTokenVerifier{})
err := consumer.VerifyPing(&msg.Ping{
PrivilegeKey: "ping-without-login",
Timestamp: time.Now().UnixMilli(),
})
r.Error(err)
r.Contains(err.Error(), "received different OIDC subject in login and ping")
}

func TestPingAfterLoginWithNewSubjectSucceeds(t *testing.T) {
r := require.New(t)
consumer := auth.NewOidcAuthVerifier([]v1.AuthScope{v1.AuthScopeHeartBeats}, &mockTokenVerifier{})
err := consumer.VerifyLogin(&msg.Login{
PrivilegeKey: "ping-after-login",
})
r.NoError(err)

err = consumer.VerifyPing(&msg.Ping{
PrivilegeKey: "ping-after-login",
Timestamp: time.Now().UnixMilli(),
})
r.NoError(err)
}

func TestPingAfterLoginWithDifferentSubjectFails(t *testing.T) {
r := require.New(t)
consumer := auth.NewOidcAuthVerifier([]v1.AuthScope{v1.AuthScopeHeartBeats}, &mockTokenVerifier{})
err := consumer.VerifyLogin(&msg.Login{
PrivilegeKey: "login-with-first-subject",
})
r.NoError(err)

err = consumer.VerifyPing(&msg.Ping{
PrivilegeKey: "ping-with-different-subject",
Timestamp: time.Now().UnixMilli(),
})
r.Error(err)
r.Contains(err.Error(), "received different OIDC subject in login and ping")
}

0 comments on commit d082976

Please sign in to comment.