Merge PR #250 while preserving local dependency updates

Retain the merged upstream LDAP fix and original contributor history. Keep the existing Go/dependency refresh and published-release documentation; combine the changelog credit without repeating the Go 1.27.3 note. Signed-off-by: Feng Ruohang <rh@vonng.com>
2026-10-28 19:47:23 +03:00 · 2026-10-23 11:22:52 +08:00
parent 4059c25e22 d43adc0f2b
commit a07d655dc6
4 changed files with 348 additions and 0 deletions
@@ -2,6 +2,11 @@

 ## Unreleased

+- Fix LDAP login for long normalized DNs without hiding unreadable IAM
+  revocation records. Only length errors for valid keys with an ASCII path
+  component over 255 bytes are treated as absent; long-DN writes still fail.
+  Thanks to [@910patin](https://github.com/910patin) for the original fix and
+  reproduction in [#250](https://github.com/pgsty/silo/pull/250).
 - Build the server and standalone debugging tools with Go 1.27.3 and refresh
  Go dependencies. Keep the published Console, mcli and silo-pkg references.
 - Preserve Kafka's default broker protocol 2.1 and terminal-width behavior;
@@ -134,9 +134,34 @@ func decryptData(data []byte, objPath string) ([]byte, error) {
 	return nil, err
 }

+// iamConfigNameTooLong identifies length errors for valid keys with an ASCII
+// component that cannot be stored. Normalized LDAP DNs escape non-ASCII bytes.
+// Byte length alone is insufficient for Unicode filenames on macOS/Windows,
+// and a total-path limit can make an existing record unreadable after a move.
+func iamConfigNameTooLong(objPath string, err error) bool {
+	var invalid ObjectNameInvalid
+	if !errors.Is(err, errFileNameTooLong) && !errors.As(err, &invalid) {
+		return false
+	}
+	if checkBucketAndObjectNames(context.Background(), minioMetaBucket, objPath) != nil {
+		return false
+	}
+	for component := range strings.SplitSeq(objPath, SlashSeparator) {
+		if len(component) > 255 && utf8.RuneCountInString(component) == len(component) {
+			return true
+		}
+	}
+	return false
+}
+
 func (iamOS *IAMObjectStore) loadIAMConfigBytesWithMetadata(ctx context.Context, objPath string) ([]byte, ObjectInfo, error) {
 	data, meta, err := readConfigWithMetadata(ctx, iamOS.objAPI, objPath, ObjectOptions{})
 	if err != nil {
+		if iamConfigNameTooLong(objPath, err) {
+			// A missing record, not a storage failure: lookups of the
+			// identity proceed, while saving its record still fails.
+			err = errConfigNotFound
+		}
 		return nil, meta, err
 	}
 	data, err = decryptData(data, objPath)
@@ -7,14 +7,19 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"os"
+	"path/filepath"
+	"runtime"
 	"strings"
 	"sync/atomic"
 	"testing"
 	"time"

 	"github.com/minio/madmin-go/v3"
+	"github.com/minio/minio/internal/auth"
 	"github.com/minio/minio/internal/grid"
+	"github.com/pgsty/silo-pkg/v3/ldap"
 	xnet "github.com/pgsty/silo-pkg/v3/net"
 )

@@ -93,6 +98,211 @@ func TestIAMRevocationTombstoneReplayIsIdempotent(t *testing.T) {
 	}
 }

+// An LDAP parent named by a normalized DN longer than NAME_MAX has no record
+// on object storage. Its lookups must find nothing instead of failing login.
+func TestLoadIAMRevisionNameTooLong(t *testing.T) {
+	for _, backend := range []string{"object", "etcd"} {
+		t.Run(backend, func(t *testing.T) {
+			ctx, sys, _ := prepareIAMRevisionFixture(t, backend)
+			store := sys.store.IAMStorageAPI
+			// 160 bytes of UTF-8 become 384 characters.
+			parent, err := ldap.NormalizeDN("uid=Сотрудник,ou=Отдел разработки,ou=Дирекция информационных технологий,ou=people,ou=swengg,dc=min,dc=io")
+			mustIAM(t, err)
+			if len(parent) <= 255 {
+				t.Fatalf("normalized DN has only %d characters", len(parent))
+			}
+			parentPath := getUserIdentityPath(parent, regUser)
+			for _, path := range []string{parentPath, getUserIdentityPath("missing-parent", regUser)} {
+				r, err := loadIAMRevision(ctx, store, path)
+				if err != nil || r.Deleted || !r.timestamp().IsZero() || !r.RevokedBefore.IsZero() {
+					t.Fatalf("absent revision %.40s...: %+v, %v", path, r, err)
+				}
+			}
+			if backend == "object" {
+				var invalid ObjectNameInvalid
+				if _, err := loadIAMRevision(ctx, store, iamConfigUsersPrefix+"a//b/"+iamIdentityFile); !errors.As(err, &invalid) {
+					t.Fatalf("invalid name not caused by length was masked: %v", err)
+				}
+			}
+
+			claims := map[string]any{}
+			mustIAM(t, setIAMParentRevocationClaim(ctx, store, parent, claims))
+			if _, ok := claims[iamParentRevocationClaim]; ok {
+				t.Fatalf("unexpected revocation claim: %v", claims)
+			}
+			if _, err := sys.PolicyDBGet(parent); err != nil {
+				t.Fatalf("policy lookup failed: %v", err)
+			}
+			// The call that failed in AssumeRoleWithLDAPIdentity.
+			secret, err := getTokenSigningKey()
+			mustIAM(t, err)
+			cred, err := auth.GetNewCredentialsWithMetadata(map[string]any{"exp": UTCNow().Add(time.Hour).Unix(), parentClaim: parent}, secret)
+			mustIAM(t, err)
+			cred.ParentUser = parent
+			_, err = sys.SetTempUser(ctx, cred.AccessKey, cred, "")
+			mustIAM(t, err)
+			if _, ok := sys.store.GetUser(cred.AccessKey); !ok {
+				t.Fatal("temporary credential not cached")
+			}
+
+			// Object storage still cannot record a revocation for this parent;
+			// the failure must be reported. Etcd has no such limit.
+			err = saveIAMRevision(ctx, store, parentPath, &iamUserRevocation{})
+			if backend == "object" {
+				if !iamConfigNameTooLong(parentPath, err) {
+					t.Fatalf("expected name-too-long error, got %v", err)
+				}
+				return
+			}
+			mustIAM(t, err)
+			if err := checkIAMParentRevision(ctx, store, cred); !errors.Is(err, errIAMStaleUpdate) {
+				t.Fatalf("revoked parent accepted: %v", err)
+			}
+		})
+	}
+}
+
+// checkPathLength does not count backslashes outside Windows, so this escaped
+// DN passes it. Once config/iam/users exists, as after creating any built-in
+// user, the filesystem rejects the element with ENAMETOOLONG instead.
+func TestLoadIAMRevisionFilesystemNameTooLong(t *testing.T) {
+	ctx, sys, obj := prepareIAMRevisionFixture(t)
+	parent, err := ldap.NormalizeDN("cn=Сидоров П.П.,ou=Отдел ИТ-поддержки,ou=Пользователи,ou=people,ou=swengg,dc=min,dc=io")
+	mustIAM(t, err)
+	parentPath := getUserIdentityPath(parent, regUser)
+	if len(parent) <= 255 || checkPathLength(parentPath) != nil {
+		t.Fatalf("normalized DN must exceed NAME_MAX yet pass checkPathLength: %d characters, %v", len(parent), checkPathLength(parentPath))
+	}
+	_, err = sys.CreateUser(ctx, "existing-user", madmin.AddOrUpdateUserReq{SecretKey: "synthetic-user-password", Status: madmin.AccountEnabled})
+	mustIAM(t, err)
+	mustIAM(t, saveIAMRevision(ctx, sys.store.IAMStorageAPI, getMappedPolicyPath("existing-parent", stsUser, false), &MappedPolicy{Version: 1, Policies: "readwrite"}))
+	store := &IAMStoreSys{IAMStorageAPI: newIAMObjectStore(obj, LDAPUsersSysType)}
+	mustIAM(t, store.LoadIAMCache(ctx, false))
+	r, err := loadIAMRevision(ctx, store.IAMStorageAPI, parentPath)
+	if err != nil || r.Deleted || !r.timestamp().IsZero() {
+		t.Fatalf("absent revision: %+v, %v", r, err)
+	}
+	// LDAP mode reads policydb/sts-users/<DN>.json.
+	if _, err := store.PolicyDBGet(parent); err != nil {
+		t.Fatalf("policy lookup failed: %v", err)
+	}
+	secret, err := getTokenSigningKey()
+	mustIAM(t, err)
+	cred, err := auth.GetNewCredentialsWithMetadata(map[string]any{"exp": UTCNow().Add(time.Hour).Unix(), parentClaim: parent}, secret)
+	mustIAM(t, err)
+	cred.ParentUser = parent
+	if _, err := store.SetTempUser(ctx, cred.AccessKey, cred, ""); err != nil {
+		t.Fatalf("temporary credential rejected: %v", err)
+	}
+	err = saveIAMRevision(ctx, store.IAMStorageAPI, parentPath, &iamUserRevocation{})
+	if !iamConfigNameTooLong(parentPath, err) {
+		t.Fatalf("expected name-too-long error, got %v", err)
+	}
+}
+
+func TestIAMConfigNameTooLong(t *testing.T) {
+	long := iamConfigUsersPrefix + strings.Repeat("x", 256) + "/" + iamIdentityFile
+	short := iamConfigUsersPrefix + strings.Repeat("x", 200) + "/" + iamIdentityFile
+	invalid := iamConfigUsersPrefix + "a//b/" + iamIdentityFile
+	escaped := iamConfigUsersPrefix + strings.Repeat("\\d0", 86) + "/" + iamIdentityFile
+	limit := iamConfigUsersPrefix + strings.Repeat("x", 255) + "/" + iamIdentityFile
+	unicode := iamConfigUsersPrefix + strings.Repeat("界", 100) + "/" + iamIdentityFile
+	deep := iamConfigUsersPrefix + strings.Repeat("nested/", 160) + iamIdentityFile
+	for _, tc := range []struct {
+		name string
+		path string
+		err  error
+		want bool
+	}{
+		{"NAME_MAX", long, ObjectNameInvalid{Bucket: minioMetaBucket, Object: long}, true},
+		{"escaped DN", escaped, ObjectNameInvalid{Bucket: minioMetaBucket, Object: escaped}, runtime.GOOS != "windows"},
+		{"short component", short, ObjectNameInvalid{Bucket: minioMetaBucket, Object: short}, false},
+		{"exact component limit", limit, ObjectNameInvalid{Bucket: minioMetaBucket, Object: limit}, false},
+		{"unicode byte length", unicode, ObjectNameInvalid{Bucket: minioMetaBucket, Object: unicode}, false},
+		{"total path length", deep, ObjectNameInvalid{Bucket: minioMetaBucket, Object: deep}, false},
+		{"short drive path", short, errFileNameTooLong, false},
+		{"policy suffix at limit", getMappedPolicyPath(strings.Repeat("x", 250), stsUser, false), errFileNameTooLong, false},
+		{"policy suffix exceeds limit", getMappedPolicyPath(strings.Repeat("x", 251), stsUser, false), errFileNameTooLong, true},
+		{"drive", long, errFileNameTooLong, true},
+		{"wrapped drive", long, fmt.Errorf("read: %w", errFileNameTooLong), true},
+		{"wrapped object", long, fmt.Errorf("read: %w", ObjectNameInvalid{Bucket: minioMetaBucket, Object: long}), true},
+		{"no error", long, nil, false},
+		{"not found", long, errConfigNotFound, false},
+		{"offline drive", long, errDiskNotFound, false},
+		{"read quorum", long, InsufficientReadQuorum{}, false},
+		{"access denied", long, PrefixAccessDenied{}, false},
+		{"deadline", long, context.DeadlineExceeded, false},
+		{"invalid name", invalid, ObjectNameInvalid{Bucket: minioMetaBucket, Object: invalid}, false},
+		{"invalid long name", invalid + strings.Repeat("x", 256), errFileNameTooLong, false},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := iamConfigNameTooLong(tc.path, tc.err); got != tc.want {
+				t.Errorf("got %t, want %t", got, tc.want)
+			}
+		})
+	}
+}
+
+// Moving a drive can make an existing record exceed the total path limit.
+// A length error must not turn that parent's revocation into a missing record.
+func TestLoadIAMRevisionDeepPath(t *testing.T) {
+	if runtime.GOOS != "darwin" {
+		t.Skip("exercises the Darwin total-path limit")
+	}
+	for _, tc := range []struct {
+		name   string
+		parent string
+	}{
+		{"ASCII", strings.Repeat("p", 200)},
+		{"Unicode", strings.Repeat("界", 100)},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			resetTestGlobals()
+			defer resetTestGlobals()
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+			base := t.TempDir()
+			oldRoot := filepath.Join(base, "drive")
+			mustIAM(t, os.Mkdir(oldRoot, 0o700))
+			obj, _, err := initObjectLayer(ctx, mustGetPoolEndpoints(0, oldRoot))
+			mustIAM(t, err)
+			defer func() { obj.Shutdown(ctx) }()
+			store := newIAMObjectStore(obj, LDAPUsersSysType)
+			cred := auth.Credentials{ParentUser: tc.parent}
+			parentPath := getUserIdentityPath(tc.parent, regUser)
+			mustIAM(t, store.saveIAMConfig(ctx, &UserIdentity{Version: 1, Deleted: true, RevokedBefore: UTCNow()}, parentPath))
+			if err := checkIAMParentRevision(ctx, store, cred); !errors.Is(err, errIAMStaleUpdate) {
+				t.Fatalf("original parent revocation ineffective: %v", err)
+			}
+			mustIAM(t, obj.Shutdown(ctx))
+			deep := base
+			for len(deep) < 850 {
+				deep = filepath.Join(deep, strings.Repeat("d", 80))
+			}
+			mustIAM(t, os.MkdirAll(deep, 0o700))
+			newRoot := filepath.Join(deep, "drive")
+			mustIAM(t, checkPathLength(newRoot))
+			mustIAM(t, os.Rename(oldRoot, newRoot))
+			// The record still exists; a relative read avoids the total-path limit.
+			root, err := os.OpenRoot(newRoot)
+			mustIAM(t, err)
+			defer root.Close()
+			data, err := root.ReadFile(filepath.Join(minioMetaBucket, parentPath, xlStorageFormatFile))
+			mustIAM(t, err)
+			if len(data) == 0 {
+				t.Fatal("missing physical revocation record")
+			}
+			obj, _, err = initObjectLayer(ctx, mustGetPoolEndpoints(0, newRoot))
+			mustIAM(t, err)
+			store = newIAMObjectStore(obj, LDAPUsersSysType)
+			var invalid ObjectNameInvalid
+			if err := checkIAMParentRevision(ctx, store, cred); !errors.As(err, &invalid) {
+				t.Fatalf("expected path error for an existing parent revocation, got %v", err)
+			}
+		})
+	}
+}
+
 // Run with both object storage and etcd through TestIAMRevocation*Lifecycle.
 // The object-store case uses the real peer RPC and deletion handler, so a
 // spurious notification actually destroys the parent instead of only counting it.
@@ -36,6 +36,7 @@ import (
 	"testing"
 	"time"

+	goldap "github.com/go-ldap/ldap/v3"
 	"github.com/klauspost/compress/zip"
 	"github.com/minio/madmin-go/v3"
 	"github.com/minio/minio-go/v7"
@@ -1095,6 +1096,7 @@ func TestIAMWithLDAPServerSuite(t *testing.T) {
 				suite.TestLDAPSTSServiceAccountsWithGroups(c)
 				suite.TestLDAPAttributesLookup(c)
 				suite.TestLDAPCyrillicUser(c)
+				suite.TestLDAPLongNonASCIIDN(c)
 				suite.TestLDAPSlashDN(c)
 				suite.TearDownSuite(c)
 			},
@@ -3291,6 +3293,112 @@ func (s *TestSuiteIAM) TestLDAPCyrillicUser(c *check) {
 	}
 }

+// Normalized DNs hex-escape every non-ASCII byte, so a user in nested Cyrillic
+// OUs has a DN longer than NAME_MAX: 160 bytes of UTF-8 become 384 characters.
+// Records named after such a DN can never be stored by the object backend.
+// The user is entitled only through a group, as is typical for AD deployments.
+func (s *TestSuiteIAM) TestLDAPLongNonASCIIDN(c *check) {
+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+	defer cancel()
+
+	const (
+		username = "Сотрудник"
+		parentOU = "ou=Дирекция информационных технологий,ou=people,ou=swengg,dc=min,dc=io"
+		childOU  = "ou=Отдел разработки," + parentOU
+		userDN   = "uid=" + username + "," + childOU
+		groupDN  = "cn=long-dn-staff,ou=groups,ou=swengg,dc=min,dc=io"
+	)
+	normDN, err := ldap.NormalizeDN(userDN)
+	if err != nil {
+		c.Fatal(err)
+	}
+	if len(normDN) <= 255 {
+		c.Fatalf("normalized test DN must exceed 255 characters, got %d", len(normDN))
+	}
+
+	conn, err := globalIAMSys.LDAPConfig.LDAP.Connect()
+	if err != nil {
+		c.Fatalf("LDAP connect failed: %v", err)
+	}
+	defer conn.Close()
+	if err = conn.Bind("cn=admin,dc=min,dc=io", "admin"); err != nil {
+		c.Fatalf("LDAP admin bind failed: %v", err)
+	}
+	newEntry := func(dn string, attrs map[string]string) *goldap.AddRequest {
+		req := goldap.NewAddRequest(dn, nil)
+		for name, value := range attrs {
+			req.Attribute(name, []string{value})
+		}
+		return req
+	}
+	entries := []*goldap.AddRequest{
+		newEntry(parentOU, map[string]string{"objectClass": "organizationalUnit", "ou": "Дирекция информационных технологий"}),
+		newEntry(childOU, map[string]string{"objectClass": "organizationalUnit", "ou": "Отдел разработки"}),
+		newEntry(userDN, map[string]string{"objectClass": "inetOrgPerson", "uid": username, "cn": username, "sn": username, "userPassword": "example"}),
+		newEntry(groupDN, map[string]string{"objectClass": "groupOfNames", "cn": "long-dn-staff", "member": userDN}),
+	}
+	removeEntries := func() {
+		for i := len(entries) - 1; i >= 0; i-- {
+			err := conn.Del(goldap.NewDelRequest(entries[i].DN, nil))
+			if err != nil && !goldap.IsErrorWithCode(err, goldap.LDAPResultNoSuchObject) {
+				c.Errorf("Unable to remove LDAP entry %s: %v", entries[i].DN, err)
+			}
+		}
+	}
+	removeEntries() // left over from an interrupted run
+	defer removeEntries()
+	for _, entry := range entries {
+		if err = conn.Add(entry); err != nil {
+			c.Fatalf("Unable to add LDAP entry %s: %v", entry.DN, err)
+		}
+	}
+
+	groupReq := madmin.PolicyAssociationReq{
+		Policies: []string{"readwrite"},
+		Group:    groupDN,
+	}
+	if _, err = s.adm.AttachPolicyLDAP(ctx, groupReq); err != nil {
+		c.Fatalf("Unable to attach group policy: %v", err)
+	}
+	defer func() {
+		if _, err := s.adm.DetachPolicyLDAP(ctx, groupReq); err != nil {
+			c.Errorf("Unable to detach group policy: %v", err)
+		}
+	}()
+
+	bucket := getRandomBucketName()
+	if err = s.client.MakeBucket(ctx, bucket, minio.MakeBucketOptions{}); err != nil {
+		c.Fatalf("bucket create error: %v", err)
+	}
+
+	ldapID := cr.LDAPIdentity{
+		Client:       s.TestSuiteCommon.client,
+		STSEndpoint:  s.endPoint,
+		LDAPUsername: username,
+		LDAPPassword: "example",
+	}
+	value, err := ldapID.Retrieve()
+	if err != nil {
+		c.Fatalf("Expected to generate STS creds, got err: %#v", err)
+	}
+	u, ok := globalIAMSys.GetUser(ctx, value.AccessKeyID)
+	if !ok {
+		c.Fatalf("Expected to find user %s", value.AccessKeyID)
+	}
+	if u.Credentials.ParentUser != normDN {
+		c.Fatalf("unexpected parent user: %s", u.Credentials.ParentUser)
+	}
+	c.mustListObjects(ctx, s.getUserClient(c, value.AccessKeyID, value.SecretAccessKey, value.SessionToken), bucket)
+
+	// An access key created by this user inherits the same parent.
+	userAdmClient := s.getAdminClient(c, value.AccessKeyID, value.SecretAccessKey, value.SessionToken)
+	svc := c.mustCreateSvcAccount(ctx, value.AccessKeyID, userAdmClient)
+	c.assertSvcAccS3Access(ctx, s, svc, bucket)
+	if err = s.adm.DeleteServiceAccount(ctx, svc.AccessKey); err != nil {
+		c.Fatalf("Unable to delete service account: %v", err)
+	}
+}
+
 func (s *TestSuiteIAM) TestLDAPSlashDN(c *check) {
 	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
 	defer cancel()