LDAP Public SSH Keys synchronization

[dnmgns]

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]

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

[strk]

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]

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)

[dnmgns]

Thanks, fixed in #1856

[lafriks]

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.

[dnmgns]

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]

It would be better to add single migration per PR

[lafriks]

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

[dnmgns]

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?

[lafriks]

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.

[dnmgns]

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]

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

[dnmgns]

Option deleted.

[dnmgns]

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

[dnmgns]

@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]

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

[dnmgns]

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]

This isn't go-fmt'd

[dnmgns]

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

[bkcsoft]

indentation

[dnmgns]

Fixed

[bkcsoft]

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

[bkcsoft]

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

[dnmgns]

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

[bkcsoft]

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

[dnmgns]

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?

[bkcsoft]

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
}
  

[dnmgns]

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]

attributes_ssh_public_key

[dnmgns]

changed it to attribute_ssh_public_key (singular)

[lunny]

err handle

[lunny]

sess.Begin() is needed

[lunny]

Quick fix

return sshKeysNeedUpdate, sess.Commit()

[lunny]

sess.Begin() is needed

[lunny]

Quick fix

return sshKeysNeedUpdate, sess.Commit()

[lafriks]

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

[lafriks]

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]

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

[lafriks]

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

[lafriks]

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

[dnmgns]

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

[dnmgns]

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

[dnmgns]

@lunny - rebased
@lafriks - looks good?

[lunny]

please resolve conflicts.

[dnmgns]

@lunny - resolved

[lafriks]

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

[dnmgns]

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

[lafriks]

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