Skip to content

Conversation

@trestletech
Copy link
Contributor

nil packets have been observed when using StartTLS on a connection that
doesn't support it. These previously caused a panic but now will just
result in an unhelpful error.

Mimicking a similar strategy in

ldap/ldap.go

Lines 89 to 93 in 07a7330

defer func() {
if r := recover(); r != nil {
err = NewError(ErrorDebugging, errors.New("ldap: cannot process packet to add descriptions"))
}
}()

The nil packets on invalid StartTLS attempts should eventually be
corrected, but this makes the failure much more graceful.

Closes #58

error.go Outdated
}
}()

if len(packet.Children) >= 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather see a nil check on packet, and on response below, than expect a panic

@trestletech
Copy link
Contributor Author

Updated accordingly, @liggitt

@trestletech
Copy link
Contributor Author

Hi @liggitt. Not trying to push, but curious if you had an ETA on this change. We're trying to decide if we should vendor our fork temporarily or wait for the merge.

Thanks again.

@liggitt
Copy link
Contributor

liggitt commented Apr 13, 2016

sorry for the delay, go ahead and squash to a single commit for merge

nil packets have been observed when using StartTLS on a connection that
doesn't support it. These previously caused a panic but now will just
result in an unhelpful error.

The nil packets on invalid StartTLS attempts should eventually be
corrected, but this makes the failure much more graceful.
@trestletech
Copy link
Contributor Author

Thanks, @liggitt. Just force-pushed the squashed commit.

@liggitt liggitt merged commit a3bce49 into go-ldap:master Apr 13, 2016
@liggitt
Copy link
Contributor

liggitt commented Apr 13, 2016

thanks for the contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants