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

Adds support for certificates with empty Subject #94

Merged
merged 2 commits into from
Apr 3, 2017

Conversation

CriGoT
Copy link
Contributor

@CriGoT CriGoT commented Mar 30, 2017

Node.js default server identity verification does not consider the possibility of certificates with an empty subjects (issue). This type of certificates are sometimes used by Windows Active Directory, see this article for details, making it necessary that the connector supports validating them to use ldaps.

This change adds a new configuration option SSL_ENABLE_EMPTY_SUBJECT that enables using a patched verification for the server identity that allows using certificates with empty subject. We want to avoid modifying the default behaviour unless required so this flag defaults to false.

Node.js default verification does not consider the possibility of an empty subjects ([issue](nodejs/node#11771)).

In somce cases Windows Active Directory will use certificates with empty subjects, see this [article](https://blogs.technet.microsoft.com/askds/2008/09/16/third-party-application-fails-using-ldap-over-ssl/) for details, making necessary that the connector supports them to use `ldaps`.
This change adds a new configuration option `SSL_ENABLE_EMPTY_SUBJECT` that enables using a patched verification for the server identity that allows using certificates with empty subject. We want to avoid modifying the default behaviour unless required so this flag defaults to `false` and can be enabled when required.
@jfromaniello
Copy link
Member

@CriGoT looks good to me, the only thing I don't understand is why we would have this disabled by default?

@CriGoT
Copy link
Contributor Author

CriGoT commented Apr 3, 2017

I did that for two reasons:

  • To avoid changing behaviour. Maybe some people run into this problem before and fixed some other way. For those cases I want to avoid changing a preexisting behaviour unless specified.
  • When eventually node issue is solved we can safely remove in the default installation

However, I think we can enable it by default and allow people that customized the setup to turn it off.

@jfromaniello
Copy link
Member

yes, I think we should enable by default since as you mentioned this is valid in the specification

@CriGoT CriGoT changed the title Adds support for certificates with empty Subject [DO NOT MERGE] Adds support for certificates with empty Subject Apr 3, 2017
@CriGoT CriGoT force-pushed the support-for-empty-subject-certs branch from e64042c to 58bb60a Compare April 3, 2017 17:11
@CriGoT CriGoT changed the title [DO NOT MERGE] Adds support for certificates with empty Subject Adds support for certificates with empty Subject Apr 3, 2017
@jfromaniello jfromaniello merged commit 8ffb788 into auth0:master Apr 3, 2017
@jfromaniello
Copy link
Member

Awesome, published as 3.7.0

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.

2 participants