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

escape DN attribute values #4117

Merged
merged 2 commits into from
Jul 7, 2022
Merged

escape DN attribute values #4117

merged 2 commits into from
Jul 7, 2022

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented Jul 6, 2022

Description

Escape the DN when creating a user or a group.

Motivation and Context

The DN can contain characters which need to be escaped. Otherwise some subsequent steps which involve parsing the DN will fail.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • Code changes
  • Unit tests added

@C0rby C0rby requested review from rhafer, butonic and kobergj July 6, 2022 13:18
@C0rby C0rby self-assigned this Jul 6, 2022
Copy link
Contributor

@rhafer rhafer left a comment

Choose a reason for hiding this comment

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

Nice work! Just a small nit. Looks good to me otherwise.

Somehow I think we should also try to submit this to https://github.com/go-ldap/ldap. wdyt?

">", "\\>",
";", "\\;",
"=", "\\=",
"\000", "\\\000",
Copy link
Contributor

@rhafer rhafer Jul 6, 2022

Choose a reason for hiding this comment

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

If I am reading rfc4514 correctly the null byte as to be escaped as \00 which would be:

Suggested change
"\000", "\\\000",
"\000", "\\00",

Only ' ', '"', '#', '+', ',', ';', '<', '=', '>', or '\' can be escape by prepending a backslash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a tricky one but I argue that this is correct.

The left side is the "search" pattern so we are looking for the null byte which as a string is represented like \000 (octal) or \x00 (hex) both forms result in the null byte.

Now the right (replace) part is actually to things. First \\ and second \000 Together it results in \00 but in string form it needs to be typed as it is above.

https://go.dev/play/p/IzDMsTunmaj

Copy link
Contributor

Choose a reason for hiding this comment

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

I am still not fully convinced 🙂 what I am referring to is this sentence of the RFC:

Each octet of the character to be escaped is replaced by a backslash
and two hex digits, which form a single octet in the code of the
character.  Alternatively, if and only if the character to be escaped
is one of

      ' ', '"', '#', '+', ',', ';', '<', '=', '>', or '\'

it can be prefixed by a backslash ('\' U+005C).

So the null Byte (\000 in go) needs to be translated into the string \00 (backslash + two hex digits representing the single octet of the "null character"). As the backslash needs to be escaped in go we get \\00. (At least if I am not missing anything obvious)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, after researching a bit I found that you are right.
This also means that the python library I took inspiration from must be wrong 😅 : https://github.com/python-ldap/python-ldap/blob/44a593d1c55a007a43fcf30d2b027ac910ea1b96/Lib/ldap/dn.py#L31

Copy link
Contributor Author

@C0rby C0rby Jul 6, 2022

Choose a reason for hiding this comment

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

This is a tricky one but I argue that this is correct.

The left side is the "search" pattern so we are looking for the null byte which as a string is represented like \000 (octal) or \x00 (hex) both forms result in the null byte.

Now the right (replace) part is actually to things. First \\ and second \000 Together it results in \00 but in string form it needs to be typed as it is above.

https://go.dev/play/p/

I see now why this doesn't make sense. :D

@C0rby
Copy link
Contributor Author

C0rby commented Jul 6, 2022

Somehow I think we should also try to submit this to https://github.com/go-ldap/ldap. wdyt?

Yeah, I guess this could be helpful to more people. 👍

@ownclouders
Copy link
Contributor

💥 Acceptance test cs3ApiTests-ocis failed. Further test are cancelled...

@sonarcloud
Copy link

sonarcloud bot commented Jul 6, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

70.0% 70.0% Coverage
0.0% 0.0% Duplication

@C0rby C0rby merged commit ce6f85d into master Jul 7, 2022
@delete-merged-branch delete-merged-branch bot deleted the ldap-escape-dn branch July 7, 2022 07:25
ownclouders pushed a commit that referenced this pull request Jul 7, 2022
Merge: f739d18 00a27bf
Author: David Christofas <dchristofas@owncloud.com>
Date:   Thu Jul 7 09:25:23 2022 +0200

    Merge pull request #4117 from owncloud/ldap-escape-dn

    escape DN attribute values
@micbar micbar mentioned this pull request Jul 19, 2022
36 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants