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

LDAP Public SSH Keys synchronization #1844

Merged
merged 50 commits into from
May 24, 2018
Merged

LDAP Public SSH Keys synchronization #1844

merged 50 commits into from
May 24, 2018

Conversation

dnmgns
Copy link
Contributor

@dnmgns dnmgns commented May 31, 2017

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
Copy link

psuet commented May 31, 2017

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

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 31, 2017
@lunny lunny added this to the 1.3.0 milestone Jun 1, 2017
@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jun 1, 2017
@strk
Copy link
Member

strk commented Jun 1, 2017

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...

Copy link
Member

@strk strk left a comment

Choose a reason for hiding this comment

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

Why two migrations ?

conf/app.ini Outdated
@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed in #1856

conf/app.ini Outdated
@@ -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
Copy link
Member

@lafriks lafriks Jun 1, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

// 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),
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to add single migration per PR

AttributesInBind bool
Filter string
AdminFilter string
IsActive bool
IsSyncEnabled bool
IsLdapPublicSSHKeySyncEnabled bool
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

@lafriks lafriks Jun 2, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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"`
Copy link
Member

@lafriks lafriks Jun 1, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Option deleted.

@dnmgns
Copy link
Contributor Author

dnmgns commented Jun 1, 2017

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

@dnmgns
Copy link
Contributor Author

dnmgns commented Jun 2, 2017

@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

@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

@dnmgns
Copy link
Contributor Author

dnmgns commented Jun 9, 2017

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

Copy link
Member

@bkcsoft bkcsoft left a comment

Choose a reason for hiding this comment

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

Otherwise it looks good 😄

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"`
Copy link
Member

Choose a reason for hiding this comment

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

This isn't go-fmt'd

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 - and also fixed "some" other no-go-fmt's :)

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

indentation

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

models/user.go Outdated
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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@bkcsoft bkcsoft Jun 11, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

models/user.go Outdated

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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
}
  

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

attributes_ssh_public_key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed it to attribute_ssh_public_key (singular)

models/user.go Outdated
sshKeysNeedUpdate = true
}

sess.Commit()
Copy link
Member

Choose a reason for hiding this comment

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

err handle

Copy link
Member

Choose a reason for hiding this comment

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

sess.Begin() is needed

Copy link
Member

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()

models/user.go Outdated
sshKeysNeedUpdate = true
}

sess.Commit()
Copy link
Member

Choose a reason for hiding this comment

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

sess.Begin() is needed

models/user.go Outdated
sshKeysNeedUpdate = true
}

sess.Commit()
Copy link
Member

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()

models/user.go Outdated
// IsEqualSlice returns true if slices are equal
func IsEqualSlice(target []string, source []string) bool {
sort.Strings(target)
sort.Strings(source)
Copy link
Member

Choose a reason for hiding this comment

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

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

models/user.go Outdated
@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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)
}

models/user.go Outdated
func deleteKeysMarkedForDeletion(keys []string) (sshKeysNeedUpdate bool, err error) {
// Start session
sess := x.NewSession()
defer sessionRelease(sess)
Copy link
Member

Choose a reason for hiding this comment

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

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

@lafriks
Copy link
Member

lafriks commented Jun 25, 2017

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

@dnmgns dnmgns force-pushed the master branch 3 times, most recently from 7018469 to 6cb1894 Compare June 25, 2017 21:25
models/user.go Outdated
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@dnmgns
Copy link
Contributor Author

dnmgns commented Aug 16, 2017

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

@dnmgns
Copy link
Contributor Author

dnmgns commented Mar 8, 2018

@lunny - rebased
@lafriks - looks good?

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

lunny commented May 11, 2018

please resolve conflicts.

@dnmgns
Copy link
Contributor Author

dnmgns commented May 19, 2018

@lunny - resolved

@lafriks
Copy link
Member

lafriks commented May 24, 2018

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

@lafriks lafriks added the type/changelog Adds the changelog for a new Gitea version label May 24, 2018
@lafriks lafriks merged commit cdb9478 into go-gitea:master May 24, 2018
@dnmgns
Copy link
Contributor Author

dnmgns commented May 24, 2018

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

@lafriks
Copy link
Member

lafriks commented May 24, 2018

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

aswild added a commit to aswild/gitea that referenced this pull request Jul 6, 2018
* 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)
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@delvh delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants