Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Refactor ParseDN function to fix resource usage and invalid parsings (fixes #487) #497

Merged
merged 4 commits into from
Apr 1, 2024

Conversation

cpuschma
Copy link
Member

@cpuschma cpuschma commented Apr 1, 2024

This PR reworks the ParseDN function, which has been very resource consuming for reading one byte at a time and couldn't handle certain characters. This implementation is based upon inteons PR for cert-manager, which wasn't fully compatible with the RFC 4514.

This PR also:

  • Moves an unexported variable to a function scoped constant to free up the name hex
  • Renames the imported package enchex to it's original name hex for better readabilty
  • Improves the readability of ParseDN test failures

If this resolves the reported problems, we can revert #466.

@cpuschma
Copy link
Member Author

cpuschma commented Apr 1, 2024

Additionally, results of some benchmarking benchmark:

Summary

goos: linux
goarch: amd64
pkg: github.com/go-ldap/ldap
cpu: AMD Ryzen 5 5600X 6-Core Processor             
                │   old.txt    │               new.txt               │
                │    sec/op    │   sec/op     vs base                │
ParseSubject-12   2392.0n ± 1%   754.2n ± 1%  -68.47% (p=0.000 n=15)

                │   old.txt   │              new.txt               │
                │    B/op     │    B/op     vs base                │
ParseSubject-12   1865.0 ± 0%   408.0 ± 0%  -78.12% (p=0.000 n=15)

                │  old.txt   │              new.txt               │
                │ allocs/op  │ allocs/op   vs base                │
ParseSubject-12   58.00 ± 0%   10.00 ± 0%  -82.76% (p=0.000 n=15)

Old implementation:

BenchmarkParseSubject-12    	  673922	      2258 ns/op	    1865 B/op	      58 allocs/op
BenchmarkParseSubject-12    	  519794	      2403 ns/op	    1865 B/op	      58 allocs/op
BenchmarkParseSubject-12    	  458138	      2411 ns/op	    1865 B/op	      58 allocs/op
BenchmarkParseSubject-12    	  426906	      2440 ns/op	    1865 B/op	      58 allocs/op
BenchmarkParseSubject-12    	  502710	      2369 ns/op	    1865 B/op	      58 allocs/op

New implementation::

BenchmarkParseSubject-12    	 1606474	       773.1 ns/op	     408 B/op	      10 allocs/op
BenchmarkParseSubject-12    	 1554079	       768.1 ns/op	     408 B/op	      10 allocs/op
BenchmarkParseSubject-12    	 1594885	       760.0 ns/op	     408 B/op	      10 allocs/op
BenchmarkParseSubject-12    	 1564299	       754.2 ns/op	     408 B/op	      10 allocs/op
BenchmarkParseSubject-12    	 1547341	       755.9 ns/op	     408 B/op	      10 allocs/op

Copy link
Member

@johnweldon johnweldon left a comment

Choose a reason for hiding this comment

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

LGTM, except for the comment typo

dn.go Outdated Show resolved Hide resolved
v3/dn.go Outdated Show resolved Hide resolved
@cpuschma cpuschma merged commit ff74b2c into go-ldap:master Apr 1, 2024
22 checks passed
}

var rawValue asn1.RawValue
result, err := asn1.Unmarshal(decoded, &rawValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

@cpuschma @johnweldon
Great to see you are improving this function!
There is one small issue with this approach that I also figured out too late: asn1.Unmarshal( only supports DER parsing, while the github.com/go-asn1-ber/asn1-ber library supports BER parsing too.
This means that parseDN deviates from the RFC, possibly resulting in unexpected limitations for the user.

Copy link
Member Author

@cpuschma cpuschma Apr 2, 2024

Choose a reason for hiding this comment

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

I'll look into this and also extend the test cases. We didn't catch on that either. Thank you for pointing this out, @inteon!

Copy link
Contributor

Choose a reason for hiding this comment

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

I also have some extra testcases that might be useful. Will create a PR soon.

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

Successfully merging this pull request may close these issues.

3 participants