Feature #15881

[controller] LDAP login support

Added by Peter Amstutz 7 months ago. Updated about 1 month ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Login
Target version:
Start date:
05/06/2020
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-

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.

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

Task #16350: Determine set of features required to support existing LDAP usersClosedTom Clegg

Task #16351: Review 15881-ldapResolvedTom Clegg

Task #16467: Review 15881-ldapResolved


Related issues

Related to Arvados Epics - Story #15322: Replace and delete sso-providerIn Progress03/11/202006/03/2020

Related to Arvados - Story #16453: [controller] Expand config comment about LDAP search filtersNew

Related to Arvados - Bug #16475: Javascript mystery investigation on WB2 workaroundNew

Has duplicate Arvados - Story #15883: Support LDAP loginsDuplicate

Associated revisions

Revision 374cc9ff
Added by Tom Clegg about 2 months ago

Merge branch '15881-ldap'

refs #15881

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

Revision 8e504f3c
Added by Lucas Di Pentima about 2 months ago

Merge branch '15881-ldap'
Refs #15881

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

Revision 46db403c
Added by Tom Clegg about 1 month ago

Merge branch '15881-ldap'

refs #15881

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

History

#1 Updated by Peter Amstutz 7 months ago

#2 Updated by Peter Amstutz 7 months ago

  • Related to Story #15322: Replace and delete sso-provider added

#3 Updated by Peter Amstutz 6 months ago

  • Category set to Login

#4 Updated by Peter Amstutz 4 months ago

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

#5 Updated by Peter Amstutz 4 months ago

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

#6 Updated by Tom Clegg 4 months ago

  • Assigned To set to Tom Clegg

#7 Updated by Peter Amstutz 3 months ago

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

#8 Updated by Peter Amstutz 3 months ago

  • Status changed from New to In Progress

#9 Updated by Tom Clegg 3 months ago

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

#10 Updated by Peter Amstutz 3 months ago

  • Description updated (diff)

#11 Updated by Peter Amstutz 3 months ago

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

#12 Updated by Peter Amstutz 3 months ago

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

#13 Updated by Tom Clegg about 2 months ago

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

#16 Updated by Tom Clegg about 2 months 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
    # ...

#17 Updated by Peter Amstutz about 2 months 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.

#18 Updated by Tom Clegg about 2 months 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.

#21 Updated by Lucas Di Pentima about 2 months 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.

#22 Updated by Tom Clegg about 2 months 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 -- https://ci.arvados.org/view/Developer/job/developer-run-tests/1850/

#23 Updated by Tom Clegg about 2 months 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 -- https://ci.arvados.org/view/Developer/job/developer-run-tests/1852/

#24 Updated by Lucas Di Pentima about 2 months 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!

#26 Updated by Tom Clegg about 2 months 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.

#27 Updated by Lucas Di Pentima about 2 months ago

Updates at arvados-workbench2|366e40d9
Test run: https://ci.arvados.org/view/Developer/job/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.

#28 Updated by Tom Clegg about 2 months 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.

#29 Updated by Tom Clegg about 1 month ago

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

#30 Updated by Tom Clegg about 1 month ago

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

#31 Updated by Tom Clegg about 1 month ago

15881-ldap @ 95d08c91f6d902054eb9ed4f79cb6bda2c3e8342

Expands comment on SearchFilters config.

#32 Updated by Peter Amstutz about 1 month ago

Tom Clegg wrote:

15881-ldap @ 95d08c91f6d902054eb9ed4f79cb6bda2c3e8342

Expands comment on SearchFilters config.

LGTM.

#33 Updated by Tom Clegg about 1 month ago

  • Status changed from In Progress to Resolved

#34 Updated by Lucas Di Pentima about 1 month ago

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

Also available in: Atom PDF