From ec1b45cc38c58a408649ca846e3f4eccede7a336 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Fri, 24 Jul 2020 18:24:44 +0200 Subject: [PATCH 1/2] query numeric attribute values without quotes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some LDAP properties like `uidnumber` and `gidnumber` are numeric. When an OS tries to look up a user it will not only try to lookup the user by username, but also by the `uidnumber`: `(&(objectclass=posixAccount)(uidnumber=20000))`. The accounts backend for glauth was sending that as a string query `uid_number eq '20000'` in the ListAccounts query. This PR changes that to `uid_number eq 20000`. The removed quotes allow the parser in ocis-accounts to identify the numeric literal. Related: - https://github.com/owncloud/ocis-accounts/pull/68 - https://github.com/owncloud/ocis-glauth/issues/28 Signed-off-by: Jörn Friedrich Dreyer --- changelog/unreleased/fix-int-queries.md | 7 +++++++ pkg/server/glauth/handler.go | 5 +++-- 2 files changed, 10 insertions(+), 2 deletions(-) create mode 100644 changelog/unreleased/fix-int-queries.md diff --git a/changelog/unreleased/fix-int-queries.md b/changelog/unreleased/fix-int-queries.md new file mode 100644 index 0000000000..3c3e98d241 --- /dev/null +++ b/changelog/unreleased/fix-int-queries.md @@ -0,0 +1,7 @@ +Bugfix: query numeric attribute values without quotes + +Some LDAP properties like `uidnumber` and `gidnumber` are numeric. When an OS tries to look up a user it will not only try to lookup the user by username, but also by the `uidnumber`: `(&(objectclass=posixAccount)(uidnumber=20000))`. The accounts backend for glauth was sending that as a string query `uid_number eq '20000'` in the ListAccounts query. This PR changes that to `uid_number eq 20000`. The removed quotes allow the parser in ocis-accounts to identify the numeric literal. + +https://github.com/owncloud/ocis-glauth/issues/28 +https://github.com/owncloud/ocis-glauth/pull/29 +https://github.com/owncloud/ocis-accounts/pull/68 \ No newline at end of file diff --git a/pkg/server/glauth/handler.go b/pkg/server/glauth/handler.go index dcadb1a289..ed7fab1736 100644 --- a/pkg/server/glauth/handler.go +++ b/pkg/server/glauth/handler.go @@ -224,6 +224,7 @@ func (h ocisHandler) mapAccounts(accounts []*accounts.Account) []*ldap.Entry { attribute("cn", accounts[i].PreferredName), attribute("uid", accounts[i].PreferredName), attribute("sn", accounts[i].PreferredName), + attribute("homeDirectory", ""), attribute("ownCloudUUID", accounts[i].Id), // see https://github.com/butonic/owncloud-ldap-schema/blob/master/owncloud.schema#L28-L34 } if accounts[i].DisplayName != "" { @@ -330,9 +331,9 @@ func parseFilter(f *ber.Packet) (qtype queryType, q string, err error) { case "displayname": q = fmt.Sprintf("display_name eq '%s'", escapeValue(value)) case "uidnumber": - q = fmt.Sprintf("uid_number eq '%s'", escapeValue(value)) + q = fmt.Sprintf("uid_number eq %s", value) // TODO check it is a number? case "gidnumber": - q = fmt.Sprintf("gid_number eq '%s'", escapeValue(value)) + q = fmt.Sprintf("gid_number eq %s", value) // TODO check it is a number? case "description": q = fmt.Sprintf("description eq '%s'", escapeValue(value)) default: From 692cf503cc2b4066448de0eea6029056ecfd2a7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Mon, 27 Jul 2020 10:57:46 +0200 Subject: [PATCH 2/2] validate input and return proper ldap result codes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- pkg/server/glauth/handler.go | 40 ++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/pkg/server/glauth/handler.go b/pkg/server/glauth/handler.go index ed7fab1736..4ca9408f80 100644 --- a/pkg/server/glauth/handler.go +++ b/pkg/server/glauth/handler.go @@ -119,6 +119,7 @@ func (h ocisHandler) Search(bindDN string, searchReq ldap.SearchRequest, conn ne var qtype queryType = "" query := "" + var code ldap.LDAPResultCode var err error if searchReq.Filter == "(&)" { // see Absolute True and False Filters in https://tools.ietf.org/html/rfc4526#section-2 query = "" @@ -136,10 +137,10 @@ func (h ocisHandler) Search(bindDN string, searchReq ldap.SearchRequest, conn ne ResultCode: ldap.LDAPResultOperationsError, }, fmt.Errorf("Search Error: error parsing filter: %s", searchReq.Filter) } - qtype, query, err = parseFilter(cf) + qtype, query, code, err = parseFilter(cf) if err != nil { return ldap.ServerSearchResult{ - ResultCode: ldap.LDAPResultOperationsError, + ResultCode: code, }, fmt.Errorf("Search Error: error parsing filter: %s", searchReq.Filter) } } @@ -302,11 +303,11 @@ func (h ocisHandler) mapGroups(groups []*accounts.Group) []*ldap.Entry { // "" not determined // "users" // "groups" -func parseFilter(f *ber.Packet) (qtype queryType, q string, err error) { +func parseFilter(f *ber.Packet) (qtype queryType, q string, code ldap.LDAPResultCode, err error) { switch ldap.FilterMap[f.Tag] { case "Equality Match": if len(f.Children) != 2 { - return "", "", errors.New("equality match must have only two children") + return "", "", ldap.LDAPResultOperationsError, errors.New("equality match must have exactly two children") } attribute := strings.ToLower(f.Children[0].Value.(string)) value := f.Children[1].Value.(string) @@ -331,13 +332,22 @@ func parseFilter(f *ber.Packet) (qtype queryType, q string, err error) { case "displayname": q = fmt.Sprintf("display_name eq '%s'", escapeValue(value)) case "uidnumber": - q = fmt.Sprintf("uid_number eq %s", value) // TODO check it is a number? + if i, err := strconv.ParseUint(value, 10, 64); err != nil { + code = ldap.LDAPResultInvalidAttributeSyntax + } else { + q = fmt.Sprintf("uid_number eq %d", i) + } case "gidnumber": - q = fmt.Sprintf("gid_number eq %s", value) // TODO check it is a number? + if i, err := strconv.ParseUint(value, 10, 64); err != nil { + code = ldap.LDAPResultInvalidAttributeSyntax + } else { + q = fmt.Sprintf("gid_number eq %d", i) + } case "description": q = fmt.Sprintf("description eq '%s'", escapeValue(value)) default: - err = fmt.Errorf("filter by %s not implemented", attribute) + code = ldap.LDAPResultUndefinedAttributeType + err = fmt.Errorf("unrecognized assertion type '%s' in filter item", attribute) } return @@ -346,32 +356,32 @@ func parseFilter(f *ber.Packet) (qtype queryType, q string, err error) { for i := range f.Children { var subQuery string var qt queryType - qt, subQuery, err = parseFilter(f.Children[i]) + qt, subQuery, code, err = parseFilter(f.Children[i]) if err != nil { - return "", "", err + return "", "", code, err } if qtype == "" { qtype = qt } else if qt != "" && qt != qtype { - return "", "", fmt.Errorf("mixing user and group filters not supported") + return "", "", ldap.LDAPResultUnwillingToPerform, fmt.Errorf("mixing user and group filters not supported") } if subQuery != "" { subQueries = append(subQueries, subQuery) } } - return qtype, strings.Join(subQueries, " "+strings.ToLower(ldap.FilterMap[f.Tag])+" "), nil + return qtype, strings.Join(subQueries, " "+strings.ToLower(ldap.FilterMap[f.Tag])+" "), ldap.LDAPResultSuccess, nil case "Not": if len(f.Children) != 1 { - return "", "", errors.New("not filter must have only one child") + return "", "", ldap.LDAPResultOperationsError, errors.New("not filter match must have exactly one child") } - qtype, subQuery, err := parseFilter(f.Children[0]) + qtype, subQuery, code, err := parseFilter(f.Children[0]) if err != nil { - return "", "", err + return "", "", code, err } if subQuery != "" { q = fmt.Sprintf("not %s", subQuery) } - return qtype, q, nil + return qtype, q, code, nil } return }