-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Setting to disable authorized_keys backup #1856
Changes from 3 commits
bf51dd0
78314cc
714e98b
de19116
15b88b7
cbf811a
80a106c
078f2a6
56b37ad
4bd7c2b
8b1f5b6
9ab07db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -557,53 +557,54 @@ func RewriteAllPublicKeys() error { | |
sshOpLocker.Lock() | ||
defer sshOpLocker.Unlock() | ||
|
||
fpath := filepath.Join(setting.SSH.RootPath, "authorized_keys") | ||
tmpPath := fpath + ".tmp" | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we still need a temp file? Since we're locking SSH access (line 324) we shouldn't need to do that :/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I agree that it should not be part of this one. Just noting that we're still trashing disk here :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah cool, I've gotten so much great feedback here so I just thought that this was feedback as well :) |
||
if err != nil { | ||
return err | ||
} | ||
defer func() { | ||
f.Close() | ||
t.Close() | ||
os.Remove(tmpPath) | ||
}() | ||
|
||
if com.IsExist(fPath) && !setting.SSH.DisableAuthorizedKeysBackup { | ||
bakPath := fPath + fmt.Sprintf("_%d.gitea_bak", time.Now().Unix()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While you're moving this line, mind changing it to bakPath := fmt.Sprintf("%s_%d.gitea_bak", fPath, time.Now().Unix()) |
||
if err = com.Copy(fPath, bakPath); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
err = x.Iterate(new(PublicKey), func(idx int, bean interface{}) (err error) { | ||
_, err = f.WriteString((bean.(*PublicKey)).AuthorizedString()) | ||
_, err = t.WriteString((bean.(*PublicKey)).AuthorizedString()) | ||
return err | ||
}) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if com.IsExist(fpath) { | ||
bakPath := fpath + fmt.Sprintf("_%d.gitea_bak", time.Now().Unix()) | ||
if err = com.Copy(fpath, bakPath); err != nil { | ||
return err | ||
} | ||
|
||
p, err := os.Open(bakPath) | ||
if com.IsExist(fPath) { | ||
f, err := os.Open(fPath) | ||
if err != nil { | ||
return err | ||
} | ||
defer p.Close() | ||
|
||
scanner := bufio.NewScanner(p) | ||
scanner := bufio.NewScanner(f) | ||
for scanner.Scan() { | ||
line := scanner.Text() | ||
if strings.HasPrefix(line, tplCommentPrefix) { | ||
scanner.Scan() | ||
continue | ||
} | ||
_, err = f.WriteString(line + "\n") | ||
_, err = t.WriteString(line + "\n") | ||
if err != nil { | ||
return err | ||
} | ||
} | ||
defer f.Close() | ||
} | ||
|
||
f.Close() | ||
if err = os.Rename(tmpPath, fpath); err != nil { | ||
t.Close() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're closing this twice There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
if err = os.Rename(tmpPath, fPath); err != nil { | ||
return err | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like negations in configs. Would not
SSH_ENABLE_AUTHORIZED_KEYS_BACKUP = true
make more sense?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer enable, reading negations is sometimes hard for non-native speakers 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And actually that would just be
SSH_BACKUP_AUTHORIZED_KEYS = true