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

LDAP sign-in #383

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
@@ -74,6 +74,9 @@ gem "terraform"
gem "nokogiri"
gem "fezzik"

# For LDAP signins
gem "net-ldap"

group :test do
# NOTE(caleb): require rr >= 1.0.3 and scope >= 0.2.3 for mutual compatibility
gem "rr", ">= 1.0.3"
2 changes: 2 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
@@ -48,6 +48,7 @@ GEM
multi_json (1.1.0)
mustache (0.99.4)
mysql2 (0.3.11)
net-ldap (0.3.1)
nokogiri (1.5.0)
open4 (1.0.1)
pathological (0.2.4)
@@ -139,6 +140,7 @@ DEPENDENCIES
methodchain
mustache
mysql2
net-ldap
nokogiri
pathological
pinion
129 changes: 112 additions & 17 deletions barkeep_server.rb
Original file line number Diff line number Diff line change
@@ -3,6 +3,7 @@

require "bourbon"
require "coffee-script"
require "net/ldap"
require "methodchain"
require "nokogiri"
require "open-uri"
@@ -40,7 +41,7 @@

NODE_MODULES_BIN_PATH = "./node_modules/.bin"
OPENID_AX_EMAIL_SCHEMA = "http://axschema.org/contact/email"
UNAUTHENTICATED_ROUTES = ["/signin", "/signout", "/inspire", "/statusz", "/api/"]
UNAUTHENTICATED_ROUTES = ["/signin", "/signout", "/inspire", "/statusz", "/api/", "/favicon.ico"]
# NOTE(philc): Currently we let you see previews of individual commits and the code review stats without
# being logged in, as a friendly UX. When we flesh out our auth model, we should intentionally make this
# configurable.
@@ -94,6 +95,7 @@ class BarkeepServer < Sinatra::Base
"/vendor/flot/jquery.flot.pie.min.js"
]
pinion.create_bundle :user_settings_app_js, :concatenate_and_uglify_js, ["/coffee/user_settings.js"]
pinion.create_bundle :ldap_signin_app_js, :concatenate_and_uglify_js, ["/coffee/ldap_signin.js"]
end

helpers Pinion::SinatraHelpers
@@ -106,7 +108,7 @@ class BarkeepServer < Sinatra::Base
# Session hijacking protection breaks Chrome sessions and offers little protection anyway
set :protection, except: :session_hijacking

raise "You must have an OpenID provider defined in OPENID_PROVIDERS." if OPENID_PROVIDERS.empty?
raise "You must have an OpenID or LDAP provider defined in OPENID_PROVIDERS or LDAP_PROVIDERS, respectively." if OPENID_PROVIDERS.empty? and LDAP_PROVIDERS.empty?

configure :development do
STDOUT.sync = true # Flush any output right away when running via Foreman.
@@ -172,6 +174,15 @@ def ensure_required_params(*required_params)
end
end
end

def start_session(name, email)
session[:email] = email
unless User.find(:email => email)
# If there are no admin users yet, make the first user to log in the first admin.
permission = User.find(:permission => "admin").nil? ? "admin" : "normal"
User.new(:email => email, :name => name, :permission => permission).save
end
end
end

before do
@@ -200,9 +211,7 @@ def ensure_required_params(*required_params)

# Save url to return to it after login completes.
session[:login_started_url] = request.url
redirect(OPENID_PROVIDERS_ARRAY.size == 1 ?
get_openid_login_redirect(OPENID_PROVIDERS_ARRAY.first) :
"/signin/select_openid_provider")
redirect get_signin_redirect()
end
end

@@ -213,23 +222,104 @@ def ensure_required_params(*required_params)
get "/signin" do
session.clear
session[:login_started_url] = request.referrer
redirect(OPENID_PROVIDERS_ARRAY.size == 1 ?
get_openid_login_redirect(OPENID_PROVIDERS_ARRAY.first) :
"/signin/select_openid_provider")
redirect get_signin_redirect()
end

get "/signin/select_openid_provider" do
erb :select_openid_provider, :locals => { :openid_providers => OPENID_PROVIDERS_ARRAY }
get "/signin/select_signin_provider" do
erb :select_signin_provider, :locals => { :openid_providers => OPENID_PROVIDERS_ARRAY, :ldap_providers => LDAP_PROVIDERS }
end

# Users navigate to here from select_openid_provider.
# Users navigate to here from select_signin_provider.
# - provider_id: an integer indicating which provider from OPENID_PROVIDERS_ARRAY to use for authentication.
get "/signin/signin_using_openid_provider" do
provider = OPENID_PROVIDERS_ARRAY[params[:provider_id].to_i]
halt 400, "OpenID provider not found." unless provider
redirect get_openid_login_redirect(provider)
end

get "/signin/signin_using_ldap_provider" do
provider = LDAP_PROVIDERS[params[:provider_id].to_i]
halt 400, "LDAP provider not found." unless provider
erb :ldap_signin, :locals => { :provider_id => params[:provider_id], :name => provider[:name] || provider[:host] }
end

post "/signin/ldap_authenticate" do
provider = LDAP_PROVIDERS[params[:provider_id].to_i]
halt 400, "LDAP provider not found." unless provider

begin
Timeout::timeout(5) do
Copy link
Contributor

Choose a reason for hiding this comment

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

Timeout should be treated like try/catch -- we should try to only put code inside the body which needs timeout protection. For instance, setting up the vars conn_params and auth_params does not need to be in the timeout, and makes it hard to see the code that needs the timeout protection.

conn_params = { :host => provider[:host], :port => provider[:port] }
auth_params = { :method => provider[:method] }

if provider.has_key?(:username)
Copy link
Contributor

Choose a reason for hiding this comment

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

For those of us unfamiliar with ldap, it would be helpful to give a name to this condition, so it's clear what the else branch pertains to, e.g.:

use_simple_auth = provider.has_key?(:username)
if use_simple_auth
  ...
else
  ...

use_simple_auth is probably not the right name for this explanatory variable, but you get my drift.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the comments literally just below this snippet were sufficient, but okay.

# Here we use separate credentials, i.e. those in the config file, to query
# the LDAP server in order to locate the correct user entry.
conn = Net::LDAP.new(conn_params.merge({
:auth => auth_params.merge({
:username => provider[:username],
:password => provider[:password] })}))

if conn.bind
# Using the field name provided in the config file, search for the entry
# whose value in that field is equal to the username provided on the web
# form.
filter = Net::LDAP::Filter.eq(provider[:uid], params[:username])
entries = conn.search(:base => provider[:base], :filter => filter)
if entries.nil? or entries.empty?
halt 401, "Invalid username or password"
elsif entries.size > 1
# TODO log error message for duplicate user state
halt 401, "LDAP misconfigured"
end

conn2 = Net::LDAP.new(conn_params.merge({
:auth => auth_params.merge({
:username => entries.first.dn,
:password => params[:password] })}))

if conn2.bind
start_session(entries.first.cn.first, entries.first.mail.first)
session[:login_started_url]
else
halt 401, "Invalid username or password"
end
else
halt 401, "Barkeep could not bind as query user"
end
else
# This is the simpler form of LDAP authentication, wherein we simply
# attempt to bind using the credentials provided on the web form in order
# to validate the user.
user_dn = "#{provider[:uid]}=#{params[:username]},#{provider[:base]}\n"
conn = Net::LDAP.new(conn_params.merge({
:auth => auth_params.merge({
:username => user_dn,
:password => params[:password] })}))

if conn.bind
filter = Net::LDAP::Filter.contains(provider[:uid], params[:username])

Choose a reason for hiding this comment

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

This Filter.contains needs to actually be Filter.eq. If, for instance, you happen to have Alice Smith (asmith) and Adam Smithson (asmithson) then Alice won't be able to log in because this seach will return both, and the entries.size > 1 error condition a few line below will activate.

entries = conn.search(:base => provider[:base], :filter => filter)
if entries.nil? or entries.empty?
halt 401, "Invalid username or password"
elsif entries.size > 1
# TODO log error message for duplicate user state
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "duplicate user state" mean?

halt 401, "LDAP misconfigured"
end

start_session(entries.first.cn.first, entries.first.mail.first)
session[:login_started_url]
else
halt 401, "Invalid username or password"
end
end
end
rescue Timeout::Error => e
session.clear
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to clear the session here? Add a NOTE

halt 401, "LDAP server took too long to respond"
end
end

Copy link
Contributor

Choose a reason for hiding this comment

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

This ldap_authenticate is a really long route. Please break up the functionality into named methods to make the body of the route easier to read.

# Handle login complete from openid provider.
get "/signin/complete" do
@openid_consumer ||= OpenID::Consumer.new(session,
@@ -248,12 +338,7 @@ def ensure_required_params(*required_params)
halt 401, "Your email #{email} is not authorized to login to Barkeep."
end
end
session[:email] = email
unless User.find(:email => email)
# If there are no admin users yet, make the first user to log in the first admin.
permission = User.find(:permission => "admin").nil? ? "admin" : "normal"
User.new(:email => email, :name => email, :permission => permission).save
end
start_session(email, email)
redirect session[:login_started_url] || "/"
end
end
@@ -561,6 +646,16 @@ def ensure_required_params(*required_params)

def logged_in?() current_user && !current_user.demo? end

def get_signin_redirect()
if OPENID_PROVIDERS_ARRAY.size == 1 and LDAP_PROVIDERS.empty?
get_openid_login_redirect(OPENID_PROVIDERS_ARRAY.first)
elsif OPENID_PROVIDERS_ARRAY.empty? and LDAP_PROVIDERS.size == 1
"/signin/signin_using_ldap_provider?provider_id=0"
else
"/signin/select_signin_provider"
end
end

# Construct redirect url to google openid.
def get_openid_login_redirect(openid_provider_url)
@openid_consumer ||= OpenID::Consumer.new(session,
71 changes: 71 additions & 0 deletions doc/ldap.markdown
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
Barkeep's LDAP user authentication has been tested against OpenLDAP and Active Directory, but in a limited permutation of setups. If you have issues authenticating against a particular setup, please check your server configuration, talk to your sysadmin, and check the [open issues list](https://github.com/ooyala/barkeep/issues). If you still have trouble, please [open a new issue](https://github.com/ooyala/barkeep/issues/new).

## Unsupported features
LDAP group authentication is not supported, nor is any form of SSL encryption. Pull requests are welcome!

## Configuration
Add a hash to the list in `LDAP_PROVIDERS` in `environment.rb`. Barkeep supports multiple LDAP providers, and can be used in conjunction with any number of OpenID providers. However, usernames must be unique across all providers.

### Parameters
* `:name` : The name of this provider that will be displayed above the signin form. If multiple providers are specified, it will be used on the provider selection page.
* `:host` : The host name or IP address of the LDAP server.
* `:port` : The port to talk to on the LDAP server.
* `:base` : The RDN under which user accounts may be found.
* `:method` : The authentication type to use when binding. According to the net-ldap source code, it appears that supported values are `:simple`, `:anon`, `:anonymous`, `:sasl`, and `:gss_spnego`. **LDAP authentication has only been tested with `:simple`.**
* `:uid` : This can mean two different things, depending on whether `:username` is provided.

If `:username` is not provided, it is used as the top-level attribute name used to construct the user's DN. It's probably something like `"CN"` or `"uid"`. For example, if `:uid` is `"CN"`, `:base` is `"OU=Developers,dc=company,dc=com"`, and the user provided `Joe Smith` as their username, then Barkeep will attempt to bind as `CN=Joe Smith,OU=Developers,dc=company,dc=com` with the password that the user provided.

On the other hand, if `:username` is provided, `uid` is interpreted as the LDAP attribute to search for among the entries under `:base`. This is how Active Directory authentication via LDAP can work. Suppose `:uid` is set to `"employee_name"`, the user provided `jsmith` as their username, and `:base` is as before. Then Barkeep will bind as `:username` with `:password`, query the LDAP server for all entries under `OU=Developers,dc=company,dc=com`, and find the first entry whose attribute `employee_name` is `jsmith`. Let's say this entry is again `"CN=Joe Smith,OU=Developers,dc=company,dc=com"`. Barkeep will then attempt to bind as *that* DN, with the password the user provided. When authenticating with Active Directory, use `"sAMAccountName"` for `:uid`. If there is a cleaner way to accomplish these use cases, please do let us know.
* `:username` : (optional) The DN of to bind as when executing a query under `:base` for `:uid`. For an anonymous bind, specify as `""`. Active Directory also seems to let you get away with `"name@company.com", in lieu of a DN.
* `:password` : (optional) The password to go with `:username`. For an anonymous bind, specify as `""`.

## Examples

### OpenLDAP
LDAP_PROVIDERS = [{
:name => "OpenLDAP",
:host => "ldap.example.com",
:port => 389,
:base => "OU=users,DC=example,DC=com",
:method => :simple,
:uid => "CN"
}]

### Active Directory

#### Bind as DN
LDAP_PROVIDERS = [{
:name => "Active Directory",
:host => "ad.example.com",
:port => 389,
:base => "OU=users,DC=example,DC=com",
:method => :simple,
:uid => "sAMAccountName",
:username => "CN=Account Query Bot,OU=users,DC=example,DC=com",
:password => "pass123"
}]

#### Bind as username
LDAP_PROVIDERS = [{
:name => "Active Directory",
:host => "ad.example.com",
:port => 389,
:base => "OU=users,DC=example,DC=com",
:method => :simple,
:uid => "sAMAccountName",
:username => "account_query_bot@example.com",
:password => "pass123"
}]

#### Anonymous bind (untested)
LDAP_PROVIDERS = [{
:name => "Active Directory",
:host => "ad.example.com",
:port => 389,
:base => "OU=users,DC=example,DC=com",
:method => :simple,
:uid => "sAMAccountName",
:username => "",
:password => ""
}]
3 changes: 3 additions & 0 deletions environment.rb
Original file line number Diff line number Diff line change
@@ -27,6 +27,9 @@
# Besides Google, another popular OpenID endpoint is https://me.yahoo.com
OPENID_PROVIDERS = "https://www.google.com/accounts/o8/ud"

# A list of hashes of LDAP parameters for signing in your users. For documentation, see doc/ldap.markdown.
LDAP_PROVIDERS = []

# This is the read-only demo mode which is used in the Barkeep demo linked from getbarkeep.com.
# Most production deployments will not want to enable the demo mode, but we want it while developing.
ENABLE_READONLY_DEMO_MODE = true
26 changes: 26 additions & 0 deletions public/coffee/ldap_signin.coffee
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
window.LdapSignin =
init: ->
KeyboardShortcuts.createShortcutContext($("#ldapSignin input"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary?


$("#ldapSigninForm").submit (e) =>
e.preventDefault()
signinParams = { "provider_id": $("#ldapSignin #providerId").val(), "username": $("#ldapSignin #userName").val(), "password": $("#ldapSignin #password").val() }
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to quote the key names in this javascript hash.

$.ajax({
"type": "POST",
Copy link
Contributor

Choose a reason for hiding this comment

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

"type" need not be quoted.

url: "/signin/ldap_authenticate",
data: signinParams,
success: (e) ->
window.location.replace(e)
error: (jqXHR, textStatus, errorThrown) =>
@showErrorMessage(if jqXHR.responseText? and $.trim(jqXHR.responseText).length > 0 then jqXHR.responseText else "Unknown error")
console.log("error:")
Copy link
Contributor

Choose a reason for hiding this comment

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

You can collapse these 4 console.log statements into one. Do we need to log all of this stuff?

console.log(jqXHR)
console.log(textStatus)
console.log(errorThrown)
})

showErrorMessage: (message) ->
$("#errorMessage").show()
$("#errorMessage").html(message)

$(document).ready(-> LdapSignin.init())
8 changes: 0 additions & 8 deletions public/css/admin.scss
Original file line number Diff line number Diff line change
@@ -43,14 +43,6 @@
}

#repos {
#confirmationMessage {
margin: 10px 0;
padding: 10px;
background-color: #FFF7B1;
border: 1px dashed #333;
display: none;
}

table {
width: 100%;
border-collapse: collapse;
29 changes: 29 additions & 0 deletions public/css/styles.scss
Original file line number Diff line number Diff line change
@@ -22,6 +22,14 @@ body {

strong { font-weight: bold; }

.confirmationMessage {
margin: 10px 0;
padding: 10px;
background-color: #FFF7B1;
border: 1px dashed #333;
display: none;
}

// There are cases where we want focus for tooltips but we don't want to show an outline
.noFocusOutline {
// FF
@@ -609,3 +617,24 @@ table.commitsList, table.authorList {
}
}
}


#ldapSignin {
width: 500px;
margin: 0 auto;
form {
margin-top: 12px;
}
label {
font-weight: bold;
font-size: 14px;
}
input.text {
width: 200px;
font-size: 18px;
margin: 0 0 8px 4px;
}
input.standard {
margin-top: 8px;
}
}
Loading