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.

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

Only authenticate logins when password is set #13147

Merged
merged 4 commits into from Sep 5, 2018
Merged

Only authenticate logins when password is set #13147

merged 4 commits into from Sep 5, 2018
+30 −1

Conversation

xlson
Copy link
Contributor

@xlson xlson commented Sep 5, 2018

Currently, it's possible to go through the login flow with an empty password. This is a bit strange as we require the password to be at least 4 characters long when a user is created. It also means we are vulnerable to the go-ldap vulnerability in #13010 that bypasses authentication if "unauthenticated bind" is enabled in the ldap server.

As I see it there are three potential fixes to the go-ldap problem:

  • upgrade to a fixed go-ldap version (not yet possible, we'd have to use master)
  • set a password length requirement (this pr sets it to 4, we could potentially make this configurable but do we want another config option?)
  • require the password to be non-empty before we do auth (the safest option as this will still allow someone using a short password to login while simulaneously fixing the go-ldap problem)

Closes #13010

@xlson xlson added this to the 5.3 milestone Sep 5, 2018
@xlson xlson requested a review from torkelo September 5, 2018 07:57
@xlson
Copy link
Contributor Author

xlson commented Sep 5, 2018

While writing the PR I started leaning towards option 3, requiring the password to be non-empty. It will fix the go-ldap issue without causing problems for users that use external auth systems with passwords shorter than 4. We could definitely set the limit to 4, as that is what we use in our auth system, but it feels rather arbitrary and isn't really that safe.

@xlson xlson changed the title Only authenticate logins when password is at least 4 characters Only authenticate logins when password is set Sep 5, 2018
@torkelo torkelo merged commit 275f613 into master Sep 5, 2018
@xlson xlson deleted the validate-password-length branch September 5, 2018 13:06
ryantxu added a commit to NatelEnergy/grafana that referenced this pull request Sep 6, 2018
@ryantxu
Merge remote-tracking branch 'grafana/master'
f3efec9 
* grafana/master:
  changelog: order changes by group (ocd)
  changelog: add notes about closing grafana#13030
  added radix rule and changed files to follow rule (grafana#13153)
  docs: default paths in the docker container.
  added only-arrow-functions rule and changed files to follow new rule (grafana#13154)
  build: uses 1.1.0 of the build container.
  Only authenticate logins when password is set (grafana#13147)
  refatoring: minor changes to PR grafana#13149
  build: updated build-container with go1.11.
  added no-conditional-assignment rule and changed files to follow new rule
  Changed functions to arrow functions for only-arrow-functions rule. (grafana#13131)
  Changed functions to arrow functions for only-arrow-functions rule.
  Changed functions to arrow functions for only-arrow-functions rule.
  changed functions to arrowfunctions for only-arrow-functions rule
  string formating fixes
  go fmt fixes
  upgrades to golang 1.11
ryantxu added a commit to ryantxu/grafana that referenced this pull request Sep 6, 2018
@ryantxu
Merge remote-tracking branch 'grafana/master' into dashboard-panels
d87c2e9 
* grafana/master:
  changelog: order changes by group (ocd)
  changelog: add notes about closing grafana#13030
  added radix rule and changed files to follow rule (grafana#13153)
  docs: default paths in the docker container.
  added only-arrow-functions rule and changed files to follow new rule (grafana#13154)
  build: uses 1.1.0 of the build container.
  Only authenticate logins when password is set (grafana#13147)
  refatoring: minor changes to PR grafana#13149
  build: updated build-container with go1.11.
  added no-conditional-assignment rule and changed files to follow new rule
  Changed functions to arrow functions for only-arrow-functions rule. (grafana#13131)
  Changed functions to arrow functions for only-arrow-functions rule.
  Changed functions to arrow functions for only-arrow-functions rule.
  changed functions to arrowfunctions for only-arrow-functions rule
  string formating fixes
  go fmt fixes
  upgrades to golang 1.11
ryantxu added a commit to ryantxu/grafana that referenced this pull request Sep 6, 2018
@ryantxu
Merge remote-tracking branch 'grafana/master' into app-tabs
fad846a 
* grafana/master:
  changelog: order changes by group (ocd)
  changelog: add notes about closing grafana#13030
  added radix rule and changed files to follow rule (grafana#13153)
  docs: default paths in the docker container.
  added only-arrow-functions rule and changed files to follow new rule (grafana#13154)
  build: uses 1.1.0 of the build container.
  Only authenticate logins when password is set (grafana#13147)
  refatoring: minor changes to PR grafana#13149
  build: updated build-container with go1.11.
  added no-conditional-assignment rule and changed files to follow new rule
  Changed functions to arrow functions for only-arrow-functions rule. (grafana#13131)
  Changed functions to arrow functions for only-arrow-functions rule.
  Changed functions to arrow functions for only-arrow-functions rule.
  changed functions to arrowfunctions for only-arrow-functions rule
  string formating fixes
  go fmt fixes
  upgrades to golang 1.11
ryantxu added a commit to ryantxu/grafana that referenced this pull request Sep 6, 2018
@ryantxu
Merge remote-tracking branch 'grafana/master' into panel-visibility-flag
e6da3c5 
* grafana/master:
  changelog: order changes by group (ocd)
  changelog: add notes about closing grafana#13030
  added radix rule and changed files to follow rule (grafana#13153)
  docs: default paths in the docker container.
  added only-arrow-functions rule and changed files to follow new rule (grafana#13154)
  build: uses 1.1.0 of the build container.
  Only authenticate logins when password is set (grafana#13147)
  refatoring: minor changes to PR grafana#13149
  build: updated build-container with go1.11.
  added no-conditional-assignment rule and changed files to follow new rule
  Changed functions to arrow functions for only-arrow-functions rule. (grafana#13131)
  Changed functions to arrow functions for only-arrow-functions rule.
  Changed functions to arrow functions for only-arrow-functions rule.
  changed functions to arrowfunctions for only-arrow-functions rule
  string formating fixes
  go fmt fixes
  upgrades to golang 1.11
ryantxu added a commit to ryantxu/grafana that referenced this pull request Sep 6, 2018
@ryantxu
Merge remote-tracking branch 'grafana/master' into lazy-load-panels
91d4fdb 
* grafana/master:
  changelog: order changes by group (ocd)
  changelog: add notes about closing grafana#13030
  added radix rule and changed files to follow rule (grafana#13153)
  docs: default paths in the docker container.
  added only-arrow-functions rule and changed files to follow new rule (grafana#13154)
  build: uses 1.1.0 of the build container.
  Only authenticate logins when password is set (grafana#13147)
  refatoring: minor changes to PR grafana#13149
  build: updated build-container with go1.11.
  added no-conditional-assignment rule and changed files to follow new rule
  Changed functions to arrow functions for only-arrow-functions rule. (grafana#13131)
  Changed functions to arrow functions for only-arrow-functions rule.
  Changed functions to arrow functions for only-arrow-functions rule.
  changed functions to arrowfunctions for only-arrow-functions rule
  string formating fixes
  go fmt fixes
  upgrades to golang 1.11
ryantxu added a commit to ryantxu/grafana that referenced this pull request Sep 6, 2018
@ryantxu
Merge remote-tracking branch 'grafana/master' into dynamic-height-panels
7a3f597 
* grafana/master:
  changelog: order changes by group (ocd)
  changelog: add notes about closing grafana#13030
  added radix rule and changed files to follow rule (grafana#13153)
  docs: default paths in the docker container.
  added only-arrow-functions rule and changed files to follow new rule (grafana#13154)
  build: uses 1.1.0 of the build container.
  Only authenticate logins when password is set (grafana#13147)
  refatoring: minor changes to PR grafana#13149
  build: updated build-container with go1.11.
  added no-conditional-assignment rule and changed files to follow new rule
  Changed functions to arrow functions for only-arrow-functions rule. (grafana#13131)
  Changed functions to arrow functions for only-arrow-functions rule.
  Changed functions to arrow functions for only-arrow-functions rule.
  changed functions to arrowfunctions for only-arrow-functions rule
  string formating fixes
  go fmt fixes
  upgrades to golang 1.11
ryantxu added a commit to ryantxu/grafana that referenced this pull request Sep 6, 2018
@ryantxu
Merge remote-tracking branch 'grafana/master' into timepicker-tooltip
0977591 
* grafana/master:
  changelog: order changes by group (ocd)
  changelog: add notes about closing grafana#13030
  added radix rule and changed files to follow rule (grafana#13153)
  docs: default paths in the docker container.
  added only-arrow-functions rule and changed files to follow new rule (grafana#13154)
  build: uses 1.1.0 of the build container.
  Only authenticate logins when password is set (grafana#13147)
  refatoring: minor changes to PR grafana#13149
  build: updated build-container with go1.11.
  added no-conditional-assignment rule and changed files to follow new rule
  Changed functions to arrow functions for only-arrow-functions rule. (grafana#13131)
  Changed functions to arrow functions for only-arrow-functions rule.
  Changed functions to arrow functions for only-arrow-functions rule.
  changed functions to arrowfunctions for only-arrow-functions rule
  string formating fixes
  go fmt fixes
  upgrades to golang 1.11
@marefr marefr modified the milestones: 5.3, 5.3.0-beta1 Sep 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@torkelo torkelo Awaiting requested review from torkelo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
5.3.0-beta1
Development

Successfully merging this pull request may close these issues.

Change to use go-ldap master due to security vulnerability CVE-2017-14623
3 participants
@xlson @torkelo @marefr