feat: ensure only expected ssh public keys are in authorized_keys file (#10010)

A security vulnerability that was fixed in #9840 had the potential to corrupt the `authorized_keys` file that Forgejo is managing to allow ssh access.  In the event that it was corrupted, the existing behaviour of Forgejo is to maintain the contents that it finds in the `authorized_keys` file, potentially making an exploit of a Forgejo server persistent despite attempts to rewrite the key file.

This feature adds a new layer of security resiliency in order to prevent persistent ssh key corruption.  When Forgejo starts up, if relevant, Forgejo will read the `authorized_keys` file and validate the file's contents.  If any keys are found in the file that are not expected, then Forgejo will terminate its startup in order to signal to the server administrator that a critical security risk is present that must be addressed:

```
2025/11/07 10:13:50 modules/ssh/init.go:86:Init() [F] An unexpected ssh public key was discovered. Forgejo will shutdown to require this to be fixed. Fix by either:
Option 1: Delete the file /home/forgejo/.ssh/authorized_keys, and Forgejo will recreate it with only expected ssh public keys.
Option 2: Permit unexpected keys by setting [server].SSH_ALLOW_UNEXPECTED_AUTHORIZED_KEYS=true in Forgejo's config file.
        Unexpected key on line 1 of /home/forgejo/.ssh/authorized_keys
        Unexpected key on line 2 of /home/forgejo/.ssh/authorized_keys
        Unexpected key on line 3 of /home/forgejo/.ssh/authorized_keys
        Unexpected key on line 4 of /home/forgejo/.ssh/authorized_keys
        Unexpected key on line 5 of /home/forgejo/.ssh/authorized_keys
```

As noted in the log message, the server administrator can address this problem in one of two ways:
- If they delete the file that contains the unexpected keys, Forgejo will regenerate it containing only the expected keys from the Forgejo database.
- If they would like to run their server with ssh keys that are not managed by Forgejo (for example, if they're reusing a `git` ssh user that is accessed through `git@server` and does not invoke Forgejo's ssh handlers), then they can disable the new security check by setting `[server].SSH_ALLOW_UNEXPECTED_AUTHORIZED_KEYS = true` in their `app.ini`.

**This is a breaking change**: the default behaviour is to be restrictive in the contents of `authorized_keys` in order to ensure that server administrators with unexpected keys in `authorized_keys` are aware of those keys.

If `SSH_ALLOW_UNEXPECTED_AUTHORIZED_KEYS=false`, then the behaviour when Forgejo rewrites the `authorized_keys` file is changed to not maintain any unexpected keys in the file.  If the value is `true`, then the old behaviour is retained.

The `doctor check` subcommand is updated to use the new validity routines:
```
[4] Check if OpenSSH authorized_keys file is up-to-date
 - [E] Unexpected key on line 1 of /home/forgejo/.ssh/authorized_keys
 - [E] Key in database is not present in /home/forgejo/.ssh/authorized_keys: ...
 - [E] authorized_keys file "/home/forgejo/.ssh/authorized_keys" contains validity errors.
Regenerate it with:
        "forgejo admin regenerate keys"
or
        "forgejo doctor check --run authorized-keys --fix"
ERROR
```

## Checklist

The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).

### Tests

- I added test coverage for Go changes...
  - [x] in their respective `*_test.go` for unit tests.
  - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I added test coverage for JavaScript changes...
  - [ ] in `web_src/js/*.test.js` if it can be unit tested.
  - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)).

### Documentation

- [x] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
    - **Documentation updates required**; pending initial reviews of this change.
- [ ] I did not document these changes and I do not expect someone else to do it.

### Release notes

- [ ] I do not want this change to show in the release notes.
- [ ] I want the title to show in the release notes with a link to this pull request.
- [x] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10010
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: mfenniak <mfenniak@noreply.codeberg.org>
Co-committed-by: mfenniak <mfenniak@noreply.codeberg.org>
This commit is contained in:
mfenniak 2025-11-09 01:06:04 +01:00 committed by Mathieu Fenniak
parent a68791d707
commit 99dd35d3e4
7 changed files with 315 additions and 84 deletions

View file

@ -6,6 +6,7 @@ package asymkey
import (
"bufio"
"context"
"errors"
"fmt"
"io"
"os"
@ -15,6 +16,7 @@ import (
"time"
"forgejo.org/models/db"
"forgejo.org/modules/container"
"forgejo.org/modules/log"
"forgejo.org/modules/setting"
"forgejo.org/modules/util"
@ -111,6 +113,93 @@ func appendAuthorizedKeysToFile(keys ...*PublicKey) error {
return nil
}
type InspectionFinding struct {
Type InspectionFindingType
Comment string
}
type InspectionFindingType int
const (
InspectionResultFileMissing InspectionFindingType = iota // authorized_keys is absent, must regenerate
InspectionResultUnexpectedKey // authorized_keys contains at least one unexpected key
InspectionResultMissingExpectedKey // authorized_keys does not contain a key that is in the DB
)
func InspectPublicKeys(ctx context.Context) ([]InspectionFinding, error) {
if setting.SSH.StartBuiltinServer || !setting.SSH.CreateAuthorizedKeysFile {
return []InspectionFinding{}, nil
}
sshOpLocker.Lock()
defer sshOpLocker.Unlock()
path := filepath.Join(setting.SSH.RootPath, "authorized_keys")
file, err := os.OpenFile(path, os.O_RDONLY, 0o600)
if errors.Is(err, os.ErrNotExist) {
return []InspectionFinding{{Type: InspectionResultFileMissing}}, nil
} else if err != nil {
return nil, err
}
defer file.Close()
// Create a set of all the expected output in the `authorized_keys` file.
expectedKeys := make(container.Set[string])
if err := db.GetEngine(ctx).Where("type != ?", KeyTypePrincipal).Iterate(new(PublicKey), func(idx int, bean any) (err error) {
keyWithComment := (bean.(*PublicKey)).AuthorizedString()
if !strings.HasPrefix(keyWithComment, tplCommentPrefix) {
return fmt.Errorf("unexpected AuthorizedString")
}
key := strings.TrimSpace(keyWithComment[len(tplCommentPrefix):])
expectedKeys.Add(key)
return nil
}); err != nil {
return nil, err
}
findings := []InspectionFinding{}
scanner := bufio.NewScanner(file)
lineNumber := 0
for scanner.Scan() {
lineNumber++
line := scanner.Text()
if strings.HasPrefix(line, tplCommentPrefix) {
// After the public key comment, we expect a public key
lineNumber++
if !scanner.Scan() {
return nil, fmt.Errorf("authorized_keys file %s had a prefix comment but no key on line %d", path, lineNumber)
}
key := strings.TrimSpace(scanner.Text())
if !expectedKeys.Remove(key) {
findings = append(findings, InspectionFinding{
Type: InspectionResultUnexpectedKey,
Comment: fmt.Sprintf("Key on line %d of %s does not exist in database", lineNumber, path),
})
}
} else {
findings = append(findings, InspectionFinding{
Type: InspectionResultUnexpectedKey,
Comment: fmt.Sprintf("Unexpected key on line %d of %s", lineNumber, path),
})
}
}
if err = scanner.Err(); err != nil {
return nil, err
}
for key := range expectedKeys {
findings = append(findings, InspectionFinding{
Type: InspectionResultMissingExpectedKey,
Comment: fmt.Sprintf("Key in database is not present in %s: %s", path, key),
})
}
return findings, nil
}
// RewriteAllPublicKeys removes any authorized key and rewrite all keys from database again.
// Note: db.GetEngine(ctx).Iterate does not get latest data after insert/delete, so we have to call this function
// outside any session scope independently.
@ -162,7 +251,7 @@ func RewriteAllPublicKeys(ctx context.Context) error {
}
}
if err := RegeneratePublicKeys(ctx, t); err != nil {
if err := regeneratePublicKeys(ctx, t); err != nil {
return err
}
@ -175,8 +264,8 @@ func RewriteAllPublicKeys(ctx context.Context) error {
return util.Rename(tmpPath, fPath)
}
// RegeneratePublicKeys regenerates the authorized_keys file
func RegeneratePublicKeys(ctx context.Context, t io.StringWriter) error {
// regeneratePublicKeys regenerates the authorized_keys file
func regeneratePublicKeys(ctx context.Context, t io.StringWriter) error {
if err := db.GetEngine(ctx).Where("type != ?", KeyTypePrincipal).Iterate(new(PublicKey), func(idx int, bean any) (err error) {
_, err = t.WriteString((bean.(*PublicKey)).AuthorizedString())
return err
@ -184,34 +273,38 @@ func RegeneratePublicKeys(ctx context.Context, t io.StringWriter) error {
return err
}
fPath := filepath.Join(setting.SSH.RootPath, "authorized_keys")
isExist, err := util.IsExist(fPath)
if err != nil {
log.Error("Unable to check if %s exists. Error: %v", fPath, err)
return err
}
if isExist {
f, err := os.Open(fPath)
// Preserve any authorized_keys contents that are not generated from Forgejo
if setting.SSH.AllowUnexpectedAuthorizedKeys {
fPath := filepath.Join(setting.SSH.RootPath, "authorized_keys")
isExist, err := util.IsExist(fPath)
if err != nil {
log.Error("Unable to check if %s exists. Error: %v", fPath, err)
return err
}
defer f.Close()
scanner := bufio.NewScanner(f)
for scanner.Scan() {
line := scanner.Text()
if strings.HasPrefix(line, tplCommentPrefix) {
scanner.Scan()
continue
}
_, err = t.WriteString(line + "\n")
if isExist {
f, err := os.Open(fPath)
if err != nil {
return err
}
}
if err = scanner.Err(); err != nil {
return fmt.Errorf("RegeneratePublicKeys scan: %w", err)
defer f.Close()
scanner := bufio.NewScanner(f)
for scanner.Scan() {
line := scanner.Text()
if strings.HasPrefix(line, tplCommentPrefix) {
scanner.Scan()
continue
}
_, err = t.WriteString(line + "\n")
if err != nil {
return err
}
}
if err = scanner.Err(); err != nil {
return fmt.Errorf("regeneratePublicKeys scan: %w", err)
}
}
}
return nil
}

View file

@ -0,0 +1,130 @@
// Copyright 2025 The Forgejo Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package asymkey
import (
"fmt"
"os"
"path/filepath"
"strings"
"testing"
"forgejo.org/models/db"
"forgejo.org/modules/setting"
"forgejo.org/modules/test"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func TestInspectPublicKeys(t *testing.T) {
defer test.MockVariableValue(&setting.SSH.RootPath, t.TempDir())()
authorizedKeysPath := filepath.Join(setting.SSH.RootPath, "authorized_keys")
t.Run("Missing", func(t *testing.T) {
findings, err := InspectPublicKeys(t.Context())
require.NoError(t, err)
require.Len(t, findings, 1)
f := findings[0]
assert.Equal(t, InspectionResultFileMissing, f.Type)
})
t.Run("Generated cleanly", func(t *testing.T) {
err := RewriteAllPublicKeys(t.Context())
require.NoError(t, err)
findings, err := InspectPublicKeys(t.Context())
require.NoError(t, err)
assert.Empty(t, findings)
})
t.Run("Extra unexpected key", func(t *testing.T) {
err := RewriteAllPublicKeys(t.Context())
require.NoError(t, err)
file, err := os.OpenFile(authorizedKeysPath, os.O_WRONLY|os.O_APPEND, 0o600)
require.NoError(t, err)
_, err = file.WriteString("ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIHnbqnHNKh/Td/1O6t9Q8qdJmitCAApnvHImHV8TkptX hacker@example.com\n")
require.NoError(t, err)
err = file.Close()
require.NoError(t, err)
findings, err := InspectPublicKeys(t.Context())
require.NoError(t, err)
require.Len(t, findings, 1)
f := findings[0]
assert.Equal(t, InspectionResultUnexpectedKey, f.Type)
assert.Contains(t, f.Comment, "Unexpected key on line")
})
t.Run("Missing expected key", func(t *testing.T) {
file, err := os.OpenFile(authorizedKeysPath, os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0o600)
require.NoError(t, err)
err = db.GetEngine(t.Context()).Where("type != ?", KeyTypePrincipal).Iterate(new(PublicKey), func(idx int, bean any) (err error) {
if idx == 0 {
// Skip one key
return nil
}
_, err = file.WriteString((bean.(*PublicKey)).AuthorizedString())
return err
})
require.NoError(t, err)
err = file.Close()
require.NoError(t, err)
findings, err := InspectPublicKeys(t.Context())
require.NoError(t, err)
require.Len(t, findings, 1)
f := findings[0]
assert.Equal(t, InspectionResultMissingExpectedKey, f.Type)
assert.Contains(t, f.Comment, "Key in database is not present in")
})
}
func TestRewriteAllPublicKeys(t *testing.T) {
defer test.MockVariableValue(&setting.SSH.RootPath, t.TempDir())()
authorizedKeysPath := filepath.Join(setting.SSH.RootPath, "authorized_keys")
t.Run("Generated cleanly", func(t *testing.T) {
err := RewriteAllPublicKeys(t.Context())
require.NoError(t, err)
count, err := db.GetEngine(t.Context()).Where("type != ?", KeyTypePrincipal).Count(&PublicKey{})
require.NoError(t, err)
content, err := os.ReadFile(authorizedKeysPath)
require.NoError(t, err)
stringContent := string(content)
lines := strings.Split(stringContent, "\n")
assert.Len(t, lines, int((count*2)+1)) // one comment + one key for each key (*2), plus a newline at the end (+1)
})
for _, allowUnexpectedAuthorizedKeys := range []bool{true, false} {
t.Run(fmt.Sprintf("AllowUnexpectedAuthorizedKeys=%v", allowUnexpectedAuthorizedKeys), func(t *testing.T) {
defer test.MockVariableValue(&setting.SSH.AllowUnexpectedAuthorizedKeys, allowUnexpectedAuthorizedKeys)()
err := RewriteAllPublicKeys(t.Context())
require.NoError(t, err)
file, err := os.OpenFile(authorizedKeysPath, os.O_WRONLY|os.O_APPEND, 0o600)
require.NoError(t, err)
_, err = file.WriteString("ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIHnbqnHNKh/Td/1O6t9Q8qdJmitCAApnvHImHV8TkptX hacker@example.com\n")
require.NoError(t, err)
err = file.Close()
require.NoError(t, err)
err = RewriteAllPublicKeys(t.Context())
require.NoError(t, err)
content, err := os.ReadFile(authorizedKeysPath)
require.NoError(t, err)
stringContent := string(content)
if allowUnexpectedAuthorizedKeys {
assert.Contains(t, stringContent, "hacker@example.com")
} else {
assert.NotContains(t, stringContent, "hacker@example.com")
}
})
}
}

View file

@ -41,6 +41,7 @@ var SSH = struct {
MinimumKeySizeCheck bool `ini:"-"`
MinimumKeySizes map[string]int `ini:"-"`
CreateAuthorizedKeysFile bool `ini:"SSH_CREATE_AUTHORIZED_KEYS_FILE"`
AllowUnexpectedAuthorizedKeys bool `ini:"SSH_ALLOW_UNEXPECTED_AUTHORIZED_KEYS"`
CreateAuthorizedPrincipalsFile bool `ini:"SSH_CREATE_AUTHORIZED_PRINCIPALS_FILE"`
ExposeAnonymous bool `ini:"SSH_EXPOSE_ANONYMOUS"`
AuthorizedPrincipalsAllow []string `ini:"SSH_AUTHORIZED_PRINCIPALS_ALLOW"`

View file

@ -4,6 +4,7 @@
package ssh
import (
"context"
"fmt"
"net"
"os"
@ -11,13 +12,14 @@ import (
"strconv"
"strings"
"forgejo.org/models/asymkey"
"forgejo.org/modules/log"
"forgejo.org/modules/setting"
)
var logger = log.GetManager().GetLogger("ssh")
func Init() error {
func Init(ctx context.Context) error {
if setting.SSH.Disabled {
builtinUnused()
return nil
@ -53,5 +55,40 @@ func Init() error {
}
}
if !setting.SSH.AllowUnexpectedAuthorizedKeys {
findings, err := asymkey.InspectPublicKeys(ctx)
if err != nil {
return fmt.Errorf("inspect authorized_keys failed: %w", err)
}
unexpectedKeys := []string{}
for _, finding := range findings {
switch finding.Type {
case asymkey.InspectionResultFileMissing:
err := asymkey.RewriteAllPublicKeys(ctx)
if err != nil {
return fmt.Errorf("rewrite authorized_keys failed: %w", err)
}
case asymkey.InspectionResultUnexpectedKey:
unexpectedKeys = append(unexpectedKeys, finding.Comment)
case asymkey.InspectionResultMissingExpectedKey:
// MissingExpectingKey can happen at the same time as UnexpectedKey -- so while it might seem to make
// sense to regenerate the key file automatically in this case, it could overwrite keys that someone
// wants present there when they want SSH_ALLOW_UNEXPECTED_AUTHORIZED_KEYS=true but haven't set it yet.
// So, just warn as this isn't an insecure situation.
log.Warn("authorized_keys is missing a key from the database; regenerate authorized_keys from the admin panel to repair this")
}
}
if len(unexpectedKeys) > 0 {
detailConcat := strings.Join(unexpectedKeys, "\n\t")
log.Fatal("An unexpected ssh public key was discovered. Forgejo will shutdown to require this to be fixed. Fix by either:\n"+
"Option 1: Delete the file %s, and Forgejo will recreate it with only expected ssh public keys.\n"+
"Option 2: Permit unexpected keys by setting [server].SSH_ALLOW_UNEXPECTED_AUTHORIZED_KEYS=true in Forgejo's config file.\n\t"+
detailConcat, filepath.Join(setting.SSH.RootPath, "authorized_keys"))
}
}
return nil
}

1
release-notes/10010.md Normal file
View file

@ -0,0 +1 @@
If SSH is enabled and an `authorized_keys` file is managed by Forgejo, when Forgejo starts up it will read the SSH authorized_keys file and validate the file's contents. If any keys are found in the file that are not expected, then Forgejo will terminate its startup in order to signal to the server administrator that a security risk is present that must be addressed. The server administrator can address this problem either by deleting the `authorized_keys` file, which Forgejo will regenerate with valid keys; or by disabling the new check by setting `[server].SSH_ALLOW_UNEXPECTED_AUTHORIZED_KEYS = true` in their `app.ini` file.

View file

@ -163,7 +163,7 @@ func InitWebInstalled(ctx context.Context) {
mustInitCtx(ctx, syncAppConfForGit)
mustInit(ssh.Init)
mustInitCtx(ctx, ssh.Init)
auth.Init()
mustInit(svg.Init)

View file

@ -1,92 +1,61 @@
// Copyright 2020 The Gitea Authors. All rights reserved.
// Copyright 2025 The Forgejo Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package doctor
import (
"bufio"
"bytes"
"context"
"errors"
"fmt"
"os"
"path/filepath"
"strings"
asymkey_model "forgejo.org/models/asymkey"
"forgejo.org/modules/container"
"forgejo.org/modules/log"
"forgejo.org/modules/setting"
)
const tplCommentPrefix = `# gitea public key`
func checkAuthorizedKeys(ctx context.Context, logger log.Logger, autofix bool) error {
if setting.SSH.StartBuiltinServer || !setting.SSH.CreateAuthorizedKeysFile {
return nil
}
fPath := filepath.Join(setting.SSH.RootPath, "authorized_keys")
f, err := os.Open(fPath)
findings, err := asymkey_model.InspectPublicKeys(ctx)
if err != nil {
return fmt.Errorf("inspect authorized_keys failed: %w", err)
}
if !autofix {
for _, finding := range findings {
switch finding.Type {
case asymkey_model.InspectionResultFileMissing:
logger.Critical("authorized_keys file is missing")
case asymkey_model.InspectionResultUnexpectedKey:
if !setting.SSH.AllowUnexpectedAuthorizedKeys {
logger.Critical(finding.Comment)
}
case asymkey_model.InspectionResultMissingExpectedKey:
logger.Critical(finding.Comment)
}
}
}
if len(findings) > 0 {
if !autofix {
logger.Critical("Unable to open authorized_keys file. ERROR: %v", err)
return fmt.Errorf("Unable to open authorized_keys file. ERROR: %w", err)
}
logger.Warn("Unable to open authorized_keys. (ERROR: %v). Attempting to rewrite...", err)
if err = asymkey_model.RewriteAllPublicKeys(ctx); err != nil {
logger.Critical("Unable to rewrite authorized_keys file. ERROR: %v", err)
return fmt.Errorf("Unable to rewrite authorized_keys file. ERROR: %w", err)
}
}
defer f.Close()
linesInAuthorizedKeys := make(container.Set[string])
scanner := bufio.NewScanner(f)
for scanner.Scan() {
line := scanner.Text()
if strings.HasPrefix(line, tplCommentPrefix) {
continue
}
linesInAuthorizedKeys.Add(line)
}
if err = scanner.Err(); err != nil {
return fmt.Errorf("scan: %w", err)
}
// although there is a "defer close" above, here close explicitly before the generating, because it needs to open the file for writing again
_ = f.Close()
// now we regenerate and check if there are any lines missing
regenerated := &bytes.Buffer{}
if err := asymkey_model.RegeneratePublicKeys(ctx, regenerated); err != nil {
logger.Critical("Unable to regenerate authorized_keys file. ERROR: %v", err)
return fmt.Errorf("Unable to regenerate authorized_keys file. ERROR: %w", err)
}
scanner = bufio.NewScanner(regenerated)
for scanner.Scan() {
line := scanner.Text()
if strings.HasPrefix(line, tplCommentPrefix) {
continue
}
if linesInAuthorizedKeys.Contains(line) {
continue
}
if !autofix {
fPath := filepath.Join(setting.SSH.RootPath, "authorized_keys")
logger.Critical(
"authorized_keys file %q is out of date.\nRegenerate it with:\n\t\"%s\"\nor\n\t\"%s\"",
"authorized_keys file %q contains validity errors.\nRegenerate it with:\n\t\"%s\"\nor\n\t\"%s\"",
fPath,
"forgejo admin regenerate keys",
"forgejo doctor check --run authorized-keys --fix")
return errors.New(`authorized_keys is out of date and should be regenerated with "forgejo admin regenerate keys" or "forgejo doctor check --run authorized-keys --fix"`)
return errors.New("errors discovered from InspectPublicKeys")
}
logger.Warn("authorized_keys is out of date. Attempting rewrite...")
err = asymkey_model.RewriteAllPublicKeys(ctx)
err := asymkey_model.RewriteAllPublicKeys(ctx)
if err != nil {
logger.Critical("Unable to rewrite authorized_keys file. ERROR: %v", err)
return fmt.Errorf("Unable to rewrite authorized_keys file. ERROR: %w", err)
return fmt.Errorf("rewrite authorized_keys failed: %w", err)
}
}
return nil
}