LDAP Public SSH Keys synchronization #1844

dnmgns · 2017-05-31T16:03:25Z

This PR will implement a way to synchronize Public SSH Keys from LDAP when Public SSH Key(s) are stored as LDAP attribute in LDAP server.

Not long ago, #1478 was merged, which added a LDAP user synchronization feature. This PR will sort-of extend that one and and include a (optional) Public SSH Key synchronization feature.

The PR introduces the following changes in short:

Public SSH key attribute setting for add/edit Login Source (when Authentication Type = LDAP (via BindDN))
* Enable Public SSH key synchronization checkbox for add/edit Login Source
* REPLACE_PUBLIC_SSH_KEYS setting in app.ini (Default: false), when set to true it replaces all public ssh keys for users synchronized from LDAP and replaces them with the public keys from LDAP
login_source_id column in public_key table to keep track of the public_key source (When deleting keys, we therefore can choose to only delete those synchronized with LDAP. Plus it makes life easier for sysadmin)

~~# Why d54d92f and 9e09a9c are included in this PR~~
Note that d54d92f0b81e1d71fa2a4b2aedca985e7aa841d7 and 9e09a9ccd83130cccdfa71c8000db48310d7a8ea addresses two issues found in Gitea, they are not specifically for this synchronization feature. But I choose to include them, as using this key sync feature could lead to some pretty serious issues otherwise. Depending on the update interval for LDAP User synchronization and available resources, the system could end up with lots of bak-files for authorized_keys and lots of public tmp-files causing inode issue and running out of free space. Therefore I choose to also the following changes in this PR:
* Delete the temporary public key file after it has been used to calculate the public key fingerprint
* Setting in app.ini to completely disable authorized_keys backup (default: disabled)
Edit: Created two separate PR for this.

Thoughts

While the synchronization part (where LDAP keys are added to DB) could be a lot smarter, It is a thoughtful choice to always drop all LDAP keys from DB and add all LDAP keys fetched from the LDAP server again. This reduces the complexity with handling every case (and requires fewer lines of code) while meeting the desired result.
Edit: I've added some logic for the key synchronization now, as dropping all keys and adding them again caused some auto increment leakage along with the key not being available for a split second.

This is almost my first time writing any go code at all, I've just read plenty of go code before and made some one-row-changes here and there, so feel free to comment on the code itself along with the design.

There's currently no known issues with this new functionality.

psuet · 2017-05-31T21:14:15Z

Great to see this implemented. I was waiting for this feature in gogs

strk · 2017-06-01T12:55:30Z

Are d54d92f and 9e09a9c present in a separate PR ? If so, which one ? If not: could you file it ? I'm trying to keep the size of this PR smaller for easier review...

strk

Why two migrations ?

strk · 2017-06-01T12:57:14Z

conf/app.ini

@@ -120,6 +120,8 @@ SSH_ROOT_PATH =
 SSH_KEY_TEST_PATH =
 ; Path to ssh-keygen, default is 'ssh-keygen' and let shell find out which one to call.
 SSH_KEYGEN_PATH = ssh-keygen
+; Disable SSH Authorized Key Backup when rewriting all keys, default is false
+SSH_DISABLE_AUTHORIZED_KEYS_BACKUP = true


The value given here should be equal to the default value, so if the documentation is correct (default is false) the value should also be false. I see UPDATE_EXISTING has the same problem, but I don't expect you to fix it here, not being your code (would not be against doing it though)

Thanks, fixed in #1856

lafriks · 2017-06-01T19:59:14Z

conf/app.ini

@@ -453,6 +455,8 @@ SCHEDULE = @every 24h
 ; Create new users, update existing user data and disable users that are not in external source anymore (default)
 ;   or only create new users if UPDATE_EXISTING is set to false
 UPDATE_EXISTING = true
+; Replace all Public SSH Keys that has been added manually with keys from LDAP (default false)
+REPLACE_PUBLIC_SSH_KEYS = false


Wouldn't it be better to add option in source to disable users (that are created from that source) ability add keys manually then just removing his added keys? It seems nonintuitive for users perspective.

I agree. Either such option and/or:
If there's a need to delete the keys that users have already added manually, it seems better to create some sort of button for admins. Such as "delete all keys synchronized from source x for [user-y|all-users]".

But this can also be performed directly in the database for now, I guess the need for this wouldn't be recurring and I'm not sure that it's needed at all.

I'll remove this and create a new PR for such option if I see that there's any need to disable/remove keys

lafriks · 2017-06-01T20:00:26Z

models/migrations/migrations.go

+	// v34 -> v35
+	NewMigration("add field for ldap public ssh key synchronization", addLoginSourceLdapPublicSSHKeySyncEnabled),
+	// v35 -> v36
+	NewMigration("add field for login source id to public key", addPublicKeyLoginSourceIDColumn),


It would be better to add single migration per PR

lafriks · 2017-06-01T20:03:50Z

modules/auth/auth_form.go

 	AttributesInBind              bool
 	Filter                        string
 	AdminFilter                   string
 	IsActive                      bool
 	IsSyncEnabled                 bool
+	IsLdapPublicSSHKeySyncEnabled bool


There is no need for this option as you can just check if AttributeSSHPublicKey is set

I think there's a need for this option, consider the following case:
The administrator wants to synchronize LDAP users, but skip LDAP SSH Key synchronization and still have the AttributeSSHPublicKey value set.

It's not intuitive that setting the attribute automatically enables the key sync, as there's other options for enabling/disabling other login source features. If this option is not needed, I could argue that the "Enable user synchronization" checkbox and "This authentication is activated" is also not needed as those can test/check if the needed attributes for activation is set.

Given this input, do you still consider this option not needed?

IsSyncEnabled and IsActive can't be checked by any other setting. Currently it is same with AdminFilter there is no special option for enabling that it is checked or not - if AdminFilter is set than Admin rights are set based on that filter, otherwise Admin rights can be set only manually. I don't see why anyone would want to set AttributeSSHPublicKey and would not want it to be used anyway.

Ok, I've thought about this for some more time and I agree with you that it's really not needed. Will delete it.

lafriks · 2017-06-01T20:09:46Z

models/login_source.go

+	Name                          string          `xorm:"UNIQUE"`
+	IsActived                     bool            `xorm:"INDEX NOT NULL DEFAULT false"`
+	IsSyncEnabled                 bool            `xorm:"INDEX NOT NULL DEFAULT false"`
+	IsLdapPublicSSHKeySyncEnabled bool            `xorm:"INDEX NOT NULL DEFAULT false"`


Also this option (if really needed) should not be added to Login source as it is common to all Login sources but to LDAP Source type that is LDAP specific

Option deleted.

dnmgns · 2017-06-01T21:40:12Z

Thanks for some great feedback! I'll check this trough in a few hours and make some nice changes.

dnmgns · 2017-06-02T09:20:55Z

@strk - d54d92f and 9e09a9c was not present in a separate PR, so I've reverted them from this PR and created two separate branches which I've created two separate PR for: #1855 and #1856

lafriks · 2017-06-02T10:20:42Z

models/migrations/v35.go

@@ -10,12 +10,18 @@ import (
 	"github.com/go-xorm/xorm"
 )

-func addLoginSourceLdapPublicSSHKeySyncEnabled(x *xorm.Engine) error {
-	// LoginSource see models/login_source.go
+func NewPublicKeyandLoginSourceColumns(x *xorm.Engine) (err error) {


Add comment // NewPublicKeyandLoginSourceColumns ... otherwise make lint is failing

dnmgns · 2017-06-09T17:13:34Z

I've added some logic for the key synchronization now, as dropping all keys and adding them again caused some auto increment leakage along with the key not being available for a split second.

If the key exists in LDAP but not in DB, add it
If the key exists in DB but not in LDAP, and is synchronized from LDAP earlier, delete it

bkcsoft

Otherwise it looks good 😄

bkcsoft · 2017-06-11T07:42:08Z

models/login_source.go

-	IsActived     bool            `xorm:"INDEX NOT NULL DEFAULT false"`
-	IsSyncEnabled bool            `xorm:"INDEX NOT NULL DEFAULT false"`
-	Cfg           core.Conversion `xorm:"TEXT"`
+	ID                            int64           `xorm:"pk autoincr"`


This isn't go-fmt'd

Fixed - and also fixed "some" other no-go-fmt's :)

bkcsoft · 2017-06-11T07:42:25Z

models/migrations/migrations.go

@@ -118,6 +118,8 @@ var migrations = []Migration{
 	NewMigration("remove columns from action", removeActionColumns),
 	// v34 -> v35
 	NewMigration("give all units to owner teams", giveAllUnitsToOwnerTeams),
+  // v35 -> v36


indentation

bkcsoft · 2017-06-11T07:45:09Z

models/user.go

+							for _, giteaKey := range giteaKeys {
+								i := sort.SearchStrings(ldapKeys, giteaKey)
+								if i < len(ldapKeys) && ldapKeys[i] == giteaKey {
+									log.Trace("SyncExternalUsers[%s]: Key exists in LDAP: %s", s.Name, giteaKey)


Please try to split this into separate functions. All these nested if-for-if-for's are just a pain to read

To clarify, this whole function should be split into multiple functions. not just that for-loop ;)

@bkcsoft - Please check my latest commit, does it seem like I'm on the right track to splitting this up nicely?

bkcsoft · 2017-06-15T00:03:47Z

models/user.go

+
+						// Check if sync is needed
+						var giteaKeysToDelete []string
+						if reflect.DeepEqual(giteaKeys, ldapKeys) {


Don't use reflect, make a common interface instead.

What's the reason for not using reflect for this purpose? A common interface - no idea how to do that, could you point me in the right direction?

Because reflect.DeepEqual is so compute intense that you could just as well just sync all keys instead.

As for common interface, seems like giteaKeys and ldapKeys are of the same type no? In that case just make a compare function like so:

func (pk *PublicKey) IsEqual(that *PublicKey) bool { // No need to check `Content`... if it has changed, so has `Fingerprint`... if that != nil && that.Fingerprint == pk.Fingerprint && that.Name == pk.Name && that.Mode == pk.Mode && that.Type == pk.Type { return true } return false }

giteaKeys and ldapKeys are of the same types, slice of strings.

If I were to perform a compare between giteaKeys and ldapKeys in the suggested way, I'd need to create PublicKey types instead of string slices. While this is doable, it would require a fingerprint calculation for every LDAP key as the keys are stored as strings (content only) in LDAP (this is usually how sysadmins choose to store them). In other words; the fingerprint is only stored in database (calcFingerprint is called from addKey).

This also seems most convenient as the addKey function takes a string for content-value (content string).

As both the database and LDAP contains the key content, it seems more appropriate to just compare them.

As I'm building the key name out of the key string (LDAPPublicSSHKey[0:40]) it doesn't matter if the key name (description) for the key has been updated in LDAP.

We're therefore only interested in checking if the actual content has changed.

I've created a new function to compare the slices (giteaKeys and ldapKeys), which is more efficient than reflect.DeepEqual.

bkcsoft · 2017-06-15T00:05:10Z

options/locale/locale_en-US.ini

@@ -1187,6 +1187,7 @@ auths.attribute_username_placeholder = Leave empty to use sign-in form field val
 auths.attribute_name = First name attribute
 auths.attribute_surname = Surname attribute
 auths.attribute_mail = Email attribute
+auths.attribute_sshpublickey=Public SSH key attribute


attributes_ssh_public_key

changed it to attribute_ssh_public_key (singular)

lunny · 2017-06-16T00:59:10Z

models/user.go

+		sshKeysNeedUpdate = true
+	}
+
+	sess.Commit()


sess.Begin() is needed

Quick fix

return sshKeysNeedUpdate, sess.Commit()

lunny · 2017-06-16T00:59:27Z

models/user.go

+		sshKeysNeedUpdate = true
+	}
+
+	sess.Commit()


sess.Begin() is needed

lunny · 2017-06-20T15:07:56Z

models/user.go

+		sshKeysNeedUpdate = true
+	}
+
+	sess.Commit()


Quick fix

return sshKeysNeedUpdate, sess.Commit()

lafriks · 2017-06-21T15:14:37Z

models/user.go

+// IsEqualSlice returns true if slices are equal
+func IsEqualSlice(target []string, source []string) bool {
+	sort.Strings(target)
+	sort.Strings(source)


sort should be moved lower in code after len and nil checks

lafriks · 2017-06-21T15:19:35Z

models/user.go

@@ -1332,6 +1333,142 @@ func GetWatchedRepos(userID int64, private bool) ([]*Repository, error) {
 	return repos, nil
 }

+// existsInSlice returns true if string exists in slice
+func existsInSlice(target string, slice []string) bool {
+	sort.Strings(slice)


this function can be rewritten to:

func existsInSlice(target string, slice []string) bool { i := sort.Search(len(slice), func(i int) bool { return slice[i] == target }) return i < len(slice) }

lafriks · 2017-06-21T15:21:22Z

models/user.go

+func deleteKeysMarkedForDeletion(keys []string) (sshKeysNeedUpdate bool, err error) {
+	// Start session
+	sess := x.NewSession()
+	defer sessionRelease(sess)


Rebase from master is needed as sessionRelease function has been removed and this line must be replaced with defer sess.Close()

lafriks · 2017-06-25T13:36:08Z

Please rebase as otherwise is hard to understand what has actually changed

lafriks · 2017-06-25T21:29:32Z

models/user.go

+				}
+
+				// Check if user data has changed
+				if (len(s.LDAP().AdminFilter) > 0 && usr.IsAdmin != su.IsAdmin) ||


Why did this code was moved out of } else if updateExisting {? As now user data will be updated from LDAP even if updateExisting is set to false

Ah my misstake, not intentional. Fixed it now, nice catch!

dnmgns · 2017-08-16T08:30:44Z

FYI: The requested code-changes has been performed, unfortunately I messed up history so they are displayed as outdated.

dnmgns · 2018-03-08T20:27:40Z

@lunny - rebased
@lafriks - looks good?

lunny · 2018-05-11T08:18:06Z

please resolve conflicts.

dnmgns · 2018-05-19T18:46:02Z

@lunny - resolved

lafriks · 2018-05-24T04:43:03Z

@dnmgns I hope you don't mind that I did some minor fixes and added ssh key sync integration test

dnmgns · 2018-05-24T06:07:40Z

@lafriks - I don't mind at all. Nice changes, thanks!

lafriks · 2018-05-24T14:35:51Z

@dnmgns thanks for your PR and sorry for taking so long to merge it

* SECURITY * Limit uploaded avatar image-size to 4096x3072 by default (go-gitea#4353) * Do not allow to reuse TOTP passcode (go-gitea#3878) * FEATURE * Add cli commands to regen hooks & keys (go-gitea#3979) * Add support for FIDO U2F (go-gitea#3971) * Added user language setting (go-gitea#3875) * LDAP Public SSH Keys synchronization (go-gitea#1844) * Add topic support (go-gitea#3711) * Multiple assignees (go-gitea#3705) * Add protected branch whitelists for merging (go-gitea#3689) * Global code search support (go-gitea#3664) * Add label descriptions (go-gitea#3662) * Add issue search via API (go-gitea#3612) * Add repository setting to enable/disable health checks (go-gitea#3607) * Emoji Autocomplete (go-gitea#3433) * Implements generator cli for secrets (go-gitea#3531) * ENHANCEMENT * Add more webhooks support and refactor webhook templates directory (go-gitea#3929) * Add new option to allow only OAuth2/OpenID user registration (go-gitea#3910) * Add option to use paged LDAP search when synchronizing users (go-gitea#3895) * Symlink icons (go-gitea#1416) * Improve release page UI (go-gitea#3693) * Add admin dashboard option to run health checks (go-gitea#3606) * Add branch link in branch list (go-gitea#3576) * Reduce sql query times in retrieveFeeds (go-gitea#3547) * Option to enable or disable swagger endpoints (go-gitea#3502) * Add missing licenses (go-gitea#3497) * Reduce repo indexer disk usage (go-gitea#3452) * Enable caching on assets and avatars (go-gitea#3376) * Add repository search ordered by stars/forks. Forks column in admin repo list (go-gitea#3969) * Add Environment Variables to Docker template (go-gitea#4012) * LFS: make HTTP auth period configurable (go-gitea#4035) * Add config path as an optionial flag when changing pass via CLI (go-gitea#4184) * Refactor User Settings sections (go-gitea#3900) * Allow square brackets in external issue patterns (go-gitea#3408) * Add Attachment API (go-gitea#3478) * Add EnableTimetracking option to app settings (go-gitea#3719) * Add config option to enable or disable log executed SQL (go-gitea#3726) * Shows total tracked time in issue and milestone list (go-gitea#3341) * TRANSLATION * Improve English grammar and consistency (go-gitea#3614) * DEPLOYMENT * Allow Gitea to run as different USER in Docker (go-gitea#3961) * Provide compressed release binaries (go-gitea#3991) * Sign release binaries (go-gitea#4188)

dnmgns mentioned this pull request May 31, 2017

LDAP authentication: fetch SSH public keys from LDAP gogs/gogs#2488

Open

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 31, 2017

lunny added this to the 1.3.0 milestone Jun 1, 2017

lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jun 1, 2017

strk reviewed Jun 1, 2017

View reviewed changes

lafriks reviewed Jun 1, 2017

View reviewed changes

This was referenced Jun 2, 2017

Delete Public SSH Key tmp file after calculating fingerprint #1855

Merged

Setting to disable authorized_keys backup #1856

Merged

lafriks requested changes Jun 2, 2017

View reviewed changes

bkcsoft suggested changes Jun 11, 2017

View reviewed changes

bkcsoft suggested changes Jun 15, 2017

View reviewed changes

lunny reviewed Jun 16, 2017

View reviewed changes

lunny requested changes Jun 16, 2017

View reviewed changes

models/user.go Outdated

sshKeysNeedUpdate = true

}

sess.Commit()

Copy link

Member

lunny Jun 16, 2017

Choose a reason for hiding this comment

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

sess.Begin() is needed

lunny requested changes Jun 20, 2017

View reviewed changes

models/user.go Outdated

sshKeysNeedUpdate = true

}

sess.Commit()

Copy link

Member

lunny Jun 20, 2017

Choose a reason for hiding this comment

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

Quick fix

return sshKeysNeedUpdate, sess.Commit()

lafriks requested changes Jun 21, 2017

View reviewed changes

dnmgns force-pushed the master branch 3 times, most recently from 7018469 to 6cb1894 Compare June 25, 2017 21:25

lafriks requested changes Jun 25, 2017

View reviewed changes

bkcsoft approved these changes Jun 26, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master'

b4876d3

dnmgns force-pushed the master branch 2 times, most recently from b4b5e37 to b4876d3 Compare March 22, 2018 22:32

Magnus Lindvall and others added 6 commits April 10, 2018 12:14

Merge remote-tracking branch 'upstream/master'

dc85ddc

Fix import

3ff65c7

empty

3181cb9

Merge remote-tracking branch 'upstream/master'

f05fe63

Merge branch 'master' into master

ee1fcd9

Merge remote-tracking branch 'upstream/master'

3b75a05

Merge remote-tracking branch 'upstream/master'

9f10f71

Set default login source id to 0

237ae23

dnmgns force-pushed the master branch from 5f63673 to 237ae23 Compare May 19, 2018 19:37

Magnus Lindvall and others added 4 commits May 19, 2018 22:27

Fixes spacing for LDAPConfig

f3b4b31

empty

32d8e0f

Merge branch 'master' into master

47cab1d

Some minor cleanup. Add integration tests (updete dep testify)

bc32826

lafriks approved these changes May 24, 2018

View reviewed changes

lafriks added 2 commits May 24, 2018 07:40

Revert to update only needed user columns

cb13880

Merge branch 'master' into master

8a56bf3

lafriks added the type/changelog Adds the changelog for a new Gitea version label May 24, 2018

lafriks merged commit cdb9478 into go-gitea:master May 24, 2018

go-gitea locked and limited conversation to collaborators Nov 24, 2020

delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023

LDAP Public SSH Keys synchronization #1844

LDAP Public SSH Keys synchronization #1844

Conversation

dnmgns commented May 31, 2017 • edited Loading

The PR introduces the following changes in short:

Thoughts

psuet commented May 31, 2017

strk commented Jun 1, 2017

strk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnmgns commented Jun 1, 2017

dnmgns commented Jun 2, 2017

Choose a reason for hiding this comment

dnmgns commented Jun 9, 2017

bkcsoft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkcsoft Jun 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks commented Jun 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnmgns commented Aug 16, 2017

dnmgns commented Mar 8, 2018

lunny commented May 11, 2018

dnmgns commented May 19, 2018

lafriks commented May 24, 2018

dnmgns commented May 24, 2018

lafriks commented May 24, 2018

dnmgns commented May 31, 2017 •

edited

Loading

lafriks Jun 1, 2017 •

edited

Loading

lafriks Jun 2, 2017 •

edited

Loading

lafriks Jun 1, 2017 •

edited

Loading

bkcsoft Jun 11, 2017 •

edited

Loading