r21806: I've been working over the last week to fix up the LDAP backend for

Samba4.  This only broke on global catalog queries, which turned out to
be due to changes in the partitions module that metze needed for his
DRSUAPI work.

I've reworked partitions.c to always include the 'problematic' control,
and therefore demonstrated that this is the issue.  This ensures
consistency, and should help with finding issues like this in future.

As this control (DSDB_CONTROL_CURRENT_PARTITION_OID) is not intended to
be linearised, I've added logic to allow it to be skipped when creating
network packets.

I've likewise make our LDAP server skip unknown controls, when marked
'not critical' on it's input, rather than just dropping the entire
request.  I need some help to generate a correct error packet when it is
marked critical.

Further work could perhaps be to have the ldap_encode routine return a
textual description of what failed to encode, as that would have saved
me a lot of time...

Andrew Bartlett

diff --git a/source/dsdb/samdb/ldb_modules/partition.c b/source/dsdb/samdb/ldb_modules/partition.c
index bd037066ca81f2d1565b851c327df518961c3325..614431c5630fc46683f27186b9f991e8f57f16ac 100644 (file)
--- a/source/dsdb/samdb/ldb_modules/partition.c
+++ b/source/dsdb/samdb/ldb_modules/partition.c
@@ -205,11 +205,13 @@ error:
 }
 
 
-static int partition_send_request(struct partition_context *ac, struct dsdb_control_current_partition *partition)
+static int partition_send_request(struct partition_context *ac, struct ldb_control *remove_control, 
+                                 struct dsdb_control_current_partition *partition)
 {
        int ret;
        struct ldb_module *backend;
        struct ldb_request *req;
+       struct ldb_control **saved_controls;
 
        if (partition) {
                backend = make_module_for_next_request(ac, ac->module->ldb, partition->module);
@@ -231,7 +233,7 @@ static int partition_send_request(struct partition_context *ac, struct dsdb_cont
        
        *req = *ac->orig_req; /* copy the request */
 
-       if (ac->orig_req->controls) {
+       if (req->controls) {
                req->controls
                        = talloc_memdup(req,
                                        ac->orig_req->controls, talloc_get_size(ac->orig_req->controls));
@@ -259,6 +261,12 @@ static int partition_send_request(struct partition_context *ac, struct dsdb_cont
                req->context = ac;
        }
 
+       /* Remove a control, so we don't confuse a backend server */
+       if (remove_control && !save_controls(remove_control, req, &saved_controls)) {
+               ldb_oom(ac->module->ldb);
+               return LDB_ERR_OPERATIONS_ERROR;
+       }
+       
        if (partition) {
                ret = ldb_request_add_control(req, DSDB_CONTROL_CURRENT_PARTITION_OID, false, partition);
                if (ret != LDB_SUCCESS) {
@@ -278,17 +286,19 @@ static int partition_send_request(struct partition_context *ac, struct dsdb_cont
 
 /* Send a request down to all the partitions */
 static int partition_send_all(struct ldb_module *module, 
-                             struct partition_context *ac, struct ldb_request *req) 
+                             struct partition_context *ac, 
+                             struct ldb_control *remove_control, 
+                             struct ldb_request *req) 
 {
        int i;
        struct partition_private_data *data = talloc_get_type(module->private_data, 
                                                              struct partition_private_data);
-       int ret = partition_send_request(ac, NULL);
+       int ret = partition_send_request(ac, remove_control, NULL);
        if (ret != LDB_SUCCESS) {
                return ret;
        }
        for (i=0; data && data->partitions && data->partitions[i]; i++) {
-               ret = partition_send_request(ac, data->partitions[i]);
+               ret = partition_send_request(ac, remove_control, data->partitions[i]);
                if (ret != LDB_SUCCESS) {
                        return ret;
                }
@@ -307,18 +317,20 @@ static int partition_replicate(struct ldb_module *module, struct ldb_request *re
        struct partition_private_data *data = talloc_get_type(module->private_data, 
                                                              struct partition_private_data);
        
-       /* Is this a special DN, we need to replicate to every backend? */
-       for (i=0; data->replicate && data->replicate[i]; i++) {
-               if (ldb_dn_compare(data->replicate[i], 
-                                  dn) == 0) {
-                       struct partition_context *ac;
-                       
-                       ac = partition_init_handle(req, module);
-                       if (!ac) {
-                               return LDB_ERR_OPERATIONS_ERROR;
+       if (req->operation != LDB_SEARCH) {
+               /* Is this a special DN, we need to replicate to every backend? */
+               for (i=0; data->replicate && data->replicate[i]; i++) {
+                       if (ldb_dn_compare(data->replicate[i], 
+                                          dn) == 0) {
+                               struct partition_context *ac;
+                               
+                               ac = partition_init_handle(req, module);
+                               if (!ac) {
+                                       return LDB_ERR_OPERATIONS_ERROR;
+                               }
+                               
+                               return partition_send_all(module, ac, NULL, req);
                        }
-                       
-                       return partition_send_all(module, ac, req);
                }
        }
 
@@ -371,7 +383,11 @@ static int partition_search(struct ldb_module *module, struct ldb_request *req)
        if (search_options && (search_options->search_options & LDB_SEARCH_OPTION_PHANTOM_ROOT)) {
                int ret, i;
                struct partition_context *ac;
-               
+               struct ldb_control *remove_control = NULL;
+               if ((search_options->search_options & ~LDB_SEARCH_OPTION_PHANTOM_ROOT) == 0) {
+                       /* We have processed this flag, so we are done with this control now */
+                       remove_control = search_control;
+               }
                ac = partition_init_handle(req, module);
                if (!ac) {
                        return LDB_ERR_OPERATIONS_ERROR;
@@ -379,12 +395,12 @@ static int partition_search(struct ldb_module *module, struct ldb_request *req)
 
                /* Search from the base DN */
                if (!req->op.search.base || ldb_dn_is_null(req->op.search.base)) {
-                       return partition_send_all(module, ac, req);
+                       return partition_send_all(module, ac, remove_control, req);
                }
                for (i=0; data && data->partitions && data->partitions[i]; i++) {
                        /* Find all partitions under the search base */
                        if (ldb_dn_compare_base(req->op.search.base, data->partitions[i]->dn) == 0) {
-                               ret = partition_send_request(ac, data->partitions[i]);
+                               ret = partition_send_request(ac, remove_control, data->partitions[i]);
                                if (ret != LDB_SUCCESS) {
                                        return ret;
                                }
@@ -399,9 +415,8 @@ static int partition_search(struct ldb_module *module, struct ldb_request *req)
                
                return LDB_SUCCESS;
        } else {
-               struct ldb_module *backend = find_backend(module, req, req->op.search.base);
-       
-               return ldb_next_request(backend, req);
+               /* Handle this like all other requests */
+               return partition_replicate(module, req, req->op.search.base);
        }
 }
 
@@ -672,7 +687,7 @@ static int partition_extended(struct ldb_module *module, struct ldb_request *req
                return LDB_ERR_OPERATIONS_ERROR;
        }
                        
-       return partition_send_all(module, ac, req);
+       return partition_send_all(module, ac, NULL, req);
 }
 
 static int sort_compare(void *void1,

diff --git a/source/ldap_server/ldap_server.c b/source/ldap_server/ldap_server.c
index 4a05ac88515f71c1dcd279df8bcc424c234d8fdb..738215cda524b95927366bcee24289ea981bd265 100644 (file)
--- a/source/ldap_server/ldap_server.c
+++ b/source/ldap_server/ldap_server.c
@@ -131,6 +131,7 @@ static void ldapsrv_process_message(struct ldapsrv_connection *conn,
 */
 static NTSTATUS ldapsrv_decode(void *private, DATA_BLOB blob)
 {
+       NTSTATUS status;
        struct ldapsrv_connection *conn = talloc_get_type(private, 
                                                          struct ldapsrv_connection);
        struct asn1_data asn1;
@@ -144,9 +145,10 @@ static NTSTATUS ldapsrv_decode(void *private, DATA_BLOB blob)
                return NT_STATUS_NO_MEMORY;
        }
 
-       if (!ldap_decode(&asn1, msg)) {
+       status = ldap_decode(&asn1, msg);
+       if (!NT_STATUS_IS_OK(status)) {
                asn1_free(&asn1);
-               return NT_STATUS_LDAP(LDAP_PROTOCOL_ERROR);
+               return status;
        }
 
        data_blob_free(&blob);

diff --git a/source/libcli/cldap/cldap.c b/source/libcli/cldap/cldap.c
index 9dfa8e81b1441a235ce470c74c44fb0a4bc63585..96b60da25f1110cd104991e0852eae8bd9874823 100644 (file)
--- a/source/libcli/cldap/cldap.c
+++ b/source/libcli/cldap/cldap.c
@@ -107,8 +107,9 @@ static void cldap_socket_recv(struct cldap_socket *cldap)
        }
 
        /* this initial decode is used to find the message id */
-       if (!ldap_decode(&asn1, ldap_msg)) {
-               DEBUG(2,("Failed to decode ldap message\n"));
+       status = ldap_decode(&asn1, ldap_msg);
+       if (!NT_STATUS_IS_OK(status)) {
+               DEBUG(2,("Failed to decode ldap message: %s\n", nt_errstr(status)));
                talloc_free(tmp_ctx);
                return;
        }
@@ -428,6 +429,7 @@ NTSTATUS cldap_search_recv(struct cldap_request *req,
                           struct cldap_search *io)
 {
        struct ldap_message *ldap_msg;
+       NTSTATUS status;
 
        if (req == NULL) {
                return NT_STATUS_NO_MEMORY;
@@ -448,9 +450,11 @@ NTSTATUS cldap_search_recv(struct cldap_request *req,
        ldap_msg = talloc(mem_ctx, struct ldap_message);
        NT_STATUS_HAVE_NO_MEMORY(ldap_msg);
 
-       if (!ldap_decode(&req->asn1, ldap_msg)) {
+       status = ldap_decode(&req->asn1, ldap_msg);
+       if (!NT_STATUS_IS_OK(status)) {
+               DEBUG(2,("Failed to decode cldap search reply: %s\n", nt_errstr(status)));
                talloc_free(req);
-               return NT_STATUS_LDAP(LDAP_PROTOCOL_ERROR);
+               return status;
        }
 
        ZERO_STRUCT(io->out);
@@ -462,9 +466,11 @@ NTSTATUS cldap_search_recv(struct cldap_request *req,
                *io->out.response = ldap_msg->r.SearchResultEntry;
 
                /* decode the 2nd part */
-               if (!ldap_decode(&req->asn1, ldap_msg)) {
+               status = ldap_decode(&req->asn1, ldap_msg);
+               if (!NT_STATUS_IS_OK(status)) {
+                       DEBUG(2,("Failed to decode cldap search result entry: %s\n", nt_errstr(status)));
                        talloc_free(req);
-                       return NT_STATUS_LDAP(LDAP_PROTOCOL_ERROR);
+                       return status;
                }
        }
 

diff --git a/source/libcli/ldap/ldap.c b/source/libcli/ldap/ldap.c
index 5a7174b41dbadc095533517ad27484b654351f9e..1e308d584783cfdafe7df66a12d119d2dba3d7e7 100644 (file)
--- a/source/libcli/ldap/ldap.c
+++ b/source/libcli/ldap/ldap.c
@@ -925,7 +925,9 @@ static void ldap_decode_attribs(TALLOC_CTX *mem_ctx, struct asn1_data *data,
        asn1_end_tag(data);
 }
 
-BOOL ldap_decode(struct asn1_data *data, struct ldap_message *msg)
+/* This routine returns LDAP status codes */
+
+NTSTATUS ldap_decode(struct asn1_data *data, struct ldap_message *msg)
 {
        uint8_t tag;
 
@@ -933,7 +935,7 @@ BOOL ldap_decode(struct asn1_data *data, struct ldap_message *msg)
        asn1_read_Integer(data, &msg->messageid);
 
        if (!asn1_peek_uint8(data, &tag))
-               return False;
+               return NT_STATUS_LDAP(LDAP_PROTOCOL_ERROR);
 
        switch(tag) {
 
@@ -950,12 +952,12 @@ BOOL ldap_decode(struct asn1_data *data, struct ldap_message *msg)
                        asn1_start_tag(data, ASN1_CONTEXT_SIMPLE(0));
                        pwlen = asn1_tag_remaining(data);
                        if (pwlen == -1) {
-                               return False;
+                               return NT_STATUS_LDAP(LDAP_PROTOCOL_ERROR);
                        }
                        if (pwlen != 0) {
                                char *pw = talloc_size(msg, pwlen+1);
                                if (!pw) {
-                                       return False;
+                                       return NT_STATUS_LDAP(LDAP_OPERATIONS_ERROR);
                                }
                                asn1_read(data, pw, pwlen);
                                pw[pwlen] = '\0';
@@ -971,7 +973,7 @@ BOOL ldap_decode(struct asn1_data *data, struct ldap_message *msg)
                                asn1_read_OctetString(data, &tmp_blob);
                                r->creds.SASL.secblob = talloc(msg, DATA_BLOB);
                                if (!r->creds.SASL.secblob) {
-                                       return False;
+                                       return NT_STATUS_LDAP(LDAP_OPERATIONS_ERROR);
                                }
                                *r->creds.SASL.secblob = data_blob_talloc(r->creds.SASL.secblob,
                                                                          tmp_blob.data, tmp_blob.length);
@@ -982,7 +984,7 @@ BOOL ldap_decode(struct asn1_data *data, struct ldap_message *msg)
                        asn1_end_tag(data);
                } else {
                        /* Neither Simple nor SASL bind */
-                       return False;
+                       return NT_STATUS_LDAP(LDAP_PROTOCOL_ERROR);
                }
                asn1_end_tag(data);
                break;
@@ -998,7 +1000,7 @@ BOOL ldap_decode(struct asn1_data *data, struct ldap_message *msg)
                        asn1_read_ContextSimple(data, 7, &tmp_blob);
                        r->SASL.secblob = talloc(msg, DATA_BLOB);
                        if (!r->SASL.secblob) {
-                               return False;
+                               return NT_STATUS_LDAP(LDAP_OPERATIONS_ERROR);
                        }
                        *r->SASL.secblob = data_blob_talloc(r->SASL.secblob,
                                                            tmp_blob.data, tmp_blob.length);
@@ -1030,7 +1032,7 @@ BOOL ldap_decode(struct asn1_data *data, struct ldap_message *msg)
 
                r->tree = ldap_decode_filter_tree(msg, data);
                if (r->tree == NULL) {
-                       return False;
+                       return NT_STATUS_LDAP(LDAP_PROTOCOL_ERROR);
                }
 
                asn1_start_tag(data, ASN1_SEQUENCE(0));
@@ -1038,15 +1040,16 @@ BOOL ldap_decode(struct asn1_data *data, struct ldap_message *msg)
                r->num_attributes = 0;
                r->attributes = NULL;
 
-               while (asn1_tag_remaining(data) > 0) {
+               while (asn1_tag_remaining(data) > 0) {                                  
+
                        const char *attr;
                        if (!asn1_read_OctetString_talloc(msg, data,
                                                          &attr))
-                               return False;
+                               return NT_STATUS_LDAP(LDAP_PROTOCOL_ERROR);
                        if (!add_string_to_array(msg, attr,
                                                 &r->attributes,
                                                 &r->num_attributes))
-                               return False;
+                               return NT_STATUS_LDAP(LDAP_PROTOCOL_ERROR);
                }
 
                asn1_end_tag(data);
@@ -1106,7 +1109,7 @@ BOOL ldap_decode(struct asn1_data *data, struct ldap_message *msg)
                        asn1_end_tag(data);
                        if (!add_mod_to_array_talloc(msg, &mod,
                                                     &r->mods, &r->num_mods)) {
-                               return False;
+                               return NT_STATUS_LDAP(LDAP_OPERATIONS_ERROR);
                        }
                }
 
@@ -1157,7 +1160,7 @@ BOOL ldap_decode(struct asn1_data *data, struct ldap_message *msg)
                               ASN1_APPLICATION_SIMPLE(LDAP_TAG_DelRequest));
                len = asn1_tag_remaining(data);
                if (len == -1) {
-                       return False;
+                       return NT_STATUS_LDAP(LDAP_PROTOCOL_ERROR);
                }
                dn = talloc_size(msg, len+1);
                if (dn == NULL)
@@ -1193,11 +1196,11 @@ BOOL ldap_decode(struct asn1_data *data, struct ldap_message *msg)
                        asn1_start_tag(data, ASN1_CONTEXT_SIMPLE(0));
                        len = asn1_tag_remaining(data);
                        if (len == -1) {
-                               return False;
+                               return NT_STATUS_LDAP(LDAP_PROTOCOL_ERROR);
                        }
                        newsup = talloc_size(msg, len+1);
                        if (newsup == NULL) {
-                               return False;
+                               return NT_STATUS_LDAP(LDAP_OPERATIONS_ERROR);
                        }
                        asn1_read(data, newsup, len);
                        newsup[len] = '\0';
@@ -1259,19 +1262,19 @@ BOOL ldap_decode(struct asn1_data *data, struct ldap_message *msg)
                msg->type = LDAP_TAG_ExtendedRequest;
                asn1_start_tag(data,tag);
                if (!asn1_read_ContextSimple(data, 0, &tmp_blob)) {
-                       return False;
+                       return NT_STATUS_LDAP(LDAP_PROTOCOL_ERROR);
                }
                r->oid = blob2string_talloc(msg, tmp_blob);
                data_blob_free(&tmp_blob);
                if (!r->oid) {
-                       return False;
+                       return NT_STATUS_LDAP(LDAP_OPERATIONS_ERROR);
                }
 
                if (asn1_peek_tag(data, ASN1_CONTEXT_SIMPLE(1))) {
                        asn1_read_ContextSimple(data, 1, &tmp_blob);
                        r->value = talloc(msg, DATA_BLOB);
                        if (!r->value) {
-                               return False;
+                               return NT_STATUS_LDAP(LDAP_OPERATIONS_ERROR);
                        }
                        *r->value = data_blob_talloc(r->value, tmp_blob.data, tmp_blob.length);
                        data_blob_free(&tmp_blob);
@@ -1296,7 +1299,7 @@ BOOL ldap_decode(struct asn1_data *data, struct ldap_message *msg)
                        r->oid = blob2string_talloc(msg, tmp_blob);
                        data_blob_free(&tmp_blob);
                        if (!r->oid) {
-                               return False;
+                               return NT_STATUS_LDAP(LDAP_OPERATIONS_ERROR);
                        }
                } else {
                        r->oid = NULL;
@@ -1306,7 +1309,7 @@ BOOL ldap_decode(struct asn1_data *data, struct ldap_message *msg)
                        asn1_read_ContextSimple(data, 1, &tmp_blob);
                        r->value = talloc(msg, DATA_BLOB);
                        if (!r->value) {
-                               return False;
+                               return NT_STATUS_LDAP(LDAP_OPERATIONS_ERROR);
                        }
                        *r->value = data_blob_talloc(r->value, tmp_blob.data, tmp_blob.length);
                        data_blob_free(&tmp_blob);
@@ -1318,34 +1321,45 @@ BOOL ldap_decode(struct asn1_data *data, struct ldap_message *msg)
                break;
        }
        default: 
-               return False;
+               return NT_STATUS_LDAP(LDAP_PROTOCOL_ERROR);
        }
 
        msg->controls = NULL;
 
        if (asn1_peek_tag(data, ASN1_CONTEXT(0))) {
-               int i;
+               int i = 0;
                struct ldb_control **ctrl = NULL;
 
                asn1_start_tag(data, ASN1_CONTEXT(0));
 
-               for (i=0; asn1_peek_tag(data, ASN1_SEQUENCE(0)); i++) {
+               while (asn1_peek_tag(data, ASN1_SEQUENCE(0))) {
+                       DATA_BLOB value;
                        /* asn1_start_tag(data, ASN1_SEQUENCE(0)); */
 
                        ctrl = talloc_realloc(msg, ctrl, struct ldb_control *, i+2);
                        if (!ctrl) {
-                               return False;
+                               return NT_STATUS_LDAP(LDAP_OPERATIONS_ERROR);
                        }
 
                        ctrl[i] = talloc(ctrl, struct ldb_control);
                        if (!ctrl[i]) {
-                               return False;
+                               return NT_STATUS_LDAP(LDAP_OPERATIONS_ERROR);
                        }
 
-                       if (!ldap_decode_control(ctrl, data, ctrl[i])) {
-                               return False;
+                       if (!ldap_decode_control_wrapper(ctrl, data, ctrl[i], &value)) {
+                               return NT_STATUS_LDAP(LDAP_PROTOCOL_ERROR);
+                       }
+                       
+                       if (!ldap_decode_control_value(ctrl, value, ctrl[i])) {
+                               if (ctrl[i]->critical) {
+                                       return NT_STATUS_LDAP(LDAP_UNAVAILABLE_CRITICAL_EXTENSION);
+                               } else {
+                                       talloc_free(ctrl[i]);
+                                       ctrl[i] = NULL;
+                               }
+                       } else {
+                               i++;
                        }
-
                }
 
                if (ctrl != NULL) {
@@ -1358,7 +1372,10 @@ BOOL ldap_decode(struct asn1_data *data, struct ldap_message *msg)
        }
 
        asn1_end_tag(data);
-       return ((!data->has_error) && (data->nesting == NULL));
+       if ((data->has_error) || (data->nesting != NULL)) {
+               return NT_STATUS_LDAP(LDAP_PROTOCOL_ERROR);
+       }
+       return NT_STATUS_OK;
 }
 
 

diff --git a/source/libcli/ldap/ldap_client.c b/source/libcli/ldap/ldap_client.c
index 82cafd721a00654c3c405a72520ed58e124aa366..8852fd542728f5d39231e1a96b2fb50e35656ae4 100644 (file)
--- a/source/libcli/ldap/ldap_client.c
+++ b/source/libcli/ldap/ldap_client.c
@@ -169,6 +169,8 @@ static void ldap_match_message(struct ldap_connection *conn, struct ldap_message
 */
 static NTSTATUS ldap_recv_handler(void *private_data, DATA_BLOB blob)
 {
+       int ret;
+       NTSTATUS status;
        struct asn1_data asn1;
        struct ldap_connection *conn = talloc_get_type(private_data, 
                                                       struct ldap_connection);
@@ -182,8 +184,9 @@ static NTSTATUS ldap_recv_handler(void *private_data, DATA_BLOB blob)
                return NT_STATUS_LDAP(LDAP_PROTOCOL_ERROR);
        }
        
-       if (!ldap_decode(&asn1, msg)) {
-               return NT_STATUS_LDAP(LDAP_PROTOCOL_ERROR);
+       status = ldap_decode(&asn1, msg);
+       if (!NT_STATUS_IS_OK(status)) {
+               return status;
        }
 
        ldap_match_message(conn, msg);

diff --git a/source/libcli/ldap/ldap_controls.c b/source/libcli/ldap/ldap_controls.c
index 5cd0451136ac64c97800f5f9978415e4805edba0..bbb0cb1aa575411b123d082668f02f89a97406b1 100644 (file)
--- a/source/libcli/ldap/ldap_controls.c
+++ b/source/libcli/ldap/ldap_controls.c
@@ -1059,14 +1059,35 @@ struct control_handler ldap_known_controls[] = {
        { "2.16.840.1.113730.3.4.2", decode_manageDSAIT_request, encode_manageDSAIT_request },
        { "2.16.840.1.113730.3.4.9", decode_vlv_request, encode_vlv_request },
        { "2.16.840.1.113730.3.4.10", decode_vlv_response, encode_vlv_response },
+/* DSDB_CONTROL_CURRENT_PARTITION_OID is internal only, and has no network representation */
+       { "1.3.6.1.4.1.7165.4.3.2", NULL, NULL },
+/* DSDB_EXTENDED_REPLICATED_OBJECTS_OID is internal only, and has no network representation */
+       { "1.3.6.1.4.1.7165.4.4.1", NULL, NULL },
        { NULL, NULL, NULL }
 };
 
-BOOL ldap_decode_control(void *mem_ctx, struct asn1_data *data, struct ldb_control *ctrl)
+BOOL ldap_decode_control_value(void *mem_ctx, DATA_BLOB value, struct ldb_control *ctrl)
 {
        int i;
+
+       for (i = 0; ldap_known_controls[i].oid != NULL; i++) {
+               if (strcmp(ldap_known_controls[i].oid, ctrl->oid) == 0) {
+                       if (!ldap_known_controls[i].decode || !ldap_known_controls[i].decode(mem_ctx, value, &ctrl->data)) {
+                               return False;
+                       }
+                       break;
+               }
+       }
+       if (ldap_known_controls[i].oid == NULL) {
+               return False;
+       }
+
+       return True;
+}
+
+BOOL ldap_decode_control_wrapper(void *mem_ctx, struct asn1_data *data, struct ldb_control *ctrl, DATA_BLOB *value)
+{
        DATA_BLOB oid;
-       DATA_BLOB value;
 
        if (!asn1_start_tag(data, ASN1_SEQUENCE(0))) {
                return False;
@@ -1096,19 +1117,7 @@ BOOL ldap_decode_control(void *mem_ctx, struct asn1_data *data, struct ldb_contr
                goto end_tag;
        }
 
-       if (!asn1_read_OctetString(data, &value)) {
-               return False;
-       }
-
-       for (i = 0; ldap_known_controls[i].oid != NULL; i++) {
-               if (strcmp(ldap_known_controls[i].oid, ctrl->oid) == 0) {
-                       if (!ldap_known_controls[i].decode(mem_ctx, value, &ctrl->data)) {
-                               return False;
-                       }
-                       break;
-               }
-       }
-       if (ldap_known_controls[i].oid == NULL) {
+       if (!asn1_read_OctetString(data, value)) {
                return False;
        }
 
@@ -1125,6 +1134,26 @@ BOOL ldap_encode_control(void *mem_ctx, struct asn1_data *data, struct ldb_contr
        DATA_BLOB value;
        int i;
 
+       for (i = 0; ldap_known_controls[i].oid != NULL; i++) {
+               if (strcmp(ldap_known_controls[i].oid, ctrl->oid) == 0) {
+                       if (!ldap_known_controls[i].encode) {
+                               if (ctrl->critical) {
+                                       return False;
+                               } else {
+                                       /* not encoding this control */
+                                       return True;
+                               }
+                       }
+                       if (!ldap_known_controls[i].encode(mem_ctx, ctrl->data, &value)) {
+                               return False;
+                       }
+                       break;
+               }
+       }
+       if (ldap_known_controls[i].oid == NULL) {
+               return False;
+       }
+
        if (!asn1_push_tag(data, ASN1_SEQUENCE(0))) {
                return False;
        }
@@ -1143,18 +1172,6 @@ BOOL ldap_encode_control(void *mem_ctx, struct asn1_data *data, struct ldb_contr
                goto pop_tag;
        }
 
-       for (i = 0; ldap_known_controls[i].oid != NULL; i++) {
-               if (strcmp(ldap_known_controls[i].oid, ctrl->oid) == 0) {
-                       if (!ldap_known_controls[i].encode(mem_ctx, ctrl->data, &value)) {
-                               return False;
-                       }
-                       break;
-               }
-       }
-       if (ldap_known_controls[i].oid == NULL) {
-               return False;
-       }
-
        if (!asn1_write_OctetString(data, value.data, value.length)) {
                return False;
        }