Account autocreation from LDAP after reverse proxy authentication #12960

pboguslawski · 2020-09-28T10:09:25Z

Gitea allows autocreation of account from external source after successful
basic auth but not after successful reverse proxy auth. This mod adds such
feature.

Related: gogs/gogs#2498
Author-Change-Id: IB#1104925

Gitea allows autocreation of account from external source after successful basic auth but not after successful reverse proxy auth. This mod adds such feature. Unfortunaltely gitea does not sync all user attributes from LDAP for existing users on login like cron.sync_external_users does so changes of first name, surname, e-mail are not updated from LDAP on login for exiting users - only after first login and after sync_external_users task. Related: gogs/gogs#2498 Author-Change-Id: IB#1104925

pboguslawski · 2020-09-28T10:10:00Z

BTW: please consider refactoring gitea code to separate authentication logic from authorization logic and from user data synchronization logic and moving all auth/authz/sync configuration from SQL to app.ini; mixing these things makes changes harder and error prone; configuration in SQL is a pain when using automated provisioning; also external passwords like LDAP are problably less secure when stored in SQL (and every SQL backup...).

BTW2: please consider synchronization of user avatar from jpegPhoto LDAP attribute (if exists).

BTW3: please consider disallowing local (in gitea) user attributes modifications for attributes imported from LDAP (i.e. first name, surname, e-mail, is active, is admin, etc.); LDAP synchronization often (always?) means that admin wants to manage this data in LDAP not in app.

codecov-commenter · 2020-09-28T10:43:35Z

Codecov Report

Merging #12960 into master will decrease coverage by 0.20%.
The diff coverage is 30.55%.

@@            Coverage Diff             @@
##           master   #12960      +/-   ##
==========================================
- Coverage   42.84%   42.63%   -0.21%     
==========================================
  Files         662      669       +7     
  Lines       73002    73506     +504     
==========================================
+ Hits        31279    31341      +62     
- Misses      36652    37077     +425     
- Partials     5071     5088      +17

Impacted Files	Coverage Δ
modules/auth/sso/reverseproxy.go	`8.16% <0.00%> (-1.37%)`	⬇️
routers/org/setting.go	`0.00% <0.00%> (ø)`
routers/user/auth_openid.go	`0.00% <0.00%> (ø)`
routers/user/setting/account.go	`26.04% <0.00%> (ø)`
modules/auth/ldap/ldap.go	`45.78% <11.11%> (-1.03%)`	⬇️
routers/user/auth.go	`11.59% <50.00%> (ø)`
models/login_source.go	`26.25% <100.00%> (ø)`
modules/auth/sso/basic.go	`51.06% <100.00%> (ø)`
routers/repo/http.go	`43.11% <100.00%> (ø)`
modules/indexer/stats/db.go	`43.47% <0.00%> (-26.09%)`	⬇️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95ff559...bd8361d. Read the comment docs.

lafriks · 2020-09-28T19:59:10Z

BTW: please consider refactoring gitea code to separate authentication logic from authorization logic and from user data synchronization logic and moving all auth/authz/sync configuration from SQL to app.ini; mixing these things makes changes harder and error prone; configuration in SQL is a pain when using automated provisioning; also external passwords like LDAP are problably less secure when stored in SQL (and every SQL backup...).

I don't agree as that loses ability to configure from UI and secrets in DB is maybe even more secure than in config file.

BTW2: please consider synchronization of user avatar from jpegPhoto LDAP attribute (if exists).

There is already issue for this. Avatar should probably be synchronized only on login not to have too much load on sync process.

BTW3: please consider disallowing local (in gitea) user attributes modifications for attributes imported from LDAP (i.e. first name, surname, e-mail, is active, is admin, etc.); LDAP synchronization often (always?) means that admin wants to manage this data in LDAP not in app.

Please submit separate issue for this.

pboguslawski · 2020-10-07T19:25:16Z

please consider refactoring gitea code to separate authentication logic from authorization logic and from user data synchronization logic and moving all auth/authz/sync configuration from SQL to app.ini

Current design does not allow application admin to be denied access for messing low level configuration like LDAP backends (this should be server admin not application admin business).

Please submit separate issue for this.

Please take a look at #13068

Gitea does not update user language on first login when in reverse proxy mode; this causes errors when user tries to update their settings without setting language. This mod sets language in reverse mode also like for regular logins in routers/user/auth.go > handleSignInFull(). Fixes: bd8361d Author-Change-Id: IB#1104925

Gitea does not initialize user session after login using reverse proxy header. This fixes it. Fixes: 3df7fb6 Author-Change-Id: IB#1104925

Gitea does not initialize user session after login using reverse proxy header. This fixes it. Fixes: 45ea55d Author-Change-Id: IB#1104925

Gitea does not initialize user session after login using reverse proxy header. This fixes it. Fixes: 41ddcd0 Author-Change-Id: IB#1104925

Gitea does not initialize user session after login using reverse proxy header. This fixes it. Fixes: 53a9b26 Author-Change-Id: IB#1104925

Gitea does not initialize user session after login using reverse proxy header. This fixes it. Fixes: 22441d9 Author-Change-Id: IB#1104925

Gitea does not initialize user session after login using reverse proxy header. This fixes it. Fixes: 46304f2 Author-Change-Id: IB#1104925

Gitea does not initialize user session after login using reverse proxy header. This fixes it. Fixes: cb48990 Author-Change-Id: IB#1104925

pboguslawski · 2020-10-20T20:24:21Z

Just fixed another problem with reverse proxy login - user lang was ignored and csrf error was fired on first user settings saving. Login with header use now session to avoid setting cookies with every request; session is skipped for API calls like in sspi_windows.go.

zeripath · 2020-10-21T12:27:37Z

agh wrong button!

zeripath · 2020-10-21T13:02:14Z

modules/auth/ldap/ldap.go

+	if len(passwd) == 0 && !alreadyAuthenticated {
 		log.Debug("Auth. failed for %s, password cannot be empty", name)
 		return nil
 	}
+	if directBind && alreadyAuthenticated {
+		log.Debug("Cannot bind using user %s credentials - user already authenticated. BindDN must be used.", name)
+		return nil
+	}
+
+	if !ls.AttributesInBind && alreadyAuthenticated {
+		log.Debug("Cannot get attributes using user %s credentials - user already authenticated; --attributes-in-bind must be used.", name)
+		return nil
+	}
+


Suggested change

if len(passwd) == 0 && !alreadyAuthenticated {

log.Debug("Auth. failed for %s, password cannot be empty", name)

return nil

}

if directBind && alreadyAuthenticated {

log.Debug("Cannot bind using user %s credentials - user already authenticated. BindDN must be used.", name)

return nil

}

if !ls.AttributesInBind && alreadyAuthenticated {

log.Debug("Cannot get attributes using user %s credentials - user already authenticated; --attributes-in-bind must be used.", name)

return nil

}

if alreadyAuthenticated {

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

}

zeripath · 2020-10-21T13:14:45Z

models/login_source.go

-// UserSignIn validates user name and password.
-func UserSignIn(username, password string) (*User, error) {
+// UserSignIn validates user name and password. Password verification in LDAP skipped if already authenticated.
+func UserSignIn(username, password string, alreadyAuthenticated bool) (*User, error) {


I guess one thing I don't really like about this is that the user has already signed in - we're just abusing this function to lookup the user in the external source - and only in one place (at least so far). Maybe we should be doing a ExternalUserLookup or something like that.

Further if the user is in the db then this alreadyAuthenticated path fails.

Further if the user is in the db then this alreadyAuthenticated path fails.

This mod works ok for us in scenario with LDAP user backend + reverse proxy auth using HTTP header. No problems with initial account creation nor logging in/updating existing accounts from LDAP. If you see any bug please describe how to reproduce.

Maybe we should be doing a ExternalUserLookup or something like that.

This mod was created for our needs with minimal changes in upstream code. Feel free to enhance it.

stale · 2020-12-25T17:49:27Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

6543 · 2021-03-01T21:14:42Z

@pboguslawski what is the current status if this?

pboguslawski · 2021-03-02T07:19:03Z

This mod was created for our needs with minimal changes in upstream code (which should be refactored as above IHMO) and works ok for us. Feel free to enhance it or close this PR if you don't like it.

zeripath · 2021-06-23T08:57:55Z

I guess this is going to miss 1.15 - My suspicion is that we need to think about authentication, authorization and user registration slightly differently. Currently registration is very tightly bound to the authentication source that it is associated with.

I'm gonna move this to 1.16 but reference #16199 to remind myself.

pboguslawski · 2022-01-29T16:40:27Z

Replaced with #18452

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 28, 2020

lafriks added the type/enhancement An improvement of existing functionality label Sep 28, 2020

lafriks added this to the 1.14.0 milestone Sep 28, 2020

pboguslawski added 8 commits October 20, 2020 12:42

Fixed user session initialization on login with reverse proxy header

0149559

Gitea does not initialize user session after login using reverse proxy header. This fixes it. Fixes: 3df7fb6 Author-Change-Id: IB#1104925

Fixed user session initialization on login with reverse proxy header

41ddcd0

Gitea does not initialize user session after login using reverse proxy header. This fixes it. Fixes: 45ea55d Author-Change-Id: IB#1104925

Fixed user session initialization on login with reverse proxy header

53a9b26

Gitea does not initialize user session after login using reverse proxy header. This fixes it. Fixes: 41ddcd0 Author-Change-Id: IB#1104925

Fixed user session initialization on login with reverse proxy header

22441d9

Gitea does not initialize user session after login using reverse proxy header. This fixes it. Fixes: 53a9b26 Author-Change-Id: IB#1104925

Fixed user session initialization on login with reverse proxy header

46304f2

Gitea does not initialize user session after login using reverse proxy header. This fixes it. Fixes: 22441d9 Author-Change-Id: IB#1104925

Fixed user session initialization on login with reverse proxy header

cb48990

Gitea does not initialize user session after login using reverse proxy header. This fixes it. Fixes: 46304f2 Author-Change-Id: IB#1104925

Fixed user session initialization on login with reverse proxy header

6bba0ba

Gitea does not initialize user session after login using reverse proxy header. This fixes it. Fixes: cb48990 Author-Change-Id: IB#1104925

zeripath approved these changes Oct 21, 2020

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 21, 2020

zeripath self-requested a review October 21, 2020 12:27

zeripath reviewed Oct 21, 2020

View reviewed changes

stale bot added the issue/stale label Dec 25, 2020

stale bot removed the issue/stale label Mar 1, 2021

techknowlogick modified the milestones: 1.14.0, 1.15.0 Mar 1, 2021

zeripath added the topic/authentication label Jun 23, 2021

zeripath modified the milestones: 1.15.0, 1.16.0 Jun 23, 2021

techknowlogick modified the milestones: 1.16.0, 1.17.0 Nov 23, 2021

pboguslawski closed this Jan 29, 2022

pboguslawski mentioned this pull request Jan 29, 2022

Account autocreation from LDAP after reverse proxy authentication #18452

Open

go-gitea locked and limited conversation to collaborators Apr 28, 2022

wxiaoguang removed this from the 1.17.0 milestone Jun 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account autocreation from LDAP after reverse proxy authentication #12960

Account autocreation from LDAP after reverse proxy authentication #12960

pboguslawski commented Sep 28, 2020 •

edited

Loading

pboguslawski commented Sep 28, 2020

codecov-commenter commented Sep 28, 2020

lafriks commented Sep 28, 2020

pboguslawski commented Oct 7, 2020

pboguslawski commented Oct 20, 2020

zeripath commented Oct 21, 2020

zeripath Oct 21, 2020

zeripath Oct 21, 2020

zeripath Oct 21, 2020

pboguslawski Oct 21, 2020 •

edited

Loading

stale bot commented Dec 25, 2020

6543 commented Mar 1, 2021

pboguslawski commented Mar 2, 2021

zeripath commented Jun 23, 2021

pboguslawski commented Jan 29, 2022 •

edited

Loading

Account autocreation from LDAP after reverse proxy authentication #12960

Account autocreation from LDAP after reverse proxy authentication #12960

Conversation

pboguslawski commented Sep 28, 2020 • edited Loading

pboguslawski commented Sep 28, 2020

codecov-commenter commented Sep 28, 2020

Codecov Report

lafriks commented Sep 28, 2020

pboguslawski commented Oct 7, 2020

pboguslawski commented Oct 20, 2020

zeripath commented Oct 21, 2020

zeripath Oct 21, 2020

Choose a reason for hiding this comment

zeripath Oct 21, 2020

Choose a reason for hiding this comment

zeripath Oct 21, 2020

Choose a reason for hiding this comment

pboguslawski Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

stale bot commented Dec 25, 2020

6543 commented Mar 1, 2021

pboguslawski commented Mar 2, 2021

zeripath commented Jun 23, 2021

pboguslawski commented Jan 29, 2022 • edited Loading

pboguslawski commented Sep 28, 2020 •

edited

Loading

pboguslawski Oct 21, 2020 •

edited

Loading

pboguslawski commented Jan 29, 2022 •

edited

Loading