Browse Source

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
pull/8003/head
Chris Lu 2 months ago
parent
commit
207e031811
  1. 12
      weed/iam/ldap/ldap_provider.go
  2. 4
      weed/s3api/s3api_server_routing_test.go
  3. 30
      weed/s3api/s3api_sts.go

12
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{

4
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)",
},
}

30
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 {

Loading…
Cancel
Save