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.

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

Account autocreation from LDAP after reverse proxy authentication #18452

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
60 changes: 52 additions & 8 deletions services/auth/reverseproxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@
package auth

import (
"fmt"
"net/http"
"strings"

"code.gitea.io/gitea/models/db"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
Expand Down Expand Up @@ -55,32 +57,74 @@ func (r *ReverseProxy) Name() string {
// the revese proxy.
// If a username is available in the "setting.ReverseProxyAuthUser" header an existing
// user object is returned (populated with username or email found in header).
// Returns nil if header is empty.
// Returns nil if header is empty or internal API is being called.
func (r *ReverseProxy) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User {

// Internal API should not use this auth method.
if middleware.IsInternalPath(req) {
return nil
}

// Just return user if session is estabilshed already.
user := SessionUser(sess)
if user != nil {
return user
}
Copy link
Member

Choose a reason for hiding this comment

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

This mixes the Session provider into the reverse proxy. I think we should not do this. Is it needed for the new logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This mixes the Session provider into the reverse proxy. I think we should not do this. Is it needed for the new logic?

Reverse proxy is/should be for user authentication stuff not Gitea's session handling IHMO. So there is no point in user verification if already verified (established session). Don't see problem with it.

Copy link
Member

Choose a reason for hiding this comment

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

Then it's just a wrong execution order and the session provider should be checked first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SessionUser(sess) is called first and used if session is already estabilshed; if no session is established, new session is created. Don't see any problem here. If you see problem here, please propose exact code fix.

Copy link
Member

Choose a reason for hiding this comment

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

The session handler is already called before the reverse proxy handler:

gitea/routers/web/web.go

Lines 104 to 111 in cb10f27

group := auth_service.NewGroup(
&auth_service.OAuth2{}, // FIXME: this should be removed and only applied in download and oauth related routers
&auth_service.Basic{}, // FIXME: this should be removed and only applied in download and git/lfs routers
&auth_service.Session{},
)
if setting.Service.EnableReverseProxyAuth {
group.Add(&auth_service.ReverseProxy{})
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in dfc544c. Thanks for pointing.


username := r.getUserName(req)
if len(username) == 0 {
return nil
}
log.Trace("ReverseProxy Authorization: Found username: %s", username)

user, err := user_model.GetUserByName(username)
if err != nil {
if !user_model.IsErrUserNotExist(err) || !r.isAutoRegisterAllowed() {
log.Error("GetUserByName: %v", err)
var err error

if r.isAutoRegisterAllowed() {
// Use auto registration from reverse proxy if ENABLE_REVERSE_PROXY_AUTO_REGISTRATION enabled.
if user, err = user_model.GetUserByName(username); err != nil {
if user_model.IsErrUserNotExist(err) && r.isAutoRegisterAllowed() {
if user = r.newUser(req); user == nil {
return nil
}
} else {
log.Error("GetUserByName: %v", err)
return nil
}
}
} else {
// Use auto registration from other backends if ENABLE_REVERSE_PROXY_AUTO_REGISTRATION not enabled.
if user, _, err = UserSignIn(username, ""); err != nil {
if !user_model.IsErrUserNotExist(err) {
log.Error("UserSignIn: %v", err)
}
return nil
}
user = r.newUser(req)
}

// Make sure requests to API paths, attachment downloads, git and LFS do not create a new session
if !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isGitRawReleaseOrLFSPath(req) {
if sess != nil && (sess.Get("uid") == nil || sess.Get("uid").(int64) != user.ID) {

// Register last login.
user.SetLastLogin()

if err = user_model.UpdateUserCols(db.DefaultContext, user, "last_login_unix"); err != nil {
log.Error(fmt.Sprintf("ReverseProxy Authorization: error updating user last login time [user: %d]", user.ID))
}

// Initialize new session. Will set lang and CSRF cookies.
handleSignIn(w, req, sess, user)

log.Trace("ReverseProxy Authorization: Logged in user %-v", user)
}

// Unfortunatelly we cannot do redirect here (would break git HTTP requests) to
// reload page with user locale so first page after login may be displayed in
// wrong language. Language handling in SSO mode should be reconsidered
// in future gitea versions.
}
store.GetData()["IsReverseProxy"] = true

log.Trace("ReverseProxy Authorization: Logged in user %-v", user)
store.GetData()["IsReverseProxy"] = true
return user
}

Expand Down
16 changes: 16 additions & 0 deletions services/auth/source/ldap/source_authenticate.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,17 @@ func (source *Source) Authenticate(user *user_model.User, userName, password str
}
if user != nil && !user.ProhibitLogin {
cols := make([]string, 0)
fullName := composeFullName(sr.Name, sr.Surname, sr.Username)
if user.FullName != fullName {
// Update user fullname if changed.
user.FullName = fullName
cols = append(cols, "full_name")
}
if !strings.EqualFold(user.Email, sr.Mail) {
// Update user e-mail if changed.
user.Email = sr.Mail
cols = append(cols, "email")
}
if len(source.AdminFilter) > 0 && user.IsAdmin != sr.IsAdmin {
// Change existing admin flag only if AdminFilter option is set
user.IsAdmin = sr.IsAdmin
Expand All @@ -49,6 +60,11 @@ func (source *Source) Authenticate(user *user_model.User, userName, password str
user.IsRestricted = sr.IsRestricted
cols = append(cols, "is_restricted")
}
if !user.IsActive {
// User existing in LDAP should be active in application.
user.IsActive = true
cols = append(cols, "is_active")
}
if len(cols) > 0 {
err = user_model.UpdateUserCols(db.DefaultContext, user, cols...)
if err != nil {
Expand Down
19 changes: 16 additions & 3 deletions services/auth/source/ldap/source_search.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"strings"

"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"

"github.com/go-ldap/ldap/v3"
)
Expand Down Expand Up @@ -194,11 +195,23 @@ func checkRestricted(l *ldap.Conn, ls *Source, userDN string) bool {

// SearchEntry : search an LDAP source if an entry (name, passwd) is valid and in the specific filter
func (ls *Source) SearchEntry(name, passwd string, directBind bool) *SearchResult {

// See https://tools.ietf.org/search/rfc4513#section-5.1.2
if len(passwd) == 0 {
// Don't authenticate against LDAP if already authenticated by reverse proxy.
if setting.Service.EnableReverseProxyAuth {
if directBind {
log.Debug("Cannot bind pre-authenticated user %s. BindDN must be used.", name)
return nil
}
if !ls.AttributesInBind {
log.Debug("Cannot get attributes for pre-authenticated user %s without --attributes-in-bind.", name)
return nil
}
} else if len(passwd) == 0 {
log.Debug("Auth. failed for %s, password cannot be empty", name)
return nil
}

l, err := dial(ls)
if err != nil {
log.Error("LDAP Connect error, %s:%v", ls.Host, err)
Expand Down Expand Up @@ -361,8 +374,8 @@ func (ls *Source) SearchEntry(name, passwd string, directBind bool) *SearchResul
isRestricted = checkRestricted(l, ls, userDN)
}

if !directBind && ls.AttributesInBind {
// binds user (checking password) after looking-up attributes in BindDN context
if !directBind && ls.AttributesInBind && !setting.Service.EnableReverseProxyAuth {
// Binds user (checking password) after looking-up attributes in BindDN context if not already authenticated.
err = bindUser(l, userDN, passwd)
if err != nil {
return nil
Expand Down