360035 - server response controls lost in referral chase

Status: RESOLVED FIXED

(Directory :: LDAP C SDK, defect)

Description

[Ulf Weltman]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7
Build Identifier: 

A client product fails to mention that the password has expired or needs changing when a bind has been referred.

It looks like the result messages of referred operations (child requests) get rebuilt with just the message ID, message type, errno, matched DN and error message.  At that point we lose any server response controls, including password policy controls.

Bug 231705 might be referring to this, it talks about the page control but I believe it's a more generic issue since the page cookie comes back in a control in the the search result.  But since I haven't tested with the page control I wont assume it's the same.

Reproducible: Always

Comment 1

[Ulf Weltman]

Specifically I'm looking at result.c:read1msg()
                        /*
                         * if this response is to a child request, we toss
                         * the message contents and just merge error info.
                         * into the parent.
                         */
                        if ( has_parent ) {
                                ber_free( ber, 1 );
                                ber = NULLBER;
                        }
And...
                                        if ( build_result_ber( ld, &ber, lr )
                                            != LDAP_SUCCESS ) {
                                                rc = NSLDAPI_RESULT_ERROR;
                                        } else {
                                                manufactured_result = 1;
                                        }

Any suggestions, is it dropping controls on purpose?  Can I just parse the original ber for controls before it's destroyed and pass them to build_result_ber to add to the constructed ber?

Comment 2

[Rich Megginson]

(In reply to comment #1)
> Specifically I'm looking at result.c:read1msg()
> ...
> Any suggestions, is it dropping controls on purpose?  Can I just parse the
> original ber for controls before it's destroyed and pass them to
> build_result_ber to add to the constructed ber?

I don't know.  You might want to check the openldap code to see how they handle this situation.

Comment 3

[Mark Smith [:mcs]]

I think the code that combines child error info. into the parent request struct was written before response controls were (widely) used.  I am not sure what the right thing to do is, but perhaps merging the controls from all responses into one list makes sense.  I am not sure what to do if > 1 child response and/or the parent contain a response control with the same OID.  And it is possible that "conflicting" response controls might be returned.

Comment 4

[Ulf Weltman]

(In reply to comment #3)
> I think the code that combines child error info. into the parent request struct
> was written before response controls were (widely) used.  I am not sure what
> the right thing to do is, but perhaps merging the controls from all responses
> into one list makes sense.  I am not sure what to do if > 1 child response
> and/or the parent contain a response control with the same OID.  And it is
> possible that "conflicting" response controls might be returned.
> 
I was wondering about this, what are the circumstances where there would be multiple responses with controls?  For a bind you only get one actual bind response right - though perhaps there could be response controls in the referred bind requests on the path to the actual bind?  For a search I guess the situation would be trickier.  Would it make sense to handle bind specially?

(In reply to comment #2)
> I don't know.  You might want to check the openldap code to see how they handle
> this situation.
> 
I looked through the corresponding section of OL 2.3.28 libldap and it seems to have nearly identical code.  I also built and tried their ldapsearch; same thing, no controls from a referred bind.

Comment 5

[Ulf Weltman]

Mark, what do you think about retaining controls only when the result is not a search entry or search reference?  I think for the rest of the possible result messages in a referral chase, there would only be one so there wouldn't be a need to figure out how to deal with controls from multiple messages.

Comment 6

[Ulf Weltman]

Attachment: patch proposal

Extracts controls from original message before it's tossed, and inserts them into the generated result message when it's sent to the client.  Not sure if I'm allowed to skip forward in the BER to find the controls in the original message the way I did...

Comment 7

[Mark Smith [:mcs]]

(In reply to comment #5)
> Mark, what do you think about retaining controls only when the result is not a
> search entry or search reference?  I think for the rest of the possible result
> messages in a referral chase, there would only be one so there wouldn't be a
> need to figure out how to deal with controls from multiple messages.

I think it is OK (for now) to ignore controls return within intermediate result messages such as entries or references, but you probably need something like the merge_error_info() function for controls.  Why?  Because there can be more than one referral hop involved, which means you will need to store the controls for a child response in the request structure until you are ready to call build_result_ber() (which might happen inside a different call to ldap_result()).

Comment 8

[Mark Smith [:mcs]]

(In reply to comment #6)
> Not sure if
> I'm allowed to skip forward in the BER to find the controls in the original
> message the way I did...

It feels a little fragile (too much knowledge of how the liblber functions work) but at the moment I cannot think of a cleaner approach.

Comment 9

[Ulf Weltman]

Attachment: revised patch proposal

Thanks Mark!  Something along these lines?
- Added an nsldapi_find_controls() function which works the same as nsldapi_get_controls() except it skips forward to find the controls tag first.  The guts of that routine can be changed later if I think of a better way to do it.
- Revised previously proposed change to read1msg() to stash the response controls in the original parent request.  I'm not making any attempt to merge controls since I'm not trying to support controls from intermediate messages.  I hope I understood you correctly here: that there can be multiple referral hops, but only one of them would return a response which is not a search entry or a reference.
- Added a field LDAPControl **lr_res_ctrls to LDAPRequest object, and updated nsldapi_free_request() to free it if present.  Nothing needed in nsldapi_new_request() since the memory is calloced.
- Revised previously proposed change to build_result_ber() to grab controls from lr rather than looking for them passed in separately.

Comment 10

[Mark Smith [:mcs]]

(In reply to comment #9)
> - Revised previously proposed change to read1msg() to stash the response
> controls in the original parent request.  I'm not making any attempt to merge
> controls since I'm not trying to support controls from intermediate messages. 
> I hope I understood you correctly here: that there can be multiple referral
> hops, but only one of them would return a response which is not a search entry
> or a reference.

Your changes look pretty good.

It is possible to receive several LDAPResult messages, and several of those might contain controls.  For the common case where only the last / deepest request in the referral chain includes response controls, it probably makes sense to keep the last controls received rather than the first ones.  What do other people think?

I added Anton to the CC list so he can take a look at this too.

Comment 11

[Anton Bobrov]

Mark, i think it makes sense. i'm not sure how to fix it good & proper without
any major rewrite so i would say its good enough from my perspective. re ctrls
it makes sense to skip them during hops and only store the last one for when
the chase is over. cant imagine the server returning any useful controls with
referral anyway as long as it returns the actual referral successfully.

Comment 12

[Ulf Weltman]

Attachment: improved revised patch proposal

hanks Mark and Anton, that makes sense.
New diff, same as previous except:
- Keep the response controls received last rather than the first ones received.
- Have build_result_ber() call nsldapi_put_controls() and rely on it to close the sequence whether there are controls or not.  This seems to be how nsldapi_put_controls() is used everywhere else.
- Added check to nsldapi_find_controls() to not hoark on bogus BER pointer, so that it behaves the same as original nsldapi_get_controls().
- Cleaned up some comments.

Comment 13

[Anton Bobrov]

(In reply to comment #12)

Ulf, the code looks ok assuming you done some basic testing of it
just to make sure it works as expected and we dont introduce any
regressions here.
 

Comment 14

[Mark Smith [:mcs]]

The code looks good but please add a:

  if (NULL != lr->lr_res_ctrls)

check before the call to nsldapi_put_controls() (otherwise it may add ld->ld_servercontrols, which would not be good).

And to Anton's point, please make sure you have done some testing (I suspect you already have).

Comment 15

[Rich Megginson]

Do you have a final patch ready to go?

Comment 16

[Ulf Weltman]

Attachment: updated proposal

Updated to incorporate Mark's last feedback.

I've run various functional tests and also through valgrind.  There are no new leaks or memory errors but there are some pre-existing problems in the referral chasing code related to the LDAPConn reference count causing reads into freed memory and double frees.  I'll file a separate bug for that.

Comment 17

[Rich Megginson]

Looks good to me.

Comment 18

[Mark Smith [:mcs]]

Comment on attachment 268402 [details] [diff] [review]
updated proposal

Looks good.
r=mcs

Comment 19

[Ulf Weltman]

Thanks Rich and Mark!  Checked in on trunk:

Checking in control.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/control.c,v  <--  control.c
new revision: 5.5; previous revision: 5.4
done
Checking in ldap-int.h;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/ldap-int.h,v  <--  ldap-int.h
new revision: 5.10; previous revision: 5.9
done
Checking in request.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/request.c,v  <--  request.c
new revision: 5.9; previous revision: 5.8
done
Checking in result.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/result.c,v  <--  result.c
new revision: 5.8; previous revision: 5.7
done

Comment 20

[Ulf Weltman]

Oops, by habit I had used unsigned long in place of ber_len_t and ber_tag_t.  Fixed...

Checking in control.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/control.c,v  <--  control.c
new revision: 5.6; previous revision: 5.5
done