From 207e03181196e7e666d5ce7c68654b70f6118407 Mon Sep 17 00:00:00 2001
From: Chris Lu <chris.lu@gmail.com>
Date: Sun, 11 Jan 2026 20:31:49 -0800
Subject: [PATCH] fix: address PR feedback (Round 4) - LDAP hardening, Authz
 check & Routing fix

- LDAP Provider Hardening:
  - Prevent re-initialization
  - Enforce single user match in `GetUserInfo` (was explicit only in Authenticate)
  - Ensure connection closure if StartTLS fails
- STS Handlers:
  - Add robust provider detection using type assertion
  - **Security**: Implement authorization check (`VerifyActionPermission`) after LDAP authentication
- Routing:
  - Update tests to reflect that STS actions are handled by STS handler, not generic IAM
---
 weed/iam/ldap/ldap_provider.go          | 12 +++++++++-
 weed/s3api/s3api_server_routing_test.go |  4 ++--
 weed/s3api/s3api_sts.go                 | 30 ++++++++++++++++++++-----
 3 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/weed/iam/ldap/ldap_provider.go b/weed/iam/ldap/ldap_provider.go
index 0b67e5f7b..d3eb69c27 100644
--- a/weed/iam/ldap/ldap_provider.go
+++ b/weed/iam/ldap/ldap_provider.go
@@ -85,6 +85,10 @@ func (p *LDAPProvider) Initialize(config interface{}) error {
 	p.mu.Lock()
 	defer p.mu.Unlock()
 
+	if p.initialized {
+		return fmt.Errorf("LDAP provider already initialized")
+	}
+
 	cfg, ok := config.(*LDAPConfig)
 	if !ok {
 		// Try to convert from map
@@ -220,7 +224,10 @@ func (p *LDAPProvider) connect() (*ldap.Conn, error) {
 				InsecureSkipVerify: p.config.InsecureSkipVerify,
 				MinVersion:         tls.VersionTLS12,
 			}
-			err = conn.StartTLS(tlsConfig)
+			if err = conn.StartTLS(tlsConfig); err != nil {
+				conn.Close()
+				return nil, fmt.Errorf("failed to start TLS: %w", err)
+			}
 		}
 	}
 
@@ -410,6 +417,9 @@ func (p *LDAPProvider) GetUserInfo(ctx context.Context, userID string) (*provide
 	if len(result.Entries) == 0 {
 		return nil, fmt.Errorf("user not found")
 	}
+	if len(result.Entries) > 1 {
+		return nil, fmt.Errorf("multiple users found")
+	}
 
 	userEntry := result.Entries[0]
 	identity := &providers.ExternalIdentity{
diff --git a/weed/s3api/s3api_server_routing_test.go b/weed/s3api/s3api_server_routing_test.go
index 5aed24d39..2746d59fe 100644
--- a/weed/s3api/s3api_server_routing_test.go
+++ b/weed/s3api/s3api_server_routing_test.go
@@ -150,8 +150,8 @@ func TestRouting_IAMMatcherLogic(t *testing.T) {
 			name:        "AWS4 signature with STS action in body",
 			authHeader:  "AWS4-HMAC-SHA256 Credential=AKIA.../...",
 			queryParams: "",
-			expectsIAM:  true,
-			description: "Authenticated STS action should still route to IAM (auth takes precedence)",
+			expectsIAM:  false,
+			description: "Authenticated STS action should route to STS handler (STS handlers handle their own auth)",
 		},
 	}
 
diff --git a/weed/s3api/s3api_sts.go b/weed/s3api/s3api_sts.go
index 77b21c8cb..20f02ff9c 100644
--- a/weed/s3api/s3api_sts.go
+++ b/weed/s3api/s3api_sts.go
@@ -16,7 +16,7 @@ import (
 	"time"
 
 	"github.com/seaweedfs/seaweedfs/weed/glog"
-	"github.com/seaweedfs/seaweedfs/weed/iam/providers"
+	"github.com/seaweedfs/seaweedfs/weed/iam/ldap"
 	"github.com/seaweedfs/seaweedfs/weed/iam/sts"
 	"github.com/seaweedfs/seaweedfs/weed/s3api/s3err"
 )
@@ -386,11 +386,12 @@ func (h *STSHandlers) handleAssumeRoleWithLDAPIdentity(w http.ResponseWriter, r
 	}
 
 	// Find an LDAP provider from the registered providers
-	var ldapProvider providers.IdentityProvider
+	// Find an LDAP provider from the registered providers
+	var ldapProvider *ldap.LDAPProvider
 	for _, provider := range h.stsService.GetProviders() {
-		// Check if this is an LDAP provider by looking at the name or type
-		if strings.Contains(strings.ToLower(provider.Name()), "ldap") {
-			ldapProvider = provider
+		// Check if this is an LDAP provider by type assertion
+		if p, ok := provider.(*ldap.LDAPProvider); ok {
+			ldapProvider = p
 			break
 		}
 	}
@@ -416,6 +417,25 @@ func (h *STSHandlers) handleAssumeRoleWithLDAPIdentity(w http.ResponseWriter, r
 	glog.V(2).Infof("AssumeRoleWithLDAPIdentity: user %s authenticated successfully, groups=%v",
 		ldapUsername, identity.Groups)
 
+	// Verify that the identity is allowed to assume the role
+	// We create a temporary identity to represent the LDAP user for permission checking
+	// The checking logic will verify if the role's trust policy allows this principal
+	ldapUserIdentity := &Identity{
+		Name: identity.UserID,
+		Account: &Account{
+			DisplayName:  identity.DisplayName,
+			EmailAddress: identity.Email,
+			Id:           identity.UserID,
+		},
+		PrincipalArn: fmt.Sprintf("arn:aws:iam::%s:user/%s", "aws", identity.UserID),
+	}
+
+	if authErr := h.iam.VerifyActionPermission(r, ldapUserIdentity, Action("sts:AssumeRole"), "", roleArn); authErr != s3err.ErrNone {
+		glog.V(2).Infof("AssumeRoleWithLDAPIdentity: authorization failed for %s to assume %s: %v", ldapUsername, roleArn, authErr)
+		h.writeSTSErrorResponse(w, r, STSErrAccessDenied, fmt.Errorf("access denied"))
+		return
+	}
+
 	// Calculate duration
 	duration := time.Hour // Default 1 hour
 	if durationSeconds != nil {