diff --git a/cmd/iam-object-store.go b/cmd/iam-object-store.go index c3ece5a2b..946e62eb1 100644 --- a/cmd/iam-object-store.go +++ b/cmd/iam-object-store.go @@ -134,9 +134,30 @@ func decryptData(data []byte, objPath string) ([]byte, error) { return nil, err } +// iamConfigNameTooLong reports whether the drives rejected objPath for its +// length. IAM records are named after external identities: a normalized LDAP +// DN hex-escapes every non-ASCII byte, so one Cyrillic letter takes six +// characters. Such a record cannot have been stored. checkPathLength does not +// count backslashes outside Windows, so an escaped DN can pass it and still +// fail on the filesystem with ENAMETOOLONG once its parent directory exists. +// toObjectErr reports both as ObjectNameInvalid, as it does for names that the +// object layer rejects itself; repeat that validation to tell them apart. +func iamConfigNameTooLong(objPath string, err error) bool { + if errors.Is(err, errFileNameTooLong) { + return true + } + var invalid ObjectNameInvalid + return errors.As(err, &invalid) && checkBucketAndObjectNames(context.Background(), minioMetaBucket, objPath) == nil +} + 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..e605a0296 100644 --- a/cmd/iam-revision_test.go +++ b/cmd/iam-revision_test.go @@ -14,7 +14,9 @@ import ( "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 +95,133 @@ 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 + for _, tc := range []struct { + name string + path string + err error + want bool + }{ + {"NAME_MAX", long, ObjectNameInvalid{Bucket: minioMetaBucket, Object: long}, true}, + {"filesystem", short, ObjectNameInvalid{Bucket: minioMetaBucket, Object: short}, true}, + {"drive", long, errFileNameTooLong, true}, + {"no error", long, nil, false}, + {"not found", long, errConfigNotFound, false}, + {"offline drive", long, errDiskNotFound, false}, + {"read quorum", long, InsufficientReadQuorum{}, false}, + {"invalid name", invalid, ObjectNameInvalid{Bucket: minioMetaBucket, Object: invalid}, false}, + } { + if got := iamConfigNameTooLong(tc.path, tc.err); got != tc.want { + t.Errorf("%s: got %t, want %t", tc.name, got, tc.want) + } + } +} + // 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()