Feature #15881
closed[controller] LDAP login support
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
Related issues
Updated by Peter Amstutz over 4 years ago
- Has duplicate Idea #15883: Support LDAP logins added
Updated by Peter Amstutz over 4 years ago
- Related to Idea #15322: Replace and delete sso-provider added
Updated by Peter Amstutz over 4 years ago
- Target version changed from To Be Groomed to 2020-03-25 Sprint
Updated by Peter Amstutz over 4 years ago
- Blocked by Feature #16212: Can choose PAM as an authentication backend added
Updated by Peter Amstutz over 4 years ago
- Target version changed from 2020-03-25 Sprint to 2020-04-08 Sprint
Updated by Peter Amstutz over 4 years ago
- Status changed from New to In Progress
Updated by Tom Clegg over 4 years ago
- Target version changed from 2020-04-08 Sprint to 2020-04-22
Updated by Peter Amstutz over 4 years ago
- Blocked by deleted (Feature #16212: Can choose PAM as an authentication backend)
Updated by Peter Amstutz over 4 years ago
- Target version changed from 2020-04-22 to 2020-05-06 Sprint
Updated by Tom Clegg over 4 years ago
- Target version changed from 2020-05-06 Sprint to 2020-05-20 Sprint
Updated by Tom Clegg over 4 years ago
15881-ldap @ bbb132e983f9ec5c7d50cf0ab709ec041af1f844 -- developer-run-tests: #1836
Updated by Tom Clegg over 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
# ...
Updated by Peter Amstutz over 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.
Updated by Tom Clegg over 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.
Updated by Tom Clegg over 4 years ago
15881-ldap @ 0634b763dd27914cff5ca49c6cfe11233746ee31 -- developer-run-tests: #1842
Updated by Lucas Di Pentima over 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
toInsecure
, or just useTLS.Insecure
? Maybe the latter isn’t convenient becauseTLS.Insecure
is about Arvados’ services. - Arvbox config needs an update on the
Login
section.
- Do you think we could maintain config naming consistency by renaming
- 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 deprecatedRails.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 theLDAPSuite
? 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.
Updated by Tom Clegg over 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.
Not sure about this. Some thoughts
- Do you think we could maintain config naming consistency by renaming
InsecureTLS
toInsecure
, or just useTLS.Insecure
? Maybe the latter isn’t convenient becauseTLS.Insecure
is about Arvados’ services.
- 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 deprecatedRails.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 theLDAPSuite
? 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
Updated by Tom Clegg over 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
Updated by Lucas Di Pentima over 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 withinstall/install-manual-prerequisites.html.textile.liquid
(also found because of SSO being mentioned)
And with that, it LGTM. Thanks!
Updated by Tom Clegg over 4 years ago
15881-ldap @ f26e4dec04ed9d68a9f826f72b4a9e627ddc4a5c -- developer-run-tests: #1856
Updated by Tom Clegg over 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.
Updated by Lucas Di Pentima over 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.
Updated by Tom Clegg over 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 returnedundefined
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.
Updated by Tom Clegg over 4 years ago
- Related to Idea #16453: [controller] Expand config comment about LDAP search filters added
Updated by Tom Clegg over 4 years ago
- Target version changed from 2020-05-20 Sprint to 2020-06-03 Sprint
Updated by Tom Clegg over 4 years ago
15881-ldap @ 95d08c91f6d902054eb9ed4f79cb6bda2c3e8342
Expands comment on SearchFilters config.
Updated by Peter Amstutz about 4 years ago
Tom Clegg wrote:
15881-ldap @ 95d08c91f6d902054eb9ed4f79cb6bda2c3e8342
Expands comment on SearchFilters config.
LGTM.
Updated by Tom Clegg about 4 years ago
- Status changed from In Progress to Resolved
Updated by Lucas Di Pentima about 4 years ago
- Related to Bug #16475: Javascript mystery investigation on WB2 workaround added