Project

General

Profile

Actions

Feature #15881

closed

[controller] LDAP login support

Added by Peter Amstutz over 4 years ago. Updated over 3 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Login
Target version:
Story points:
-
Release relationship:
Auto

Description

Discussed at engineering meeting 2020 April 15, using PAM to access LDAP is unlikely to be satisfactory since it doesn't provide any user profile information. Add first-class LDAP support to the new username/password authenticate endpoint.


Files

15881-doc.png (76.5 KB) 15881-doc.png Tom Clegg, 05/07/2020 03:13 PM
15881-docs.png (252 KB) 15881-docs.png Tom Clegg, 05/08/2020 05:54 PM

Subtasks 3 (0 open3 closed)

Task #16350: Determine set of features required to support existing LDAP usersClosedTom Clegg05/06/2020Actions
Task #16351: Review 15881-ldapResolvedTom Clegg05/08/2020Actions
Task #16467: Review 15881-ldapResolved05/06/2020Actions

Related issues

Related to Arvados Epics - Idea #15322: Replace and delete sso-providerResolved03/11/202008/26/2020Actions
Related to Arvados - Idea #16453: [controller] Expand config comment about LDAP search filtersNewActions
Related to Arvados - Bug #16475: Javascript mystery investigation on WB2 workaroundResolvedActions
Has duplicate Arvados - Idea #15883: Support LDAP loginsDuplicateActions
Actions #1

Updated by Peter Amstutz over 4 years ago

Actions #2

Updated by Peter Amstutz over 4 years ago

  • Related to Idea #15322: Replace and delete sso-provider added
Actions #3

Updated by Peter Amstutz about 4 years ago

  • Category set to Login
Actions #4

Updated by Peter Amstutz about 4 years ago

  • Target version changed from To Be Groomed to 2020-03-25 Sprint
Actions #5

Updated by Peter Amstutz about 4 years ago

  • Blocked by Feature #16212: Can choose PAM as an authentication backend added
Actions #6

Updated by Tom Clegg about 4 years ago

  • Assigned To set to Tom Clegg
Actions #7

Updated by Peter Amstutz about 4 years ago

  • Target version changed from 2020-03-25 Sprint to 2020-04-08 Sprint
Actions #8

Updated by Peter Amstutz about 4 years ago

  • Status changed from New to In Progress
Actions #9

Updated by Tom Clegg almost 4 years ago

  • Target version changed from 2020-04-08 Sprint to 2020-04-22
Actions #10

Updated by Peter Amstutz almost 4 years ago

  • Description updated (diff)
Actions #11

Updated by Peter Amstutz almost 4 years ago

  • Blocked by deleted (Feature #16212: Can choose PAM as an authentication backend)
Actions #12

Updated by Peter Amstutz almost 4 years ago

  • Target version changed from 2020-04-22 to 2020-05-06 Sprint
Actions #13

Updated by Tom Clegg almost 4 years ago

  • Target version changed from 2020-05-06 Sprint to 2020-05-20 Sprint
Actions #16

Updated by Tom Clegg almost 4 years ago

LDAP has several configs and it seemed sensible to put them in an LDAP section. But this means we have

Login:
  # ...
  GoogleClientID: "" 
  GoogleClientSecret: "" 
  GoogleAlternateEmailAddresses: true
  PAM: true
  PAMService: arvados
  LDAP:
    Enable: true
    URL: ldap://ldap:389
    # ...

Seems like we should pick one approach here: all flat, or all hierarchical like this:

Login:
  # ...
  Google:
    Enable: true
    ClientID: "" 
    ClientSecret: "" 
    AlternateEmailAddresses: true
  PAM:
    Enable: true
    Service: arvados
  LDAP:
    Enable: true
    URL: ldap://ldap:389
    # ...
Actions #17

Updated by Peter Amstutz almost 4 years ago

Tom Clegg wrote:

LDAP has several configs and it seemed sensible to put them in an LDAP section. But this means we have

[...]

Seems like we should pick one approach here: all flat, or all hierarchical like this:

[...]

I am fine with migrating to a more hierarchical layout, and we already have a concept of deprecated configs that are automatically migrated.

However, this also affects the exported config, so anything that's expecting the old organization breaks. We don't yet have a general solution for that.

Now on its own, probably the only thing that is affected is Workbench 2 reading the PAM config. However this is the sort of thing I was trying to avoid by proposing a "LoginEndpoint" field or some other strategy for that Workbench 2 can use to determine how to do login without having to know about every login method support by Arvados.

Also, Workbench 2 needs to support LDAP, so I think we need to address this anyway.

Actions #18

Updated by Tom Clegg almost 4 years ago

However, this also affects the exported config, so anything that's expecting the old organization breaks.

Fortunately Login.PAM is the only config here that's ever been exported, and it has only existed as an experimental feature in dev/prerelease versions, so I think we're safe.

Actions #21

Updated by Lucas Di Pentima almost 4 years ago

Some comments & questions:

  • Documentation
    • Would it be convenient to warn the reader that SSO’s going to be decommissioned in the near future? I guess new installations shouldn’t be encouraged to think that multiple auth methods will be something supported through SSO.
  • Configuration
    • Do you think we could maintain config naming consistency by renaming InsecureTLS to Insecure, or just use TLS.Insecure? Maybe the latter isn’t convenient because TLS.Insecure is about Arvados’ services.
    • Arvbox config needs an update on the Login section.
  • File lib/controller/localdb/login_ldap.go - Line 92: Would it be better to do this check before any LDAP server interaction?
  • File services/api/config/initializers/omniauth_init.rb - Lines 12 & 18 refers to deprecated Rails.configuration.Login["ProviderAppID”] config
  • On 8f435f4bac86e7ba7dbd9770d2db9bb4db6cf569, the comment is: test LDAP login using a fake LDAP server, but I’m not able to understand how this is done, as the code updates don’t seem to be about that. Is the godap dependency providing the LDAPSuite? A comment explaining how it works would be useful.
  • Is the docker test being run by Jenkins? I’m not seeing any logging about it on "developer-run-tests-remainder” so I suppose it isn’t, do you think it should be run every time? A least on my side, it isn’t too slow, took like 20 seconds.

Other than that, LGTM.

Actions #22

Updated by Tom Clegg almost 4 years ago

  • Would it be convenient to warn the reader that SSO’s going to be decommissioned in the near future? I guess new installations shouldn’t be encouraged to think that multiple auth methods will be something supported through SSO.

Yes, I've removed SSO entirely from the install guide, and added a "remove sso-provider" item to the future 2.1 upgrade notes.

  • Do you think we could maintain config naming consistency by renaming InsecureTLS to Insecure, or just use TLS.Insecure? Maybe the latter isn’t convenient because TLS.Insecure is about Arvados’ services.
Not sure about this. Some thoughts
  • Even where TLS.Insecure == true (internal communication among Arvados components is unsecured) is acceptable, revealing users' LDAP credentials to a MitM could be a real problem. So I think it should be a separate knob.
  • I agree symmetry with the other Insecure flag would be nice - but I also like that InsecureTLS makes it clear that we're talking about TLS, not other aspects of authentication that could be considered insecure, like empty/easy passwords.
  • Arvbox config needs an update on the Login section.

Updated.

  • File lib/controller/localdb/login_ldap.go - Line 92: Would it be better to do this check before any LDAP server interaction?

Yes, moved up.

  • File services/api/config/initializers/omniauth_init.rb - Lines 12 & 18 refers to deprecated Rails.configuration.Login["ProviderAppID”] config

Updated.

  • On 8f435f4bac86e7ba7dbd9770d2db9bb4db6cf569, the comment is: test LDAP login using a fake LDAP server, but I’m not able to understand how this is done, as the code updates don’t seem to be about that. Is the godap dependency providing the LDAPSuite? A comment explaining how it works would be useful.

Oops, I missed adding the actual _test.go file in that commit. Fixed.

  • Is the docker test being run by Jenkins? I’m not seeing any logging about it on "developer-run-tests-remainder” so I suppose it isn’t, do you think it should be run every time? A least on my side, it isn’t too slow, took like 20 seconds.

It was disabled by default (needed go test -tags docker) but yes, it seems quick enough. I've enabled it.

15881-ldap @ 28e68f813bd7c48847c39a9e07d66ff5cf61662d -- developer-run-tests: #1850

Actions #23

Updated by Tom Clegg almost 4 years ago

It was disabled by default (needed go test -tags docker) but yes, it seems quick enough. I've enabled it.

Oops, jenkins workers don't have docker. Now skipping the docker tests if docker info fails.

15881-ldap @ ca1eb648712232558014d648939868b2a902558a -- developer-run-tests: #1852

Actions #24

Updated by Lucas Di Pentima almost 4 years ago

Some minor documentation comments:

  • File doc/admin/migrating-providers.html.textile.liquid - L16: typo “intead"
  • The websocket installation doc page has a duplicated "Restart the API server and controller” section (one of which mentions the SSO)
  • Doc file install/install-components.html.textile.liquid can be removed as it seems to have been replaced with install/install-manual-prerequisites.html.textile.liquid (also found because of SSO being mentioned)

And with that, it LGTM. Thanks!

Actions #26

Updated by Tom Clegg almost 4 years ago

I tried making the obvious changes to workbench2:

15881-ldap @ arvados-workbench2|6d2e6d292161d566f54e94f048805569ede8e3d5

I've managed to run unit tests, but not integration tests or actual browser tests.

Actions #27

Updated by Lucas Di Pentima almost 4 years ago

Updates at arvados-workbench2|366e40d9
Test run: developer-tests-workbench2: #27

Simplified and somehow fixed the code deciding if it should show the login form (integration tests were failing):

diff --git a/src/views/login-panel/login-panel.tsx b/src/views/login-panel/login-panel.tsx
index ba0f584f..f60f032a 100644
--- a/src/views/login-panel/login-panel.tsx
+++ b/src/views/login-panel/login-panel.tsx
@@ -11,6 +11,7 @@ import { ArvadosTheme } from '~/common/custom-theme';
 import { RootState } from '~/store/store';
 import { LoginForm } from '~/views-components/login-form/login-form';
 import Axios from 'axios';
+import { Config } from '~/common/config';

 type CssRules = 'root' | 'container' | 'title' | 'content' | 'content__bolder' | 'button';

@@ -69,6 +70,13 @@ type LoginPanelProps = DispatchProp<any> & WithStyles<CssRules> & {
     passwordLogin: boolean,
 };

+const requirePasswordLogin = (config: Config): boolean => {
+    if (config && config.clusterConfig) {
+        return config.clusterConfig.Login.LDAP.Enable || config.clusterConfig.Login.PAM.Enable || false;
+    }
+    return false;
+};
+
 export const LoginPanel = withStyles(styles)(
     connect((state: RootState) => ({
         remoteHosts: state.auth.remoteHosts,
@@ -76,10 +84,8 @@ export const LoginPanel = withStyles(styles)(
         localCluster: state.auth.localCluster,
         loginCluster: state.auth.loginCluster,
         welcomePage: state.auth.config.clusterConfig.Workbench.WelcomePageHTML,
-        passwordLogin: state.auth.remoteHostsConfig[state.auth.loginCluster || state.auth.homeCluster] &&
-            state.auth.remoteHostsConfig[state.auth.loginCluster || state.auth.homeCluster].clusterConfig.Login.LDAP.Enable ||
-            state.auth.remoteHostsConfig[state.auth.loginCluster || state.auth.homeCluster].clusterConfig.Login.PAM.Enable || false,
-    }))(({ classes, dispatch, remoteHosts, homeCluster, localCluster, loginCluster, welcomePage, passwordLogin }: LoginPanelProps) => {
+        passwordLogin: requirePasswordLogin(state.auth.remoteHostsConfig[state.auth.loginCluster || state.auth.homeCluster]),
+        }))(({ classes, dispatch, remoteHosts, homeCluster, localCluster, loginCluster, welcomePage, passwordLogin }: LoginPanelProps) => {
         const loginBtnLabel = `Log in${(localCluster !== homeCluster && loginCluster !== homeCluster) ? " to "+localCluster+" with user from "+homeCluster : ''}`;

         return (<Grid container justify="center" alignItems="center" 

It seems that reevaluating state.auth.remoteHostsConfig[state.auth.loginCluster || state.auth.homeCluster] sometimes returned undefined and made the code break.

Also, fixed the trailing slashes issue on URLs from #16392, so this makes wb2 work with arvbox again.

Actions #28

Updated by Tom Clegg almost 4 years ago

Simplified and somehow fixed the code deciding if it should show the login form (integration tests were failing):

Much better, thanks.

It seems that reevaluating state.auth.remoteHostsConfig[state.auth.loginCluster || state.auth.homeCluster] sometimes returned undefined and made the code break.

That is a bit mysterious, isn't it?

Also, fixed the trailing slashes issue on URLs from #16392, so this makes wb2 work with arvbox again.

Excellent, thanks. LGTM.

Actions #29

Updated by Tom Clegg almost 4 years ago

  • Related to Idea #16453: [controller] Expand config comment about LDAP search filters added
Actions #30

Updated by Tom Clegg almost 4 years ago

  • Target version changed from 2020-05-20 Sprint to 2020-06-03 Sprint
Actions #31

Updated by Tom Clegg almost 4 years ago

15881-ldap @ 95d08c91f6d902054eb9ed4f79cb6bda2c3e8342

Expands comment on SearchFilters config.

Actions #32

Updated by Peter Amstutz almost 4 years ago

Tom Clegg wrote:

15881-ldap @ 95d08c91f6d902054eb9ed4f79cb6bda2c3e8342

Expands comment on SearchFilters config.

LGTM.

Actions #33

Updated by Tom Clegg almost 4 years ago

  • Status changed from In Progress to Resolved
Actions #34

Updated by Lucas Di Pentima almost 4 years ago

  • Related to Bug #16475: Javascript mystery investigation on WB2 workaround added
Actions #35

Updated by Peter Amstutz over 3 years ago

  • Release set to 25
Actions

Also available in: Atom PDF