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

Use safer defaults for TLS verification on LDAP connections #2053

Merged
merged 4 commits into from Sep 16, 2021

Conversation

rhafer
Copy link
Contributor

@rhafer rhafer commented Sep 8, 2021

The LDAP client connections where hardcoded to ignore certificate validation
errors everywhere. This commit changes that to uses a secure default,
which can be overridden by the new config parameter 'insecure'.
Also the LDAP related test configs are updated to set that override for
the tests.

@rhafer rhafer requested a review from labkode as a code owner September 8, 2021 10:13
@update-docs
Copy link

update-docs bot commented Sep 8, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

labkode
labkode previously approved these changes Sep 8, 2021
Copy link
Member

@labkode labkode left a comment

Choose a reason for hiding this comment

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

Thanks, great improvement!
We should probably run a grep -i insecure around the codebase :)

@lgtm-com
Copy link

lgtm-com bot commented Sep 8, 2021

This pull request fixes 9 alerts when merging 6c787c5 into f2109fc - view on LGTM.com

fixed alerts:

  • 9 for Disabled TLS certificate check

@rhafer
Copy link
Contributor Author

rhafer commented Sep 8, 2021

We should probably run a grep -i insecure around the codebase :)

AFAICS those were the only remaining places where i was hardcoded.

@lgtm-com
Copy link

lgtm-com bot commented Sep 8, 2021

This pull request fixes 9 alerts when merging 2759a8b into f2109fc - view on LGTM.com

fixed alerts:

  • 9 for Disabled TLS certificate check

C0rby
C0rby previously approved these changes Sep 8, 2021
Copy link
Contributor

@C0rby C0rby left a comment

Choose a reason for hiding this comment

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

Awesome improvement!

@rhafer rhafer marked this pull request as draft September 8, 2021 14:15
@rhafer
Copy link
Contributor Author

rhafer commented Sep 8, 2021

Converted to draft for now. As I think this needs some adjustments in ocis.

@lgtm-com
Copy link

lgtm-com bot commented Sep 13, 2021

This pull request fixes 9 alerts when merging 2b7410f into 36d6211 - view on LGTM.com

fixed alerts:

  • 9 for Disabled TLS certificate check

@labkode
Copy link
Member

labkode commented Sep 14, 2021

@rhafer please remove the draft state if you think is completed.

refs
refs previously approved these changes Sep 14, 2021
The LDAP client connections were hardcoded to ignore certificate validation
errors everywhere. This commit changes that to uses a secure default,
which can be overridden by the new config parameter 'insecure'.
Also the LDAP related test configs are updated to set that override for
the tests.
This should reduce code duplication a bit. Currently this only handles the
initial setup of the LDAP connection (e.g. the TLS parameters). Could be
enhanced to also handle the initial authentication in the future.
This add a new configparameter "cacert" to allow to add trusted CAs
and Server Certificates for the LDAP connections. This allows us to
avoid using "insecure" when running against self-signed certificates.
(As e.g. issued for glauth by default)
@lgtm-com
Copy link

lgtm-com bot commented Sep 14, 2021

This pull request fixes 9 alerts when merging ee401dd into d04e363 - view on LGTM.com

fixed alerts:

  • 9 for Disabled TLS certificate check

@labkode labkode merged commit af7f2a2 into cs3org:master Sep 16, 2021
glpatcern pushed a commit to glpatcern/reva that referenced this pull request Sep 23, 2021
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.

None yet

4 participants