diff --git a/CHANGELOG.md b/CHANGELOG.md index 98cd5a52b..1f872017f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/cmd/iam-object-store.go b/cmd/iam-object-store.go index c3ece5a2b..c5ede946e 100644 --- a/cmd/iam-object-store.go +++ b/cmd/iam-object-store.go @@ -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) diff --git a/cmd/iam-revision_test.go b/cmd/iam-revision_test.go index 70f5d71a5..b2209d1bb 100644 --- a/cmd/iam-revision_test.go +++ b/cmd/iam-revision_test.go @@ -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/.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. diff --git a/cmd/sts-handlers_test.go b/cmd/sts-handlers_test.go index dc58dcc69..a0c93a29f 100644 --- a/cmd/sts-handlers_test.go +++ b/cmd/sts-handlers_test.go @@ -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() diff --git a/go.mod b/go.mod index 331eb4c01..48d549999 100644 --- a/go.mod +++ b/go.mod @@ -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.