ping... On Sun, 8 Mar 2020 12:18:39 +0100 Robert Klein <[email protected]> wrote:
> 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 <[email protected]> wrote: > > > On Fri, 6 Mar 2020 21:50:34 +0100 > > Robert Klein <[email protected]> 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 <[email protected]> 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 > > > > > >
