8000 fix/ldap_anonymous_bind by tpw56j · Pull Request #2831 · cobbler/cobbler · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix/ldap_anonymous_bind #2831

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Dec 7, 2021
Merged

fix/ldap_anonymous_bind #2831

merged 17 commits into from
Dec 7, 2021

Conversation

tpw56j
Copy link
Contributor
@tpw56j tpw56j commented Nov 9, 2021

Inversion of the ldap_anonymous_bind value

@SchoolGuy
Copy link
Member

@tpw56j Could you somehow add tests so this doesn't happen again?

@codecov
Copy link
codecov bot commented Nov 9, 2021 •
edited
Loading

Codecov Report

Merging #2831 (0ac402b) into master (cc95e61) will increase coverage by 0.47%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2831      +/-   ##
==========================================
+ Coverage   43.44%   43.92%   +0.47%     
==========================================
  Files          99       99              
  Lines       14258    14301      +43     
==========================================
+ Hits         6195     6281      +86     
+ Misses       8063     8020      -43
Impacted Files Coverage Δ
cobbler/settings/migrations/V3_3_1.py 63.15% <37.50%> (-16.85%) ⬇️
cobbler/validate.py 70.37% <63.63%> (-0.24%) ⬇️
cobbler/modules/authentication/ldap.py 92.85% <96.42%> (+85.04%) ⬆️
cobbler/enums.py 100.00% <100.00%> (ø)
cobbler/settings/__init__.py 85.28% <100.00%> (+0.16%) ⬆️
cobbler/settings/migrations/V2_8_5.py 57.14% <0.00%> (-14.29%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc95e61...0ac402b. Read the comment docs.

@tpw56j
Copy link
Contributor Author
tpw56j commented Nov 9, 2021

@SchoolGuy I will try to write additional tests for the parameters in this PR.

@SchoolGuy
Copy link
Member

@tpw56j Thanks a lot!

@watologo1 watologo1 self-requested a review November 12, 2021 07:53
watologo1
watologo1 approved these changes Nov 12, 2021
View reviewed changes
Copy link
Contributor
@watologo1 watologo1 left a comment

Choose a reason for hiding this comment

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

I needed an ldap connect and used this code as reference code.
So I can confirm that the code works (I even could try an anonymous and non-anonymous connect) and that the patch is correct.

SchoolGuy
SchoolGuy approved these changes Nov 12, 2021
View reviewed changes
Copy link
Member
@SchoolGuy SchoolGuy left a comment

Choose a reason for hiding this comment

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

LGTM

@tpw56j
Copy link
Contributor Author
tpw56j commented Nov 16, 2021

@SchoolGuy To test authentication via ldap, we need an ldap server. 389-ds is available in all supported distributions. Can we include it in the docker image for testing?

@SchoolGuy
Copy link
Member

@tpw56j Yes please do so. I had my hopes up for openLDAP but whatever works is fine. Those images are for testing, so size does not matter.

@tpw56j
Copy link
Contributor Author
tpw56j commented Nov 16, 2021

@SchoolGuy I was also hoping that it would be possible to use openLDAP, but found that RedHat stopped supporting it and removed openLDAP from its repos. I myself use openLDAP and have never used 389-ds before, so it will take me a while to figure out how to test with 389-ds in different distros.

@SchoolGuy
Copy link
< 8000 /details>
Member

@tpw56j I would prefer you get started with openLDAP then on Debian and/or openSUSE. I believe that our code should be not that different for authentication on the different distros. At least that would be my proposal.

@tpw56j
Copy link
Contributor Author
tpw56j commented Nov 16, 2021

@SchoolGuy OpenLDAP is not available only in CentOS 8. With openLDAP it will be easier for me to start, and with CentOS it will be possible to figure it out later. Therefore, I agree that I will need to start with openLDAP.

@xavierba xavierba mentioned this pull request Nov 16, 2021
Closed
@SchoolGuy SchoolGuy added this to the v3.3.1 milestone Nov 17, 2021
@SchoolGuy SchoolGuy linked an issue Nov 17, 2021 that may be closed by this pull request
ldap authentication via anonymous bind is broken #2832
Closed
@SchoolGuy
Copy link
Member

@tpw56j The tests are all currently failing. Is this because you forgot to add & start the LDAP Server or is this not yet finished?

@tpw56j
Copy link
Contributor Author
tpw56j commented Nov 24, 2021

@SchoolGuy I haven't added openldap to the docker image yet, so this error is expected. I will continue to work with this PR.

It was a surprise for me that for a long time I could not pass a negative test for establishing a TLS connection until I understood the difference between ldaps and starttls and that in these cases different objects should be used to set TLS parameters. All code examples I could find as well as in authentication/ldap.py used the ldap object to set TLS options. At first glance, this code looks correct, but in reality, certificates are not checked and only create the illusion of security.

Maybe add to cobbler the ability to set other TLS parameters from python-ldap? At least OPT_X_TLS_REQUIRE_CERT, OPT_X_TLS_CIPHER_SUITE, OPT_X_TLS_CRLCHECK seem important enough to me.

@SchoolGuy
Copy link
Member

@tpw56j Okay let's do the following. You add the tests. We accept that they are failing because we know that we have a bug. Then at the same time we open a bug for this and anyone who has the time opens a followup PR for this. I see this as a separate matter from what you are trying to do with this PR.

@SchoolGuy
Copy link
Member

If you have a better way to do it then I am open for suggestions!

@tpw56j
Copy link
Contributor Author
tpw56j commented Nov 25, 2021

@SchoolGuy Maybe I will include the necessary packages in the docker image of at least one distro and we will get a passing test in it?

Fedora requires openldap-servers, openldap-clients, python3-ldap, sscg.
Please help me add the following commands to the docker image:

# FQDN=`hostname`
# sscg -f -v \
     --ca-file=/etc/pki/tls/certs/ca-slapd.crt \
     --ca-key-file=/etc/pki/tls/private/ca-slapd.key \
     --ca-key-password=cobbler \
     --cert-file=/etc/pki/tls/certs/slapd.crt \
     --cert-key-file=/etc/pki/tls/private/slapd.key \
     --client-file=/etc/pki/tls/certs/ldap.crt \
     --client-key-file=/etc/pki/tls/private/ldap.key \
     --lifetime=365 \
     --hostname=$FQDN \
     --email=root@$FQDN \
# chown ldap:ldap /etc/pki/tls/{certs/slapd.crt,private/slapd.key}
# systemctl stop slapd
# rm -rf /var/lib/ldap/*  /etc/openldap/slapd.d/*
# cp /test_dir/tests/test_data/slapd.conf /etc/openldap
# systemctl start slapd
# ldapadd -Y EXTERNAL -H ldapi:/// -f /test_dir/tests/test_data/ldap_test.ldif

@SchoolGuy
Copy link
Member

@tpw56j

ENV FQDN=`hostname`
RUN sscg -f -v \
     --ca-file=/etc/pki/tls/certs/ca-slapd.crt \
     --ca-key-file=/etc/pki/tls/private/ca-slapd.key \
     --ca-key-password=cobbler \
     --cert-file=/etc/pki/tls/certs/slapd.crt \
     --cert-key-file=/etc/pki/tls/private/slapd.key \
     --client-file=/etc/pki/tls/certs/ldap.crt \
     --client-key-file=/etc/pki/tls/private/ldap.key \
     --lifetime=365 \
     --hostname=$FQDN \
     --email=root@$FQDN \
RUN chown ldap:ldap /etc/pki/tls/{certs/slapd.crt,private/slapd.key}
RUN rm -rf /var/lib/ldap/*  /etc/openldap/slapd.d/*
RUN cp /test_dir/tests/test_data/slapd.conf /etc/openldap
RUN supervisorctl start slapd && ldapadd -Y EXTERNAL -H ldapi:/// -f /test_dir/tests/test_data/ldap_test.ldif

The only missing thing is not the supervisorctl config for slapd:

I found this:

[program:slapd]
command=/usr/sbin/slapd -h "ldap:/// ldapi:/// ldaps:///" -g openldap -u openldap -F /srv/config -d 1

You need to put that in the right directory and then you should be fine.

@tpw56j
Copy link
Contributor Author
tpw56j commented Nov 25, 2021

@SchoolGuy Thanks! I will try to do this.

@tpw56j
Copy link
Contributor Author
tpw56j commented Nov 26, 2021

@SchoolGuy I was unable to add openldap to the Fedora34 container this way. And I couldn't find a way to select this container for testing.

@SchoolGuy
Copy link
Member

We do the testing on GitHub with the openSUSE Tumbleweed container built in the openSUSE Buildservice here. I will take care of the submission on OBS side once you have added all required things to the Dockerfile here in Git. For local testing we use the same dockerfile and thus you can build that container yourself like described in the Wikipage "Testing". To select the Fedora 34 container locally for testing (sadly only one-shot possible which makes it really slow) you need the --with-tests flag and/or --with-system-tests flag while building the RPMs. I saw that we don't explain this in the Wiki and I will try to add it during the day to the Wiki.

SchoolGuy
SchoolGuy reviewed Nov 29, 2021
View reviewed changes
@SchoolGuy
Copy link
Member

@tpw56j You are aware that https://pypi.org/project/ldap/ is a placeholder which we do not want probably? Or is this an intended change?

@tpw56j
Copy link
Contributor Author
tpw56j commented Nov 29, 2021

@SchoolGuy cobbler/modules/authentication/ldap.py does not use ldap3, so I replaced ldap3 with ldap everywhere. I will remove https://pypi.org/project/ldap.

@SchoolGuy
Copy link
Member

@tpw56j Oh that sounds like a lot of tech-debt. Do you need help with something or should I just monitor your progress and review?

@tpw56j
Copy link
Contributor Author
tpw56j commented Nov 29, 2021

@SchoolGuy Thanks for your willingness to help, I'll try to finish the ldap-related code changes myself.

@tpw56j
Copy link
Contributor Author
tpw56j commented Nov 29, 2021

@SchoolGuy My branch is missing Debian 11, so I cannot make changes there. And in system-test there is an error not related to my changes (dhcpd).

@SchoolGuy
Copy link
Member
SchoolGuy commented Nov 29, 2021 •
edited
Loading

@tpw56j You could rebase the branch on master, right? But yeah the error on the system-tests is fine. I am aware of that.

Edit: I see you have that but it is failing...

@SchoolGuy
Copy link
Member

So building works fine but installing does not. Does Debian 11 not have the required packages?

@tpw56j
Copy link
Contributor Author
tpw56j commented Nov 29, 2021

@SchoolGuy Debian 11 requires the python3-ldap package, as does Debian 10. I will try to rebase and modify there.

@tpw56j tpw56j force-pushed the fix/ldap_anonymous_bind branch from df5bc8d to 04631e1 Compare November 29, 2021 15:39
@tpw56j
Copy link
Contributor Author
tpw56j commented Nov 29, 2021

@SchoolGuy The rebase seems to have been successful.

SchoolGuy
SchoolGuy approved these changes Dec 6, 2021
View reviewed changes
Copy link
Member
@SchoolGuy SchoolGuy left a comment

Choose a reason for hiding this comment

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

LGTM even with the new changes!

@SchoolGuy
Copy link
Member

@tpw56j Is this fully ready in your eyes?

@tpw56j
Copy link
Contributor Author
tpw56j commented Dec 6, 2021

@SchoolGuy So far I don't like this added code:

        if tls_reqcert:
            if tls_reqcert == "never":
                ldaps_tls.set_option(ldap.OPT_X_TLS_REQUIRE_CERT, ldap.OPT_X_TLS_NEVER)
            elif tls_reqcert == "allow":
                ldaps_tls.set_option(ldap.OPT_X_TLS_REQUIRE_CERT, ldap.OPT_X_TLS_ALLOW)
            elif tls_reqcert == "demand":
                ldaps_tls.set_option(ldap.OPT_X_TLS_REQUIRE_CERT, ldap.OPT_X_TLS_DEMAND)
            elif tls_reqcert == "hard":
                ldaps_tls.set_option(ldap.OPT_X_TLS_REQUIRE_CERT, ldap.OPT_X_TLS_HARD)
            else:
                api_handle.logger.error("authn_ldap: ldap_tls_reqcert choices include: %s" %
                                        "never, allow, demand, hard")

I think I figured out how to change it in this PR and I'll try to do it.

And we also do not have a mechanism for validating a fixed list of supported values in the settings, such as, for example, ldap_tls_reqcert or default_template_type, default_virt_type. But this change would probably be better done in a separate PR.

@SchoolGuy
Copy link
Member

@tpw56j Thanks a lot for the status update! Take your time and don't feel rushed!

@tpw56j
Copy link
Contributor Author
tpw56j commented Dec 6, 2021

@SchoolGuy PR is ready to review

SchoolGuy
SchoolGuy approved these changes Dec 7, 2021
View reviewed changes
Copy link
Member
@SchoolGuy SchoolGuy left a comment

Choose a reason for hiding this comment

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

System-Tests are broken currently but this is not the concern of this PR. Since we have new tests which cover most/some of the new code and this was previously untested, I will accept the new PR. Thanks for your work @tpw56j !

@SchoolGuy SchoolGuy merged commit dba8e10 into cobbler:master Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@watologo1 watologo1 watologo1 approved these changes

@SchoolGuy SchoolGuy SchoolGuy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.3.1
Development

Successfully merging this pull request may close these issues.

ldap authentication via anonymous bind is broken
3 participants
@tpw56j @SchoolGuy @watologo1
0