Proper BER value for "true"

[MikeWillCook]

I think I have an answer to AZ's code comment on the "true" value BER encoding...

According to RFC4511, section 5.1, it looks like "true" should be an 0xFF instead of 1:

If the value of a BOOLEAN type is true, the encoding of the value octet is set to hex "FF".

I've confirmed this matches other LDAP client protocol communications. However, since the default string encoding is UTF-8 it has potential to cause an encoding exception on the array join, so I think it should also be forced to ASCII-8BIT to prevent problems.

So, here's a couple tiny patches with relevant changes for consideration (including 8BIT'ing the "false" string):

--- lib/net/ber/core_ext/true_class.rb
+++ lib/net/ber/core_ext/true_class.rb
@@ -5,8 +5,6 @@ module Net::BER::Extensions::TrueClass
   ##
   # Converts +true+ to the BER wireline representation of +true+.
   def to_ber
-    # 20100319 AZ: Note that this may not be the completely correct value,
-    # per some test documentation. We need to determine the truth of this.
-    "\001\001\001"
+    "\x01\x01\xFF".force_encoding("ASCII-8BIT")
   end
 end

--- lib/net/ber/core_ext/false_class.rb
+++ lib/net/ber/core_ext/false_class.rb
@@ -5,6 +5,6 @@ module Net::BER::Extensions::FalseClass
   ##
   # Converts +false+ to the BER wireline representation of +false+.
   def to_ber
-    "\001\001\000"
+    "\x01\x01\x00".force_encoding("ASCII-8BIT")
   end
 end

[RoryO]

Just off the top of my head, I would think this could cause problems if you tried to concatenate with parts of an UTF-8 array, which everything should be now according LDAP spec. Where would it cause problems to just encode this as UTF-8 and not force it to ASCII-8?

[MikeWillCook]

I found if I left it UTF-8 then the array join could fail (e.g., rename with delete true and a new_superior set) as all the other array values are ASCII-8 and this was the only UTF-8. If you check lib/net/ber/core_ext/string.rb:21 you can see the comment about strings being UTF-8 but then encoded as ASCII-8 for actual communication since "the BER code is not necessarily valid UTF-8".

[halostatue]

Using #force_encoding makes this only support Ruby 1.9. This may not be a bad thing, but it should be a deliberate choice to shift that direction.

If this is done correctly (string-to-BER does UTF-8-to-ASCII-8BIT; BER-to-string does ASCII-8BIT-to-UTF-8), this should be safe. Again, this is something that should be done deliberately.

[RoryO]

Ah yeah I missed that. Unfortunately wherever that comment came from doesn't specify why the BER isn't valid UTF-8. From what I understand, any LDAP strings should be UTF-8 according to 4.1.2 (http://tools.ietf.org/html/rfc4511 latest RFC but all other previous ones are the same). That should be the actual problem to attack, why doesn't Net::BER::Extensions::String isn't UTF-8?

[RoryO]

Also requiring 1.9 is a good idea in theory, but many things are still requiring 1.8.7, mostly big stuff that uses REE. I don't think that jump should be made in the near future.

[MikeWillCook]

Yeah, I wouldn't recommend requiring 1.9, but if it's already using force_encoding in lib/net/ber/core_ext/strings.rb has that already been decided? I think we should either change strings to not require it or add it to true/false, but at least be consistent in either case.

[Folkman]

+1 to MikeWillCook's suggestion. When using the "rename" functionality, I would get the error message "Unwilling to perform. Old RDN must be deleted," even though I was passing "true" to the "delete_attributes" key. With the above changes, it worked perfectly.

[sammcj]

Did this end up getting resolved?

[mtodd]

@sammcj looks like this didn't, since this is still in master:

ruby-net-ldap/lib/net/ber/core_ext/true_class.rb

Line 10 in 449370b

"\001\001\001"

Unclear what the right way to handle the encoding is. I think that value should end up looking like: "\x01\x01\0xff" but I'm not certain. Tried to look for some concrete examples of correct handling elsewhere but came up short in my cursory search.

[mtodd]

Opened #142 to fix this issue. 1.8.7 was retired over a year ago, so we can safely move on assuming 1.9+ support.