Summary

Improve TLS support for Active Directory.

Review Request #11896 — Created Dec. 17, 2021 and submitted Jan. 12, 2022, 12:07 a.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

The Active Directory support was largely built around ldap:// URIs,
rather than supporting ldaps://. This did attempt to invoke TLS
support, which would switch over to a secure connection if available,
but that either wasn't working or didn't work in all configurations.

This change reworks the TLS support to do a few things:

1) Assign a default port of 636 (the LDAP TLS port) if an explicit port
was not provided and TLS was enabled.

2) Assume TLS if the port is 636 (even if TLS is not enabled -- mostly
intended for DNS-based AD domain controller lookup).

3) Specifically opt into LDAP 3, which should be safe for modern usage
(and we opt into it for the standard LDAP backend). This is
documented as being the default anyway, but it's better to opt into
it.

4) Use ldaps:// when TLS is enabled.

5) Avoid calling start_tls_s(), which may not be required for ldaps://
URLs (and appears to cause errors stating that TLS has already been
started with ldaps://). Based on other python-ldap examples for
Active Directory, it seems this should not be required.

Testing Done

Unit tests passed.

Tested this with a customer using TLS, with and without an explicit
port configured. Tested that start_tls_s() failed with a ldaps://
URI and that TLS support worked with ldaps:// without start_tls_s().

Commits

Summary	ID
Improve TLS support for Active Directory. The Active Directory support was largely built around `ldap://` URIs, rather than supporting `ldaps://`. This did attempt to invoke TLS support, which would switch over to a secure connection if available, but that either wasn't working or didn't work in all configurations. This change reworks the TLS support to do a few things: 1) Assign a default port of 636 (the LDAP TLS port) if an explicit port was not provided and TLS was enabled. 2) Assume TLS if the port is 636 (even if TLS is not enabled -- mostly intended for DNS-based AD domain controller lookup). 3) Specifically opt into LDAP 3, which should be safe for modern usage (and we opt into it for the standard LDAP backend). This is documented as being the default anyway, but it's better to opt into it. 4) Use `ldaps://` when TLS is enabled. 5) Avoid calling `start_tls_s()`, which may not be required for `ldaps://` URLs (and appears to cause errors stating that TLS has already been started with `ldaps://`). Based on other python-ldap examples for Active Directory, it seems this should not be required.	d2380ff99a7a05b497bc09b0006c0a950daf1d4e

Summary

Improve TLS support for Active Directory.

The Active Directory support was largely built around `ldap://` URIs, rather than supporting `ldaps://`. This did attempt to invoke TLS support, which would switch over to a secure connection if available, but that either wasn't working or didn't work in all configurations. This change reworks the TLS support to do a few things: 1) Assign a default port of 636 (the LDAP TLS port) if an explicit port was not provided and TLS was enabled. 2) Assume TLS if the port is 636 (even if TLS is not enabled -- mostly intended for DNS-based AD domain controller lookup). 3) Specifically opt into LDAP 3, which should be safe for modern usage (and we opt into it for the standard LDAP backend). This is documented as being the default anyway, but it's better to opt into it. 4) Use `ldaps://` when TLS is enabled. 5) Avoid calling `start_tls_s()`, which may not be required for `ldaps://` URLs (and appears to cause errors stating that TLS has already been started with `ldaps://`). Based on other python-ldap examples for Active Directory, it seems this should not be required.

d2380ff99a7a05b497bc09b0006c0a950daf1d4e

Issues

Description	From	Last Updated
F841 local variable 'E' is assigned to but never used	reviewbot	Dec. 17, 2021, 1:03 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/accounts/backends/ad.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'E' is assigned to but never used
```

Change Summary:

Fixed a missing e and an accidental capitalization of another.

Commits:

	Summary	ID
	Improve TLS support for Active Directory. The Active Directory support was largely built around `ldap://` URIs, rather than supporting `ldaps://`. This did attempt to invoke TLS support, which would switch over to a secure connection if available, but that either wasn't working or didn't work in all configurations. This change reworks the TLS support to do a few things: 1) Assign a default port of 636 (the LDAP TLS port) if an explicit port was not provided and TLS was enabled. 2) Assume TLS if the port is 636 (even if TLS is not enabled -- mostly intended for DNS-based AD domain controller lookup). 3) Specifically opt into LDAP 3, which should be safe for modern usage (and we opt into it for the standard LDAP backend). This is documented as being the default anyway, but it's better to opt into it. 4) Use `ldaps://` when TLS is enabled. 5) Avoid calling `start_tls_s()`, which may not be required for `ldaps://` URLs (and appears to cause errors stating that TLS has already been started with `ldaps://`). Based on other python-ldap examples for Active Directory, it seems this should not be required.	84b141a89eff705d33746f93ffd483dc6d5b97c7
	Improve TLS support for Active Directory. The Active Directory support was largely built around `ldap://` URIs, rather than supporting `ldaps://`. This did attempt to invoke TLS support, which would switch over to a secure connection if available, but that either wasn't working or didn't work in all configurations. This change reworks the TLS support to do a few things: 1) Assign a default port of 636 (the LDAP TLS port) if an explicit port was not provided and TLS was enabled. 2) Assume TLS if the port is 636 (even if TLS is not enabled -- mostly intended for DNS-based AD domain controller lookup). 3) Specifically opt into LDAP 3, which should be safe for modern usage (and we opt into it for the standard LDAP backend). This is documented as being the default anyway, but it's better to opt into it. 4) Use `ldaps://` when TLS is enabled. 5) Avoid calling `start_tls_s()`, which may not be required for `ldaps://` URLs (and appears to cause errors stating that TLS has already been started with `ldaps://`). Based on other python-ldap examples for Active Directory, it seems this should not be required.	d2380ff99a7a05b497bc09b0006c0a950daf1d4e

Diff:

Revision 2 (+68 -50)

Show changes

reviewboard/accounts/backends/ad.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-4.0.x (5f67587)