Setting to disable authorized_keys backup #1856

dnmgns · 2017-06-02T09:18:28Z

When using LDAP User Synchronization (#1478) with LDAP Public SSH Keys synchronization (#1844), the authorized_keys backup might be running with a quite high frequency (as it's called every time when rewriting public keys).

The system could end up with lots of backup-files causing inode issue and running out of free space.

This PR addresses this issue by giving the administrator the option to disable the authorized_keys backup when rewriting the public keys. This enables the administrator to perform the backup on his own outside of Gitea instead, or simply skip the backup as the keys are already stored in LDAP.

Signed-off-by: Magnus Lindvall <magnus@dnmgns.com>

sapk · 2017-06-04T02:08:45Z

It's seems that the disabled part also have a part that recopy line from old version (that kind of weird but maybe to keep manually added keys). I am not against also disabling that but that should be explicitly describe in description of VAR (ex: this will override manually added keys). https://github.com/dnmgns/gitea/blob/78314cc1cdbcf2ba88bcf124de578c3b830af085/models/ssh_key.go#L591

dnmgns · 2017-06-08T07:11:49Z

@sapk Yeah, seems that it has that part to save keys that has been added manually (written directly to authorized_keys file). As there's a tmp-file being created, we could just use that one for this purpose instead of the backup-file (old file). I've made some changes now, use the tmp-file for saving old keys instead (authorized_keys => tmp-file). And if backup is enabled, simply just copy the current authorized_keys file to a backup file.

Signed-off-by: Magnus Lindvall <magnus@dnmgns.com>

bkcsoft

Nitpicking 🙂

bkcsoft · 2017-06-11T07:50:23Z

models/ssh_key.go

 		os.Remove(tmpPath)
 	}()

+	if com.IsExist(fPath) && !setting.SSH.DisableAuthorizedKeysBackup {
+		bakPath := fPath + fmt.Sprintf("_%d.gitea_bak", time.Now().Unix())


While you're moving this line, mind changing it to

bakPath := fmt.Sprintf("%s_%d.gitea_bak", fPath, time.Now().Unix())

bkcsoft · 2017-06-11T07:51:37Z

models/ssh_key.go

 	}

-	f.Close()
-	if err = os.Rename(tmpPath, fpath); err != nil {
+	t.Close()


You're closing this twice

Thanks, I thought it looked like it but I also thought that maybe it was there for a reason that I didn't know about. FYI: This wasn't introduced in this PR, closing twice also exists in master (first and last row here: https://github.com/go-gitea/gitea/blob/master/models/ssh_key.go#L568-L606)

But I can fix it in this PR.

Signed-off-by: Magnus Lindvall <magnus@dnmgns.com>

dnmgns · 2017-06-13T07:56:53Z

@bkcsoft - Sent in some new commits that addresses the issues you found, also I'm changing the casing for vars from mixed to lowercase only. This seems to better follow the variable style from other functions.

Signed-off-by: Magnus Lindvall <magnus@dnmgns.com>

bkcsoft · 2017-06-15T00:11:27Z

Question: why did you change the casing? tmpPath => tmppath. The go way of naming things is tmpPath (and is used throughout the rest of the code-base)

Signed-off-by: Magnus Lindvall <magnus@dnmgns.com>

dnmgns · 2017-06-15T07:39:11Z

Thanks @bkcsoft - Temporary confusion I believe :)

bkcsoft

Apart from this question you have my LGTM 🙂

bkcsoft · 2017-06-19T07:05:22Z

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 = false


I don't really like negations in configs. Would not SSH_ENABLE_AUTHORIZED_KEYS_BACKUP = true make more sense?

There's some other negations in the config, that's simply why I choose that style. But I don't mind changing this to enable/true instead.

I prefer enable, reading negations is sometimes hard for non-native speakers 🙂

And actually that would just be SSH_BACKUP_AUTHORIZED_KEYS = true

Signed-off-by: Magnus Lindvall <magnus@dnmgns.com>

lafriks · 2017-06-19T13:49:31Z

models/ssh_key.go

 		os.Remove(tmpPath)
 	}()

+	if com.IsExist(fPath) && setting.SSH.AuthorizedKeysBackup {


It would be better: if setting.SSH.AuthorizedKeysBackup && com.IsExist(fPath) { to not check for directory existence if backup is disabled anyway, disk i/o operations are not cheap especially on rpi

Signed-off-by: Magnus Lindvall <magnus@dnmgns.com>

bkcsoft · 2017-06-26T22:08:27Z

~~LGTM~~ actually I found more things 😛

bkcsoft · 2017-06-26T22:12:46Z

models/ssh_key.go

-	f, err := os.OpenFile(tmpPath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0600)
+	fPath := filepath.Join(setting.SSH.RootPath, "authorized_keys")
+	tmpPath := fPath + ".tmp"
+	t, err := os.OpenFile(tmpPath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0600)


Do we still need a temp file? Since we're locking SSH access (line 324) we shouldn't need to do that :/

Well, it seems that the lock was introduced 3 years ago in ab747f2 and temp file has been there since then. This PR doesn't introduce any new tmp file, it just updates the variable name for the already existing one. While I may investigate if the tmp file is really needed (I also guess it isn't) and make the change for this in this PR, I believe it should be part of another PR intended to fix that specific issue. Your thoughts on this?

Temp file could actually be needed because generating new one is more time consuming than file move operation so that it would not disrupt ssh service

Yeah I agree that it should not be part of this one. Just noting that we're still trashing disk here :)

Ah cool, I've gotten so much great feedback here so I just thought that this was feedback as well :)

…d_key_backup

bkcsoft · 2017-06-27T18:49:33Z

LGTM

cez81 · 2017-06-27T21:11:11Z

LGTM

dnmgns added 2 commits June 2, 2017 10:59

Add setting to disable authorized_keys backup when rewriting public keys

bf51dd0

Signed-off-by: Magnus Lindvall <magnus@dnmgns.com>

Update default value to comply with documentation

78314cc

Signed-off-by: Magnus Lindvall <magnus@dnmgns.com>

dnmgns mentioned this pull request Jun 2, 2017

LDAP Public SSH Keys synchronization #1844

Merged

dnmgns changed the title ~~Setting to disable authorized key backup~~ Setting to disable authorized_keys backup Jun 2, 2017

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

lunny added this to the 1.3.0 milestone Jun 5, 2017

Use tmp-file instead of bak-file for saving manually added keys.

714e98b

Signed-off-by: Magnus Lindvall <magnus@dnmgns.com>

bkcsoft reviewed Jun 11, 2017

View reviewed changes

dnmgns added 3 commits June 13, 2017 09:52

Change casing

de19116

Signed-off-by: Magnus Lindvall <magnus@dnmgns.com>

Change casing and build bakpath with sprintf only

15b88b7

Signed-off-by: Magnus Lindvall <magnus@dnmgns.com>

Only close file once

cbf811a

Signed-off-by: Magnus Lindvall <magnus@dnmgns.com>

Do not modify calcFingerprint

80a106c

Signed-off-by: Magnus Lindvall <magnus@dnmgns.com>

Fix casing

078f2a6

Signed-off-by: Magnus Lindvall <magnus@dnmgns.com>

bkcsoft reviewed Jun 19, 2017

View reviewed changes

dnmgns added 2 commits June 19, 2017 11:47

Change style from disable to enable

56b37ad

Signed-off-by: Magnus Lindvall <magnus@dnmgns.com>

Change name, just SSH_BACKUP_AUTHORIZED_KEYS

4bd7c2b

Signed-off-by: Magnus Lindvall <magnus@dnmgns.com>

lafriks requested changes Jun 19, 2017

View reviewed changes

Do not check for directory existence if backup is disabled

8b1f5b6

Signed-off-by: Magnus Lindvall <magnus@dnmgns.com>

lafriks approved these changes Jun 20, 2017

View reviewed changes

tboerger 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 Jun 26, 2017

bkcsoft reviewed Jun 26, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into disable_authorize…

9ab07db

…d_key_backup

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 27, 2017

lunny modified the milestones: 1.2.0, 1.3.0 Jun 28, 2017

lunny merged commit 79daf31 into go-gitea:master Jun 28, 2017

lunny added the type/enhancement An improvement of existing functionality label Aug 25, 2017

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

dnmgns deleted the disable_authorized_key_backup branch January 20, 2023 11:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setting to disable authorized_keys backup #1856

Setting to disable authorized_keys backup #1856

dnmgns commented Jun 2, 2017

sapk commented Jun 4, 2017

dnmgns commented Jun 8, 2017

bkcsoft left a comment

bkcsoft Jun 11, 2017

bkcsoft Jun 11, 2017

dnmgns Jun 13, 2017 •

edited

Loading

dnmgns commented Jun 13, 2017

bkcsoft commented Jun 15, 2017

dnmgns commented Jun 15, 2017

bkcsoft left a comment

bkcsoft Jun 19, 2017

dnmgns Jun 19, 2017

bkcsoft Jun 19, 2017

bkcsoft Jun 19, 2017

lafriks Jun 19, 2017

bkcsoft commented Jun 26, 2017 •

edited

Loading

bkcsoft Jun 26, 2017

dnmgns Jun 27, 2017

lafriks Jun 27, 2017

bkcsoft Jun 27, 2017

dnmgns Jun 27, 2017

bkcsoft commented Jun 27, 2017

cez81 commented Jun 27, 2017

Setting to disable authorized_keys backup #1856

Setting to disable authorized_keys backup #1856

Conversation

dnmgns commented Jun 2, 2017

sapk commented Jun 4, 2017

dnmgns commented Jun 8, 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

dnmgns Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

dnmgns commented Jun 13, 2017

bkcsoft commented Jun 15, 2017

dnmgns commented Jun 15, 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 commented Jun 26, 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

bkcsoft commented Jun 27, 2017

cez81 commented Jun 27, 2017

dnmgns Jun 13, 2017 •

edited

Loading

bkcsoft commented Jun 26, 2017 •

edited

Loading