OIDC: Improve auth api logs and user verification #782

Signed-off-by: Michael Mayer <michael@photoprism.app>
This commit is contained in:
Michael Mayer
2024-07-04 10:24:10 +02:00
parent ed14877488
commit 3ecee16848
10 changed files with 167 additions and 51 deletions

View File

@@ -29,7 +29,6 @@ func OIDCLogin(router *gin.RouterGroup) {
// Get client IP address for logs and rate limiting checks.
clientIp := ClientIP(c)
actor := "unknown user"
action := "sign in"
// Get global config.
@@ -37,11 +36,11 @@ func OIDCLogin(router *gin.RouterGroup) {
// Abort in public mode and if OIDC is disabled.
if get.Config().Public() {
event.AuditErr([]string{clientIp, "oidc", actor, action, authn.ErrDisabledInPublicMode.Error()})
event.AuditErr([]string{clientIp, "oidc", action, authn.ErrDisabledInPublicMode.Error()})
Abort(c, http.StatusForbidden, i18n.ErrForbidden)
return
} else if !conf.OIDCEnabled() {
event.AuditErr([]string{clientIp, "oidc", actor, action, authn.ErrAuthenticationDisabled.Error()})
event.AuditErr([]string{clientIp, "oidc", action, authn.ErrAuthenticationDisabled.Error()})
Abort(c, http.StatusMethodNotAllowed, i18n.ErrUnsupported)
return
}
@@ -60,7 +59,7 @@ func OIDCLogin(router *gin.RouterGroup) {
provider := get.OIDC()
if provider == nil {
event.AuditErr([]string{clientIp, "oidc", actor, action, authn.ErrAuthenticationDisabled.Error()})
event.AuditErr([]string{clientIp, "oidc", action, authn.ErrInvalidProvider.Error()})
Abort(c, http.StatusInternalServerError, i18n.ErrConnectionFailed)
return
}

View File

@@ -1,7 +1,9 @@
package api
import (
"fmt"
"net/http"
"strings"
"time"
"github.com/gin-gonic/gin"
@@ -25,12 +27,9 @@ import (
// GET /api/v1/oidc/redirect
func OIDCRedirect(router *gin.RouterGroup) {
router.GET("/oidc/redirect", func(c *gin.Context) {
// Get global config.
conf := get.Config()
// Prevent CDNs from caching this endpoint.
if header.IsCdn(c.Request) {
c.Redirect(http.StatusTemporaryRedirect, conf.LoginUri())
AbortNotFound(c)
return
}
@@ -39,16 +38,20 @@ func OIDCRedirect(router *gin.RouterGroup) {
// Get client IP address for logs and rate limiting checks.
clientIp := ClientIP(c)
actor := "unknown user"
userAgent := UserAgent(c)
userName := "unknown user"
action := "sign in"
// Get global config.
conf := get.Config()
// Abort in public mode and if OIDC is disabled.
if get.Config().Public() {
event.AuditErr([]string{clientIp, "oidc", actor, action, authn.ErrDisabledInPublicMode.Error()})
event.AuditErr([]string{clientIp, "oidc", action, authn.ErrDisabledInPublicMode.Error()})
c.Redirect(http.StatusTemporaryRedirect, conf.LoginUri())
return
} else if !conf.OIDCEnabled() {
event.AuditErr([]string{clientIp, "oidc", actor, action, authn.ErrAuthenticationDisabled.Error()})
event.AuditErr([]string{clientIp, "oidc", action, authn.ErrAuthenticationDisabled.Error()})
c.Redirect(http.StatusTemporaryRedirect, conf.LoginUri())
return
}
@@ -65,7 +68,7 @@ func OIDCRedirect(router *gin.RouterGroup) {
// Check if the required request parameters are present.
if c.Query("state") == "" || c.Query("code") == "" {
event.AuditErr([]string{clientIp, "oidc", actor, action, authn.ErrAuthCodeRequired.Error()})
event.AuditErr([]string{clientIp, "oidc", action, authn.ErrAuthCodeRequired.Error()})
c.Redirect(http.StatusTemporaryRedirect, conf.LoginUri())
return
}
@@ -74,7 +77,7 @@ func OIDCRedirect(router *gin.RouterGroup) {
provider := get.OIDC()
if provider == nil {
event.AuditErr([]string{clientIp, "oidc", actor, action, authn.ErrAuthenticationDisabled.Error()})
event.AuditErr([]string{clientIp, "oidc", action, authn.ErrAuthenticationDisabled.Error()})
c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials)))
return
}
@@ -82,7 +85,7 @@ func OIDCRedirect(router *gin.RouterGroup) {
userInfo, tokens, claimErr := provider.CodeExchangeUserInfo(c)
if claimErr != nil {
event.AuditErr([]string{clientIp, "oidc", actor, action, claimErr.Error()})
event.AuditErr([]string{clientIp, "oidc", action, claimErr.Error()})
return
}
@@ -90,24 +93,47 @@ func OIDCRedirect(router *gin.RouterGroup) {
var user *entity.User
var err error
userEmail := clean.Email(userInfo.GetEmail())
// Optionally check if the email domain matches.
if domain := conf.OIDCDomain(); domain == "" {
// Do nothing.
} else if _, emailDomain, _ := strings.Cut(userEmail, "@"); emailDomain == "" || !userInfo.IsEmailVerified() {
event.AuditErr([]string{clientIp, "oidc", action, authn.ErrVerifiedEmailRequired.Error()})
event.LoginError(clientIp, "oidc", userEmail, userAgent, authn.ErrVerifiedEmailRequired.Error())
c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrForbidden)))
return
} else if !strings.HasSuffix("."+emailDomain, "."+domain) {
message := fmt.Sprintf("domain must match '%s'", domain)
event.AuditErr([]string{clientIp, "oidc", action, userEmail, message})
event.LoginError(clientIp, "oidc", userEmail, userAgent, message)
c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrForbidden)))
return
}
// Find existing user record and update it, if necessary.
if oidcUser := entity.OidcUser(userInfo, conf.OIDCUsername()); oidcUser.UserName == "" || authn.ProviderOIDC.NotEqual(oidcUser.AuthProvider) {
event.AuditErr([]string{clientIp, "oidc", actor, action, authn.ErrInvalidUsername.Error()})
event.AuditErr([]string{clientIp, "oidc", action, authn.ErrInvalidUsername.Error()})
event.LoginError(clientIp, "oidc", oidcUser.UserName, userAgent, authn.ErrInvalidUsername.Error())
c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials)))
return
} else if user = entity.FindUser(oidcUser); user != nil {
// Check if username and subject UID match.
if user.Username() == "" || oidcUser.UserName == "" || user.Username() != oidcUser.UserName {
event.AuditErr([]string{clientIp, "oidc", actor, action, authn.ErrInvalidUsername.Error()})
c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials)))
return
} else if user.AuthID == "" || oidcUser.AuthID == "" || user.AuthID != oidcUser.AuthID {
event.AuditErr([]string{clientIp, "oidc", actor, action, authn.ErrInvalidAuthID.Error()})
event.AuditErr([]string{clientIp, "oidc", action, authn.ErrInvalidUsername.Error()})
event.LoginError(clientIp, "oidc", oidcUser.UserName, userAgent, authn.ErrInvalidUsername.Error())
c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials)))
return
}
actor = user.Username()
userName = user.Username()
if user.AuthID == "" || oidcUser.AuthID == "" || user.AuthID != oidcUser.AuthID {
event.AuditErr([]string{clientIp, "oidc", action, userName, authn.ErrInvalidAuthID.Error()})
event.LoginError(clientIp, "oidc", userName, userAgent, authn.ErrInvalidAuthID.Error())
c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials)))
return
}
// Update user profile information.
details := user.Details()
@@ -163,9 +189,10 @@ func OIDCRedirect(router *gin.RouterGroup) {
user.VerifiedAt = entity.TimeStamp()
}
// Update user account.
// Update existing user account.
if err = user.Save(); err != nil {
event.AuditErr([]string{clientIp, "oidc", actor, action, authn.ErrAccountUpdateFailed.Error(), err.Error()})
event.AuditErr([]string{clientIp, "oidc", action, userName, authn.ErrAccountUpdateFailed.Error(), err.Error()})
event.LoginError(clientIp, "oidc", userName, userAgent, authn.ErrAccountUpdateFailed.Error()+": "+err.Error())
c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials)))
return
}
@@ -174,14 +201,14 @@ func OIDCRedirect(router *gin.RouterGroup) {
if avatarUrl := userInfo.GetPicture(); avatarUrl == "" || user.HasAvatar() {
// Do nothing.
} else if err = avatar.SetUserImageURL(user, avatarUrl, entity.SrcOIDC); err != nil {
event.AuditWarn([]string{clientIp, "oidc", actor, action, "failed to set avatar image", err.Error()})
event.AuditWarn([]string{clientIp, "oidc", action, userName, "failed to set avatar image", err.Error()})
}
} else if conf.OIDCRegister() {
action = "sign up"
// Create new user record.
user = &oidcUser
actor = user.Username()
userName = user.Username()
// Set user profile information.
user.SetDisplayName(userInfo.GetName(), entity.SrcOIDC)
@@ -227,28 +254,31 @@ func OIDCRedirect(router *gin.RouterGroup) {
user.CanLogin = true
user.WebDAV = conf.OIDCWebDAV()
// Create user account.
// Create new user account.
if err = user.Create(); err != nil {
event.AuditErr([]string{clientIp, "oidc", actor, action, authn.ErrAccountCreateFailed.Error(), err.Error()})
event.AuditErr([]string{clientIp, "oidc", action, userName, authn.ErrAccountCreateFailed.Error(), err.Error()})
event.LoginError(clientIp, "oidc", userName, userAgent, authn.ErrAccountCreateFailed.Error()+": "+err.Error())
c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials)))
return
}
// Set user avatar image.
if avatarUrl := userInfo.GetPicture(); avatarUrl == "" {
event.AuditDebug([]string{clientIp, "oidc", actor, action, "no avatar image provided"})
event.AuditDebug([]string{clientIp, "oidc", action, userName, "no avatar image provided"})
} else if err = avatar.SetUserImageURL(user, avatarUrl, entity.SrcOIDC); err != nil {
event.AuditWarn([]string{clientIp, "oidc", actor, action, "failed to set avatar image", err.Error()})
event.AuditWarn([]string{clientIp, "oidc", action, userName, "failed to set avatar image", err.Error()})
}
} else {
event.AuditErr([]string{clientIp, "oidc", actor, action, authn.ErrRegistrationDisabled.Error()})
event.AuditErr([]string{clientIp, "oidc", action, userName, authn.ErrRegistrationDisabled.Error()})
event.LoginError(clientIp, "oidc", userName, userAgent, authn.ErrRegistrationDisabled.Error())
c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials)))
return
}
// Login allowed?
if !user.CanLogIn() {
event.AuditErr([]string{clientIp, "oidc", actor, action, authn.ErrAccountDisabled.Error()})
event.AuditErr([]string{clientIp, "oidc", action, userName, authn.ErrAccountDisabled.Error()})
event.LoginError(clientIp, "oidc", userName, userAgent, authn.ErrAccountDisabled.Error())
c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials)))
return
}
@@ -271,10 +301,11 @@ func OIDCRedirect(router *gin.RouterGroup) {
// Save session after successful authentication.
if sess, err = get.Session().Save(sess); err != nil {
event.AuditErr([]string{clientIp, "oidc", actor, action, "%s"}, err)
event.AuditErr([]string{clientIp, "oidc", action, userName, "%s"}, err)
c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials)))
return
} else if sess == nil {
event.AuditErr([]string{clientIp, "oidc", action, userName, "session is nil"})
c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrUnexpected)))
return
}
@@ -286,7 +317,8 @@ func OIDCRedirect(router *gin.RouterGroup) {
response := CreateSessionResponse(sess.AuthToken(), sess, conf.ClientSession(sess))
// Log success.
event.AuditInfo([]string{clientIp, "oidc", actor, action, authn.Succeeded})
event.AuditInfo([]string{clientIp, "oidc", action, userName, authn.Succeeded})
event.LoginInfo(clientIp, "oidc", userName, userAgent)
// Update login timestamp.
user.UpdateLoginTime()

View File

@@ -30,7 +30,7 @@ type Client struct {
debug bool
}
func NewClient(iss *url.URL, clientId, clientSecret, customScopes, siteUrl string, debug bool) (result *Client, err error) {
func NewClient(oidcUri *url.URL, oidcClient, oidcSecret, oidcScopes, siteUrl string, debug bool) (result *Client, err error) {
u, err := url.Parse(siteUrl)
if err != nil {
@@ -68,7 +68,7 @@ func NewClient(iss *url.URL, clientId, clientSecret, customScopes, siteUrl strin
}),
}
discover, err := client.Discover(iss.String(), httpClient)
discover, err := client.Discover(oidcUri.String(), httpClient)
if err != nil {
log.Debugf("oidc: %q (discover)", err)
@@ -81,9 +81,13 @@ func NewClient(iss *url.URL, clientId, clientSecret, customScopes, siteUrl strin
}
}
scopes := strings.Split(strings.TrimSpace("openid email profile "+customScopes), " ")
if oidcScopes == "" {
oidcScopes = "openid email profile"
}
provider, err := rp.NewRelyingPartyOIDC(iss.String(), clientId, clientSecret, u.String(), scopes, clientOpt...)
scopes := strings.Split(strings.TrimSpace(oidcScopes), " ")
provider, err := rp.NewRelyingPartyOIDC(oidcUri.String(), oidcClient, oidcSecret, u.String(), scopes, clientOpt...)
if err != nil {
log.Debugf("oidc: %s (issuer)", err)

View File

@@ -104,6 +104,11 @@ func (c *Config) OIDCUsername() string {
return authn.ClaimUsername
}
// OIDCDomain returns the email domain name for restricted single sign-on via OIDC.
func (c *Config) OIDCDomain() string {
return clean.Domain(c.options.OIDCDomain)
}
// OIDCRole returns the default user role when signing up via OIDC.
func (c *Config) OIDCRole() acl.Role {
if c.options.OIDCRole == "" {
@@ -153,10 +158,17 @@ func (c *Config) OIDCReport() (rows [][]string, cols []string) {
{"oidc-redirect", fmt.Sprintf("%t", c.OIDCRedirect())},
{"oidc-register", fmt.Sprintf("%t", c.OIDCRegister())},
{"oidc-username", c.OIDCUsername()},
}
if domain := c.OIDCDomain(); domain != "" {
rows = append(rows, []string{"oidc-domain", domain})
}
rows = append(rows, [][]string{
{"oidc-role", c.OIDCRole().String()},
{"oidc-webdav", fmt.Sprintf("%t", c.OIDCWebDAV())},
{"disable-oidc", fmt.Sprintf("%t", c.DisableOIDC())},
}
}...)
return rows, cols
}

View File

@@ -124,6 +124,24 @@ func TestConfig_OIDCUsername(t *testing.T) {
assert.Equal(t, authn.ClaimUsername, c.OIDCUsername())
}
func TestConfig_OIDCDomain(t *testing.T) {
c := NewConfig(CliTestContext())
assert.Equal(t, "", c.OIDCDomain())
c.options.OIDCDomain = "example.com"
assert.Equal(t, "example.com", c.OIDCDomain())
c.options.OIDCDomain = "foo"
assert.Equal(t, "", c.OIDCDomain())
c.options.OIDCDomain = ""
assert.Equal(t, "", c.OIDCDomain())
}
func TestConfig_OIDCRegister(t *testing.T) {
c := NewConfig(CliTestContext())

View File

@@ -4,12 +4,11 @@ import (
"fmt"
"time"
"github.com/photoprism/photoprism/internal/config/ttl"
"github.com/klauspost/cpuid/v2"
"github.com/urfave/cli"
"github.com/photoprism/photoprism/internal/ai/face"
"github.com/photoprism/photoprism/internal/config/ttl"
"github.com/photoprism/photoprism/internal/entity"
"github.com/photoprism/photoprism/internal/ffmpeg"
"github.com/photoprism/photoprism/internal/thumb"

View File

@@ -29,8 +29,8 @@ type Options struct {
AdminPassword string `yaml:"AdminPassword" json:"-" flag:"admin-password"`
PasswordLength int `yaml:"PasswordLength" json:"-" flag:"password-length"`
PasswordResetUri string `yaml:"PasswordResetUri" json:"-" flag:"password-reset-uri"`
RegisterUri string `yaml:"RegisterUri" json:"-" flag:"register-uri"`
LoginUri string `yaml:"LoginUri" json:"-" flag:"login-uri"`
RegisterUri string `yaml:"-" json:"-" flag:"register-uri"`
LoginUri string `yaml:"-" json:"-" flag:"login-uri"`
OIDCUri string `yaml:"OIDCUri" json:"-" flag:"oidc-uri"`
OIDCClient string `yaml:"OIDCClient" json:"-" flag:"oidc-client"`
OIDCSecret string `yaml:"OIDCSecret" json:"-" flag:"oidc-secret"`
@@ -40,7 +40,8 @@ type Options struct {
OIDCRedirect bool `yaml:"OIDCRedirect" json:"OIDCRedirect" flag:"oidc-redirect"`
OIDCRegister bool `yaml:"OIDCRegister" json:"OIDCRegister" flag:"oidc-register"`
OIDCUsername string `yaml:"OIDCUsername" json:"-" flag:"oidc-username"`
OIDCRole string `yaml:"OIDCRole" json:"-" flag:"oidc-role"`
OIDCDomain string `yaml:"-" json:"-" flag:"oidc-domain"`
OIDCRole string `yaml:"-" json:"-" flag:"oidc-role"`
OIDCWebDAV bool `yaml:"OIDCWebDAV" json:"-" flag:"oidc-webdav"`
DisableOIDC bool `yaml:"DisableOIDC" json:"DisableOIDC" flag:"disable-oidc"`
SessionMaxAge int64 `yaml:"SessionMaxAge" json:"-" flag:"session-maxage"`

View File

@@ -31,14 +31,16 @@ var (
// OIDC and OAuth2-related error messages:
var (
ErrInvalidGrantType = errors.New("invalid grant type")
ErrInvalidClientID = errors.New("invalid client id")
ErrInvalidAuthID = errors.New("invalid auth id")
ErrAuthCodeRequired = errors.New("auth code required")
ErrClientIDRequired = errors.New("client id required")
ErrInvalidClientSecret = errors.New("invalid client secret")
ErrClientSecretRequired = errors.New("client secret required")
ErrRegistrationDisabled = errors.New("registration disabled")
ErrInvalidProvider = errors.New("invalid provider")
ErrInvalidGrantType = errors.New("invalid grant type")
ErrInvalidClientID = errors.New("invalid client id")
ErrInvalidAuthID = errors.New("invalid auth id")
ErrAuthCodeRequired = errors.New("auth code required")
ErrClientIDRequired = errors.New("client id required")
ErrInvalidClientSecret = errors.New("invalid client secret")
ErrClientSecretRequired = errors.New("client secret required")
ErrVerifiedEmailRequired = errors.New("verified email required")
ErrRegistrationDisabled = errors.New("registration disabled")
)
// User-related error messages:

View File

@@ -9,6 +9,7 @@ import (
)
var EmailRegexp = regexp.MustCompile("^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$")
var DomainRegexp = regexp.MustCompile("^(?:[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?\\.)+[a-z0-9][a-z0-9-]{0,61}[a-z0-9]$")
// Auth returns the sanitized authentication identifier trimmed to a maximum length of 255 characters.
func Auth(s string) string {
@@ -115,6 +116,22 @@ func Email(s string) string {
return ""
}
// Domain returns the normalized domain name with trimmed whitespace and in lowercase.
func Domain(s string) string {
// Empty or too long?
if s == "" || reject(s, txt.ClipName) {
return ""
}
s = strings.ToLower(strings.TrimSpace(s))
if DomainRegexp.MatchString(s) {
return s
}
return ""
}
// Role returns the sanitized role with trimmed whitespace and in lowercase.
func Role(s string) string {
// Remove unwanted characters.

View File

@@ -1,6 +1,7 @@
package clean
import (
"strings"
"testing"
"github.com/stretchr/testify/assert"
@@ -107,6 +108,37 @@ func TestEmail(t *testing.T) {
})
}
func TestDomain(t *testing.T) {
t.Run("Valid", func(t *testing.T) {
assert.Equal(t, "photoprism.app", Domain("photoprism.app"))
})
t.Run("Whitespace", func(t *testing.T) {
assert.Equal(t, "photoprism.app", Domain(" photoprism.app "))
})
t.Run("Hostname", func(t *testing.T) {
assert.Equal(t, "foo.example.com", Domain(" FOO.example.Com "))
})
t.Run("Example", func(t *testing.T) {
assert.Equal(t, "", Domain("example"))
})
t.Run("Invalid", func(t *testing.T) {
assert.Equal(t, "", Domain(" hello-photoprism "))
})
t.Run("Empty", func(t *testing.T) {
assert.Equal(t, "", Domain(""))
})
t.Run("Match", func(t *testing.T) {
email := "john.doe@example.com"
domain := Domain("example.com")
_, emailDomain, _ := strings.Cut(Email(email), "@")
assert.True(t, strings.HasSuffix("."+emailDomain, "."+domain))
assert.False(t, strings.HasSuffix(".my-"+emailDomain, "."+domain))
assert.True(t, strings.HasSuffix("my-"+emailDomain, domain))
})
}
func TestRole(t *testing.T) {
t.Run("Admin ", func(t *testing.T) {
assert.Equal(t, "admin", Role("Admin "))