From fd9b76c6a74af3b21c783fc52fadf31795e2651d Mon Sep 17 00:00:00 2001 From: Jan Eitzinger Date: Mon, 9 Feb 2026 09:12:06 +0100 Subject: [PATCH] Security hardening of ldap and oicd auth implementations --- internal/auth/auth.go | 5 ++ internal/auth/ldap.go | 126 ++++++++++++++++++++-------------------- internal/auth/oidc.go | 119 ++++++++++++++++++++++++++++--------- internal/auth/schema.go | 8 +++ 4 files changed, 166 insertions(+), 92 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index df618a3f..8a2073b5 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -294,6 +294,11 @@ func handleOIDCUser(OIDCUser *schema.User) { handleUserSync(OIDCUser, Keys.OpenIDConfig.SyncUserOnLogin, Keys.OpenIDConfig.UpdateUserOnLogin) } +// handleLdapUser syncs LDAP user with database +func handleLdapUser(ldapUser *schema.User) { + handleUserSync(ldapUser, Keys.LdapConfig.SyncUserOnLogin, Keys.LdapConfig.UpdateUserOnLogin) +} + func (auth *Authentication) SaveSession(rw http.ResponseWriter, r *http.Request, user *schema.User) error { session, err := auth.sessionStore.New(r, "session") if err != nil { diff --git a/internal/auth/ldap.go b/internal/auth/ldap.go index 5e12f07b..831568d9 100644 --- a/internal/auth/ldap.go +++ b/internal/auth/ldap.go @@ -6,11 +6,12 @@ package auth import ( - "errors" "fmt" + "net" "net/http" "os" "strings" + "time" "github.com/ClusterCockpit/cc-backend/internal/repository" cclog "github.com/ClusterCockpit/cc-lib/v2/ccLogger" @@ -25,16 +26,19 @@ type LdapConfig struct { UserBind string `json:"user-bind"` UserFilter string `json:"user-filter"` UserAttr string `json:"username-attr"` + UidAttr string `json:"uid-attr"` SyncInterval string `json:"sync-interval"` // Parsed using time.ParseDuration. SyncDelOldUsers bool `json:"sync-del-old-users"` - // Should an non-existent user be added to the DB if user exists in ldap directory - SyncUserOnLogin bool `json:"sync-user-on-login"` + // Should a non-existent user be added to the DB if user exists in ldap directory + SyncUserOnLogin bool `json:"sync-user-on-login"` + UpdateUserOnLogin bool `json:"update-user-on-login"` } type LdapAuthenticator struct { syncPassword string UserAttr string + UidAttr string } var _ Authenticator = (*LdapAuthenticator)(nil) @@ -51,6 +55,12 @@ func (la *LdapAuthenticator) Init() error { la.UserAttr = "gecos" } + if Keys.LdapConfig.UidAttr != "" { + la.UidAttr = Keys.LdapConfig.UidAttr + } else { + la.UidAttr = "uid" + } + return nil } @@ -66,55 +76,44 @@ func (la *LdapAuthenticator) CanLogin( if user.AuthSource == schema.AuthViaLDAP { return user, true } - } else { - if lc.SyncUserOnLogin { - l, err := la.getLdapConnection(true) - if err != nil { - cclog.Error("LDAP connection error") - return nil, false - } - defer l.Close() - - // Search for the given username - searchRequest := ldap.NewSearchRequest( - lc.UserBase, - ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false, - fmt.Sprintf("(&%s(uid=%s))", lc.UserFilter, username), - []string{"dn", "uid", la.UserAttr}, nil) - - sr, err := l.Search(searchRequest) - if err != nil { - cclog.Warn(err) - return nil, false - } - - if len(sr.Entries) != 1 { - cclog.Warn("LDAP: User does not exist or too many entries returned") - return nil, false - } - - entry := sr.Entries[0] - name := entry.GetAttributeValue(la.UserAttr) - var roles []string - roles = append(roles, schema.GetRoleString(schema.RoleUser)) - projects := make([]string, 0) - - user = &schema.User{ - Username: username, - Name: name, - Roles: roles, - Projects: projects, - AuthType: schema.AuthSession, - AuthSource: schema.AuthViaLDAP, - } - - if err := repository.GetUserRepository().AddUser(user); err != nil { - cclog.Errorf("User '%s' LDAP: Insert into DB failed", username) - return nil, false - } - - return user, true + } else if lc.SyncUserOnLogin { + l, err := la.getLdapConnection(true) + if err != nil { + cclog.Error("LDAP connection error") + return nil, false } + defer l.Close() + + // Search for the given username + searchRequest := ldap.NewSearchRequest( + lc.UserBase, + ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false, + fmt.Sprintf("(&%s(%s=%s))", lc.UserFilter, la.UidAttr, ldap.EscapeFilter(username)), + []string{"dn", la.UidAttr, la.UserAttr}, nil) + + sr, err := l.Search(searchRequest) + if err != nil { + cclog.Warn(err) + return nil, false + } + + if len(sr.Entries) != 1 { + cclog.Warn("LDAP: User does not exist or too many entries returned") + return nil, false + } + + entry := sr.Entries[0] + user = &schema.User{ + Username: username, + Name: entry.GetAttributeValue(la.UserAttr), + Roles: []string{schema.GetRoleString(schema.RoleUser)}, + Projects: make([]string, 0), + AuthType: schema.AuthSession, + AuthSource: schema.AuthViaLDAP, + } + + handleLdapUser(user) + return user, true } return nil, false @@ -132,7 +131,7 @@ func (la *LdapAuthenticator) Login( } defer l.Close() - userDn := strings.ReplaceAll(Keys.LdapConfig.UserBind, "{username}", user.Username) + userDn := strings.ReplaceAll(Keys.LdapConfig.UserBind, "{username}", ldap.EscapeDN(user.Username)) if err := l.Bind(userDn, r.FormValue("password")); err != nil { cclog.Errorf("AUTH/LDAP > Authentication for user %s failed: %v", user.Username, err) @@ -170,7 +169,7 @@ func (la *LdapAuthenticator) Sync() error { lc.UserBase, ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false, lc.UserFilter, - []string{"dn", "uid", la.UserAttr}, nil)) + []string{"dn", la.UidAttr, la.UserAttr}, nil)) if err != nil { cclog.Warn("LDAP search error") return err @@ -178,9 +177,9 @@ func (la *LdapAuthenticator) Sync() error { newnames := map[string]string{} for _, entry := range ldapResults.Entries { - username := entry.GetAttributeValue("uid") + username := entry.GetAttributeValue(la.UidAttr) if username == "" { - return errors.New("no attribute 'uid'") + return fmt.Errorf("no attribute '%s'", la.UidAttr) } _, ok := users[username] @@ -194,20 +193,19 @@ func (la *LdapAuthenticator) Sync() error { for username, where := range users { if where == InDB && lc.SyncDelOldUsers { - ur.DelUser(username) + if err := ur.DelUser(username); err != nil { + cclog.Errorf("User '%s' LDAP: Delete from DB failed: %v", username, err) + return err + } cclog.Debugf("sync: remove %v (does not show up in LDAP anymore)", username) } else if where == InLdap { name := newnames[username] - var roles []string - roles = append(roles, schema.GetRoleString(schema.RoleUser)) - projects := make([]string, 0) - user := &schema.User{ Username: username, Name: name, - Roles: roles, - Projects: projects, + Roles: []string{schema.GetRoleString(schema.RoleUser)}, + Projects: make([]string, 0), AuthSource: schema.AuthViaLDAP, } @@ -224,11 +222,13 @@ func (la *LdapAuthenticator) Sync() error { func (la *LdapAuthenticator) getLdapConnection(admin bool) (*ldap.Conn, error) { lc := Keys.LdapConfig - conn, err := ldap.DialURL(lc.URL) + conn, err := ldap.DialURL(lc.URL, + ldap.DialWithDialer(&net.Dialer{Timeout: 10 * time.Second})) if err != nil { cclog.Warn("LDAP URL dial failed") return nil, err } + conn.SetTimeout(30 * time.Second) if admin { if err := conn.Bind(lc.SearchDN, la.syncPassword); err != nil { diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index f81b651f..ec6c77a7 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -9,6 +9,7 @@ import ( "context" "crypto/rand" "encoding/base64" + "fmt" "io" "net/http" "os" @@ -50,6 +51,7 @@ func setCallbackCookie(w http.ResponseWriter, r *http.Request, name, value strin MaxAge: int(time.Hour.Seconds()), Secure: r.TLS != nil, HttpOnly: true, + SameSite: http.SameSiteLaxMode, } http.SetCookie(w, c) } @@ -77,8 +79,7 @@ func NewOIDC(a *Authentication) *OIDC { ClientID: clientID, ClientSecret: clientSecret, Endpoint: provider.Endpoint(), - RedirectURL: "oidc-callback", - Scopes: []string{oidc.ScopeOpenID, "profile", "email"}, + Scopes: []string{oidc.ScopeOpenID, "profile"}, } oa := &OIDC{provider: provider, client: client, clientID: clientID, authentication: a} @@ -122,54 +123,93 @@ func (oa *OIDC) OAuth2Callback(rw http.ResponseWriter, r *http.Request) { token, err := oa.client.Exchange(ctx, code, oauth2.VerifierOption(codeVerifier)) if err != nil { - http.Error(rw, "Failed to exchange token: "+err.Error(), http.StatusInternalServerError) + cclog.Errorf("token exchange failed: %s", err.Error()) + http.Error(rw, "Authentication failed during token exchange", http.StatusInternalServerError) return } // Get user info from OIDC provider with same timeout userInfo, err := oa.provider.UserInfo(ctx, oauth2.StaticTokenSource(token)) if err != nil { - http.Error(rw, "Failed to get userinfo: "+err.Error(), http.StatusInternalServerError) + cclog.Errorf("failed to get userinfo: %s", err.Error()) + http.Error(rw, "Failed to retrieve user information", http.StatusInternalServerError) return } - // // Extract the ID Token from OAuth2 token. - // rawIDToken, ok := token.Extra("id_token").(string) - // if !ok { - // http.Error(rw, "Cannot access idToken", http.StatusInternalServerError) - // } - // - // verifier := oa.provider.Verifier(&oidc.Config{ClientID: oa.clientID}) - // // Parse and verify ID Token payload. - // idToken, err := verifier.Verify(context.Background(), rawIDToken) - // if err != nil { - // http.Error(rw, "Failed to extract idToken: "+err.Error(), http.StatusInternalServerError) - // } + // Verify ID token and nonce to prevent replay attacks + rawIDToken, ok := token.Extra("id_token").(string) + if !ok { + http.Error(rw, "ID token not found in response", http.StatusInternalServerError) + return + } + + nonceCookie, err := r.Cookie("nonce") + if err != nil { + http.Error(rw, "nonce cookie not found", http.StatusBadRequest) + return + } + + verifier := oa.provider.Verifier(&oidc.Config{ClientID: oa.clientID}) + idToken, err := verifier.Verify(ctx, rawIDToken) + if err != nil { + cclog.Errorf("ID token verification failed: %s", err.Error()) + http.Error(rw, "ID token verification failed", http.StatusInternalServerError) + return + } + + if idToken.Nonce != nonceCookie.Value { + http.Error(rw, "Nonce mismatch", http.StatusBadRequest) + return + } projects := make([]string, 0) - // Extract custom claims + // Extract custom claims from userinfo var claims struct { Username string `json:"preferred_username"` Name string `json:"name"` - Profile struct { + // Keycloak realm-level roles + RealmAccess struct { + Roles []string `json:"roles"` + } `json:"realm_access"` + // Keycloak client-level roles + ResourceAccess struct { Client struct { Roles []string `json:"roles"` } `json:"clustercockpit"` } `json:"resource_access"` } if err := userInfo.Claims(&claims); err != nil { - http.Error(rw, "Failed to extract Claims: "+err.Error(), http.StatusInternalServerError) + cclog.Errorf("failed to extract claims: %s", err.Error()) + http.Error(rw, "Failed to extract user claims", http.StatusInternalServerError) + return + } + + if claims.Username == "" { + http.Error(rw, "Username claim missing from OIDC provider", http.StatusBadRequest) + return + } + + // Merge roles from both client-level and realm-level access + oidcRoles := append(claims.ResourceAccess.Client.Roles, claims.RealmAccess.Roles...) + + roleSet := make(map[string]bool) + for _, r := range oidcRoles { + switch r { + case "user": + roleSet[schema.GetRoleString(schema.RoleUser)] = true + case "admin": + roleSet[schema.GetRoleString(schema.RoleAdmin)] = true + case "manager": + roleSet[schema.GetRoleString(schema.RoleManager)] = true + case "support": + roleSet[schema.GetRoleString(schema.RoleSupport)] = true + } } var roles []string - for _, r := range claims.Profile.Client.Roles { - switch r { - case "user": - roles = append(roles, schema.GetRoleString(schema.RoleUser)) - case "admin": - roles = append(roles, schema.GetRoleString(schema.RoleAdmin)) - } + for role := range roleSet { + roles = append(roles, role) } if len(roles) == 0 { @@ -188,8 +228,12 @@ func (oa *OIDC) OAuth2Callback(rw http.ResponseWriter, r *http.Request) { handleOIDCUser(user) } - oa.authentication.SaveSession(rw, r, user) - cclog.Infof("login successfull: user: %#v (roles: %v, projects: %v)", user.Username, user.Roles, user.Projects) + if err := oa.authentication.SaveSession(rw, r, user); err != nil { + cclog.Errorf("session save failed for user %q: %s", user.Username, err.Error()) + http.Error(rw, "Failed to create session", http.StatusInternalServerError) + return + } + cclog.Infof("login successful: user: %#v (roles: %v, projects: %v)", user.Username, user.Roles, user.Projects) userCtx := context.WithValue(r.Context(), repository.ContextUserKey, user) http.RedirectHandler("/", http.StatusTemporaryRedirect).ServeHTTP(rw, r.WithContext(userCtx)) } @@ -206,7 +250,24 @@ func (oa *OIDC) OAuth2Login(rw http.ResponseWriter, r *http.Request) { codeVerifier := oauth2.GenerateVerifier() setCallbackCookie(rw, r, "verifier", codeVerifier) + // Generate nonce for ID token replay protection + nonce, err := randString(16) + if err != nil { + http.Error(rw, "Internal error", http.StatusInternalServerError) + return + } + setCallbackCookie(rw, r, "nonce", nonce) + + // Build redirect URL from the incoming request + scheme := "https" + if r.TLS == nil && r.Header.Get("X-Forwarded-Proto") != "https" { + scheme = "http" + } + oa.client.RedirectURL = fmt.Sprintf("%s://%s/oidc-callback", scheme, r.Host) + // Redirect user to consent page to ask for permission - url := oa.client.AuthCodeURL(state, oauth2.AccessTypeOffline, oauth2.S256ChallengeOption(codeVerifier)) + url := oa.client.AuthCodeURL(state, oauth2.AccessTypeOffline, + oauth2.S256ChallengeOption(codeVerifier), + oidc.Nonce(nonce)) http.Redirect(rw, r, url, http.StatusFound) } diff --git a/internal/auth/schema.go b/internal/auth/schema.go index 496e899b..b6ee0702 100644 --- a/internal/auth/schema.go +++ b/internal/auth/schema.go @@ -92,9 +92,17 @@ var configSchema = ` "description": "Delete obsolete users in database.", "type": "boolean" }, + "uid-attr": { + "description": "LDAP attribute used as login username. Default: uid", + "type": "string" + }, "sync-user-on-login": { "description": "Add non-existent user to DB at login attempt if user exists in Ldap directory", "type": "boolean" + }, + "update-user-on-login": { + "description": "Should an existent user attributes in the DB be updated at login attempt with values from LDAP.", + "type": "boolean" } }, "required": ["url", "user-base", "search-dn", "user-bind", "user-filter"]