Stop using github.com/go-ldap/ldap/v3 ParseDN and use a custom ParseDN function instead

[inteon]

The github.com/go-ldap/ldap/v3 ParseDN function is too complex and has memory leaks. Additionally, this PR removes the dependency on github.com/go-ldap/ldap/v3.

Note for reviewer: I would still like to backport a quick fix to older releases that removes support for =# in RDNs.

Kind

/kind cleanup

Release Note

NONE

[inteon]

@inteon wrote:

The github.com/go-ldap/ldap/v3 ParseDN function is too complex and has memory leaks. Additionally, this PR removes the dependency on github.com/go-ldap/ldap/v3.

Have these bugs been reported to the github.com/go-ldap/ldap project? Why not submit these fixes upstream and use a github.com/cert-manager/ldap fork while we're waiting for those fixes to be accepted?

1. Please create and link to two upstream issues or PRs: one explaining the memory leak and another explaining the unnecessary complexity.
2. Please change this PR to kind/bug and add a release note explaining what bug has been fixed.

@wallrj This is the upstream issue: https://github.com/go-ldap/ldap/issues/472

I do think it is not a good idea to import an LDAP library just to use a single function that has nothing to do with LDAP. ParseDN is used to parse a string representation of a Relative Distinguished Name so we can set it as the subject of our x509 certificates.

The bug has been fixed in https://github.com/cert-manager/cert-manager/pull/6770 already (which was backported). This PR aims to get rid of the LDAP library dependency and have a well-tested high quality single function in-tree instead.

Also, the same issue was reported by notary already and the issue was not yet fixed in go-ldap: https://github.com/golang/vulndb/issues/1589

[wallrj]

@wallrj This is the upstream issue: go-ldap/ldap#472

• https://github.com/go-ldap/ldap/issues/472

You'll think I'm being a pain, but given that this PR description says "The github.com/go-ldap/ldap/v3 ParseDN function is too complex and has memory leaks." I can't review this unless I know more about the memory leak. That linked issue says nothing about a memory leak.

[inteon]

@wallrj This is the upstream issue: go-ldap/ldap#472

• FuzzParseDN test was failed on GitHub Actions go-ldap/ldap#472

You'll think I'm being a pain, but given that this PR description says "The github.com/go-ldap/ldap/v3 ParseDN function is too complex and has memory leaks." I can't review this unless I know more about the memory leak. That linked issue says nothing about a memory leak.

The memory leak was already fixed in https://github.com/cert-manager/cert-manager/pull/6770 but with loss of functionality (we no longer support the "=#" notation). This PR re-introduces that notation and in the PR description there is a benchmark that shows that this implementation does not have the memory leak.

[wallrj]

The memory leak was already fixed in https://github.com/cert-manager/cert-manager/pull/6770 but with loss of functionality (we no longer support the "=#" notation). This PR re-introduces that notation and in the PR description there is a benchmark that shows that this implementation does not have the memory leak.

1. I stand by my earlier suggestion that you should create an issue in the ldap repo informing them of the memory leak and explaining how we found it, and how we have fixed it and submit a PR to them if you have time.

2. I disagree with "not a good idea to import an LDAP library just to use a single function that has nothing to do with LDAP". The original motivation for the literalSubject feature, was compatibility with LDAP servers and documentation for that field says it has something to do with LDAP, so what could be more appropriate than to import parseDN from the go-ldap library so that we can hope to parse these in a way that is and continues to be compatible with LDAP. https://github.com/cert-manager/cert-manager/blob/0b379e4b5cdaf93000335a80bbea18c992895633/pkg/apis/certmanager/v1/types_certificate.go#L121-L128

[wallrj]

The bug that we found in ParseDN (https://github.com/cert-manager/cert-manager/pull/6770) has been found before by other projects (eg. https://github.com/golang/vulndb/issues/1589), but was not fixed (so we cannot rely on them to provide us a sound implementation of this single function that we import).

As far as I can see, the problem in CVE-2023-25656 / GHSA-87x9-7grx-m28v was reported to notaryproject but not to the go-ldap project.

[ThatsMrTalbot]

/lgtm

Code looks ok to me, is also behind an alpha feature gate

[inteon]

Thanks @ThatsMrTalbot. Also thanks @wallrj for the feedback that helped me to add more context to this PR. /approve

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• ~~OWNERS~~ [inteon]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment