From da57fc251749b0b8cece44bc29300d2d69d7ae6a Mon Sep 17 00:00:00 2001 From: Peter Verraedt Date: Fri, 19 Dec 2025 12:40:11 +0100 Subject: [PATCH 1/3] Fix regression in writing authorized principals Commit ac6d38e4b7a0a565077229984a3720b2b6a0688b introduced a regression in writing of the authorized_principals file, resulting in an empty file. The function `regeneratePrincipalsKeys` in `services/asymkey/ssh_key_authorized_principals.go` calls the function `WriteAuthorizedStringForValidKey` for a PublicKey of type KeyTypePrincipal, and ssh.ParseAuthorizedKey would always fail. This commit adds additional logic to this function to restore the previous behaviour when writing the principals file. Fixes: 36212 Signed-off-by: Peter Verraedt --- models/asymkey/ssh_key_authorized_keys.go | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/models/asymkey/ssh_key_authorized_keys.go b/models/asymkey/ssh_key_authorized_keys.go index db4730f00a152..f64d8273af66f 100644 --- a/models/asymkey/ssh_key_authorized_keys.go +++ b/models/asymkey/ssh_key_authorized_keys.go @@ -51,11 +51,22 @@ func WriteAuthorizedStringForValidKey(key *PublicKey, w io.Writer) error { } func writeAuthorizedStringForKey(key *PublicKey, w io.Writer) (keyValid bool, err error) { - const tpl = AuthorizedStringCommentPrefix + "\n" + `command=%s,no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,no-user-rc,restrict %s %s` + "\n" - pubKey, _, _, _, err := ssh.ParseAuthorizedKey([]byte(key.Content)) - if err != nil { - return false, err + const tpl = AuthorizedStringCommentPrefix + "\n" + `command=%s,no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,no-user-rc,restrict %s` + "\n" + + var sshKey string + + if key.Type == KeyTypePrincipal { + sshKey = fmt.Sprintf("%s # user-%d", key.Content, key.OwnerID) + } else { + pubKey, _, _, _, err := ssh.ParseAuthorizedKey([]byte(key.Content)) + if err != nil { + return false, err + } + + sshKeyMarshalled := strings.TrimSpace(string(ssh.MarshalAuthorizedKey(pubKey))) + sshKey = fmt.Sprintf("%s user-%d", sshKeyMarshalled, key.OwnerID) } + // now the key is valid, the code below could only return template/IO related errors sbCmd := &strings.Builder{} err = setting.SSH.AuthorizedKeysCommandTemplateTemplate.Execute(sbCmd, map[string]any{ @@ -69,9 +80,7 @@ func writeAuthorizedStringForKey(key *PublicKey, w io.Writer) (keyValid bool, er return true, err } sshCommandEscaped := util.ShellEscape(sbCmd.String()) - sshKeyMarshalled := strings.TrimSpace(string(ssh.MarshalAuthorizedKey(pubKey))) - sshKeyComment := fmt.Sprintf("user-%d", key.OwnerID) - _, err = fmt.Fprintf(w, tpl, sshCommandEscaped, sshKeyMarshalled, sshKeyComment) + _, err = fmt.Fprintf(w, tpl, sshCommandEscaped, sshKey) return true, err } From 00fe4b35cde23c2d2457fb4061010951f70f16d6 Mon Sep 17 00:00:00 2001 From: Peter Verraedt Date: Fri, 19 Dec 2025 14:18:36 +0100 Subject: [PATCH 2/3] Add principals regular expression Signed-off-by: Peter Verraedt --- models/asymkey/ssh_key_authorized_keys.go | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/models/asymkey/ssh_key_authorized_keys.go b/models/asymkey/ssh_key_authorized_keys.go index f64d8273af66f..3575cebab2fbf 100644 --- a/models/asymkey/ssh_key_authorized_keys.go +++ b/models/asymkey/ssh_key_authorized_keys.go @@ -10,6 +10,7 @@ import ( "io" "os" "path/filepath" + "regexp" "strings" "sync" @@ -50,13 +51,28 @@ func WriteAuthorizedStringForValidKey(key *PublicKey, w io.Writer) error { return err } +// principalRegexp expresses whether a principal is considered valid. +// This reverse engineers how sshd parses the authorized keys file, +// see e.g. https://github.com/openssh/openssh-portable/blob/32deb00b38b4ee2b3302f261ea1e68c04e020a08/auth2-pubkeyfile.c#L221-L256 +// Any newline or # comment will be stripped when parsing, so don't allow +// those. Also, if any space or tab is present in the principal, the part +// proceeding this would be parsed as an option, so just avoid any whitespace +// altogether. +var principalRegexp = regexp.MustCompile(`^[^\s#]+$`) + func writeAuthorizedStringForKey(key *PublicKey, w io.Writer) (keyValid bool, err error) { const tpl = AuthorizedStringCommentPrefix + "\n" + `command=%s,no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,no-user-rc,restrict %s` + "\n" var sshKey string if key.Type == KeyTypePrincipal { - sshKey = fmt.Sprintf("%s # user-%d", key.Content, key.OwnerID) + principal := strings.TrimSpace(key.Content) + + if matched := principalRegexp.MatchString(principal); !matched { + return false, fmt.Errorf("does not match %s", principalRegexp.String()) + } + + sshKey = fmt.Sprintf("%s # user-%d", principal, key.OwnerID) } else { pubKey, _, _, _, err := ssh.ParseAuthorizedKey([]byte(key.Content)) if err != nil { From 5d26d16b491a4fb39af81a7bae695c6252760c71 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 19 Dec 2025 21:33:36 +0800 Subject: [PATCH 3/3] add test --- models/asymkey/ssh_key_authorized_keys.go | 32 ++++--- .../asymkey/ssh_key_authorized_keys_test.go | 90 +++++++++++++++++++ 2 files changed, 108 insertions(+), 14 deletions(-) create mode 100644 models/asymkey/ssh_key_authorized_keys_test.go diff --git a/models/asymkey/ssh_key_authorized_keys.go b/models/asymkey/ssh_key_authorized_keys.go index 3575cebab2fbf..cb2943e31dfd8 100644 --- a/models/asymkey/ssh_key_authorized_keys.go +++ b/models/asymkey/ssh_key_authorized_keys.go @@ -51,14 +51,20 @@ func WriteAuthorizedStringForValidKey(key *PublicKey, w io.Writer) error { return err } -// principalRegexp expresses whether a principal is considered valid. -// This reverse engineers how sshd parses the authorized keys file, -// see e.g. https://github.com/openssh/openssh-portable/blob/32deb00b38b4ee2b3302f261ea1e68c04e020a08/auth2-pubkeyfile.c#L221-L256 -// Any newline or # comment will be stripped when parsing, so don't allow -// those. Also, if any space or tab is present in the principal, the part -// proceeding this would be parsed as an option, so just avoid any whitespace -// altogether. -var principalRegexp = regexp.MustCompile(`^[^\s#]+$`) +var globalVars = sync.OnceValue(func() (ret struct { + principalRegexp *regexp.Regexp +}, +) { + // principalRegexp expresses whether a principal is considered valid. + // This reverse engineers how sshd parses the authorized keys file, + // see e.g. https://github.com/openssh/openssh-portable/blob/32deb00b38b4ee2b3302f261ea1e68c04e020a08/auth2-pubkeyfile.c#L221-L256 + // Any newline or # comment will be stripped when parsing, so don't allow + // those. Also, if any space or tab is present in the principal, the part + // proceeding this would be parsed as an option, so just avoid any whitespace + // altogether. + ret.principalRegexp = regexp.MustCompile(`^[^\s#]+$`) + return ret +}) func writeAuthorizedStringForKey(key *PublicKey, w io.Writer) (keyValid bool, err error) { const tpl = AuthorizedStringCommentPrefix + "\n" + `command=%s,no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,no-user-rc,restrict %s` + "\n" @@ -66,13 +72,11 @@ func writeAuthorizedStringForKey(key *PublicKey, w io.Writer) (keyValid bool, er var sshKey string if key.Type == KeyTypePrincipal { - principal := strings.TrimSpace(key.Content) - - if matched := principalRegexp.MatchString(principal); !matched { - return false, fmt.Errorf("does not match %s", principalRegexp.String()) + // TODO: actually using PublicKey to store "principal" is an abuse + if !globalVars().principalRegexp.MatchString(key.Content) { + return false, fmt.Errorf("invalid principal key: %s", key.Content) } - - sshKey = fmt.Sprintf("%s # user-%d", principal, key.OwnerID) + sshKey = fmt.Sprintf("%s # user-%d", key.Content, key.OwnerID) } else { pubKey, _, _, _, err := ssh.ParseAuthorizedKey([]byte(key.Content)) if err != 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 0000000000000..36ed57a653786 --- /dev/null +++ b/models/asymkey/ssh_key_authorized_keys_test.go @@ -0,0 +1,90 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package asymkey + +import ( + "strings" + "testing" + + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/test" + + "github.com/stretchr/testify/assert" +) + +func TestWriteAuthorizedStringForKey(t *testing.T) { + defer test.MockVariableValue(&setting.AppPath, "/tmp/gitea")() + defer test.MockVariableValue(&setting.CustomConf, "/tmp/app.ini")() + writeKey := func(t *testing.T, key *PublicKey) (bool, string, error) { + sb := &strings.Builder{} + valid, err := writeAuthorizedStringForKey(key, sb) + return valid, sb.String(), err + } + const validKeyContent = `ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAICV0MGX/W9IvLA4FXpIuUcdDcbj5KX4syHgsTy7soVgf` + + testValid := func(t *testing.T, key *PublicKey, expected string) { + valid, content, err := writeKey(t, key) + assert.True(t, valid) + assert.Equal(t, expected, content) + assert.NoError(t, err) + } + + testInvalid := func(t *testing.T, key *PublicKey) { + valid, content, err := writeKey(t, key) + assert.False(t, valid) + assert.Empty(t, content) + assert.Error(t, err) + } + + t.Run("PublicKey", func(t *testing.T) { + testValid(t, &PublicKey{ + OwnerID: 123, + Content: validKeyContent + " any-comment", + Type: KeyTypeUser, + }, `# gitea public key +command="/tmp/gitea --config=/tmp/app.ini serv key-0",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,no-user-rc,restrict ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAICV0MGX/W9IvLA4FXpIuUcdDcbj5KX4syHgsTy7soVgf user-123 +`) + }) + + t.Run("PublicKeyWithNewLine", func(t *testing.T) { + testValid(t, &PublicKey{ + OwnerID: 123, + Content: validKeyContent + "\nany-more", // the new line should be ignored + Type: KeyTypeUser, + }, `# gitea public key +command="/tmp/gitea --config=/tmp/app.ini serv key-0",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,no-user-rc,restrict ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAICV0MGX/W9IvLA4FXpIuUcdDcbj5KX4syHgsTy7soVgf user-123 +`) + }) + + t.Run("PublicKeyInvalid", func(t *testing.T) { + testInvalid(t, &PublicKey{ + OwnerID: 123, + Content: validKeyContent + "any-more", + Type: KeyTypeUser, + }) + }) + + t.Run("Principal", func(t *testing.T) { + testValid(t, &PublicKey{ + OwnerID: 123, + Content: "any-content", + Type: KeyTypePrincipal, + }, `# gitea public key +command="/tmp/gitea --config=/tmp/app.ini serv key-0",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,no-user-rc,restrict any-content # user-123 +`) + }) + + t.Run("PrincipalInvalid", func(t *testing.T) { + testInvalid(t, &PublicKey{ + OwnerID: 123, + Content: "a b", + Type: KeyTypePrincipal, + }) + testInvalid(t, &PublicKey{ + OwnerID: 123, + Content: "a\nb", + Type: KeyTypePrincipal, + }) + }) +}