From c11b89612675d0eb32a9e70f67a6101ccbaa30c0 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 12 Dec 2024 23:55:29 +0800 Subject: [PATCH 1/9] fix --- modules/ssh/ssh.go | 51 ++++++++++++++++++++++++++++++++++------- modules/ssh/ssh_test.go | 32 ++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 8 deletions(-) create mode 100644 modules/ssh/ssh_test.go diff --git a/modules/ssh/ssh.go b/modules/ssh/ssh.go index f8e4f569b87f4..c1d5988deeaa7 100644 --- a/modules/ssh/ssh.go +++ b/modules/ssh/ssh.go @@ -17,6 +17,7 @@ import ( "os" "os/exec" "path/filepath" + "reflect" "strconv" "strings" "sync" @@ -33,9 +34,21 @@ import ( gossh "golang.org/x/crypto/ssh" ) -type contextKey string - -const giteaKeyID = contextKey("gitea-key-id") +// The ssh auth overall works like this: +// NewServerConn: +// serverHandshake+serverAuthenticate: +// PublicKeyCallback: +// PublicKeyHandler (our code): +// clear(ctx.Permissions) and set ctx.Permissions.giteaKeyID = keyID +// pubKey.Verify +// return ctx.Permissions // only reaches here, the pub key is really authenticated +// set conn.Permissions from serverAuthenticate +// sessionHandler(conn) +// +// Then sessionHandler should only use the "verified keyID" from the conn. +// Otherwise, if a users provides 2 keys A and B, if A succeeds to authenticate, sessionHandler will see B's keyID + +const giteaPermissionExtensionKeyID = "gitea-perm-ext-key-id" func getExitStatusFromError(err error) int { if err == nil { @@ -61,8 +74,26 @@ func getExitStatusFromError(err error) int { return waitStatus.ExitStatus() } +type sessionPartial struct { + sync.Mutex + gossh.Channel + conn *gossh.ServerConn +} + +func ptr[T any](intf any) *T { + // https://pkg.go.dev/unsafe#Pointer + // (1) Conversion of a *T1 to Pointer to *T2. + // Provided that T2 is no larger than T1 and that the two share an equivalent memory layout, + // this conversion allows reinterpreting data of one type as data of another type. + v := reflect.ValueOf(intf) + p := v.UnsafePointer() + return (*T)(p) +} + func sessionHandler(session ssh.Session) { - keyID := fmt.Sprintf("%d", session.Context().Value(giteaKeyID).(int64)) + // it can't use session.Permissions() because it only use the ctx one, so we must use the original ssh conn + sshConn := ptr[sessionPartial](session) + keyID := sshConn.conn.Permissions.Extensions[giteaPermissionExtensionKeyID] command := session.RawCommand() @@ -164,6 +195,12 @@ func sessionHandler(session ssh.Session) { } func publicKeyHandler(ctx ssh.Context, key ssh.PublicKey) bool { + setPermExt := func(keyID int64) { + ctx.Permissions().Permissions.Extensions = map[string]string{ + giteaPermissionExtensionKeyID: fmt.Sprint(keyID), + } + } + if log.IsDebug() { // <- FingerprintSHA256 is kinda expensive so only calculate it if necessary log.Debug("Handle Public Key: Fingerprint: %s from %s", gossh.FingerprintSHA256(key), ctx.RemoteAddr()) } @@ -238,8 +275,7 @@ func publicKeyHandler(ctx ssh.Context, key ssh.PublicKey) bool { if log.IsDebug() { // <- FingerprintSHA256 is kinda expensive so only calculate it if necessary log.Debug("Successfully authenticated: %s Certificate Fingerprint: %s Principal: %s", ctx.RemoteAddr(), gossh.FingerprintSHA256(key), principal) } - ctx.SetValue(giteaKeyID, pkey.ID) - + setPermExt(pkey.ID) return true } @@ -266,8 +302,7 @@ func publicKeyHandler(ctx ssh.Context, key ssh.PublicKey) bool { if log.IsDebug() { // <- FingerprintSHA256 is kinda expensive so only calculate it if necessary log.Debug("Successfully authenticated: %s Public Key Fingerprint: %s", ctx.RemoteAddr(), gossh.FingerprintSHA256(key)) } - ctx.SetValue(giteaKeyID, pkey.ID) - + setPermExt(pkey.ID) return true } diff --git a/modules/ssh/ssh_test.go b/modules/ssh/ssh_test.go new file mode 100644 index 0000000000000..a39e7f27d215d --- /dev/null +++ b/modules/ssh/ssh_test.go @@ -0,0 +1,32 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package ssh + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +type S1 struct { + a, b, c int +} + +func (s S1) S1Func() {} + +type S1Intf interface { + S1Func() +} + +type S2 struct { + a, b int +} + +func TestPtr(t *testing.T) { + s1 := &S1{1, 2, 3} + var intf S1Intf = s1 + s2 := ptr[S2](intf) + assert.Equal(t, 1, s2.a) + assert.Equal(t, 2, s2.b) +} From c71d872d7736d1fd479ddce7fbfb02d3ee6f26e5 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 13 Dec 2024 07:28:09 +0800 Subject: [PATCH 2/9] add more comments --- modules/ssh/ssh.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/modules/ssh/ssh.go b/modules/ssh/ssh.go index c1d5988deeaa7..2a8dd7b1f0068 100644 --- a/modules/ssh/ssh.go +++ b/modules/ssh/ssh.go @@ -74,6 +74,11 @@ func getExitStatusFromError(err error) int { return waitStatus.ExitStatus() } +// sessionPartial is the private struct from "gliderlabs/ssh/session.go" +// we need to read the original "conn" field from "ssh.Session interface" which contains the "*session pointer" +// https://github.com/gliderlabs/ssh/blob/d137aad99cd6f2d9495bfd98c755bec4e5dffb8c/session.go#L109-L113 +// If upstream fixes the problem and/or changes the struct, we need to follow. +// If the struct mismatches, the builtin ssh server will fail during integration tests. type sessionPartial struct { sync.Mutex gossh.Channel From 3fe4e9e1b15302e3ec49182a422990afd8a045cc Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 13 Dec 2024 07:39:20 +0800 Subject: [PATCH 3/9] fine tune comment --- modules/ssh/ssh.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/modules/ssh/ssh.go b/modules/ssh/ssh.go index 2a8dd7b1f0068..20a408d0440ca 100644 --- a/modules/ssh/ssh.go +++ b/modules/ssh/ssh.go @@ -45,8 +45,8 @@ import ( // set conn.Permissions from serverAuthenticate // sessionHandler(conn) // -// Then sessionHandler should only use the "verified keyID" from the conn. -// Otherwise, if a users provides 2 keys A and B, if A succeeds to authenticate, sessionHandler will see B's keyID +// Then sessionHandler should only use the "verified keyID" from the original ssh conn, but not the ctx one. +// Otherwise, if a user provides 2 keys A and B, if A succeeds to authenticate, sessionHandler will see B's keyID const giteaPermissionExtensionKeyID = "gitea-perm-ext-key-id" @@ -75,7 +75,7 @@ func getExitStatusFromError(err error) int { } // sessionPartial is the private struct from "gliderlabs/ssh/session.go" -// we need to read the original "conn" field from "ssh.Session interface" which contains the "*session pointer" +// We need to read the original "conn" field from "ssh.Session interface" which contains the "*session pointer" // https://github.com/gliderlabs/ssh/blob/d137aad99cd6f2d9495bfd98c755bec4e5dffb8c/session.go#L109-L113 // If upstream fixes the problem and/or changes the struct, we need to follow. // If the struct mismatches, the builtin ssh server will fail during integration tests. @@ -96,7 +96,8 @@ func ptr[T any](intf any) *T { } func sessionHandler(session ssh.Session) { - // it can't use session.Permissions() because it only use the ctx one, so we must use the original ssh conn + // it can't use session.Permissions() because it only use the value from ctx, which might not be the authenticated one. + // so we must use the original ssh conn, which always contains the correct (verified) keyID. sshConn := ptr[sessionPartial](session) keyID := sshConn.conn.Permissions.Extensions[giteaPermissionExtensionKeyID] From 6223d43289a122e89cabdddab011f4e67b177dc5 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 13 Dec 2024 07:44:21 +0800 Subject: [PATCH 4/9] fine tune comment --- modules/ssh/ssh.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/modules/ssh/ssh.go b/modules/ssh/ssh.go index 20a408d0440ca..0f2246db1879c 100644 --- a/modules/ssh/ssh.go +++ b/modules/ssh/ssh.go @@ -201,6 +201,10 @@ func sessionHandler(session ssh.Session) { } func publicKeyHandler(ctx ssh.Context, key ssh.PublicKey) bool { + // The publicKeyHandler (PublicKeyCallback) only provides the candidate keys to authenticate, + // It does NOT really verify here, so we could only record the related information here. + // After authentication (Verify), the "Permissions" will be assigned to the ssh conn, + // then we can use it in the "session handler" setPermExt := func(keyID int64) { ctx.Permissions().Permissions.Extensions = map[string]string{ giteaPermissionExtensionKeyID: fmt.Sprint(keyID), From bb15dd621caed448c4f17281a3260d3a34cafe43 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 13 Dec 2024 08:01:49 +0800 Subject: [PATCH 5/9] make it more strict --- modules/ssh/ssh.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/modules/ssh/ssh.go b/modules/ssh/ssh.go index 0f2246db1879c..8c7015464e7ac 100644 --- a/modules/ssh/ssh.go +++ b/modules/ssh/ssh.go @@ -13,6 +13,7 @@ import ( "errors" "fmt" "io" + "maps" "net" "os" "os/exec" @@ -39,7 +40,7 @@ import ( // serverHandshake+serverAuthenticate: // PublicKeyCallback: // PublicKeyHandler (our code): -// clear(ctx.Permissions) and set ctx.Permissions.giteaKeyID = keyID +// reset(ctx.Permissions) and set ctx.Permissions.giteaKeyID = keyID // pubKey.Verify // return ctx.Permissions // only reaches here, the pub key is really authenticated // set conn.Permissions from serverAuthenticate @@ -205,6 +206,13 @@ func publicKeyHandler(ctx ssh.Context, key ssh.PublicKey) bool { // It does NOT really verify here, so we could only record the related information here. // After authentication (Verify), the "Permissions" will be assigned to the ssh conn, // then we can use it in the "session handler" + + // first, reset the ctx permissions (just like https://github.com/gliderlabs/ssh/pull/243 does) + // it shouldn't be reused across different ssh conn (sessions) + ctxPerm := ctx.Permissions().Permissions + ctx.Permissions().Permissions = &gossh.Permissions{} + ctx.Permissions().Permissions.CriticalOptions = maps.Clone(ctxPerm.CriticalOptions) + setPermExt := func(keyID int64) { ctx.Permissions().Permissions.Extensions = map[string]string{ giteaPermissionExtensionKeyID: fmt.Sprint(keyID), From 6a787fe59da6702a0936fd31553bbf45f4f08064 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 13 Dec 2024 08:05:32 +0800 Subject: [PATCH 6/9] fine tune comment --- modules/ssh/ssh.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/modules/ssh/ssh.go b/modules/ssh/ssh.go index 8c7015464e7ac..ae4f0be469638 100644 --- a/modules/ssh/ssh.go +++ b/modules/ssh/ssh.go @@ -202,16 +202,16 @@ func sessionHandler(session ssh.Session) { } func publicKeyHandler(ctx ssh.Context, key ssh.PublicKey) bool { - // The publicKeyHandler (PublicKeyCallback) only provides the candidate keys to authenticate, + // The publicKeyHandler (PublicKeyCallback) only helps to provide the candidate keys to authenticate, // It does NOT really verify here, so we could only record the related information here. // After authentication (Verify), the "Permissions" will be assigned to the ssh conn, // then we can use it in the "session handler" // first, reset the ctx permissions (just like https://github.com/gliderlabs/ssh/pull/243 does) - // it shouldn't be reused across different ssh conn (sessions) - ctxPerm := ctx.Permissions().Permissions + // it shouldn't be reused across different ssh conn (sessions), each pub key should have its own "Permissions" + oldCtxPerm := ctx.Permissions().Permissions ctx.Permissions().Permissions = &gossh.Permissions{} - ctx.Permissions().Permissions.CriticalOptions = maps.Clone(ctxPerm.CriticalOptions) + ctx.Permissions().Permissions.CriticalOptions = maps.Clone(oldCtxPerm.CriticalOptions) setPermExt := func(keyID int64) { ctx.Permissions().Permissions.Extensions = map[string]string{ From d5efdee2b7e0a73c00ad9969eaa58a28ce4219e5 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 13 Dec 2024 08:26:12 +0800 Subject: [PATCH 7/9] remove unnecessary tests --- modules/ssh/ssh_test.go | 32 -------------------------------- 1 file changed, 32 deletions(-) delete mode 100644 modules/ssh/ssh_test.go diff --git a/modules/ssh/ssh_test.go b/modules/ssh/ssh_test.go deleted file mode 100644 index a39e7f27d215d..0000000000000 --- a/modules/ssh/ssh_test.go +++ /dev/null @@ -1,32 +0,0 @@ -// Copyright 2024 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package ssh - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -type S1 struct { - a, b, c int -} - -func (s S1) S1Func() {} - -type S1Intf interface { - S1Func() -} - -type S2 struct { - a, b int -} - -func TestPtr(t *testing.T) { - s1 := &S1{1, 2, 3} - var intf S1Intf = s1 - s2 := ptr[S2](intf) - assert.Equal(t, 1, s2.a) - assert.Equal(t, 2, s2.b) -} From 159fdc99b9f48df3f9051b63fd8861210891fd53 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 13 Dec 2024 08:30:00 +0800 Subject: [PATCH 8/9] update github.com/gliderlabs/ssh (only golang.org/x/crypto change) --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 17be4cbd526d5..671151d4b633a 100644 --- a/go.mod +++ b/go.mod @@ -48,7 +48,7 @@ require ( github.com/ethantkoenig/rupture v1.0.1 github.com/felixge/fgprof v0.9.5 github.com/fsnotify/fsnotify v1.7.0 - github.com/gliderlabs/ssh v0.3.7 + github.com/gliderlabs/ssh v0.3.8 github.com/go-ap/activitypub v0.0.0-20240910141749-b4b8c8aa484c github.com/go-ap/jsonld v0.0.0-20221030091449-f2a191312c73 github.com/go-chi/chi/v5 v5.1.0 diff --git a/go.sum b/go.sum index 73bdb44e33767..afa3abece8f6e 100644 --- a/go.sum +++ b/go.sum @@ -293,8 +293,8 @@ github.com/fxamacker/cbor/v2 v2.7.0 h1:iM5WgngdRBanHcxugY4JySA0nk1wZorNOpTgCMedv github.com/fxamacker/cbor/v2 v2.7.0/go.mod h1:pxXPTn3joSm21Gbwsv0w9OSA2y1HFR9qXEeXQVeNoDQ= github.com/git-lfs/pktline v0.0.0-20230103162542-ca444d533ef1 h1:mtDjlmloH7ytdblogrMz1/8Hqua1y8B4ID+bh3rvod0= github.com/git-lfs/pktline v0.0.0-20230103162542-ca444d533ef1/go.mod h1:fenKRzpXDjNpsIBhuhUzvjCKlDjKam0boRAenTE0Q6A= -github.com/gliderlabs/ssh v0.3.7 h1:iV3Bqi942d9huXnzEF2Mt+CY9gLu8DNM4Obd+8bODRE= -github.com/gliderlabs/ssh v0.3.7/go.mod h1:zpHEXBstFnQYtGnB8k8kQLol82umzn/2/snG7alWVD8= +github.com/gliderlabs/ssh v0.3.8 h1:a4YXD1V7xMF9g5nTkdfnja3Sxy1PVDCj1Zg4Wb8vY6c= +github.com/gliderlabs/ssh v0.3.8/go.mod h1:xYoytBv1sV0aL3CavoDuJIQNURXkkfPA/wxQ1pL1fAU= github.com/glycerine/go-unsnap-stream v0.0.0-20181221182339-f9677308dec2/go.mod h1:/20jfyN9Y5QPEAprSgKAUr+glWDY39ZiUEAYOEv5dsE= github.com/glycerine/goconvey v0.0.0-20190410193231-58a59202ab31/go.mod h1:Ogl1Tioa0aV7gstGFO7KhffUsb9M4ydbEbbxpcEDc24= github.com/go-ap/activitypub v0.0.0-20240910141749-b4b8c8aa484c h1:82lzmsy5Nr6JA6HcLRVxGfbdSoWfW45C6jnY3zFS7Ks= From 181d15c8a7809726e861ea1ba9c7bcd2a031aa50 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 13 Dec 2024 08:38:15 +0800 Subject: [PATCH 9/9] fine tune comment --- modules/ssh/ssh.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/modules/ssh/ssh.go b/modules/ssh/ssh.go index ae4f0be469638..6d0695ee163fc 100644 --- a/modules/ssh/ssh.go +++ b/modules/ssh/ssh.go @@ -47,7 +47,8 @@ import ( // sessionHandler(conn) // // Then sessionHandler should only use the "verified keyID" from the original ssh conn, but not the ctx one. -// Otherwise, if a user provides 2 keys A and B, if A succeeds to authenticate, sessionHandler will see B's keyID +// Otherwise, if a user provides 2 keys A (a correct one) and B (public key matches but no private key), +// then only A succeeds to authenticate, sessionHandler will see B's keyID const giteaPermissionExtensionKeyID = "gitea-perm-ext-key-id" @@ -97,7 +98,7 @@ func ptr[T any](intf any) *T { } func sessionHandler(session ssh.Session) { - // it can't use session.Permissions() because it only use the value from ctx, which might not be the authenticated one. + // here can't use session.Permissions() because it only uses the value from ctx, which might not be the authenticated one. // so we must use the original ssh conn, which always contains the correct (verified) keyID. sshConn := ptr[sessionPartial](session) keyID := sshConn.conn.Permissions.Extensions[giteaPermissionExtensionKeyID]