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

Add more config options to saslauthd.conf #1708

Merged
merged 2 commits into from
Dec 8, 2020
Merged

Add more config options to saslauthd.conf #1708

merged 2 commits into from
Dec 8, 2020

Conversation

williamdes
Copy link
Contributor

Follow up of: #980
Fixes: #1704

  • SASLAUTHD_LDAP_TLS_CACERT_FILE => ldap_tls_cacert_file
  • SASLAUTHD_LDAP_TLS_CACERT_DIR => ldap_tls_cacert_dir
  • SASLAUTHD_LDAP_PASSWORD_ATTR => ldap_password_attr
  • SASLAUTHD_LDAP_AUTH_METHOD => ldap_auth_method
  • SASLAUTHD_LDAP_MECH => ldap_mech

Note: actually I have to have 2 values for the userPassword attribute in my LDAP because sasl will not authenticate using my ssha256 stored password value, this pull-request is my hope to address my current issue.

Documentations used:

- SASLAUTHD_LDAP_PASSWORD_ATTR => ldap_password_attr
- SASLAUTHD_LDAP_AUTH_METHOD => ldap_auth_method
- SASLAUTHD_LDAP_MECH => ldap_mech
@erik-wramner
Copy link
Contributor

This looks fine to me, but it would have been nice if someone who uses ldap could have a look. @fbartels or @aendeavor you don't use ldap either, do you?

@georglauterbach
Copy link
Member

georglauterbach commented Dec 8, 2020

I do not use LDAP either, but to be honest, this seems absolutely fine to me. The guidelines for scripting were read, implementation and documentation is flawless too. I feel like we won't get anyone anytime soon to who uses LDAP and can review these changes.

@erik-wramner I'd say it's fine to merge this, what do you think?

EDIT: The only thing that could be done is adding one or two tests to test/mail_with_ldap.bats, if it's appropriate. I will however not flag this with missing integration tests. @erik-wramner if you feel like these tests should be added, you can label it appropriately.

@williamdes
Copy link
Contributor Author

EDIT: The only thing that could be done is adding one or two tests to test/mail_with_ldap.bats, if it's appropriate. I will however not flag this with missing integration tests. @erik-wramner if you feel like these tests should be added, you can label it appropriately.

I am totally in favor of adding unit tests but I could not find a codecov like percentage and this did discourage me from writing tests
I have coverage for this project written in shell scripts: https://github.com/williamdes/sql-backup

By the way, a enormous thank you for this repository that solved a nightmare of implementations by adding a docker way of doing a cool web server.

I have only one note: It seems to be missing things or a little bit of paint to make this project 100% percent LDAP TLS strict encryption compatible ;)
But you are quite quick at review and response and I would agree to contribute more because your project is run on my production server and I need everything to be clean and 0% hacky.

@georglauterbach
Copy link
Member

I understand. Yeah, I would like to see Codecov too, but: I will propose this in #1699, where adding this action I would deem appropriate:)

I've decided that this fine for merging. @erik-wramner if there are issue in the future why are a result of this PR, I think it's fine to have @williamdes look at them (too) as he seems to use LDAP ;)

You mentioned:

I have only one note: It seems to be missing things or a little bit of paint to make this project 100% percent LDAP TLS strict encryption compatible ;)

That would be awesome.

@georglauterbach georglauterbach merged commit 2629b57 into docker-mailserver:master Dec 8, 2020
@williamdes williamdes deleted the feature/sasl-options branch December 8, 2020 21:16
@williamdes
Copy link
Contributor Author

Here is my repository, I will try to add tests using real world tests and harden the security as much as possible
https://github.com/desportes/infrastructure

@williamdes
Copy link
Contributor Author

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.

Allow to specify specify ldap_tls_cacert_file or/and ldap_tls_cacert_dir
3 participants