bug#58605: 29.0.50; Commit 2a2f5530fa2 breaks ldap-search-internal

bug#58605: 29.0.50; Commit 2a2f5530fa2 breaks ldap-search-internal

[Jens Lechtenboerger]

Hi there,

commit 2a2f5530fa230e2b994be5683e63763833bb6a0a breaks
ldap-search-internal for searches with empty results.

Consider this search with ldapsearch, returning an empty result:

ldapsearch -h ldap.pca.dfn.de -tt -x -b O=DFN-Verein,C=DE \
'(mail=x@example.org)' userCertificate
# extended LDIF
#
# LDAPv3
# base <O=DFN-Verein,C=DE> with scope subtree
# filter: (mail=x@example.org)
# requesting: userCertificate 
#

# search result
search: 2
result: 0 Success

# numResponses: 1

The problem is that ldap-search-internal interprets the 2 in line
"search: 2" as filename with a certificate, bound to variable value.
Thus, (insert-file-contents-literally value) results in an error:
File is missing: "Opening input file", "Datei oder Verzeichnis nicht
gefunden", "[currentdir]/2"

Execute that:
(let ((ldap-default-base "O=DFN-Verein,C=DE")
      (ldap-ldapsearch-args '("-x" "-tt" "-H ldaps://ldap.pca.dfn.de"))
      (search-plist '(host "" filter "mail=x@example.org" attributes ("userCertificate") attrsonly nil withdn nil)))
  (ldap-search-internal search-plist))

Previously, that returned nil, after the mentioned commit it
produces the above error.

The added “?” should be removed there again:

@@ -699,7 +699,7 @@ ldap-search-internal
 	  (forward-line 1)
           (while (looking-at "^\\([A-Za-z][-A-Za-z0-9]*\
 \\|[0-9]+\\(?:\\.[0-9]+\\)*\\)\\(;[-A-Za-z0-9]+\\)*[=:\t ]+\
-\\(<[\t ]*file://\\)\\(.*\\)$")
+\\(<[\t ]*file://\\)?\\(.*\\)$")
 	    (setq name (match-string 1)
 		  value (match-string 4))
             ;; Need to handle file:///D:/... as generated by OpenLDAP

Thanks
Jens

bug#58605: 29.0.50; Commit 2a2f5530fa2 breaks ldap-search-internal

[Eli Zaretskii]

Filipp, could you please look into this issue?

> From: Jens Lechtenboerger <lechten@wi.uni-muenster.de>
> Date: Tue, 18 Oct 2022 08:05:53 +0200
> 
> Hi there,
> 
> commit 2a2f5530fa230e2b994be5683e63763833bb6a0a breaks
> ldap-search-internal for searches with empty results.
> 
> Consider this search with ldapsearch, returning an empty result:
> 
> ldapsearch -h ldap.pca.dfn.de -tt -x -b O=DFN-Verein,C=DE \
> '(mail=x@example.org)' userCertificate
> # extended LDIF
> #
> # LDAPv3
> # base <O=DFN-Verein,C=DE> with scope subtree
> # filter: (mail=x@example.org)
> # requesting: userCertificate 
> #
> 
> # search result
> search: 2
> result: 0 Success
> 
> # numResponses: 1
> 
> The problem is that ldap-search-internal interprets the 2 in line
> "search: 2" as filename with a certificate, bound to variable value.
> Thus, (insert-file-contents-literally value) results in an error:
> File is missing: "Opening input file", "Datei oder Verzeichnis nicht
> gefunden", "[currentdir]/2"
> 
> Execute that:
> (let ((ldap-default-base "O=DFN-Verein,C=DE")
>       (ldap-ldapsearch-args '("-x" "-tt" "-H ldaps://ldap.pca.dfn.de"))
>       (search-plist '(host "" filter "mail=x@example.org" attributes ("userCertificate") attrsonly nil withdn nil)))
>   (ldap-search-internal search-plist))
> 
> Previously, that returned nil, after the mentioned commit it
> produces the above error.
> 
> The added “?” should be removed there again:
> 
> @@ -699,7 +699,7 @@ ldap-search-internal
>  	  (forward-line 1)
>            (while (looking-at "^\\([A-Za-z][-A-Za-z0-9]*\
>  \\|[0-9]+\\(?:\\.[0-9]+\\)*\\)\\(;[-A-Za-z0-9]+\\)*[=:\t ]+\
> -\\(<[\t ]*file://\\)\\(.*\\)$")
> +\\(<[\t ]*file://\\)?\\(.*\\)$")
>  	    (setq name (match-string 1)
>  		  value (match-string 4))
>              ;; Need to handle file:///D:/... as generated by OpenLDAP
> 
> Thanks
> Jens
> 
> 
> 
> 

bug#58605: 29.0.50; Commit 2a2f5530fa2 breaks ldap-search-internal

[Juri Linkov]

> commit 2a2f5530fa230e2b994be5683e63763833bb6a0a breaks
> ldap-search-internal for searches with empty results.

I confirm that ldap search is broken.  When the output contains for example:

  dn: cn=admin,dc=example,dc=com

then it raises the error:

  insert-file-contents: Opening input file: No such file or directory, ~/cn=admin,dc=example,dc=com

> The added “?” should be removed there again:
>
> @@ -699,7 +699,7 @@ ldap-search-internal
>  	  (forward-line 1)
>            (while (looking-at "^\\([A-Za-z][-A-Za-z0-9]*\
>  \\|[0-9]+\\(?:\\.[0-9]+\\)*\\)\\(;[-A-Za-z0-9]+\\)*[=:\t ]+\
> -\\(<[\t ]*file://\\)\\(.*\\)$")
> +\\(<[\t ]*file://\\)?\\(.*\\)$")
>  	    (setq name (match-string 1)
>  		  value (match-string 4))
>              ;; Need to handle file:///D:/... as generated by OpenLDAP

Removing “?” doesn't fix the problem because then nil is returned
even in case when there are results.

I guess this whole block

	      (with-current-buffer bufval
		(erase-buffer)
		(set-buffer-multibyte nil)
		(insert-file-contents-literally value)
		(delete-file value)
		(setq value (buffer-string)))

should be executed only on a condition that checks that value contains "file://".

bug#58605: 29.0.50; Commit 2a2f5530fa2 breaks ldap-search-internal

[Filipp Gunbin]

Hi,

Jens, did you by chance modify ldap-ldapsearch-args?  Its default value
contains "-LLL" which suppresses noise like "search: 2" that you see.

I've now pushed the fix to master, could you please try?


Juri,

On 20/10/2022 11:12 +0300, Juri Linkov wrote:

>> commit 2a2f5530fa230e2b994be5683e63763833bb6a0a breaks
>> ldap-search-internal for searches with empty results.
>
> I confirm that ldap search is broken.  When the output contains for example:
>
>   dn: cn=admin,dc=example,dc=com
>
> then it raises the error:
>
>   insert-file-contents: Opening input file: No such file or directory,
>   ~/cn=admin,dc=example,dc=com

"dn" at the start of each block is handled specially, as it should be
the only attribute whose value is not written to file (by "-tt"
ldapsearch option).

> I guess this whole block
>
> 	      (with-current-buffer bufval
> 		(erase-buffer)
> 		(set-buffer-multibyte nil)
> 		(insert-file-contents-literally value)
> 		(delete-file value)
> 		(setq value (buffer-string)))
>
> should be executed only on a condition that checks that value contains "file://".

Agreed.

Thanks.
Filipp

bug#58605: 29.0.50; Commit 2a2f5530fa2 breaks ldap-search-internal

[Jens Lechtenboerger]

[-- Attachment #1: Type: text/plain, Size: 922 bytes --]

Hi all,

On 2022-10-20, Filipp Gunbin wrote:

> Jens, did you by chance modify ldap-ldapsearch-args?  Its default value
> contains "-LLL" which suppresses noise like "search: 2" that you see.

yes, I change that variable as indicated in my bug report:

>> (let ((ldap-default-base "O=DFN-Verein,C=DE")
>>       (ldap-ldapsearch-args '("-x" "-tt" "-H ldaps://ldap.pca.dfn.de"))
>>       (search-plist '(host "" filter "mail=x@example.org" attributes ("userCertificate") attrsonly nil withdn nil)))
>>   (ldap-search-internal search-plist))

Previously, this worked.

> I've now pushed the fix to master, could you please try?

I do not get an error any more, which is good, but also not nil,
which might break prior code: ((("search" " ") ("result" " ")))

Also, the format of return values for existing certificates seems to
have changed.  I hope to find time during the weekend to have a
closer look...

Best wishes
Jens

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 6187 bytes --]

bug#58605: 29.0.50; Commit 2a2f5530fa2 breaks ldap-search-internal

[Filipp Gunbin]

Hi,

On 21/10/2022 08:25 +0200, Jens Lechtenboerger wrote:

> Hi all,
>
> On 2022-10-20, Filipp Gunbin wrote:
>
>> Jens, did you by chance modify ldap-ldapsearch-args?  Its default value
>> contains "-LLL" which suppresses noise like "search: 2" that you see.
>
> yes, I change that variable as indicated in my bug report:
>
>>> (let ((ldap-default-base "O=DFN-Verein,C=DE")
>>>       (ldap-ldapsearch-args '("-x" "-tt" "-H ldaps://ldap.pca.dfn.de"))
>>>       (search-plist '(host "" filter "mail=x@example.org" attributes ("userCertificate") attrsonly nil withdn nil)))
>>>   (ldap-search-internal search-plist))
>
> Previously, this worked.

Oh, so this is not just a test case, but the code you actually use.  Add
"-LLL" to ldap-ldapsearch-args?

>> I've now pushed the fix to master, could you please try?
>
> I do not get an error any more, which is good, but also not nil,
> which might break prior code: ((("search" " ") ("result" " ")))

Let's see.

> Also, the format of return values for existing certificates seems to
> have changed.  I hope to find time during the weekend to have a
> closer look...

Yeah, please report if you see problems.

BTW, that question mark in the regexp was added to fix M-x
eudc-get-attribute-list, which was broken.

Filipp

bug#58605: 29.0.50; Commit 2a2f5530fa2 breaks ldap-search-internal

[Jens Lechtenboerger]

[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]

Hi there!

On 2022-10-21, Filipp Gunbin wrote:

> Oh, so this is not just a test case, but the code you actually use.  Add
> "-LLL" to ldap-ldapsearch-args?

This was a test case.  In my code [1], I use ldap-search, not the
internal function.  Essentially, like this:

(let ((ldap-default-base "O=DFN-Verein,C=DE")
      (ldap-ldapsearch-args '("-x" "-tt" "-H ldaps://ldap.pca.dfn.de"))
      (mail "nix@example.org")
      (host ""))
  (ldap-search (concat "mail=" mail) host '("userCertificate") nil))

Previously, this just returned the userCertificate, with and without
-LLL.  If no certificate existed, it returned nil.

Now, -LLL seems to be required, which I perceive to be a breaking
change.  Can this be avoided?  Alternatively, document it?

I confirm that with -LLL, my code works.

Without it, I get ((("search" " ") ("result" " "))) instead of nil.

Also, without -LLL and with an existing certificate, ("dn" " ") and
(("search" " ") ("result" " ")) are included in the result.  Note
that I find ("dn" " ") misleading, as ldapsearch really does return
a dn value...

> BTW, that question mark in the regexp was added to fix M-x
> eudc-get-attribute-list, which was broken.

I was not aware of eudc, thanks!

Best wishes
Jens

[1] https://gitlab.com/lechten/defaultencrypt/-/blob/master/jl-smime.el

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 6187 bytes --]

bug#58605: 29.0.50; Commit 2a2f5530fa2 breaks ldap-search-internal

[Filipp Gunbin]

Hi Jens,

On 22/10/2022 11:43 +0200, Jens Lechtenboerger wrote:

> Hi there!
>
> On 2022-10-21, Filipp Gunbin wrote:
>
>> Oh, so this is not just a test case, but the code you actually use.  Add
>> "-LLL" to ldap-ldapsearch-args?
>
> This was a test case.  In my code [1], I use ldap-search, not the
> internal function.  Essentially, like this:
>
> (let ((ldap-default-base "O=DFN-Verein,C=DE")
>       (ldap-ldapsearch-args '("-x" "-tt" "-H ldaps://ldap.pca.dfn.de"))
>       (mail "nix@example.org")
>       (host ""))
>   (ldap-search (concat "mail=" mail) host '("userCertificate") nil))
>
> Previously, this just returned the userCertificate, with and without
> -LLL.  If no certificate existed, it returned nil.
>
> Now, -LLL seems to be required, which I perceive to be a breaking
> change.  Can this be avoided?  Alternatively, document it?
>
> I confirm that with -LLL, my code works.
>
> Without it, I get ((("search" " ") ("result" " "))) instead of nil.

But ldap-search expects ldap-ldapsearch-args to be at least what default
value is, to be able to parse the output (btw, "-LL" was there since
"forever", which is 20 years in this case; it's just the third L which
was added recently, to exclude ldif version from the output).  Also,
default value can change along with ldap-search internal changes.  So if
you want to let-bind it, you should merge in your additional args, not
replace them.

However, I don't see why you would want to let-bind it:

- -H: Why don't you use host parameter?
- -x: just pass 'auth = simple
- -tt: already in ldap-ldapsearch-args

Even more, I'd say that the user should set ldap-host-parameters-alist
according to his/her setup, and you should not mess with ldapsearch
arguments at all.  Like:

(setq ldap-host-parameters-alist
      '(("ldap://example.org"
         auth simple
         auth-source t)))

Then just invoke:

(ldap-search "mail=.." "ldap://example.org"  '("userCertificate"))

> Also, without -LLL and with an existing certificate, ("dn" " ") and
> (("search" " ") ("result" " ")) are included in the result.  Note
> that I find ("dn" " ") misleading, as ldapsearch really does return
> a dn value...

Without -LL(L), the parsing code works incorrectly, and this case
"worked" before only by coincidence.

HTH,
Filipp

bug#58605: 29.0.50; Commit 2a2f5530fa2 breaks ldap-search-internal

[Jens Lechtenboerger]

[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]

Hi there!

On 2022-10-24, Filipp Gunbin wrote:

> [...]
> But ldap-search expects ldap-ldapsearch-args to be at least what default
> value is, to be able to parse the output (btw, "-LL" was there since
> "forever", which is 20 years in this case; it's just the third L which
> was added recently, to exclude ldif version from the output).

How should users know about that expectation?

> Also, default value can change along with ldap-search internal
> changes.  So if you want to let-bind it, you should merge in your
> additional args, not replace them.

If the code requires this, it might separate the necessary arguments
from the customizable ones.

> However, I don't see why you would want to let-bind it:
>
> - -H: Why don't you use host parameter?
> - -x: just pass 'auth = simple
> - -tt: already in ldap-ldapsearch-args

When I started using LDAP, I could not make ldapsearch to use
encrypted connections without -H.  That may have changed since
then...

> Even more, I'd say that the user should set ldap-host-parameters-alist
> according to his/her setup, and you should not mess with ldapsearch
> arguments at all.  Like:
>
> (setq ldap-host-parameters-alist
>       '(("ldap://example.org"
>          auth simple
>          auth-source t)))
>
> Then just invoke:
>
> (ldap-search "mail=.." "ldap://example.org"  '("userCertificate"))

Does this result in encrypted connections?

Best wishes
Jens

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 6187 bytes --]

bug#58605: 29.0.50; Commit 2a2f5530fa2 breaks ldap-search-internal

[Filipp Gunbin]

Hi Jens,

On 25/10/2022 07:55 +0200, Jens Lechtenboerger wrote:

> Hi there!
>
> On 2022-10-24, Filipp Gunbin wrote:
>
>> [...]
>> But ldap-search expects ldap-ldapsearch-args to be at least what default
>> value is, to be able to parse the output (btw, "-LL" was there since
>> "forever", which is 20 years in this case; it's just the third L which
>> was added recently, to exclude ldif version from the output).
>
> How should users know about that expectation?

As I wrote, there should be no need to use it at all.

>> Also, default value can change along with ldap-search internal
>> changes.  So if you want to let-bind it, you should merge in your
>> additional args, not replace them.
>
> If the code requires this, it might separate the necessary arguments
> from the customizable ones.

It does - via ldap-host-parameters-alist.

>> However, I don't see why you would want to let-bind it:
>>
>> - -H: Why don't you use host parameter?
>> - -x: just pass 'auth = simple
>> - -tt: already in ldap-ldapsearch-args
>
> When I started using LDAP, I could not make ldapsearch to use
> encrypted connections without -H.  That may have changed since
> then...

Yeah, commit 0b4e003766f15225dede9bdba4ead33e493856e2 (June 2019) adds
support for LDAP URIs.

On a side note, OpenLDAP recently removed `-h' from ldapsearch
altogether.

>> Even more, I'd say that the user should set ldap-host-parameters-alist
>> according to his/her setup, and you should not mess with ldapsearch
>> arguments at all.  Like:
>>
>> (setq ldap-host-parameters-alist
>>       '(("ldap://example.org"
>>          auth simple
>>          auth-source t)))
>>
>> Then just invoke:
>>
>> (ldap-search "mail=.." "ldap://example.org"  '("userCertificate"))
>
> Does this result in encrypted connections?

It should work with ldaps:// as well.

I'm now closing this issue, feel free to reopen or create new if needed.

Filipp

bug#58605: 29.0.50; Commit 2a2f5530fa2 breaks ldap-search-internal

[Jens Lechtenboerger]

Hi Filipp,

I’m not sure about conventions for closed bugs but I reply once more
here...

On 2022-10-25, Filipp Gunbin wrote:

> Hi Jens,
>
> On 25/10/2022 07:55 +0200, Jens Lechtenboerger wrote:
>
>> Hi there!
>>
>> On 2022-10-24, Filipp Gunbin wrote:
>>
>>> [...]
>>> But ldap-search expects ldap-ldapsearch-args to be at least what default
>>> value is, to be able to parse the output (btw, "-LL" was there since
>>> "forever", which is 20 years in this case; it's just the third L which
>>> was added recently, to exclude ldif version from the output).
>>
>> How should users know about that expectation?
>
> As I wrote, there should be no need to use it at all.

Then maybe it should not exist (at least not as defcustom).
Alternatively, the doc string might be improved:
A list of required arguments to pass to ‘ldapsearch’.  Internal code
depends on the default value.

Best wishes
Jens

P.S. Thanks for your pointers regarding encryption with OpenLDAP!

bug#58605: 29.0.50; Commit 2a2f5530fa2 breaks ldap-search-internal

[Filipp Gunbin]

Hi Jens,

On 25/10/2022 15:42 +0200, Jens Lechtenboerger wrote:

> Hi Filipp,
>
> I’m not sure about conventions for closed bugs but I reply once more
> here...

Of course you can post in closed bugs as well.

[...]
>>>> But ldap-search expects ldap-ldapsearch-args to be at least what default
>>>> value is, to be able to parse the output (btw, "-LL" was there since
>>>> "forever", which is 20 years in this case; it's just the third L which
>>>> was added recently, to exclude ldif version from the output).
>>>
>>> How should users know about that expectation?
>>
>> As I wrote, there should be no need to use it at all.
>
> Then maybe it should not exist (at least not as defcustom).
> Alternatively, the doc string might be improved:
> A list of required arguments to pass to ‘ldapsearch’.  Internal code
> depends on the default value.

For some reason I was sure that it was a defvar, not a defcustom.  I've
now moved those options out of its default value.  Should be safe,
there's no harm in their duplication (if somebody still includes them
there), and it seems there're no other contradicting options.

Thanks,
Filipp.

bug#58605: 29.0.50; Commit 2a2f5530fa2 breaks ldap-search-internal

[Jens Lechtenboerger]

[-- Attachment #1: Type: text/plain, Size: 369 bytes --]

Hi Filipp!

On 2022-10-25, Filipp Gunbin wrote:

> [...]
> For some reason I was sure that it was a defvar, not a defcustom.  I've
> now moved those options out of its default value.  Should be safe,
> there's no harm in their duplication (if somebody still includes them
> there), and it seems there're no other contradicting options.

Works for me.

Many thanks
Jens

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 6187 bytes --]

bug#58605: 29.0.50; Commit 2a2f5530fa2 breaks ldap-search-internal

[Filipp Gunbin]

On 26/10/2022 08:07 +0200, Jens Lechtenboerger wrote:

> Hi Filipp!
>
> On 2022-10-25, Filipp Gunbin wrote:
>
>> [...]
>> For some reason I was sure that it was a defvar, not a defcustom.  I've
>> now moved those options out of its default value.  Should be safe,
>> there's no harm in their duplication (if somebody still includes them
>> there), and it seems there're no other contradicting options.
>
> Works for me.

Great, thanks for checking.

Filipp