made controls parameter optional in client starttls

[undoingtech]

Regarding #326

Warning: I was unable to properly test this code and confirm it works. I did not actually get TLS to work. I suspect I messed up the certificates somehow.

I only think this code works is because the error messages are different.
Before changes, there was an Object.key assert error - which happens when there are missing arguments (controls).
After changes, there is a "unsupported extended operation" error - which points to TLS certificates not being properly setup (but omitting controls is fine).

[jsumners]

I did not actually get TLS to work.

Can you explain? You should be able to run npm test:integration:local which starts a Docker container that exposes both ports 389 and 636.

[undoingtech]

I tried running npm run test:integration:local. I got a "no basic auth credentials" error. I tried docker-compose up -d by itself, and it produced the same auth error. Is the docker container private?

---

I will try to clarify my "testing process". Originally, the plan was to make a LDAP server, make a LDAP client, bind with TLS, make one search, and unbind.

My setup:

Server: The /etc/passwd server example. I added TLS options to the server in var server = ldap.createServer(tlsOptions). The server runs without errors.

Client: A test client that just binds, makes one search, and unbinds. Code mostly copied from Client API. I added client.starttls(opts, [], function(err, res))....

I tried to get the client to bind and make a search, but I never got it to bind at all. I still wanted to make sure my committed code worked, so I tested by error.

starttls parameters	committed code	error
(opt, [], function(err, res))	none	"unsupported extended operation"
(opt, [], function(err, res))	changes to make controls parameter optional	"unsupported extended operation"
(opt, function(err, res))	none	"Object.keys(cb).forEach(function (k) { ..." "TypeError: Cannot convert undefined or null to object"
(opt, function(err, res))	changes to make controls parameter optional	"unsupported extended operation"

TL;DR: Before I made changes, ldapjs would give a "TypeError" for omitting the controls parameter in client.starttls(). After I made changes (to make the controls parameter optional), ldapjs would give a "unsupported extended operation" error. I think the code I've committed works. It makes the controls parameter optional as intended. I am just not knowledgeable enough about LDAP or TLS to actually test it properly. :(

(p.s. sorry for much reading)

[jsumners]

I got a "no basic auth credentials" error. I tried docker-compose up -d by itself, and it produced the same auth error. Is the docker container private?

Ugh. you might have to authenticate to pull the image -- https://docs.github.com/en/packages/using-github-packages-with-your-projects-ecosystem/configuring-docker-for-use-with-github-packages . GitHub really needs to make accessing images published to their repos easier.

I think the code I've committed works. It makes the controls parameter optional as intended. I am just not knowledgeable enough about LDAP or TLS to actually test it properly. :(

Unfortunately, we can't commit code based on "I think it works." We need to figure out how to test it either via the unit tests or via the "integration" tests. Ideally, we'd test it via the unit tests.

How did you create and load the certificate for your custom LDAP server test?

[undoingtech]

Thank you for the github docs link. That was exactly what I needed.

I was able to run the tests on my code. Everything said PASS or OK. It also told me to run standard --fix. I did that and pushed those changes as well. Now the lint test is passed.

For npm run test:integration

Summary Results:
Suites:   2 passed, 2 of 2 completed
Asserts:  25 passed, of 25
Time:     2s

For npm run test

 SKIP  test/utils.js > no tests found 598ms
Suites:   56 passed, 1 skip, 57 of 57 completed
Asserts:  2703 passed, 1 skip, of 2704
Time:     57s

---

How did you create and load the certificate for your custom LDAP server test?

I followed this tutorial: https://www.golinuxcloud.com/configure-openldap-with-tls-certificates/

Server:

tlsOptions = {
  certificate: fs.readFileSync('/home/hannie/ldapjs/learn-ldapjs/golinuxcloud/ca.cert.pem'),
  key: fs.readFileSync('/home/hannie/ldapjs/learn-ldapjs/golinuxcloud/ca.key'),
  passphrase: "ldapjs",
  ca: [ fs.readFileSync('/home/hannie/ldapjs/learn-ldapjs/golinuxcloud/ldap.client.crt') ],
  rejectUnauthorized : false
};
var server = ldap.createServer(tlsOptions);

Client:

var opts = {
  ca: [fs.readFileSync('/home/hannie/ldapjs/learn-ldapjs/golinuxcloud/ca.cert.pem')],
  host: 'ldaps://127.0.0.1:1389',
  key: fs.readFileSync('/home/hannie/ldapjs/learn-ldapjs/golinuxcloud/ldap.client.key'),
  cert: fs.readFileSync('/home/hannie/ldapjs/learn-ldapjs/golinuxcloud/ldap.client.crt'),
  rejectUnauthorized: false
};

var client = ldap.createClient({
  url: 'ldap://127.0.0.1',
  port: 1389,
  tlsOptions: opts
});

client.starttls(opts, [], function(err, res) {
  console.log("Inside starttls before error assert."); // <-- runs
  assert.ifError(err); // <-- makes error

  client.bind('cn=root', 'secret', function(err) {
    assert.ifError(err);
  });
  console.log("Connection has been established.");
});

[jsumners]

So now that you have a working test suite, can you write a test that covers this change?

[undoingtech]

I tried making 2 subtests:

Subtest	Expected Result
an empty Controls object is passed to starttls: starttls(opts, new Control(), callback())	req.controls = [Control]
the controls argument is skipped: starttls(opts, callback())	req.controls = []

However, I cannot ever get req to be defined. It is only defined if a TLS connection is established successfully. I still do not know how to get LDAP over TLS to work.

[jsumners]

Doesn't our Docker container expose a TLS enabled port?

[undoingtech]

The docker container does expose a TLS port. I have remade the TLS certificates using docker instructions to work with docker. As far as I know, to use starttls, you connect to a regular port first, then run starttls, and starttls will connect to the TLS port.

I am still unsure if I have it working properly. The client says the starttls worked. JSON.stringify(client._starttls) = {"started":true,"success":true}. However, there is still no response. res = undefined.

The code I submitted changes how the controls argument is handled. To test that, I think I need the response to be defined. I think the response will contain the controls to test.

Here is the test I have written if you want to look at it. I am not experienced with ldapjs or nodejs, so I could be missing something really obvious. The t.ok(res) tests fail.

tap.beforeEach((done, t) => {
	t.context.clientTLSoptions = {
		ca: [fs.readFileSync('/home/hannie/node-ldapjs/dockerTLS/ca.pem')],
		key: fs.readFileSync('/home/hannie/node-ldapjs/dockerTLS/key.pem'),
		cert: fs.readFileSync('/home/hannie/node-ldapjs/dockerTLS/cert.pem'),
		host: '0.0.0.0',
		port: 636,
		rejectUnauthorized: false
	}

	const client = ldap.createClient({
		url: 'ldap://0.0.0.0:389'
	})
	t.context.client = client
	client.on('connect', () => done())
})

tap.afterEach((done, t) => {
	console.log('client._starttls: ' + JSON.stringify(t.context.client._starttls))
	t.context.client.unbind(function(err) {
		t.error(err, 'testing unbind error')
	})
	t.context.client.on('close', () => done())
})

tap.test('starttls with control argument', t => {
	const control = new Control()
	t.context.client.starttls(t.context.clientTLSoptions, control, function(err, res) {
		t.error(err, 'testing with control starttls error')
		t.ok(res, 'testing with control starttls res')
		console.log('res: ' + JSON.stringify(res))
		t.end()
	})
})

tap.test('starttls without control argument', t => {
	t.context.client.starttls(t.context.clientTLSoptions, function(err, res) {
		t.error(err, 'testing no control starttls error')
		t.ok(res, 'testing no control starttls res')
		t.end()
	})
})

[jsumners]

👋

On February 22, 2023, we released version 3 of this library. As a result, we are closing this issue/pull request.

Please see issue #839 for more information, including how to proceed if you feel this closure is in error.