Hi, I thought a bit more about using LDAP resultCode values and I think some intermediate values are needed so it is clearer what happens.
Also, I found out a clients connection hangs in the "Database is being reopened" case in ldap_auth_simple(). Below is a patch proposal that 1. patches the original issue, so ldapd returns "invalid credentials" instead of "operations error" where appropriate. 2. use private "result Code" values for intermediate results (see also https://www.iana.org/assignments/ldap-parameters/ldap-parameters.xhtml#ldap-parameters-6): - LDAP_X_HANDLES_BY_LDAPE_IMSGEV for SASL binds and {BSDAUTH} style passwords where the actual answer is provided via IMSGEV - LDAP_X_PASSWORD_OK in check_password() because there's still some work left until it can be called LDAP_SUCCESS. 3. returns LDAP_BUSY for "Database is being reopened": the old code did not lead to a "return ldap_respond" in bind_ldap(), so nothing is returned to the client which is trying to bind and the client hangs. Best regards Robert diff 403185e43a653dece6518a28d0750f212ff40fc5 /usr/src blob - f6f542e4e4a0af0b815643d9f622913a46707709 file + usr.sbin/ldapd/aldap.h --- usr.sbin/ldapd/aldap.h +++ usr.sbin/ldapd/aldap.h @@ -109,6 +109,8 @@ enum result_code { LDAP_AFFECTS_MULTIPLE_DSAS = 71, LDAP_OTHER = 80, + LDAP_X_PASSWORD_OK = 16384, + LDAP_X_HANDLED_BY_LDAPE_IMSGEV = 16385, }; enum filter { blob - f8debff7a2d6a0feb9cf984905de385b029b8744 file + usr.sbin/ldapd/auth.c --- usr.sbin/ldapd/auth.c +++ usr.sbin/ldapd/auth.c @@ -179,33 +179,35 @@ send_auth_request(struct request *req, const char *use const char *password) { struct auth_req auth_req; + int ret = LDAP_X_HANDLED_BY_LDAPE_IMSGEV; memset(&auth_req, 0, sizeof(auth_req)); if (strlcpy(auth_req.name, username, - sizeof(auth_req.name)) >= sizeof(auth_req.name)) + sizeof(auth_req.name)) >= sizeof(auth_req.name)) { + ret = LDAP_INVALID_CREDENTIALS; goto fail; + } if (strlcpy(auth_req.password, password, - sizeof(auth_req.password)) >= sizeof(auth_req.password)) + sizeof(auth_req.password)) >= sizeof(auth_req.password)) { + ret = LDAP_INVALID_CREDENTIALS; goto fail; + } auth_req.fd = req->conn->fd; auth_req.msgid = req->msgid; if (imsgev_compose(iev_ldapd, IMSG_LDAPD_AUTH, 0, 0, -1, &auth_req, - sizeof(auth_req)) == -1) + sizeof(auth_req)) == -1) { + ret = LDAP_OTHER; goto fail; + } req->conn->bind_req = req; - return 0; fail: memset(&auth_req, 0, sizeof(auth_req)); - return -1; + return ret; } -/* - * Check password. Returns 1 if password matches, 2 if password matching - * is in progress, 0 on mismatch and -1 on error. - */ static int check_password(struct request *req, const char *stored_passwd, const char *passwd) @@ -218,37 +220,39 @@ check_password(struct request *req, const char *stored int sz; if (stored_passwd == NULL) - return -1; + return LDAP_INVALID_CREDENTIALS; if (strncmp(stored_passwd, "{SHA}", 5) == 0) { sz = b64_pton(stored_passwd + 5, tmp, sizeof(tmp)); if (sz != SHA_DIGEST_LENGTH) - return (-1); + return (LDAP_INVALID_CREDENTIALS); SHA1_Init(&ctx); SHA1_Update(&ctx, passwd, strlen(passwd)); SHA1_Final(md, &ctx); - return (bcmp(md, tmp, SHA_DIGEST_LENGTH) == 0 ? 1 : 0); + return (bcmp(md, tmp, SHA_DIGEST_LENGTH) == 0 ? + LDAP_X_PASSWORD_OK : LDAP_INVALID_CREDENTIALS); } else if (strncmp(stored_passwd, "{SSHA}", 6) == 0) { sz = b64_pton(stored_passwd + 6, tmp, sizeof(tmp)); if (sz <= SHA_DIGEST_LENGTH) - return (-1); + return (LDAP_INVALID_CREDENTIALS); salt = tmp + SHA_DIGEST_LENGTH; SHA1_Init(&ctx); SHA1_Update(&ctx, passwd, strlen(passwd)); SHA1_Update(&ctx, salt, sz - SHA_DIGEST_LENGTH); SHA1_Final(md, &ctx); - return (bcmp(md, tmp, SHA_DIGEST_LENGTH) == 0 ? 1 : 0); + return (bcmp(md, tmp, SHA_DIGEST_LENGTH) == 0 ? + LDAP_X_PASSWORD_OK : LDAP_INVALID_CREDENTIALS); } else if (strncmp(stored_passwd, "{CRYPT}", 7) == 0) { encpw = crypt(passwd, stored_passwd + 7); if (encpw == NULL) - return (-1); - return (strcmp(encpw, stored_passwd + 7) == 0 ? 1 : 0); + return (LDAP_INVALID_CREDENTIALS); + return (strcmp(encpw, stored_passwd + 7) == 0 ? + LDAP_X_PASSWORD_OK : LDAP_INVALID_CREDENTIALS); } else if (strncmp(stored_passwd, "{BSDAUTH}", 9) == 0) { - if (send_auth_request(req, stored_passwd + 9, passwd) == -1) - return (-1); - return 2; /* Operation in progress. */ + return send_auth_request(req, stored_passwd + 9, passwd); } else - return (strcmp(stored_passwd, passwd) == 0 ? 1 : 0); + return (strcmp(stored_passwd, passwd) == 0 ? + LDAP_X_PASSWORD_OK : LDAP_INVALID_CREDENTIALS); } static int @@ -258,6 +262,7 @@ ldap_auth_sasl(struct request *req, char *binddn, stru char *authzid, *authcid, *password; char *creds; size_t len; + int ret; if (ober_scanf_elements(params, "{sx", &method, &creds, &len) != 0) return LDAP_PROTOCOL_ERROR; @@ -283,15 +288,15 @@ ldap_auth_sasl(struct request *req, char *binddn, stru log_debug("sasl authorization id = [%s]", authzid); log_debug("sasl authentication id = [%s]", authcid); - if (send_auth_request(req, authcid, password) != 0) - return LDAP_OPERATIONS_ERROR; + if ((ret = send_auth_request(req, authcid, password)) != LDAP_X_HANDLED_BY_LDAPE_IMSGEV) + return ret; free(req->conn->binddn); req->conn->binddn = NULL; if ((req->conn->pending_binddn = strdup(authcid)) == NULL) return LDAP_OTHER; - return LDAP_SUCCESS; + return LDAP_X_HANDLED_BY_LDAPE_IMSGEV; } static int @@ -341,7 +346,7 @@ ldap_auth_simple(struct request *req, char *binddn, st if (elm == NULL && errno == ESTALE) { if (namespace_queue_request(ns, req) != 0) return LDAP_BUSY; - return -1; /* Database is being reopened. */ + return LDAP_BUSY; /* Database is being reopened. */ } if (elm != NULL) @@ -352,7 +357,8 @@ ldap_auth_simple(struct request *req, char *binddn, st if (ober_get_string(elm, &user_password) != 0) continue; pwret = check_password(req, user_password, password); - if (pwret >= 1) + if (pwret == LDAP_X_PASSWORD_OK || + pwret == LDAP_X_HANDLED_BY_LDAPE_IMSGEV) break; } } @@ -361,20 +367,18 @@ ldap_auth_simple(struct request *req, char *binddn, st free(req->conn->binddn); req->conn->binddn = NULL; - if (pwret == 1) { + if (pwret == LDAP_X_PASSWORD_OK) { if ((req->conn->binddn = strdup(binddn)) == NULL) return LDAP_OTHER; log_debug("successfully authenticated as %s", req->conn->binddn); return LDAP_SUCCESS; - } else if (pwret == 2) { + } else if (pwret == LDAP_X_HANDLED_BY_LDAPE_IMSGEV) { if ((req->conn->pending_binddn = strdup(binddn)) == NULL) return LDAP_OTHER; - return -LDAP_SASL_BIND_IN_PROGRESS; - } else if (pwret == 0) - return LDAP_INVALID_CREDENTIALS; - else - return LDAP_OPERATIONS_ERROR; + return LDAP_X_HANDLED_BY_LDAPE_IMSGEV; + } else + return pwret; } void @@ -423,17 +427,18 @@ ldap_bind(struct request *req) switch (auth->be_type) { case LDAP_AUTH_SIMPLE: - if ((rc = ldap_auth_simple(req, binddn, auth)) < 0) - return rc; + rc = ldap_auth_simple(req, binddn, auth); break; case LDAP_AUTH_SASL: - if ((rc = ldap_auth_sasl(req, binddn, auth)) == LDAP_SUCCESS) - return LDAP_SUCCESS; + rc = ldap_auth_sasl(req, binddn, auth); break; default: rc = LDAP_STRONG_AUTH_NOT_SUPPORTED; break; } + + if (rc == LDAP_X_HANDLED_BY_LDAPE_IMSGEV) + return rc; /* don't call ldap_respond() */ done: return ldap_respond(req, rc); On Sat, 7 Mar 2020 20:30:31 +0100 Robert Klein <rokl...@roklein.de> wrote: > On Fri, 6 Mar 2020 21:50:34 +0100 > Robert Klein <rokl...@roklein.de> wrote: > > > Hi, > > > > > > sorry, I simply forgot ldap_auth_sasl. > > > > LDAP_OTHER is a good return code for imsg failure and I really like > > the idea of using the LDAP return codes right away instead of the > > extra mapping. > > > > Your patch however doesn't work for SASL authentication (and > > ldapsearch gives some strange messages), because > > sent_auth_request *never* returns LDAP_SUCCESS (this happens via > > imsg) but LDAP_SASL_BIND_IN_PROGRESS. See comment inline. > > > > After changing the one line bind with SASL works, too. > > Additional note: if the rest of ldap_auth_sasl succeeds it returns > LDAP_SUCCESS. This could also be turned into > LDAP_SASL_BIND_IN_PROGRESS if and only if the corresponding line(s) in > ldap_bind() below are also changes. In the end it doesn't matter what > value gets there as long as it is handled consistently. Its only > purpose is that ldap_respond() is not called from ldap_bind. The > value itself gets returned to the caller which discards is > (request_dispatch() in conn.c). > > Best regards > Robert > > > > > > All tests using ldap_auth_simple worked ok. > > > > Best regards > > Robert > > > > > > On Tue, 3 Mar 2020 20:33:41 +0100 > > Martijn van Duren <openbsd+t...@list.imperialat.at> wrote: > > > > > I agree that returning Operations Error is the wrong return value. > > > I don't agree that we should *always* return invalidCredentials, > > > however, acting like the other LDAP servers on an invalid entry > > > seems reasonable to me. One option I do see is if we can't create > > > an imsg to the parent process handling a BSDAUTH request, this > > > clearly is an internal error. > > > > > > Also, you missed the case for ldap_auth_sasl. > > > > > > Diff below should change this, but it's compile tested only. > > > > > > martijn@ > > > > > > On 3/1/20 5:24 PM, Robert Klein wrote: > > > > Hi, > > > > > > > > When trying to bind to ldapd using a dn which has an invalid > > > > userPassword entry, e.eg. a “defective” SHA valus like > > > > “{SHA}alpha”, ldapd currently returns 1 (Operations Error). > > > > > > > > The patch below changes this to 49 (Invalid Credentials). > > > > > > > > There are basically two reasons for this: > > > > > > > > 1. The userPassword is a multi-valued attribute, i.e. there can > > > > be more than one in an ldap entry. Now when you have a valid > > > > and a defective entry and the binding user supplies a password > > > > which does not match the valid entry you get different results > > > > whether the defective entry comes last (return value 1) or not > > > > (return value 49). When the supplied password matches the > > > > valid entry the bind succeeds independent of order. > > > > > > > > A change to return value 49 gets consistent results. > > > > > > > > For a single userPassword entry 49 also is not wrong, as the > > > > supplied password still doesn't match the entry (even if it is > > > > defective). > > > > > > > > 2. RFC 4511 defines the result code “Operations Error” as > > > > follows: > > > > > > > > “operationsError (1) > > > > > > > > Indicates that the operation is not properly sequenced > > > > with relation to other operations (of same or different type). > > > > > > > > For example, this code is returned if the client > > > > attempts to StartTLS [RFC4346] while there are other uncompleted > > > > operations or if a TLS layer was already installed.” > > > > > > > > A defective password entry in no way is an operations error > > > > of the ldapd server. Also I don't think it is the server's job > > > > to take care of the correctness of password entries. There is > > > > no provision in the LDAP RFCs to stop one from entering an > > > > incorrect entry. I checked this with other directory servers: > > > > 389, apache ds, and openldap. All three accept incorrect > > > > entries and all three return 49 (Invalid Credentials) on bind > > > > attempts. > > > > > > > > Best regards > > > > Robert > > > > > > > > > > > Index: auth.c > > > =================================================================== > > > RCS file: /cvs/src/usr.sbin/ldapd/auth.c,v > > > retrieving revision 1.14 > > > diff -u -p -r1.14 auth.c > > > --- auth.c 24 Oct 2019 12:39:26 -0000 1.14 > > > +++ auth.c 3 Mar 2020 19:31:41 -0000 > > > @@ -179,33 +179,34 @@ send_auth_request(struct request *req, c > > > const char *password) > > > { > > > struct auth_req auth_req; > > > + int ret = LDAP_SASL_BIND_IN_PROGRESS; > > > > > > memset(&auth_req, 0, sizeof(auth_req)); > > > if (strlcpy(auth_req.name, username, > > > - sizeof(auth_req.name)) >= sizeof(auth_req.name)) > > > + sizeof(auth_req.name)) >= sizeof(auth_req.name)) { > > > + ret = LDAP_INVALID_CREDENTIALS; > > > goto fail; > > > + } > > > if (strlcpy(auth_req.password, password, > > > - sizeof(auth_req.password)) >= > > > sizeof(auth_req.password)) > > > + sizeof(auth_req.password)) >= > > > sizeof(auth_req.password)) { > > > + ret = LDAP_INVALID_CREDENTIALS; > > > goto fail; > > > + } > > > auth_req.fd = req->conn->fd; > > > auth_req.msgid = req->msgid; > > > > > > if (imsgev_compose(iev_ldapd, IMSG_LDAPD_AUTH, 0, 0, -1, > > > &auth_req, > > > - sizeof(auth_req)) == -1) > > > + sizeof(auth_req)) == -1) { > > > + ret = LDAP_OTHER; > > > goto fail; > > > + } > > > > > > req->conn->bind_req = req; > > > - return 0; > > > - > > > fail: > > > memset(&auth_req, 0, sizeof(auth_req)); > > > - return -1; > > > + return ret; > > > } > > > > > > -/* > > > - * Check password. Returns 1 if password matches, 2 if password > > > matching > > > - * is in progress, 0 on mismatch and -1 on error. > > > - */ > > > static int > > > check_password(struct request *req, const char *stored_passwd, > > > const char *passwd) > > > @@ -218,37 +219,39 @@ check_password(struct request *req, cons > > > int sz; > > > > > > if (stored_passwd == NULL) > > > - return -1; > > > + return LDAP_INVALID_CREDENTIALS; > > > > > > if (strncmp(stored_passwd, "{SHA}", 5) == 0) { > > > sz = b64_pton(stored_passwd + 5, tmp, > > > sizeof(tmp)); if (sz != SHA_DIGEST_LENGTH) > > > - return (-1); > > > + return (LDAP_INVALID_CREDENTIALS); > > > SHA1_Init(&ctx); > > > SHA1_Update(&ctx, passwd, strlen(passwd)); > > > SHA1_Final(md, &ctx); > > > - return (bcmp(md, tmp, SHA_DIGEST_LENGTH) == 0 ? > > > 1 : 0); > > > + return (bcmp(md, tmp, SHA_DIGEST_LENGTH) == 0 ? > > > + LDAP_SUCCESS : LDAP_INVALID_CREDENTIALS); > > > } else if (strncmp(stored_passwd, "{SSHA}", 6) == 0) { > > > sz = b64_pton(stored_passwd + 6, tmp, > > > sizeof(tmp)); if (sz <= SHA_DIGEST_LENGTH) > > > - return (-1); > > > + return (LDAP_INVALID_CREDENTIALS); > > > salt = tmp + SHA_DIGEST_LENGTH; > > > SHA1_Init(&ctx); > > > SHA1_Update(&ctx, passwd, strlen(passwd)); > > > SHA1_Update(&ctx, salt, sz - SHA_DIGEST_LENGTH); > > > SHA1_Final(md, &ctx); > > > - return (bcmp(md, tmp, SHA_DIGEST_LENGTH) == 0 ? > > > 1 : 0); > > > + return (bcmp(md, tmp, SHA_DIGEST_LENGTH) == 0 ? > > > + LDAP_SUCCESS : LDAP_INVALID_CREDENTIALS); > > > } else if (strncmp(stored_passwd, "{CRYPT}", 7) == 0) { > > > encpw = crypt(passwd, stored_passwd + 7); > > > if (encpw == NULL) > > > - return (-1); > > > - return (strcmp(encpw, stored_passwd + 7) == 0 ? > > > 1 : 0); > > > + return (LDAP_INVALID_CREDENTIALS); > > > + return (strcmp(encpw, stored_passwd + 7) == 0 ? > > > + LDAP_SUCCESS : LDAP_INVALID_CREDENTIALS); > > > } else if (strncmp(stored_passwd, "{BSDAUTH}", 9) == 0) { > > > - if (send_auth_request(req, stored_passwd + 9, > > > passwd) == -1) > > > - return (-1); > > > - return 2; /* Operation in progress. */ > > > + return send_auth_request(req, stored_passwd + 9, > > > passwd); } else > > > - return (strcmp(stored_passwd, passwd) == 0 ? 1 : > > > 0); > > > + return (strcmp(stored_passwd, passwd) == 0 ? > > > + LDAP_SUCCESS : LDAP_INVALID_CREDENTIALS); > > > } > > > > > > static int > > > @@ -258,6 +261,7 @@ ldap_auth_sasl(struct request *req, char > > > char *authzid, *authcid, > > > *password; char *creds; > > > size_t len; > > > + int ret; > > > > > > if (ober_scanf_elements(params, "{sx", &method, &creds, > > > &len) != 0) return LDAP_PROTOCOL_ERROR; > > > @@ -283,8 +287,8 @@ ldap_auth_sasl(struct request *req, char > > > log_debug("sasl authorization id = [%s]", authzid); > > > log_debug("sasl authentication id = [%s]", authcid); > > > > > > - if (send_auth_request(req, authcid, password) != 0) > > > - return LDAP_OPERATIONS_ERROR; > > > + if ((ret = send_auth_request(req, authcid, password)) != > > > LDAP_SUCCESS) > > > > ---^^^ this should be LDAP_SASL_BIND_IN_PROGRESS. > > > > > + return ret; > > > > > > free(req->conn->binddn); > > > req->conn->binddn = NULL; > > > @@ -352,7 +356,8 @@ ldap_auth_simple(struct request *req, ch > > > if (ober_get_string(elm, > > > &user_password) != 0) continue; > > > pwret = check_password(req, > > > user_password, password); > > > - if (pwret >= 1) > > > + if (pwret == LDAP_SUCCESS || > > > + pwret == > > > LDAP_SASL_BIND_IN_PROGRESS) break; > > > } > > > } > > > @@ -361,20 +366,18 @@ ldap_auth_simple(struct request *req, ch > > > free(req->conn->binddn); > > > req->conn->binddn = NULL; > > > > > > - if (pwret == 1) { > > > + if (pwret == LDAP_SUCCESS) { > > > if ((req->conn->binddn = strdup(binddn)) == NULL) > > > return LDAP_OTHER; > > > log_debug("successfully authenticated as %s", > > > req->conn->binddn); > > > return LDAP_SUCCESS; > > > - } else if (pwret == 2) { > > > + } else if (pwret == LDAP_SASL_BIND_IN_PROGRESS) { > > > if ((req->conn->pending_binddn = strdup(binddn)) > > > == NULL) return LDAP_OTHER; > > > return -LDAP_SASL_BIND_IN_PROGRESS; > > > - } else if (pwret == 0) > > > - return LDAP_INVALID_CREDENTIALS; > > > - else > > > - return LDAP_OPERATIONS_ERROR; > > > + } else > > > + return pwret; > > > } > > > > > > void > > >
