Skip to content
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

fix: packet referral decoding #413

Merged
merged 2 commits into from Feb 9, 2023

Conversation

james-d-elliott
Copy link
Contributor

This adjusts the packet referral decoding behavior to more appropriately handle edge cases and allows to more easily obtain the underlying packet in the event of a decoding error. Instead of matching ASN.1 BER packets on the tag for referrals we match on the class and type. This is particularly important in regards to OpenLDAP which returns a ASN.1 BER Generalized Time tag for referrals.

While I think this is a bug on the OpenLDAP side it's also perfectly reasonable in my opinion to handle this here as it's relatively easy to do so.

This adjusts the packet referral decoding behavior to more appropriately handle edge cases and allows to more easily obtain the underlying packet in the event of a decoding error. Instead of matching ASN.1 BER packets on the tag for referrals we match on the class and type. This is particularly important in regards to OpenLDAP which returns a ASN.1 BER Generalized Time tag for referrals.
Copy link
Member

@cpuschma cpuschma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you for your PR! 🥰

v3/request.go Outdated Show resolved Hide resolved
@james-d-elliott
Copy link
Contributor Author

james-d-elliott commented Feb 9, 2023

By the way I know a lot of people are busy lately and 2 weeks is not really that long, and it's by no means a complaint just curious. In the future is there a way to get your attention that isn't irritating and that you'd welcome?

@cpuschma
Copy link
Member

cpuschma commented Feb 9, 2023

Sorry for the silence in between, I understand that this can be annoying for you who lovingly creates a PR.

I can't speak for the other maintainers, but I think one way would be you mention @go-ldap/committers. Alternatively if a topic is urgent or something has gone unnoticed for too long, my email box is open cp at lumen.sh.

@james-d-elliott
Copy link
Contributor Author

james-d-elliott commented Feb 9, 2023

Sorry for the silence in between, I understand that this can be annoying for you who lovingly creates a PR.

I can't speak for the other maintainers, but I think one way would be you mention @go-ldap/committers. Alternatively if a topic is urgent or something has gone unnoticed for too long, my email box is open cp at lumen.sh.

Yeah it's no worries at all, I really want to stress that am usually very direct / blunt to a fault (what you see is what you get). Life can get busy, especially in the early months which I totally understand. I just wondered if there was a preferred way that wasn't going to bother you guys.

@cpuschma cpuschma merged commit fe91542 into go-ldap:master Feb 9, 2023
@james-d-elliott james-d-elliott deleted the fix-ldap-referral branch February 9, 2023 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants