fix(agent/agentssh): fix X11 forwarding by improving Xauthority management (#11550)

Fixes #11531
This commit is contained in:
Mathias Fredriksson 2024-01-10 19:04:44 +02:00 committed by GitHub
parent 89ab659114
commit b1d53a68c2
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 448 additions and 2 deletions

View File

@ -6,6 +6,7 @@ import (
"encoding/hex"
"errors"
"fmt"
"io"
"net"
"os"
"path/filepath"
@ -141,7 +142,7 @@ func addXauthEntry(ctx context.Context, fs afero.Fs, host string, display string
}
// Open or create the Xauthority file
file, err := fs.OpenFile(xauthPath, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0o600)
file, err := fs.OpenFile(xauthPath, os.O_RDWR|os.O_CREATE, 0o600)
if err != nil {
return xerrors.Errorf("failed to open Xauthority file: %w", err)
}
@ -153,7 +154,105 @@ func addXauthEntry(ctx context.Context, fs afero.Fs, host string, display string
return xerrors.Errorf("failed to decode auth cookie: %w", err)
}
// Write Xauthority entry
// Read the Xauthority file and look for an existing entry for the host,
// display, and auth protocol. If an entry is found, overwrite the auth
// cookie (if it fits). Otherwise, mark the entry for deletion.
type deleteEntry struct {
start, end int
}
var deleteEntries []deleteEntry
pos := 0
updated := false
for {
entry, err := readXauthEntry(file)
if err != nil {
if errors.Is(err, io.EOF) {
break
}
return xerrors.Errorf("failed to read Xauthority entry: %w", err)
}
nextPos := pos + entry.Len()
cookieStartPos := nextPos - len(entry.authCookie)
if entry.family == 0x0100 && entry.address == host && entry.display == display && entry.authProtocol == authProtocol {
if !updated && len(entry.authCookie) == len(authCookieBytes) {
// Overwrite the auth cookie
_, err := file.WriteAt(authCookieBytes, int64(cookieStartPos))
if err != nil {
return xerrors.Errorf("failed to write auth cookie: %w", err)
}
updated = true
} else {
// Mark entry for deletion.
if len(deleteEntries) > 0 && deleteEntries[len(deleteEntries)-1].end == pos {
deleteEntries[len(deleteEntries)-1].end = nextPos
} else {
deleteEntries = append(deleteEntries, deleteEntry{
start: pos,
end: nextPos,
})
}
}
}
pos = nextPos
}
// In case the magic cookie changed, or we've previously bloated the
// Xauthority file, we may have to delete entries.
if len(deleteEntries) > 0 {
// Read the entire file into memory. This is not ideal, but it's the
// simplest way to delete entries from the middle of the file. The
// Xauthority file is small, so this should be fine.
_, err = file.Seek(0, io.SeekStart)
if err != nil {
return xerrors.Errorf("failed to seek Xauthority file: %w", err)
}
data, err := io.ReadAll(file)
if err != nil {
return xerrors.Errorf("failed to read Xauthority file: %w", err)
}
// Delete the entries in reverse order.
for i := len(deleteEntries) - 1; i >= 0; i-- {
entry := deleteEntries[i]
// Safety check: ensure the entry is still there.
if entry.start > len(data) || entry.end > len(data) {
continue
}
data = append(data[:entry.start], data[entry.end:]...)
}
// Write the data back to the file.
_, err = file.Seek(0, io.SeekStart)
if err != nil {
return xerrors.Errorf("failed to seek Xauthority file: %w", err)
}
_, err = file.Write(data)
if err != nil {
return xerrors.Errorf("failed to write Xauthority file: %w", err)
}
// Truncate the file.
err = file.Truncate(int64(len(data)))
if err != nil {
return xerrors.Errorf("failed to truncate Xauthority file: %w", err)
}
}
// Return if we've already updated the entry.
if updated {
return nil
}
// Ensure we're at the end (append).
_, err = file.Seek(0, io.SeekEnd)
if err != nil {
return xerrors.Errorf("failed to seek Xauthority file: %w", err)
}
// Append Xauthority entry.
family := uint16(0x0100) // FamilyLocal
err = binary.Write(file, binary.BigEndian, family)
if err != nil {
@ -198,3 +297,96 @@ func addXauthEntry(ctx context.Context, fs afero.Fs, host string, display string
return nil
}
// xauthEntry is an representation of an Xauthority entry.
//
// The Xauthority file format is as follows:
//
// - 16-bit family
// - 16-bit address length
// - address
// - 16-bit display length
// - display
// - 16-bit auth protocol length
// - auth protocol
// - 16-bit auth cookie length
// - auth cookie
type xauthEntry struct {
family uint16
address string
display string
authProtocol string
authCookie []byte
}
func (e xauthEntry) Len() int {
// 5 * uint16 = 10 bytes for the family/length fields.
return 2*5 + len(e.address) + len(e.display) + len(e.authProtocol) + len(e.authCookie)
}
func readXauthEntry(r io.Reader) (xauthEntry, error) {
var entry xauthEntry
// Read family
err := binary.Read(r, binary.BigEndian, &entry.family)
if err != nil {
return xauthEntry{}, xerrors.Errorf("failed to read family: %w", err)
}
// Read address
var addressLength uint16
err = binary.Read(r, binary.BigEndian, &addressLength)
if err != nil {
return xauthEntry{}, xerrors.Errorf("failed to read address length: %w", err)
}
addressBytes := make([]byte, addressLength)
_, err = r.Read(addressBytes)
if err != nil {
return xauthEntry{}, xerrors.Errorf("failed to read address: %w", err)
}
entry.address = string(addressBytes)
// Read display
var displayLength uint16
err = binary.Read(r, binary.BigEndian, &displayLength)
if err != nil {
return xauthEntry{}, xerrors.Errorf("failed to read display length: %w", err)
}
displayBytes := make([]byte, displayLength)
_, err = r.Read(displayBytes)
if err != nil {
return xauthEntry{}, xerrors.Errorf("failed to read display: %w", err)
}
entry.display = string(displayBytes)
// Read auth protocol
var authProtocolLength uint16
err = binary.Read(r, binary.BigEndian, &authProtocolLength)
if err != nil {
return xauthEntry{}, xerrors.Errorf("failed to read auth protocol length: %w", err)
}
authProtocolBytes := make([]byte, authProtocolLength)
_, err = r.Read(authProtocolBytes)
if err != nil {
return xauthEntry{}, xerrors.Errorf("failed to read auth protocol: %w", err)
}
entry.authProtocol = string(authProtocolBytes)
// Read auth cookie
var authCookieLength uint16
err = binary.Read(r, binary.BigEndian, &authCookieLength)
if err != nil {
return xauthEntry{}, xerrors.Errorf("failed to read auth cookie length: %w", err)
}
entry.authCookie = make([]byte, authCookieLength)
_, err = r.Read(entry.authCookie)
if err != nil {
return xauthEntry{}, xerrors.Errorf("failed to read auth cookie: %w", err)
}
return entry, nil
}

View File

@ -0,0 +1,254 @@
package agentssh
import (
"context"
"os"
"path/filepath"
"testing"
"github.com/google/go-cmp/cmp"
"github.com/spf13/afero"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func Test_addXauthEntry(t *testing.T) {
t.Parallel()
type testEntry struct {
address string
display string
authProtocol string
authCookie string
}
tests := []struct {
name string
authFile []byte
wantAuthFile []byte
entries []testEntry
}{
{
name: "add entry",
authFile: nil,
wantAuthFile: []byte{
// w/unix:0 MIT-MAGIC-COOKIE-1 00
//
// 00000000: 0100 0001 7700 0130 0012 4d49 542d 4d41 ....w..0..MIT-MA
// 00000010: 4749 432d 434f 4f4b 4945 2d31 0001 00 GIC-COOKIE-1...
0x01, 0x00, 0x00, 0x01, 0x77, 0x00, 0x01, 0x30,
0x00, 0x12, 0x4d, 0x49, 0x54, 0x2d, 0x4d, 0x41,
0x47, 0x49, 0x43, 0x2d, 0x43, 0x4f, 0x4f, 0x4b,
0x49, 0x45, 0x2d, 0x31, 0x00, 0x01, 0x00,
},
entries: []testEntry{
{
address: "w",
display: "0",
authProtocol: "MIT-MAGIC-COOKIE-1",
authCookie: "00",
},
},
},
{
name: "add two entries",
authFile: []byte{},
wantAuthFile: []byte{
// w/unix:0 MIT-MAGIC-COOKIE-1 00
// w/unix:1 MIT-MAGIC-COOKIE-1 11
//
// 00000000: 0100 0001 7700 0130 0012 4d49 542d 4d41 ....w..0..MIT-MA
// 00000010: 4749 432d 434f 4f4b 4945 2d31 0001 0001 GIC-COOKIE-1....
// 00000020: 0000 0177 0001 3100 124d 4954 2d4d 4147 ...w..1..MIT-MAG
// 00000030: 4943 2d43 4f4f 4b49 452d 3100 0111 IC-COOKIE-1...
0x01, 0x00, 0x00, 0x01, 0x77, 0x00, 0x01, 0x30,
0x00, 0x12, 0x4d, 0x49, 0x54, 0x2d, 0x4d, 0x41,
0x47, 0x49, 0x43, 0x2d, 0x43, 0x4f, 0x4f, 0x4b,
0x49, 0x45, 0x2d, 0x31, 0x00, 0x01, 0x00,
0x01, 0x00, 0x00, 0x01, 0x77, 0x00, 0x01, 0x31,
0x00, 0x12, 0x4d, 0x49, 0x54, 0x2d, 0x4d, 0x41,
0x47, 0x49, 0x43, 0x2d, 0x43, 0x4f, 0x4f, 0x4b,
0x49, 0x45, 0x2d, 0x31, 0x00, 0x01, 0x11,
},
entries: []testEntry{
{
address: "w",
display: "0",
authProtocol: "MIT-MAGIC-COOKIE-1",
authCookie: "00",
},
{
address: "w",
display: "1",
authProtocol: "MIT-MAGIC-COOKIE-1",
authCookie: "11",
},
},
},
{
name: "update entry with new auth cookie length",
authFile: []byte{
// w/unix:0 MIT-MAGIC-COOKIE-1 00
// w/unix:1 MIT-MAGIC-COOKIE-1 11
//
// 00000000: 0100 0001 7700 0130 0012 4d49 542d 4d41 ....w..0..MIT-MA
// 00000010: 4749 432d 434f 4f4b 4945 2d31 0001 0001 GIC-COOKIE-1....
// 00000020: 0000 0177 0001 3100 124d 4954 2d4d 4147 ...w..1..MIT-MAG
// 00000030: 4943 2d43 4f4f 4b49 452d 3100 0111 IC-COOKIE-1...
0x01, 0x00, 0x00, 0x01, 0x77, 0x00, 0x01, 0x30,
0x00, 0x12, 0x4d, 0x49, 0x54, 0x2d, 0x4d, 0x41,
0x47, 0x49, 0x43, 0x2d, 0x43, 0x4f, 0x4f, 0x4b,
0x49, 0x45, 0x2d, 0x31, 0x00, 0x01, 0x00,
0x01, 0x00, 0x00, 0x01, 0x77, 0x00, 0x01, 0x31,
0x00, 0x12, 0x4d, 0x49, 0x54, 0x2d, 0x4d, 0x41,
0x47, 0x49, 0x43, 0x2d, 0x43, 0x4f, 0x4f, 0x4b,
0x49, 0x45, 0x2d, 0x31, 0x00, 0x01, 0x11,
},
wantAuthFile: []byte{
// The order changed, due to new length of auth cookie resulting
// in remove + append, we verify that the implementation is
// behaving as expected (changing the order is not a requirement,
// simply an implementation detail).
0x01, 0x00, 0x00, 0x01, 0x77, 0x00, 0x01, 0x31,
0x00, 0x12, 0x4d, 0x49, 0x54, 0x2d, 0x4d, 0x41,
0x47, 0x49, 0x43, 0x2d, 0x43, 0x4f, 0x4f, 0x4b,
0x49, 0x45, 0x2d, 0x31, 0x00, 0x01, 0x11,
0x01, 0x00, 0x00, 0x01, 0x77, 0x00, 0x01, 0x30,
0x00, 0x12, 0x4d, 0x49, 0x54, 0x2d, 0x4d, 0x41,
0x47, 0x49, 0x43, 0x2d, 0x43, 0x4f, 0x4f, 0x4b,
0x49, 0x45, 0x2d, 0x31, 0x00, 0x02, 0xff, 0xff,
},
entries: []testEntry{
{
address: "w",
display: "0",
authProtocol: "MIT-MAGIC-COOKIE-1",
authCookie: "ffff",
},
},
},
{
name: "update entry",
authFile: []byte{
// 00000000: 0100 0001 7700 0130 0012 4d49 542d 4d41 ....w..0..MIT-MA
// 00000010: 4749 432d 434f 4f4b 4945 2d31 0001 0001 GIC-COOKIE-1....
// 00000020: 0000 0177 0001 3100 124d 4954 2d4d 4147 ...w..1..MIT-MAG
// 00000030: 4943 2d43 4f4f 4b49 452d 3100 0111 IC-COOKIE-1...
0x01, 0x00, 0x00, 0x01, 0x77, 0x00, 0x01, 0x30,
0x00, 0x12, 0x4d, 0x49, 0x54, 0x2d, 0x4d, 0x41,
0x47, 0x49, 0x43, 0x2d, 0x43, 0x4f, 0x4f, 0x4b,
0x49, 0x45, 0x2d, 0x31, 0x00, 0x01, 0x00,
0x01, 0x00, 0x00, 0x01, 0x77, 0x00, 0x01, 0x31,
0x00, 0x12, 0x4d, 0x49, 0x54, 0x2d, 0x4d, 0x41,
0x47, 0x49, 0x43, 0x2d, 0x43, 0x4f, 0x4f, 0x4b,
0x49, 0x45, 0x2d, 0x31, 0x00, 0x01, 0x11,
},
wantAuthFile: []byte{
// 00000000: 0100 0001 7700 0130 0012 4d49 542d 4d41 ....w..0..MIT-MA
// 00000010: 4749 432d 434f 4f4b 4945 2d31 0001 0001 GIC-COOKIE-1....
// 00000020: 0000 0177 0001 3100 124d 4954 2d4d 4147 ...w..1..MIT-MAG
// 00000030: 4943 2d43 4f4f 4b49 452d 3100 0111 IC-COOKIE-1...
0x01, 0x00, 0x00, 0x01, 0x77, 0x00, 0x01, 0x30,
0x00, 0x12, 0x4d, 0x49, 0x54, 0x2d, 0x4d, 0x41,
0x47, 0x49, 0x43, 0x2d, 0x43, 0x4f, 0x4f, 0x4b,
0x49, 0x45, 0x2d, 0x31, 0x00, 0x01, 0xff,
0x01, 0x00, 0x00, 0x01, 0x77, 0x00, 0x01, 0x31,
0x00, 0x12, 0x4d, 0x49, 0x54, 0x2d, 0x4d, 0x41,
0x47, 0x49, 0x43, 0x2d, 0x43, 0x4f, 0x4f, 0x4b,
0x49, 0x45, 0x2d, 0x31, 0x00, 0x01, 0x11,
},
entries: []testEntry{
{
address: "w",
display: "0",
authProtocol: "MIT-MAGIC-COOKIE-1",
authCookie: "ff",
},
},
},
{
name: "clean up old entries",
authFile: []byte{
// w/unix:0 MIT-MAGIC-COOKIE-1 80507df050756cdefa504b65adb3bcfb
// w/unix:0 MIT-MAGIC-COOKIE-1 267b37f6cbc11b97beb826bb1aab8570
// w/unix:0 MIT-MAGIC-COOKIE-1 516e22e2b11d1bd0115dff09c028ca5c
//
// 00000000: 0100 0001 7700 0130 0012 4d49 542d 4d41 ....w..0..MIT-MA
// 00000010: 4749 432d 434f 4f4b 4945 2d31 0010 8050 GIC-COOKIE-1...P
// 00000020: 7df0 5075 6cde fa50 4b65 adb3 bcfb 0100 }.Pul..PKe......
// 00000030: 0001 7700 0130 0012 4d49 542d 4d41 4749 ..w..0..MIT-MAGI
// 00000040: 432d 434f 4f4b 4945 2d31 0010 267b 37f6 C-COOKIE-1..&{7.
// 00000050: cbc1 1b97 beb8 26bb 1aab 8570 0100 0001 ......&....p....
// 00000060: 7700 0130 0012 4d49 542d 4d41 4749 432d w..0..MIT-MAGIC-
// 00000070: 434f 4f4b 4945 2d31 0010 516e 22e2 b11d COOKIE-1..Qn"...
// 00000080: 1bd0 115d ff09 c028 ca5c ...]...(.\
0x01, 0x00, 0x00, 0x01, 0x77, 0x00, 0x01, 0x30,
0x00, 0x12, 0x4d, 0x49, 0x54, 0x2d, 0x4d, 0x41,
0x47, 0x49, 0x43, 0x2d, 0x43, 0x4f, 0x4f, 0x4b,
0x49, 0x45, 0x2d, 0x31, 0x00, 0x10, 0x80, 0x50,
0x7d, 0xf0, 0x50, 0x75, 0x6c, 0xde, 0xfa, 0x50,
0x4b, 0x65, 0xad, 0xb3, 0xbc, 0xfb, 0x01, 0x00,
0x00, 0x01, 0x77, 0x00, 0x01, 0x30, 0x00, 0x12,
0x4d, 0x49, 0x54, 0x2d, 0x4d, 0x41, 0x47, 0x49,
0x43, 0x2d, 0x43, 0x4f, 0x4f, 0x4b, 0x49, 0x45,
0x2d, 0x31, 0x00, 0x10, 0x26, 0x7b, 0x37, 0xf6,
0xcb, 0xc1, 0x1b, 0x97, 0xbe, 0xb8, 0x26, 0xbb,
0x1a, 0xab, 0x85, 0x70, 0x01, 0x00, 0x00, 0x01,
0x77, 0x00, 0x01, 0x30, 0x00, 0x12, 0x4d, 0x49,
0x54, 0x2d, 0x4d, 0x41, 0x47, 0x49, 0x43, 0x2d,
0x43, 0x4f, 0x4f, 0x4b, 0x49, 0x45, 0x2d, 0x31,
0x00, 0x10, 0x51, 0x6e, 0x22, 0xe2, 0xb1, 0x1d,
0x1b, 0xd0, 0x11, 0x5d, 0xff, 0x09, 0xc0, 0x28,
0xca, 0x5c,
},
wantAuthFile: []byte{
// w/unix:0 MIT-MAGIC-COOKIE-1 516e5bc892b7162b844abd1fc1a7c16e
//
// 00000000: 0100 0001 7700 0130 0012 4d49 542d 4d41 ....w..0..MIT-MA
// 00000010: 4749 432d 434f 4f4b 4945 2d31 0010 516e GIC-COOKIE-1..Qn
// 00000020: 5bc8 92b7 162b 844a bd1f c1a7 c16e [....+.J.....n
0x01, 0x00, 0x00, 0x01, 0x77, 0x00, 0x01, 0x30,
0x00, 0x12, 0x4d, 0x49, 0x54, 0x2d, 0x4d, 0x41,
0x47, 0x49, 0x43, 0x2d, 0x43, 0x4f, 0x4f, 0x4b,
0x49, 0x45, 0x2d, 0x31, 0x00, 0x10, 0x51, 0x6e,
0x5b, 0xc8, 0x92, 0xb7, 0x16, 0x2b, 0x84, 0x4a,
0xbd, 0x1f, 0xc1, 0xa7, 0xc1, 0x6e,
},
entries: []testEntry{
{
address: "w",
display: "0",
authProtocol: "MIT-MAGIC-COOKIE-1",
authCookie: "516e5bc892b7162b844abd1fc1a7c16e",
},
},
},
}
homedir, err := os.UserHomeDir()
require.NoError(t, err)
for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
fs := afero.NewMemMapFs()
if tt.authFile != nil {
err := afero.WriteFile(fs, filepath.Join(homedir, ".Xauthority"), tt.authFile, 0o600)
require.NoError(t, err)
}
for _, entry := range tt.entries {
err := addXauthEntry(context.Background(), fs, entry.address, entry.display, entry.authProtocol, entry.authCookie)
require.NoError(t, err)
}
gotAuthFile, err := afero.ReadFile(fs, filepath.Join(homedir, ".Xauthority"))
require.NoError(t, err)
if diff := cmp.Diff(tt.wantAuthFile, gotAuthFile); diff != "" {
assert.Failf(t, "addXauthEntry() mismatch", "(-want +got):\n%s", diff)
}
})
}
}