Merge pull request #250 from 910patin/fix/ldap-long-dn

Restore long-DN LDAP login while preserving IAM revision read errors. Retain the original contribution by @910patin, add the bounded ASCII component guard and regression tests, and require the patched Go 1.27.3 toolchain. DCO, Go CI, VulnCheck and release snapshot validation passed on the final PR head. Signed-off-by: Feng Ruohang <rh@vonng.com>
2026-10-26 05:39:56 +00:00 · 2026-10-23 11:21:50 +08:00
parent 856f8f4c55 3df51549bb
commit d43adc0f2b
5 changed files with 351 additions and 1 deletions
@@ -2,6 +2,13 @@

 ## 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).
+- Require Go 1.27.3 to include the crypto/x509 fix for GO-2026-6809.
+
 Preparation target: `RELEASE.2026-10-18T00-00-00Z` (package version
 `20261018000000.0.0`). **This is a tag and Draft preparation; it is not a public
 Server release.** The latest published Server remains 20260916. Server 20261015
@@ -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()
@@ -1,6 +1,6 @@
 module github.com/minio/minio

-go 1.27.2
+go 1.27.3

 // Console and MC retain their historical module paths for best-effort upstream
 // compatibility. Pin the maintained PGSTY implementations used by SILO.