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
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
64312ab
Add LDAP Key Synchronization feature
dnmgns May 31, 2017
d17981b
Move block: user data has changed
dnmgns Jun 25, 2017
cc046c7
Add LDAP Key Synchronization feature
dnmgns May 31, 2017
26a9205
Move block: user data has changed
dnmgns Jun 25, 2017
d005086
Merge branch 'master' of https://github.com/dnmgns/gitea
Oct 15, 2017
e0da1d7
Oops
Oct 15, 2017
b6e2985
Fix space
Oct 15, 2017
a20d4f1
Merge branch 'master' of https://github.com/go-gitea/gitea
Nov 5, 2017
1d4dbed
null def null not needed
Nov 5, 2017
55424bf
move existsInSlice and IsEqualSlice to compare
Nov 5, 2017
922e76c
revert just whitespace
Nov 5, 2017
03690de
Make compare funcs exported
Nov 5, 2017
ec95606
Fix typo in comment
Nov 5, 2017
71cd39d
Change comment, try to make lint happy
Nov 5, 2017
eddb932
Add migration: add login source id column for public_key table
Nov 5, 2017
ab9f88b
Only update keys if needed
Nov 5, 2017
056734f
Make lint happy
Nov 5, 2017
83cd353
Trigger a build.
Nov 6, 2017
56453ac
Merge remote-tracking branch 'upstream/master'
dnmgns Dec 6, 2017
027c832
Simple validity check for ssh key
Dec 6, 2017
9915566
Merge remote-tracking branch 'upstream/master'
Dec 19, 2017
23b343d
Move migration to resolve conflict
Dec 19, 2017
98a8083
Only update columns which needs update
Dec 24, 2017
934223d
Merge remote-tracking branch 'upstream/master'
Jan 9, 2018
615e9c5
Do not always update all columns
Jan 21, 2018
45439fd
Add function to only list pubkey synchronized from ldap
Jan 21, 2018
a199541
Only list pub ssh keys synchronized from ldap. Do not sort strings as…
Jan 21, 2018
93f11ea
Merge remote-tracking branch 'upstream/master'
Jan 21, 2018
8b6f96c
Only get keys belonging to current login source id
Jan 25, 2018
0d8d954
Merge remote-tracking branch 'upstream/master'
Jan 25, 2018
cf6ba93
Merge remote-tracking branch 'upstream/master'
Feb 15, 2018
68a1dc7
login source, not ldap source
Feb 15, 2018
68e206a
A better argument name
Feb 15, 2018
3f3ab5e
Merge remote-tracking branch 'upstream/master'
dnmgns Mar 4, 2018
aff1346
Remove unused line
dnmgns Mar 4, 2018
b4876d3
Merge remote-tracking branch 'upstream/master'
Mar 22, 2018
dc85ddc
Merge remote-tracking branch 'upstream/master'
Apr 10, 2018
3ff65c7
Fix import
Apr 10, 2018
3181cb9
empty
Apr 10, 2018
f05fe63
Merge remote-tracking branch 'upstream/master'
Apr 19, 2018
ee1fcd9
Merge branch 'master' into master
dnmgns Apr 20, 2018
3b75a05
Merge remote-tracking branch 'upstream/master'
May 3, 2018
9f10f71
Merge remote-tracking branch 'upstream/master'
May 19, 2018
237ae23
Set default login source id to 0
May 19, 2018
f3b4b31
Fixes spacing for LDAPConfig
May 19, 2018
32d8e0f
empty
May 19, 2018
47cab1d
Merge branch 'master' into master
lafriks May 23, 2018
bc32826
Some minor cleanup. Add integration tests (updete dep testify)
lafriks May 24, 2018
cb13880
Revert to update only needed user columns
lafriks May 24, 2018
8a56bf3
Merge branch 'master' into master
lafriks May 24, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions models/migrations/migrations.go
Expand Up @@ -142,6 +142,8 @@ var migrations = []Migration{
NewMigration("remove index column from repo_unit table", removeIndexColumnFromRepoUnitTable),
// v46 -> v47
NewMigration("remove organization watch repositories", removeOrganizationWatchRepo),
// v47 -> v48
NewMigration("add field for ldap public public ssh key synchronization", addLoginSourceLdapPublicSSHKeySyncEnabled),
}

// Migrate database to current version
Expand Down
23 changes: 23 additions & 0 deletions models/migrations/v47.go
@@ -0,0 +1,23 @@
// Copyright 2017 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.

package migrations

import (
"fmt"

"github.com/go-xorm/xorm"
)

func addLoginSourceLdapPublicSSHKeySyncEnabled(x *xorm.Engine) error {
Copy link
Member

Choose a reason for hiding this comment

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

You need to add also x.Sync2 for PublicKey table

// LoginSource see models/login_source.go
type LoginSource struct {
IsLdapPublicSSHKeySyncEnabled bool `xorm:"INDEX NOT NULL DEFAULT false"`
Copy link
Member

Choose a reason for hiding this comment

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

I don't see this field used any more, so this sync should be just replaced with added LoginSourceID column for PublicKey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, right. I changed this behaviour, now the sync is enabled as soon as it's configured and there's no longer any option to enable/disable the sync from the app.ini file. I've just somehow forgotten that I did this, and to remove this part of the code as well. Grr :) Nice catch, thnx!

}

if err := x.Sync2(new(LoginSource)); err != nil {
return fmt.Errorf("Sync2: %v", err)
}
return nil
}
30 changes: 16 additions & 14 deletions models/ssh_key.go
Expand Up @@ -46,13 +46,14 @@ const (

// PublicKey represents a user or deploy SSH public key.
type PublicKey struct {
ID int64 `xorm:"pk autoincr"`
OwnerID int64 `xorm:"INDEX NOT NULL"`
Name string `xorm:"NOT NULL"`
Fingerprint string `xorm:"NOT NULL"`
Content string `xorm:"TEXT NOT NULL"`
Mode AccessMode `xorm:"NOT NULL DEFAULT 2"`
Type KeyType `xorm:"NOT NULL DEFAULT 1"`
ID int64 `xorm:"pk autoincr"`
OwnerID int64 `xorm:"INDEX NOT NULL"`
Name string `xorm:"NOT NULL"`
Fingerprint string `xorm:"NOT NULL"`
Content string `xorm:"TEXT NOT NULL"`
Mode AccessMode `xorm:"NOT NULL DEFAULT 2"`
Type KeyType `xorm:"NOT NULL DEFAULT 1"`
LoginSourceID int64 `xorm:"NULL DEFAULT NULL"`
Copy link
Member

Choose a reason for hiding this comment

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

NULL DEFAULT NULL is not needed


Created time.Time `xorm:"-"`
CreatedUnix int64 `xorm:"created"`
Expand Down Expand Up @@ -393,7 +394,7 @@ func addKey(e Engine, key *PublicKey) (err error) {
}

// AddPublicKey adds new public key to database and authorized_keys file.
func AddPublicKey(ownerID int64, name, content string) (*PublicKey, error) {
func AddPublicKey(ownerID int64, name, content string, LoginSourceID int64) (*PublicKey, error) {
log.Trace(content)

fingerprint, err := calcFingerprint(content)
Expand Down Expand Up @@ -422,12 +423,13 @@ func AddPublicKey(ownerID int64, name, content string) (*PublicKey, error) {
}

key := &PublicKey{
OwnerID: ownerID,
Name: name,
Fingerprint: fingerprint,
Content: content,
Mode: AccessModeWrite,
Type: KeyTypeUser,
OwnerID: ownerID,
Name: name,
Fingerprint: fingerprint,
Content: content,
Mode: AccessModeWrite,
Type: KeyTypeUser,
LoginSourceID: LoginSourceID,
}
if err = addKey(sess, key); err != nil {
return nil, fmt.Errorf("addKey: %v", err)
Expand Down
161 changes: 159 additions & 2 deletions models/user.go
Expand Up @@ -19,6 +19,7 @@ import (
"image/png"
"os"
"path/filepath"
"sort"
"strings"
"time"
"unicode/utf8"
Expand Down Expand Up @@ -1355,6 +1356,138 @@ 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 {
Copy link
Member

Choose a reason for hiding this comment

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

Can you please move existsInSlice and IsEqualSlice to modules/util/compare.go?

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

// IsEqualSlice returns true if slices are equal
func IsEqualSlice(target []string, source []string) bool {
if len(target) != len(source) {
return false
}

if (target == nil) != (source == nil) {
return false
}

sort.Strings(target)
sort.Strings(source)

for i, v := range target {
if v != source[i] {
return false
}
}

return true
}

// deleteKeysMarkedForDeletion returns true if ssh keys needs update
func deleteKeysMarkedForDeletion(keys []string) (sshKeysNeedUpdate bool, err error) {
// Start session
sess := x.NewSession()
defer sess.Close()
if err = sess.Begin(); err != nil {
return false, err
}

// Delete keys marked for deletion
for _, KeyToDelete := range keys {
key, err := SearchPublicKeyByContent(KeyToDelete)
err = deletePublicKeys(sess, key.ID)
if err != nil {
sshKeysNeedUpdate = false
log.Trace("deleteKeysMarkedForDeletion: Error: %v", key)
}
sshKeysNeedUpdate = true
}

return sshKeysNeedUpdate, sess.Commit()
}

func addLdapSSHPublicKeys(s *LoginSource, usr *User, SSHPublicKeys []string) (sshKeysNeedUpdate bool, err error) {
for _, LDAPPublicSSHKey := range SSHPublicKeys {
LDAPPublicSSHKeyName := strings.Join([]string{s.Name, LDAPPublicSSHKey[0:40]}, "-")
_, err := AddPublicKey(usr.ID, LDAPPublicSSHKeyName, LDAPPublicSSHKey, s.ID)
if err != nil {
log.Error(4, "addLdapSSHPublicKeys[%s]: Error adding LDAP Public SSH Key for user %s: %v", s.Name, usr.Name, err)
} else {
log.Trace("addLdapSSHPublicKeys[%s]: Added LDAP Public SSH Key for user %s: %v", s.Name, usr.Name, LDAPPublicSSHKey)
sshKeysNeedUpdate = true
}

}
return sshKeysNeedUpdate, err
}

func synchronizeLdapSSHPublicKeys(s *LoginSource, SSHPublicKeys []string, usr *User) (sshKeysNeedUpdate bool, err error) {
log.Trace("synchronizeLdapSSHPublicKeys[%s]: Handling LDAP Public SSH Key synchronization for user %s", s.Name, usr.Name)

// Get Public Keys from DB with LDAP source
var giteaKeys []string
keys, err := ListPublicKeys(usr.ID)
Copy link
Member

Choose a reason for hiding this comment

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

Why not just only load all public keys where login_source_id > 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that would be more efficient. Would you recommend modifying the current ListPublicKeys for this purpose (adding optional 'where login_source_id>x'), creating a new function or any other method to accomplish that?

Copy link
Member

Choose a reason for hiding this comment

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

@dnmgns Yes, I think you should do that for performance.

if err != nil {
log.Error(4, "synchronizeLdapSSHPublicKeys[%s]: Error listing LDAP Public SSH Keys for user %s: %v", s.Name, usr.Name, err)
}

// Get Public Keys from DB, which has been synchronized from LDAP
for _, v := range keys {
// If key was synced from LDAP, add it to list
if v.LoginSourceID > 0 {
giteaKeys = append(giteaKeys, v.OmitEmail())
}
}
sort.Strings(giteaKeys)

// Get Public Keys from LDAP and skip duplicate keys
var ldapKeys []string
for _, v := range SSHPublicKeys {
ldapKey := strings.Join(strings.Split(v, " ")[:2], " ")
if !existsInSlice(ldapKey, ldapKeys) {
ldapKeys = append(ldapKeys, ldapKey)
}
}
sort.Strings(ldapKeys)

// Check if Public Key sync is needed
var giteaKeysToDelete []string
if IsEqualSlice(giteaKeys, ldapKeys) {
log.Trace("synchronizeLdapSSHPublicKeys[%s]: LDAP Public Keys are already in sync for %s (LDAP:%v/DB:%v)", s.Name, usr.Name, len(ldapKeys), len(giteaKeys))
} else {
log.Trace("synchronizeLdapSSHPublicKeys[%s]: LDAP Public Key needs update for user %s (LDAP:%v/DB:%v)", s.Name, usr.Name, len(ldapKeys), len(giteaKeys))
// Delete giteaKeys that doesn't exist in ldapKeys and has been synced from LDAP earlier
for _, giteaKey := range giteaKeys {
if !existsInSlice(giteaKey, ldapKeys) {
log.Trace("synchronizeLdapSSHPublicKeys[%s]: Marking LDAP Public SSH Key for deletion for user %s: %v", s.Name, usr.Name, giteaKey)
giteaKeysToDelete = append(giteaKeysToDelete, giteaKey)
}
}
}

// Delete LDAP keys from DB that doesn't exist in LDAP
sshKeysNeedUpdate, err = deleteKeysMarkedForDeletion(giteaKeysToDelete)
if err != nil {
log.Error(4, "synchronizeLdapSSHPublicKeys[%s]: Error deleting LDAP Public SSH Keys marked for deletion for user %s: %v", s.Name, usr.Name, err)
}

// Add LDAP Public SSH Keys that doesn't already exist in DB
var newLdapSSHKeys []string
for _, LDAPPublicSSHKey := range ldapKeys {
if !existsInSlice(LDAPPublicSSHKey, giteaKeys) {
newLdapSSHKeys = append(newLdapSSHKeys, LDAPPublicSSHKey)
}
}
sshKeysNeedUpdate, err = addLdapSSHPublicKeys(s, usr, newLdapSSHKeys)
if err != nil {
log.Error(4, "synchronizeLdapSSHPublicKeys[%s]: Error updating LDAP Public SSH Keys for user %s: %v", s.Name, usr.Name, err)
}

return sshKeysNeedUpdate, err
}

// SyncExternalUsers is used to synchronize users with external authorization source
func SyncExternalUsers() {
if !taskStatusTable.StartIfNotRunning(syncExternalUsers) {
Expand All @@ -1376,10 +1509,13 @@ func SyncExternalUsers() {
if !s.IsActived || !s.IsSyncEnabled {
continue
}

if s.IsLDAP() {
log.Trace("Doing: SyncExternalUsers[%s]", s.Name)

var existingUsers []int64
var isAttributeSSHPublicKeySet = len(strings.TrimSpace(s.LDAP().AttributeSSHPublicKey)) > 0
var sshKeysNeedUpdate bool

// Find all users with this login type
var users []User
Expand All @@ -1388,7 +1524,6 @@ func SyncExternalUsers() {
Find(&users)

sr := s.LDAP().SearchEntries()

for _, su := range sr {
if len(su.Username) == 0 {
continue
Expand Down Expand Up @@ -1425,11 +1560,28 @@ func SyncExternalUsers() {
}

err = CreateUser(usr)

if err != nil {
log.Error(4, "SyncExternalUsers[%s]: Error creating user %s: %v", s.Name, su.Username, err)
} else if isAttributeSSHPublicKeySet {
log.Trace("SyncExternalUsers[%s]: Adding LDAP Public SSH Keys for user %s", s.Name, usr.Name)
sshKeysNeedUpdate, err = addLdapSSHPublicKeys(s, usr, su.SSHPublicKey)
if err != nil {
log.Error(4, "SyncExternalUsers[%s]: Error adding LDAP Public SSH Keys for user %s: %v", s.Name, su.Username, err)
}
}
} else if updateExisting {
existingUsers = append(existingUsers, usr.ID)

err = UpdateUserCols(usr, "full_name", "email", "is_admin", "is_active")
if err != nil {
log.Error(4, "SyncExternalUsers[%s]: Error updating user %s: %v", s.Name, usr.Name, err)
}

if isAttributeSSHPublicKeySet {
synchronizeLdapSSHPublicKeys(s, su.SSHPublicKey, usr)
}

// Check if user data has changed
if (len(s.LDAP().AdminFilter) > 0 && usr.IsAdmin != su.IsAdmin) ||
strings.ToLower(usr.Email) != strings.ToLower(su.Mail) ||
Expand All @@ -1446,14 +1598,19 @@ func SyncExternalUsers() {
}
usr.IsActive = true

err = UpdateUserCols(usr, "full_name", "email", "is_admin", "is_active")
err = UpdateUser(usr)
Copy link
Member

Choose a reason for hiding this comment

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

why update all the columns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lunny @lafriks - right, got it. this update was unnecessary. now removed.

if err != nil {
log.Error(4, "SyncExternalUsers[%s]: Error updating user %s: %v", s.Name, usr.Name, err)
}
}
}
}

// Rewrite authorized_keys file if LDAP Public SSH Key attribute is set and any key was added or removed
if isAttributeSSHPublicKeySet && sshKeysNeedUpdate {
RewriteAllPublicKeys()
}

// Deactivate users not present in LDAP
if updateExisting {
for _, usr := range users {
Expand Down
1 change: 1 addition & 0 deletions modules/auth/auth_form.go
Expand Up @@ -24,6 +24,7 @@ type AuthenticationForm struct {
AttributeName string
AttributeSurname string
AttributeMail string
AttributeSSHPublicKey string
AttributesInBind bool
Filter string
AdminFilter string
Expand Down
61 changes: 32 additions & 29 deletions modules/auth/ldap/ldap.go
Expand Up @@ -28,32 +28,34 @@ const (

// Source Basic LDAP authentication service
type Source struct {
Name string // canonical name (ie. corporate.ad)
Host string // LDAP host
Port int // port number
SecurityProtocol SecurityProtocol
SkipVerify bool
BindDN string // DN to bind with
BindPassword string // Bind DN password
UserBase string // Base search path for users
UserDN string // Template for the DN of the user for simple auth
AttributeUsername string // Username attribute
AttributeName string // First name attribute
AttributeSurname string // Surname attribute
AttributeMail string // E-mail attribute
AttributesInBind bool // fetch attributes in bind context (not user)
Filter string // Query filter to validate entry
AdminFilter string // Query filter to check if user is admin
Enabled bool // if this source is disabled
Name string // canonical name (ie. corporate.ad)
Host string // LDAP host
Port int // port number
SecurityProtocol SecurityProtocol
SkipVerify bool
BindDN string // DN to bind with
BindPassword string // Bind DN password
UserBase string // Base search path for users
UserDN string // Template for the DN of the user for simple auth
AttributeUsername string // Username attribute
AttributeName string // First name attribute
AttributeSurname string // Surname attribute
AttributeMail string // E-mail attribute
AttributeSSHPublicKey string // LDAP SSH Public Key attribute
AttributesInBind bool // fetch attributes in bind context (not user)
Filter string // Query filter to validate entry
AdminFilter string // Query filter to check if user is admin
Enabled bool // if this source is disabled
}

// SearchResult : user data
type SearchResult struct {
Username string // Username
Name string // Name
Surname string // Surname
Mail string // E-mail address
IsAdmin bool // if user is administrator
Username string // Username
Name string // Name
Surname string // Surname
Mail string // E-mail address
SSHPublicKey []string // SSH Public Key
IsAdmin bool // if user is administrator
}

func (ls *Source) sanitizedUserQuery(username string) (string, bool) {
Expand Down Expand Up @@ -292,10 +294,10 @@ func (ls *Source) SearchEntries() []*SearchResult {

userFilter := fmt.Sprintf(ls.Filter, "*")

log.Trace("Fetching attributes '%v', '%v', '%v', '%v' with filter %s and base %s", ls.AttributeUsername, ls.AttributeName, ls.AttributeSurname, ls.AttributeMail, userFilter, ls.UserBase)
log.Trace("Fetching attributes '%v', '%v', '%v', '%v', '%v' with filter %s and base %s", ls.AttributeUsername, ls.AttributeName, ls.AttributeSurname, ls.AttributeMail, ls.AttributeSSHPublicKey, userFilter, ls.UserBase)
search := ldap.NewSearchRequest(
ls.UserBase, ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false, userFilter,
[]string{ls.AttributeUsername, ls.AttributeName, ls.AttributeSurname, ls.AttributeMail},
[]string{ls.AttributeUsername, ls.AttributeName, ls.AttributeSurname, ls.AttributeMail, ls.AttributeSSHPublicKey},
nil)

sr, err := l.Search(search)
Expand All @@ -308,11 +310,12 @@ func (ls *Source) SearchEntries() []*SearchResult {

for i, v := range sr.Entries {
result[i] = &SearchResult{
Username: v.GetAttributeValue(ls.AttributeUsername),
Name: v.GetAttributeValue(ls.AttributeName),
Surname: v.GetAttributeValue(ls.AttributeSurname),
Mail: v.GetAttributeValue(ls.AttributeMail),
IsAdmin: checkAdmin(l, ls, v.DN),
Username: v.GetAttributeValue(ls.AttributeUsername),
Name: v.GetAttributeValue(ls.AttributeName),
Surname: v.GetAttributeValue(ls.AttributeSurname),
Mail: v.GetAttributeValue(ls.AttributeMail),
SSHPublicKey: v.GetAttributeValues(ls.AttributeSSHPublicKey),
IsAdmin: checkAdmin(l, ls, v.DN),
}
}

Expand Down