Skip to content

LDAP Auth#601

Open
yaemiku wants to merge 89 commits intosentriz:masterfrom
yaemiku:ldap_auth
Open

LDAP Auth#601
yaemiku wants to merge 89 commits intosentriz:masterfrom
yaemiku:ldap_auth

Conversation

@yaemiku
Copy link
Copy Markdown

@yaemiku yaemiku commented Sep 21, 2025

I have come across PR #346 while looking for a way to have a music server that integrates with LDAP. I managed to get it working for me. I have

  • Fixed the LDAP authentication logic. Auth is performed solely against the LDAP database. Users who pass the user filter are automatically registered when trying to log in for the first time. Users who pass the admin filter are assigned admin privileges
  • While testing, I noticed that the "Tempo" subsonic client sends the password parameter hex-encrypted (in the form of "p=enc:HEXHERE"). Added support for that as well

@ghost
Copy link
Copy Markdown

ghost commented Nov 1, 2025

Heya! Thanks a bunch for picking this up, I meant to fix that linting error but life got in the way. I've done a very informal glance over the code and everything looked pretty good, but I just had two questions/comments. The first is on:

+ //nolint:gochecknoglobals
  var ldapStore = make(LDAPStore)

This line is the reason that #346 wasn't getting merged, ldap.LDAPStore needs to be refactored out of ldap/ldap.go and into cmd/gonic/gonic.go to match the style of the rest of the code. I did the work to get ldap.Config into there, but never got a chance to do that with ldap.LDAPStore. In hindsight a very easy solution to this would be to create a structure within ldap/ldap.go, say ldap.State, that has fields for both the configuration and authentication cache.

- if doesLDAPUserExist(username, config, l) && !isAdmin {
+ if !doesLDAPUserExist(username, config, l) {

Does this work if someone wants to configure two different filters for admin and user accounts? I know it seems like an edge case, but given how different each LDAP server is configured it didn't seem like that bad of an idea when I was originally writing that code.

Just out of curiosity, how has it been working for you over the past couple of weeks?

@sentriz sentriz force-pushed the master branch 5 times, most recently from 7decea6 to 971d22d Compare November 11, 2025 07:01
@yaemiku
Copy link
Copy Markdown
Author

yaemiku commented Dec 28, 2025

Answering all of these in reverse order :)

Yes, it has been working great! There are still some things I'd like to see (like song play counts etc.) but it's not of concern to this PR. New users get their accounts auto-created created in gonic when they log in using a subsonic client with LDAP credentials which is really convenient - and getting to the next thing - it's possible due to the line change you have mentioned. I've tested it and it works for having a separate user and admin filter, although there isn't the nice quality-of-life capability of checking/changing admin privileges each time you log in - it gets set when you register.

As for the linting error, I'm not much of a Go developer, but I'll try to move things to cmd/gonic/gonic.go in the coming days

@mantralunar
Copy link
Copy Markdown

+1 for this!! LDAP support would really complete the usability of this as a service! Look forward to seeing where this goes.

@yaemiku
Copy link
Copy Markdown
Author

yaemiku commented Feb 15, 2026

Sorry for the delay, lots of stuff has been happening lately. I hope this is how you have envisioned moving ldap.LDAPStore into cmd/gonic/gonic.go. I have also added appropriate entries for the environmental variables concerning LDAP in README.md, including a comment about escaping commas, since I've run into not doing that being an issue (I guess flagconf interprets a string with commas as an array and picks out the last entry as the final value being passed down, but as I have stated - I'm not much of a Go developer - so I'm not acquainted with it)

@mantralunar
Copy link
Copy Markdown

Loving the work you've done @yaemiku. Im no expert, but from the env documentation you've included in your fork, and doing a bit of diving through the logs, it looks like the bind user is looked up using a hardcoded uid=, is it possible to make this part of the variable? my workflow uses cn= but Im unable to bind with it as its automatically inserting uid= beforehand (uid=cn=myuser)... Would love to get this up and running! :)

@yaemiku
Copy link
Copy Markdown
Author

yaemiku commented Feb 18, 2026

Added another flag to change that behavior. Let's for example say that you have a user username=xyz,ou=users,dc=domain,dc=org, then you'd set in your env:

GONIC_LDAP_BASE_DN='ou=users\,dc=domain\,dc=org'
GONIC_LDAP_USERNAME_ATTR=username

Hope that helps :)

@mantralunar
Copy link
Copy Markdown

Thank you for the quick fix! Seems to be working now! Now to sort out my syntax for the admin filters! Much appreciated!

@sentriz sentriz force-pushed the master branch 2 times, most recently from 66c0eb5 to b2674f8 Compare February 24, 2026 14:51
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