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.

Sign up for GitHub

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

Jump to bottom

Setting to disable authorized_keys backup #1856

Merged
merged 12 commits into from
Jun 28, 2017
Merged

Setting to disable authorized_keys backup #1856

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
bf51dd0
Add setting to disable authorized_keys backup when rewriting public keys
dnmgns May 31, 2017
78314cc
Update default value to comply with documentation
dnmgns Jun 2, 2017
714e98b
Use tmp-file instead of bak-file for saving manually added keys.
dnmgns Jun 8, 2017
de19116
Change casing
dnmgns Jun 13, 2017
15b88b7
Change casing and build bakpath with sprintf only
dnmgns Jun 13, 2017
cbf811a
Only close file once
dnmgns Jun 13, 2017
80a106c
Do not modify calcFingerprint
dnmgns Jun 13, 2017
078f2a6
Fix casing
dnmgns Jun 15, 2017
56b37ad
Change style from disable to enable
dnmgns Jun 19, 2017
4bd7c2b
Change name, just SSH_BACKUP_AUTHORIZED_KEYS
dnmgns Jun 19, 2017
8b1f5b6
Do not check for directory existence if backup is disabled
dnmgns Jun 20, 2017
9ab07db
Merge remote-tracking branch 'upstream/master' into disable_authorize…
dnmgns Jun 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions conf/app.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
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 really like negations in configs. Would not SSH_ENABLE_AUTHORIZED_KEYS_BACKUP = true make more sense?

Copy link
Contributor Author

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.

Copy link
Member

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 🙂

Copy link
Member

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

; Indicate whether to check minimum key size with corresponding type
MINIMUM_KEY_SIZE_CHECK = false
; Disable CDN even in "prod" mode
Expand Down
37 changes: 19 additions & 18 deletions models/ssh_key.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The 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 :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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 :)

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

Choose a reason for hiding this comment

The 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()
Copy link
Member

Choose a reason for hiding this comment

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

You're closing this twice

Copy link
Contributor Author

@dnmgns dnmgns Jun 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Expand Down
24 changes: 13 additions & 11 deletions modules/setting/setting.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,17 +87,18 @@ var (
EnablePprof bool

SSH = struct {
Disabled bool `ini:"DISABLE_SSH"`
StartBuiltinServer bool `ini:"START_SSH_SERVER"`
Domain string `ini:"SSH_DOMAIN"`
Port int `ini:"SSH_PORT"`
ListenHost string `ini:"SSH_LISTEN_HOST"`
ListenPort int `ini:"SSH_LISTEN_PORT"`
RootPath string `ini:"SSH_ROOT_PATH"`
KeyTestPath string `ini:"SSH_KEY_TEST_PATH"`
KeygenPath string `ini:"SSH_KEYGEN_PATH"`
MinimumKeySizeCheck bool `ini:"-"`
MinimumKeySizes map[string]int `ini:"-"`
Disabled bool `ini:"DISABLE_SSH"`
StartBuiltinServer bool `ini:"START_SSH_SERVER"`
Domain string `ini:"SSH_DOMAIN"`
Port int `ini:"SSH_PORT"`
ListenHost string `ini:"SSH_LISTEN_HOST"`
ListenPort int `ini:"SSH_LISTEN_PORT"`
RootPath string `ini:"SSH_ROOT_PATH"`
KeyTestPath string `ini:"SSH_KEY_TEST_PATH"`
KeygenPath string `ini:"SSH_KEYGEN_PATH"`
DisableAuthorizedKeysBackup bool `ini:"SSH_DISABLE_AUTHORIZED_KEYS_BACKUP"`
MinimumKeySizeCheck bool `ini:"-"`
MinimumKeySizes map[string]int `ini:"-"`
}{
Disabled: false,
StartBuiltinServer: false,
Expand Down Expand Up @@ -693,6 +694,7 @@ please consider changing to GITEA_CUSTOM`)
SSH.MinimumKeySizes[strings.ToLower(key.Name())] = key.MustInt()
}
}
SSH.DisableAuthorizedKeysBackup = sec.Key("SSH_DISABLE_AUTHORIZED_KEYS_BACKUP").MustBool(false)

if err = Cfg.Section("server").MapTo(&LFS); err != nil {
log.Fatal(4, "Failed to map LFS settings: %v", err)
Expand Down