From 62c3cdee75efbdc3133bef4322986368ae929e6b Mon Sep 17 00:00:00 2001 From: Aditya Manthramurthy Date: Thu, 25 Apr 2024 08:50:16 -0700 Subject: [PATCH] fix: IAM LDAP access key import bug (#19608) When importing access keys (i.e. service accounts) for LDAP accounts, we are requiring groups to exist under one of the configured group base DNs. This is not correct. This change fixes this by only checking for existence and storing the normalized form of the group DN - we do not return an error if the group is not under a base DN. Test is updated to illustrate an import failure that would happen without this change. --- cmd/admin-handlers-idp-ldap.go | 5 +-- cmd/admin-handlers-users.go | 5 ++- cmd/iam.go | 27 ++++++++------ cmd/site-replication.go | 5 ++- cmd/sts-handlers_test.go | 4 +- internal/config/identity/ldap/ldap.go | 54 +++++++++++++++++---------- 6 files changed, 62 insertions(+), 38 deletions(-) diff --git a/cmd/admin-handlers-idp-ldap.go b/cmd/admin-handlers-idp-ldap.go index b492904b8..efef1f0b4 100644 --- a/cmd/admin-handlers-idp-ldap.go +++ b/cmd/admin-handlers-idp-ldap.go @@ -222,9 +222,8 @@ func (a adminAPIHandlers) AddServiceAccountLDAP(w http.ResponseWriter, r *http.R err error ) - // If we are creating svc account for request sender, ensure - // that targetUser is a real user (i.e. not derived - // credentials). + // If we are creating svc account for request sender, ensure that targetUser + // is a real user (i.e. not derived credentials). if isSvcAccForRequestor { if requestorIsDerivedCredential { if requestorParentUser == "" { diff --git a/cmd/admin-handlers-users.go b/cmd/admin-handlers-users.go index 8fd5fbbfb..c524a5676 100644 --- a/cmd/admin-handlers-users.go +++ b/cmd/admin-handlers-users.go @@ -1632,9 +1632,10 @@ func (a adminAPIHandlers) SetPolicyForUserOrGroup(w http.ResponseWriter, r *http var err error if isGroup { var foundGroupDN string - if foundGroupDN, err = globalIAMSys.LDAPConfig.GetValidatedGroupDN(nil, entityName); err != nil { + var underBaseDN bool + if foundGroupDN, underBaseDN, err = globalIAMSys.LDAPConfig.GetValidatedGroupDN(nil, entityName); err != nil { iamLogIf(ctx, err) - } else if foundGroupDN == "" { + } else if foundGroupDN == "" || !underBaseDN { err = errNoSuchGroup } entityName = foundGroupDN diff --git a/cmd/iam.go b/cmd/iam.go index 84419314c..a4f918ccc 100644 --- a/cmd/iam.go +++ b/cmd/iam.go @@ -1503,12 +1503,14 @@ func (sys *IAMSys) NormalizeLDAPAccessKeypairs(ctx context.Context, accessKeyMap hasDiff := false - validatedParent, err := sys.LDAPConfig.GetValidatedUserDN(conn, parent) + // For the parent value, we require that the parent exists in the LDAP + // server and is under a configured base DN. + validatedParent, isUnderBaseDN, err := sys.LDAPConfig.GetValidatedUserDN(conn, parent) if err != nil { collectedErrors = append(collectedErrors, fmt.Errorf("could not validate `%s` exists in LDAP directory: %w", parent, err)) continue } - if validatedParent == "" { + if validatedParent == "" || !isUnderBaseDN { err := fmt.Errorf("DN `%s` was not found in the LDAP directory", parent) collectedErrors = append(collectedErrors, err) continue @@ -1518,9 +1520,11 @@ func (sys *IAMSys) NormalizeLDAPAccessKeypairs(ctx context.Context, accessKeyMap hasDiff = true } - var validatedGroups []string + var normalizedGroups []string for _, group := range groups { - validatedGroup, err := sys.LDAPConfig.GetValidatedGroupDN(conn, group) + // For a group, we store the normalized DN even if it not under a + // configured base DN. + validatedGroup, _, err := sys.LDAPConfig.GetValidatedGroupDN(conn, group) if err != nil { collectedErrors = append(collectedErrors, fmt.Errorf("could not validate `%s` exists in LDAP directory: %w", group, err)) continue @@ -1534,13 +1538,13 @@ func (sys *IAMSys) NormalizeLDAPAccessKeypairs(ctx context.Context, accessKeyMap if validatedGroup != group { hasDiff = true } - validatedGroups = append(validatedGroups, validatedGroup) + normalizedGroups = append(normalizedGroups, validatedGroup) } if hasDiff { updatedCreateReq := createReq updatedCreateReq.Parent = validatedParent - updatedCreateReq.Groups = validatedGroups + updatedCreateReq.Groups = normalizedGroups updatedKeysMap[ak] = updatedCreateReq } @@ -1611,7 +1615,7 @@ func (sys *IAMSys) NormalizeLDAPMappingImport(ctx context.Context, isGroup bool, // We map keys that correspond to LDAP DNs and validate that they exist in // the LDAP server. - var dnValidator func(*libldap.Conn, string) (string, error) = sys.LDAPConfig.GetValidatedUserDN + var dnValidator func(*libldap.Conn, string) (string, bool, error) = sys.LDAPConfig.GetValidatedUserDN if isGroup { dnValidator = sys.LDAPConfig.GetValidatedGroupDN } @@ -1625,12 +1629,12 @@ func (sys *IAMSys) NormalizeLDAPMappingImport(ctx context.Context, isGroup bool, // not a valid DN, ignore. continue } - validatedDN, err := dnValidator(conn, k) + validatedDN, underBaseDN, err := dnValidator(conn, k) if err != nil { collectedErrors = append(collectedErrors, fmt.Errorf("could not validate `%s` exists in LDAP directory: %w", k, err)) continue } - if validatedDN == "" { + if validatedDN == "" || !underBaseDN { err := fmt.Errorf("DN `%s` was not found in the LDAP directory", k) collectedErrors = append(collectedErrors, err) continue @@ -1949,10 +1953,11 @@ func (sys *IAMSys) PolicyDBUpdateLDAP(ctx context.Context, isAttach bool, } else { if isAttach { var foundGroupDN string - if foundGroupDN, err = sys.LDAPConfig.GetValidatedGroupDN(nil, r.Group); err != nil { + var underBaseDN bool + if foundGroupDN, underBaseDN, err = sys.LDAPConfig.GetValidatedGroupDN(nil, r.Group); err != nil { iamLogIf(ctx, err) return - } else if foundGroupDN == "" { + } else if foundGroupDN == "" || !underBaseDN { err = errNoSuchGroup return } diff --git a/cmd/site-replication.go b/cmd/site-replication.go index 2186c78b8..2bdf1dddf 100644 --- a/cmd/site-replication.go +++ b/cmd/site-replication.go @@ -1441,9 +1441,10 @@ func (c *SiteReplicationSys) PeerPolicyMappingHandler(ctx context.Context, mappi var err error if isGroup { var foundGroupDN string - if foundGroupDN, err = globalIAMSys.LDAPConfig.GetValidatedGroupDN(nil, entityName); err != nil { + var underBaseDN bool + if foundGroupDN, underBaseDN, err = globalIAMSys.LDAPConfig.GetValidatedGroupDN(nil, entityName); err != nil { iamLogIf(ctx, err) - } else if foundGroupDN == "" { + } else if foundGroupDN == "" || !underBaseDN { err = errNoSuchGroup } entityName = foundGroupDN diff --git a/cmd/sts-handlers_test.go b/cmd/sts-handlers_test.go index e4429cbdd..feada2f5d 100644 --- a/cmd/sts-handlers_test.go +++ b/cmd/sts-handlers_test.go @@ -829,12 +829,14 @@ func TestIAMImportAssetWithLDAP(t *testing.T) { } } `, + // The `cn=projecty,..` group below is not under a configured DN, but we + // should still import without an error. allSvcAcctsFile: `{ "u4ccRswj62HV3Ifwima7": { "parent": "uid=svc.algorithm,OU=swengg,DC=min,DC=io", "accessKey": "u4ccRswj62HV3Ifwima7", "secretKey": "ZoEoZdLlzVbOlT9rbhD7ZN7TLyiYXSAlB79uGEge", - "groups": ["cn=project.c,ou=groups,OU=swengg,DC=min,DC=io"], + "groups": ["cn=project.c,ou=groups,OU=swengg,DC=min,DC=io", "cn=projecty,ou=groups,ou=hwengg,dc=min,dc=io"], "claims": { "accessKey": "u4ccRswj62HV3Ifwima7", "ldapUser": "uid=svc.algorithm,ou=swengg,dc=min,dc=io", diff --git a/internal/config/identity/ldap/ldap.go b/internal/config/identity/ldap/ldap.go index f83d89dbc..b2d932dd7 100644 --- a/internal/config/identity/ldap/ldap.go +++ b/internal/config/identity/ldap/ldap.go @@ -86,7 +86,7 @@ func (l *Config) GetValidatedDNForUsername(username string) (string, error) { // the directory. bindDN, err := l.LDAP.LookupUserDN(conn, username) if err != nil { - if strings.Contains(err.Error(), "not found") { + if strings.Contains(err.Error(), "User DN not found for") { return "", nil } return "", fmt.Errorf("Unable to find user DN: %w", err) @@ -94,30 +94,36 @@ func (l *Config) GetValidatedDNForUsername(username string) (string, error) { return bindDN, nil } - // Since the username is a valid DN, check that it is under a configured - // base DN in the LDAP directory. - return l.GetValidatedUserDN(conn, username) + // Since the username parses as a valid DN, check that it exists and is + // under a configured base DN in the LDAP directory. + validDN, isUnderBaseDN, err := l.GetValidatedUserDN(conn, username) + if err == nil && !isUnderBaseDN { + return "", fmt.Errorf("Unable to find user DN: %w", err) + } + return validDN, err } -// GetValidatedUserDN validates the given user DN. Will error out if conn is nil. -func (l *Config) GetValidatedUserDN(conn *ldap.Conn, userDN string) (string, error) { +// GetValidatedUserDN validates the given user DN. Will error out if conn is nil. The returned +// boolean is true iff the user DN is found under one of the LDAP user base DNs. +func (l *Config) GetValidatedUserDN(conn *ldap.Conn, userDN string) (string, bool, error) { return l.GetValidatedDNUnderBaseDN(conn, userDN, l.LDAP.UserDNSearchBaseDistNames) } // GetValidatedGroupDN validates the given group DN. If conn is nil, creates a -// connection. -func (l *Config) GetValidatedGroupDN(conn *ldap.Conn, groupDN string) (string, error) { +// connection. The returned boolean is true iff the group DN is found under one +// of the configured LDAP base DNs. +func (l *Config) GetValidatedGroupDN(conn *ldap.Conn, groupDN string) (string, bool, error) { if conn == nil { var err error conn, err = l.LDAP.Connect() if err != nil { - return "", err + return "", false, err } defer conn.Close() // Bind to the lookup user account if err = l.LDAP.LookupBind(conn); err != nil { - return "", err + return "", false, err } } @@ -128,22 +134,30 @@ func (l *Config) GetValidatedGroupDN(conn *ldap.Conn, groupDN string) (string, e // and returns the DN value sent by the LDAP server. The value returned by the // server may not be equal to the input DN, as LDAP equality is not a simple // Golang string equality. However, we assume the value returned by the LDAP -// server is canonical. +// server is canonical. Additionally, the attribute type names in the DN are +// lower-cased. // -// If the DN is not found in the LDAP directory, the returned string is empty -// and err = nil. -func (l *Config) GetValidatedDNUnderBaseDN(conn *ldap.Conn, dn string, baseDNList []xldap.BaseDNInfo) (string, error) { +// Return values: +// +// If the DN is found, the normalized (string) value is returned and error is +// nil. +// +// If the DN is not found, the string returned is empty and the error is nil. +// +// The returned boolean is true iff the DN is found under one of the LDAP +// subtrees listed in `baseDNList`. +func (l *Config) GetValidatedDNUnderBaseDN(conn *ldap.Conn, dn string, baseDNList []xldap.BaseDNInfo) (string, bool, error) { if len(baseDNList) == 0 { - return "", errors.New("no Base DNs given") + return "", false, errors.New("no Base DNs given") } // Check that DN exists in the LDAP directory. validatedDN, err := xldap.LookupDN(conn, dn) if err != nil { - return "", fmt.Errorf("Error looking up DN %s: %w", dn, err) + return "", false, fmt.Errorf("Error looking up DN %s: %w", dn, err) } if validatedDN == "" { - return "", nil + return "", false, nil } // This will not return an error as the argument is validated to be a DN. @@ -153,10 +167,12 @@ func (l *Config) GetValidatedDNUnderBaseDN(conn *ldap.Conn, dn string, baseDNLis // directory. for _, baseDN := range baseDNList { if baseDN.Parsed.AncestorOf(pdn) { - return validatedDN, nil + return validatedDN, true, nil } } - return "", fmt.Errorf("DN %s is not under any configured base DN", validatedDN) + + // Not under any configured base DN so return false. + return validatedDN, false, nil } // Bind - binds to ldap, searches LDAP and returns the distinguished name of the