From b3760f56ec233fef5d1a456b75f6a96052ba79a7 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Thu, 10 Sep 2026 14:58:59 +0800 Subject: [PATCH 1/2] fix(policy): separate self-service password permissions Adapt minio/pkg #262 and #233: use ChangeMyPassword for self service, require explicit CreateUser grants, and add consolereadonly. Preserve legacy-policy parsing and cover explicit deny combinations. Pin minio-go to upstream 78bfa91607c2 and record the upstream review. Signed-off-by: Feng Ruohang --- UPSTREAM.md | 47 +++++++++++++++ go.mod | 2 +- go.sum | 4 +- policy/constants.go | 14 ++++- policy/password_test.go | 100 +++++++++++++++++++++++++++++++ policy/policy.go | 5 +- policy/policy_regression_test.go | 12 ++-- 7 files changed, 170 insertions(+), 14 deletions(-) create mode 100644 UPSTREAM.md create mode 100644 policy/password_test.go diff --git a/UPSTREAM.md b/UPSTREAM.md new file mode 100644 index 0000000..d1a2563 --- /dev/null +++ b/UPSTREAM.md @@ -0,0 +1,47 @@ +# Upstream review: 2026-09-10 + +Reviewed `minio/pkg` through +[`657d87e88168`](https://github.com/minio/pkg/commit/657d87e881680db68e3a9f9286569788651c968e) +and `minio/minio-go` through +[`78bfa91607c2`](https://github.com/minio/minio-go/commit/78bfa91607c2b9a7eb1ad5fe739de3c6c238d5ab). + +## Adopted + +- [pkg #262](https://github.com/minio/pkg/pull/262): report + `admin:ChangeMyPassword` as an implicit self-service capability, subject to + explicit Deny. `admin:CreateUser` requires an explicit Allow. Remove the + legacy CreateUser deny from the built-in read-only policy. +- [pkg #233](https://github.com/minio/pkg/pull/233): add `consolereadonly`, + with GetObject, GetBucketLocation and ListBucket. Apply #262 to this new + policy too. The original `readonly` S3 permissions remain unchanged. +- Pin the upstream SDK to `v7.3.1-0.20260909183557-78bfa91607c2`. Relative to + the previous `0e78d3f18efe` pin, it contains configurable upload limits + ([#2299](https://github.com/minio/minio-go/pull/2299)), Content-Type in + streaming SignedHeaders ([#2301](https://github.com/minio/minio-go/pull/2301)) + and caller TLS trust on RDMA ([#2302](https://github.com/minio/minio-go/pull/2302)). + +Password authorization must be deployed with the matching SILO Server and +Console changes. See the Server's `docs/iam/password-permissions.md` for old +policy behavior and migration. No public API or Go compatibility floor changes +are required by these package changes. + +## Already covered or deferred + +| Upstream work | SILO decision | +| --- | --- | +| #265, x/crypto v0.56.0 | Already selected; no additional version bump. | +| #230, RNG subkey initialization | Already fixed locally. | +| #226, exact condition key lookup | Already fixed locally. | +| #242, xtime.Duration JSON marshaling | Already implemented locally. | +| #261, NotResource deduplication, literal-policy Deny detection and wildcard backtracking | Correctness fixes already present in SILO v3.13.3, including condition-value boundary tests; retain them. The remaining indexing and classification refactor is a separate optimization. | +| #249–252, ARN and Memory resource changes | Keep SILO's strict administration writes and compatible historical-policy parsing; do not replace them with the AIStor Memory model. | +| #245–246, typed action API | Breaking source changes without a current SILO requirement; defer. | +| Memory, S3Tables, annotation, compression and new closed-product admin actions | No matching maintained SILO endpoints in this update; defer. | +| #263, bounded deduplicating channel | New helper with no current SILO consumer; defer. | +| Certificate-cache accessors, bandwidth configuration and RNG assembly | Independent features or optimizations; defer until a consumer or measured need justifies them. | + +[minio-go #2274](https://github.com/minio/minio-go/pull/2274), trimming bucket +location whitespace, is still open at this review. It is absent from the pinned +commit. Take it through a later upstream commit after merge; do not revive the +retired silo-go fork for this other-S3 compatibility issue. It does not block a +SILO release. diff --git a/go.mod b/go.mod index e1ed10b..0d18cac 100644 --- a/go.mod +++ b/go.mod @@ -22,7 +22,7 @@ require ( github.com/lestrrat-go/jwx/v3 v3.2.0 github.com/mattn/go-colorable v0.1.15 github.com/mattn/go-isatty v0.0.24 - github.com/minio/minio-go/v7 v7.3.1-0.20260828014306-0e78d3f18efe + github.com/minio/minio-go/v7 v7.3.1-0.20260909183557-78bfa91607c2 github.com/minio/mux v1.10.1 github.com/rjeczalik/notify v0.9.3 github.com/tinylib/msgp v1.6.4 diff --git a/go.sum b/go.sum index dbbf29f..6343d66 100644 --- a/go.sum +++ b/go.sum @@ -97,8 +97,8 @@ github.com/mattn/go-runewidth v0.0.29 h1:3oGF3R/S2N9DQ3ptftzVIvg2eicmojCzlwBEmqE github.com/mattn/go-runewidth v0.0.29/go.mod h1:3qAiGCV4Koz/yuveO58qUefmUTRm8r0IGEXZ9jeHp/8= github.com/minio/md5-simd v1.1.2 h1:Gdi1DZK69+ZVMoNHRXJyNcxrMA4dSxoYHZSQbirFg34= github.com/minio/md5-simd v1.1.2/go.mod h1:MzdKDxYpY2BT9XQFocsiZf/NKVtR7nkE4RoEpN+20RM= -github.com/minio/minio-go/v7 v7.3.1-0.20260828014306-0e78d3f18efe h1:By2FKNSOUGLOeb0x4D7xJMHr8x/X1ZW8PG780SpKUwQ= -github.com/minio/minio-go/v7 v7.3.1-0.20260828014306-0e78d3f18efe/go.mod h1:KUPWdecEO1LWyUz+sTGXAuf2jZHrPh5fCsRH86QbPfk= +github.com/minio/minio-go/v7 v7.3.1-0.20260909183557-78bfa91607c2 h1:nvX7IksPFOF/cBvkSg/Z+urBZkmhZrfUVmzdNFLjZ5Y= +github.com/minio/minio-go/v7 v7.3.1-0.20260909183557-78bfa91607c2/go.mod h1:KUPWdecEO1LWyUz+sTGXAuf2jZHrPh5fCsRH86QbPfk= github.com/minio/mux v1.10.1 h1:grrK8SwRKbkNFE6qG7WAvFGH09bB46d5teOOtKfQ14s= github.com/minio/mux v1.10.1/go.mod h1:INYT4sMSTJy0QWUEA/E2DZNxJ5sAxIwbnyZjkzNFRfE= github.com/philhofer/fwd v1.2.0 h1:e6DnBTl7vGY+Gz322/ASL4Gyp1FspeMvx1RNDoToZuM= diff --git a/policy/constants.go b/policy/constants.go index 3e64846..6a07f57 100644 --- a/policy/constants.go +++ b/policy/constants.go @@ -60,10 +60,20 @@ var DefaultPolicies = []struct { Actions: NewActionSet(GetBucketLocationAction, GetObjectAction), Resources: NewResourceSet(NewResource("*")), }, + }, + }, + }, + + // ConsoleReadOnly - read only with ListBucket for console browsing. + { + Name: "consolereadonly", + Definition: Policy{ + Version: DefaultVersion, + Statements: []Statement{ { SID: ID(""), - Effect: Deny, - Actions: NewActionSet(CreateUserAdminAction), + Effect: Allow, + Actions: NewActionSet(GetBucketLocationAction, GetObjectAction, ListBucketAction), Resources: NewResourceSet(NewResource("*")), }, }, diff --git a/policy/password_test.go b/policy/password_test.go new file mode 100644 index 0000000..be83c62 --- /dev/null +++ b/policy/password_test.go @@ -0,0 +1,100 @@ +// Copyright (c) 2026 Feng Ruohang +// SPDX-License-Identifier: AGPL-3.0-or-later + +package policy + +import ( + "strings" + "testing" +) + +func TestPasswordAndUserManagementActions(t *testing.T) { + for _, tt := range []struct { + name string + statements string + password bool + createUser bool + }{ + {"implicit self service", `{"Effect":"Allow","Action":"s3:GetObject","Resource":"arn:aws:s3:::*"}`, true, false}, + {"explicit password grant", `{"Effect":"Allow","Action":"admin:ChangeMyPassword"}`, true, false}, + {"explicit user management grant", `{"Effect":"Allow","Action":"admin:CreateUser"}`, true, true}, + {"legacy user management deny", `{"Effect":"Deny","Action":"admin:CreateUser","Resource":"arn:aws:s3:::*"}`, true, false}, + {"password deny", `{"Effect":"Deny","Action":"admin:ChangeMyPassword"}`, false, false}, + {"password deny with user management grant", `{"Effect":"Deny","Action":"admin:ChangeMyPassword"},{"Effect":"Allow","Action":"admin:CreateUser"}`, false, true}, + {"deny overrides allow", `{"Effect":"Allow","Action":"admin:ChangeMyPassword"},{"Effect":"Deny","Action":"admin:ChangeMyPassword"}`, false, false}, + {"wildcard deny", `{"Effect":"Deny","Action":"admin:*"}`, false, false}, + {"both denies", `{"Effect":"Deny","Action":["admin:CreateUser","admin:ChangeMyPassword"]}`, false, false}, + } { + t.Run(tt.name, func(t *testing.T) { + doc := `{"Version":"2012-10-17","Statement":[` + tt.statements + `]}` + for _, mode := range []string{"read", "write", "merge"} { + t.Run(mode, func(t *testing.T) { + parser := ParseConfig + if mode == "write" { + parser = ParseConfigStrict + } + p, err := parser(strings.NewReader(doc)) + if err != nil { + t.Fatal(err) + } + if mode == "merge" { + merged := MergePolicies(*p, Policy{}) + p = &merged + } + actions := p.IsAllowedActions("", "", nil) + for action, want := range map[Action]bool{ + ChangeMyPasswordAdminAction: tt.password, + CreateUserAdminAction: tt.createUser, + } { + if got := actions.Contains(action); got != want { + t.Errorf("reported %s = %v, want %v", action, got, want) + } + if got := p.IsAllowed(Args{Action: action, DenyOnly: action == ChangeMyPasswordAdminAction}); got != want { + t.Errorf("authorized %s = %v, want %v", action, got, want) + } + } + }) + } + }) + } +} + +func TestReadOnlySelfServicePolicies(t *testing.T) { + for _, name := range []string{"readonly", "consolereadonly"} { + t.Run(name, func(t *testing.T) { + var p *Policy + for _, canned := range DefaultPolicies { + if canned.Name == name { + p = &canned.Definition + break + } + } + if p == nil { + t.Fatal("missing canned policy") + } + actions := p.IsAllowedActions("bucket", "object", nil) + for action, want := range map[Action]bool{ + GetObjectAction: true, + GetBucketLocationAction: true, + ListBucketAction: name == "consolereadonly", + PutObjectAction: false, + DeleteObjectAction: false, + CreateUserAdminAction: false, + ChangeMyPasswordAdminAction: true, + CreateServiceAccountAdminAction: true, + } { + if got := actions.Contains(action); got != want { + t.Errorf("%s = %v, want %v", action, got, want) + } + } + // A read-only grant must compose with a separate user-admin policy. + userAdmin := Policy{Version: DefaultVersion, Statements: []Statement{{ + Effect: Allow, Actions: NewActionSet(CreateUserAdminAction), + }}} + merged := MergePolicies(*p, userAdmin) + if !merged.IsAllowed(Args{Action: CreateUserAdminAction}) { + t.Error("read-only policy overrides an independent user-management grant") + } + }) + } +} diff --git a/policy/policy.go b/policy/policy.go index c4e3f60..556ab4b 100644 --- a/policy/policy.go +++ b/policy/policy.go @@ -169,9 +169,8 @@ func (iamp Policy) IsAllowedActions(bucketName, objectName string, conditionValu ObjectName: objectName, Action: admAction, ConditionValues: conditionValues, - // checks mainly for actions that can have explicit - // deny, while without it are implicitly enabled. - DenyOnly: action == CreateServiceAccountAdminAction || action == CreateUserAdminAction, + // Match the server's implicit grants for self-service actions. + DenyOnly: action == CreateServiceAccountAdminAction || action == ChangeMyPasswordAdminAction, }) { actionSet.Add(admAction) } diff --git a/policy/policy_regression_test.go b/policy/policy_regression_test.go index 0a5f884..3ed50a2 100644 --- a/policy/policy_regression_test.go +++ b/policy/policy_regression_test.go @@ -162,12 +162,12 @@ func TestHasDenyStatementWithoutParsing(t *testing.T) { }) } for _, p := range DefaultPolicies { - if p.Name == "readonly" { - if !p.Definition.HasDenyStatement() { - t.Error("readonly's explicit Deny must be reported before parsing") - } - return + want := false + for _, statement := range p.Definition.Statements { + want = want || statement.Effect == Deny + } + if got := p.Definition.HasDenyStatement(); got != want { + t.Errorf("%s: HasDenyStatement() = %v, want %v", p.Name, got, want) } } - t.Fatal("readonly policy not found") } From abf03e6b42f6408ca6e6ea55ff8e6575e9839204 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Thu, 10 Sep 2026 17:00:01 +0800 Subject: [PATCH 2/2] docs(policy): disclose breaking password authorization changes Signed-off-by: Feng Ruohang --- UPSTREAM.md | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/UPSTREAM.md b/UPSTREAM.md index d1a2563..b3d559b 100644 --- a/UPSTREAM.md +++ b/UPSTREAM.md @@ -20,10 +20,36 @@ and `minio/minio-go` through streaming SignedHeaders ([#2301](https://github.com/minio/minio-go/pull/2301)) and caller TLS trust on RDMA ([#2302](https://github.com/minio/minio-go/pull/2302)). -Password authorization must be deployed with the matching SILO Server and -Console changes. See the Server's `docs/iam/password-permissions.md` for old -policy behavior and migration. No public API or Go compatibility floor changes -are required by these package changes. +## Breaking authorization compatibility + +Adopting #262 changes existing policy semantics; it is independent of updating +the minio-go SDK. This must be disclosed as a breaking change in the release +that includes it. The Go signatures and Go compatibility floor are unchanged, +but the public `Policy.IsAllowedActions` method returns different capabilities: +ChangeMyPassword is implicit unless denied, and CreateUser requires an explicit +Allow. Consumers must use the matching capability for each operation. + +With the matching Server change, a saved `Deny admin:CreateUser` no longer +prevents the caller from changing their own password. A saved +`Deny admin:ChangeMyPassword` now prevents it. To preserve a policy's old +combined restriction, add ChangeMyPassword to the same CreateUser Deny statement +before upgrading, preserving its other actions, scope and conditions. Saved +policy documents are not rewritten automatically. + +The built-in `readonly` policy also drops its old CreateUser deny. It now allows +self-service password changes, and a separate CreateUser Allow can grant user +administration where the old built-in deny overrode it. Saved overrides retain +their existing deny; inspect the effective policy contents. `consolereadonly` +is new and follows the split. Neither read-only policy grants user +administration on its own. + +Deploy password authorization with the matching SILO Server and Console. +During a mixed-version rollout or rollback, retain both denies if the old +combined restriction must hold: an old Server does not enforce a password-only +deny for this endpoint. See the Server's +[password-permission migration guide](https://github.com/pgsty/silo/blob/420340bc142b7dec00c26c28dd78102e3ed9d0f3/docs/iam/password-permissions.md) +for the before/after matrix, policy migration, read-only composition and +rollback limits. ## Already covered or deferred