nil pointer panic when StartTLS fails

[trestletech]

Thanks for creating and sharing this library!

I'm reproducibly getting a nil pointer panic when StartTLS fails. I can reproduce using a free account on jumpcloud.com and trying to use StartTLS on port 686 (the TLS-enabled port). Of course, this isn't expected to work, but hopefully we can avoid a panic.

conn, err := ldap.Dial("tcp", "ldap.jumpcloud.com:686")
if err != nil {
  log.Fatal("Error opening connection")
  return;
}
conn.StartTLS(&tls.Config{InsecureSkipVerify: ldapConfig.ServerTLSInsecure, ServerName: serverName})

The last line there will panic and the stack trace takes us through:
https://github.com/go-ldap/ldap/blob/v2.2.1/conn.go#L182

ldap/error.go

Line 102 in 07a7330

if len(packet.Children) >= 2 {

where we get a nil pointer. It seems that packet or Children must be nil.

The panic does not occur when conn.Debug is set to true. When Debugging is enabled, we get the following output (our own logging intermingled with this package's).

2016/04/11 19:18:11 /connect/src/connect/auth/providers/ldap/client.go:129: (ldap) Dialing unencrypted LDAP connection: ldap.jumpcloud.com:636
2016/04/11 19:18:11 /connect/src/connect/auth/providers/ldap/client.go:139: (ldap) Using StartTLS on LDAP connection: ldap.jumpcloud.com:636
LDAP Request: (Universal, Constructed, Sequence and Sequence of) Len=29 "<nil>"
 MessageID: (Universal, Primitive, Integer) Len=1 "1"
 Start TLS: (Application, Constructed, 0x17) Len=24 "<nil>"
  TLS Extended Command: (Context, Primitive, 0x00) Len=22 "1.3.6.1.4.1.1466.20037"
2016/04/11 19:18:11 flags&startTLS = 1
2016/04/11 19:18:11 1: waiting for response
2016/04/11 19:18:11 Sending message 1
2016/04/11 19:18:11 reader error: unexpected EOF
2016/04/11 19:18:11 Sending quit message and waiting for confirmation
2016/04/11 19:18:11 Shutting down - quit message received
2016/04/11 19:18:11 Closing channel for MessageID 1
2016/04/11 19:18:11 1: got response 0x0
2016/04/11 19:18:11 Closing network connection
2016/04/11 19:18:11 /connect/src/connect/auth/providers/ldap/client.go:143: (ldap) Failed to connect with TLS: LDAP Result Code 203 "": ldap: cannot process packet to add descriptions
2016/04/11 19:18:11 /connect/src/connect/auth/providers/ldap/ldap.go:180: (ldap) Unable to verify credentials: LDAP Result Code 203 "": ldap: cannot process packet to add descriptions

I'll have a PR opened with one solution for this shortly which will either defer a recover in the getLDAPResultCode function or check packet.Children for nil before checking length. But open to other approaches if anyone has any ideas.

[liggitt]

len(packet.Children) would not panic if Children was nil, I'd expect that packet is nil in this case.

[liggitt]

looks like https://github.com/go-ldap/ldap/pull/57/files#diff-a2ddca1600f34abb91244424f8431c2cL170 will also fix that issue

[trestletech]

Good point.

Narrow PR for just this issue opened in #59. I'll let you make the call on whether it's needed in light of #57.

[trestletech]

@liggitt would you consider a patch release to resolve this fix? We're trying to decide whether we should wait on gopkg.in or point to master directly. Thanks.

[liggitt]

yeah, I was looking at also including #57 in the next release, but since that has an interface change, I'll tag a bugfix version first

[liggitt]

tagged https://github.com/go-ldap/ldap/releases/tag/v2.2.2

[trestletech]

Thank you!