-
Notifications
You must be signed in to change notification settings - Fork 252
Test ber encoding of message_id > 128 #172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@jch maybe it's related to ApacheDS handling of message IDs over that limit? I'll try to reproduce locally. |
Yeah, I can confirm, this happens specifically in with ApacheDS, here: https://directory.apache.org/api/gen-docs/latest/apidocs/org/apache/directory/api/ldap/codec/actions/ldapMessage/StoreMessageId.html |
And the specific error is:
|
We're definitely not following the BER spec for integers, and OpenLDAP has proven to be lenient about its parsing/decoding (treating any int above zero as TRUE for bools). In this case, I'll work on adding a test for this. |
Specifically, I'll add BER-related unit tests instead of integration tests since that won't help us here. |
Was just looking at most significant bit.
Makes the format consistent (hex), and updated based on various sample sources.
# negatives | ||
# -1 => "\x02\x01\xFF", | ||
# -127 => "\x02\x01\x81", | ||
# -128 => "\x02\x01\x80" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These definitely fail.
This patch addresses the false-negative encoding issue, but it doesn't address encoding negative integers at all which is broken. That should be handled in a separate PR. This change appears to be working just fine with our integration tests, but I'd like to do a bit more manual validation to ensure proper functioning. |
Opened #173 with more major refactoring of the BER encoding/decoding for Integers, including adding support for negative Integer values. |
assert_equal "uid=user1,ou=People,dc=rubyldap,dc=com", entries.first.dn | ||
} | ||
message_id = client.instance_variable_get('@open_connection').instance_variable_get('@msgid') | ||
assert_operator message_id, :>, 256 # includes non-search messages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can drop this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped in 9dc1362
@mtodd feel free to ship this when you feel ready. |
Refactor BER encoding/decoding of Integers
Merged in #173 after validating these changes against OpenLDAP, Active Directory, ApacheDS, 389DS, FreeIPA, ODSEE, and OUD. Confident after these tests that this can be merged and released. |
Test ber encoding of message_id > 128
…ng-fix Test ber encoding of message_id > 128
@mtodd I'm not sure if there was an issue filed for this, but you mentioned there was a BER encoding issue for message ID's greater than 128. I expected this integration test against OpenLDAP to fail, but it does not. I double checked the same Connection is being reused, and that the message ID is incrementing. Was there other factors I missed?