diff --git a/models/asymkey/ssh_key_authorized_keys.go b/models/asymkey/ssh_key_authorized_keys.go index d3bf6fe886..11905071af 100644 --- a/models/asymkey/ssh_key_authorized_keys.go +++ b/models/asymkey/ssh_key_authorized_keys.go @@ -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 } diff --git a/models/asymkey/ssh_key_authorized_keys_test.go b/models/asymkey/ssh_key_authorized_keys_test.go new file mode 100644 index 0000000000..732419dfa5 --- /dev/null +++ b/models/asymkey/ssh_key_authorized_keys_test.go @@ -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") + } + }) + } +} diff --git a/modules/setting/ssh.go b/modules/setting/ssh.go index 1ad826a17a..a1dd1d968e 100644 --- a/modules/setting/ssh.go +++ b/modules/setting/ssh.go @@ -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"` diff --git a/modules/ssh/init.go b/modules/ssh/init.go index 09b96a7d8a..432cda0c13 100644 --- a/modules/ssh/init.go +++ b/modules/ssh/init.go @@ -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 } diff --git a/release-notes/10010.md b/release-notes/10010.md new file mode 100644 index 0000000000..96cdbe0722 --- /dev/null +++ b/release-notes/10010.md @@ -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. diff --git a/routers/init.go b/routers/init.go index 318fb170a3..59358c731d 100644 --- a/routers/init.go +++ b/routers/init.go @@ -163,7 +163,7 @@ func InitWebInstalled(ctx context.Context) { mustInitCtx(ctx, syncAppConfForGit) - mustInit(ssh.Init) + mustInitCtx(ctx, ssh.Init) auth.Init() mustInit(svg.Init) diff --git a/services/doctor/authorizedkeys.go b/services/doctor/authorizedkeys.go index 465a3fc7c0..52e5ea1d3b 100644 --- a/services/doctor/authorizedkeys.go +++ b/services/doctor/authorizedkeys.go @@ -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 }