Arvados

Interesting PAM modules (debian packages)

libpam-krb5 - PAM module for MIT Kerberos
libpam-ldapd - PAM module for using LDAP as an authentication service
libpam-mklocaluser - Configure PAM to create a local user if it do not exist already (for dev / demo)

Server side:

16212-pam-login @ d739042d5aedd9a2cef19deb591cccc57d639353 -- developer-run-tests: #1773 (tests fail because jenkins worker image doesn't have libpam-dev)

• Prompt user for username and password
• POST https://{apihost}/login, with username=x, password=y, and _method=GET in the request body (or "X-Http-Method-Override: GET" header instead of _method=GET)
• Get API token from "token" field from the response body
• If the "token" value is empty/missing, show the string in the "message" field, and allow the user to retry

16212-pam-login @ 4e0eb166fd808b32c10cccc2b4014a02edcf29a6 adds a test that authenticates to an OpenLDAP server through PAM.

It's disabled by default because it's a bit heavy and requires docker. You can run it from run-tests.sh interactive mode with "test lib/controller/localdb -tags docker -check.f=LDAP".

The LDAP test is being uncooperative:

Adding example user entry user=foo pass=secret (retrying until server comes up)
ldap_sasl_bind(SIMPLE): Can't contact LDAP server (-1)
ldap_sasl_bind(SIMPLE): Can't contact LDAP server (-1)
ldap_sasl_bind(SIMPLE): Can't contact LDAP server (-1)
ldap_sasl_bind(SIMPLE): Can't contact LDAP server (-1)
ldap_sasl_bind(SIMPLE): Can't contact LDAP server (-1)
failed to add user entry
ldap-25941

----------------------------------------------------------------------
FAIL: login_pam_docker_test.go:17: PamSuite.TestLoginLDAPViaPAM

login_pam_docker_test.go:22:
    c.Check(err, check.IsNil)
... value *exec.ExitError = &exec.ExitError{ProcessState:(*os.ProcessState)(0xc000316000), Stderr:[]uint8(nil)} ("exit status 1")

OOPS: 0 passed, 1 FAILED
--- FAIL: Test (179.04s)
FAIL

I'm trying to authenticate manually:

$ curl  -XPOST -F username=foo -F password=bar -k https://172.17.0.2:8000/login?_method=GET
{"message":"Authentication failure"}

2020-03-24_23:27:59.01338 {"PID":6052,"RequestID":"req-19dm6gasl4kkmgtt4bcy","level":"info","msg":"request","remoteAddr":"127.0.0.1:36490",
"reqBytes":246,"reqForwardedFor":"172.17.0.1","reqHost":"172.17.0.2:8000","reqMethod":"POST",
"reqPath":"login","reqQuery":"_method=GET","time":"2020-03-24T23:27:59.013280588Z"}

2020-03-24_23:28:01.34490 {"PID":6052,"RequestID":"req-19dm6gasl4kkmgtt4bcy","level":"info","msg":"response","remoteAddr":"127.0.0.1:36490",
"reqBytes":246,"reqForwardedFor":"172.17.0.1","reqHost":"172.17.0.2:8000","reqMethod":"POST",
"reqPath":"login","reqQuery":"_method=GET","respBytes":37,"respStatus":"OK","respStatusCode":200,
"time":"2020-03-24T23:28:01.344837921Z","timeToStatus":2.331543,"timeTotal":2.331551,"timeWriteBody":0.000007}

• Nothing logged indicating why it is rejected
• Return code is 200, should be an error like 401 or 403

This branch also needs an install documentation update about how to use PAM.

I think this should support HTTP basic auth, because it would make it possible for Workbench 1 to support the new login strategy with little or no change.

1. The workbench 1 login button sends you to the controller /login endpoint
2. It responds with 401 with supported method HTTP basic auth
3. The browser collects the username and password and resubmits using basic auth
4. Controller responds with a redirect back to workbench with the API token in the URL.

I've been trying to send the POST request from WB2 and getting CORS problems. The following made the browser accept the outgoing request, is it correct?

diff --git a/lib/controller/router/router.go b/lib/controller/router/router.go
index 69d707703..b2fd5e4ff 100644
--- a/lib/controller/router/router.go
+++ b/lib/controller/router/router.go
@@ -382,11 +382,11 @@ func (rtr *router) addRoute(endpoint arvados.APIEndpoint, defaultOpts func() int

 func (rtr *router) ServeHTTP(w http.ResponseWriter, r *http.Request) {
        switch strings.SplitN(strings.TrimLeft(r.URL.Path, "/"), "/", 2)[0] {
-       case "login", "logout", "auth":
+       case "logout", "auth":
        default:
                w.Header().Set("Access-Control-Allow-Origin", "*")
                w.Header().Set("Access-Control-Allow-Methods", "GET, HEAD, PUT, POST, PATCH, DELETE")
-               w.Header().Set("Access-Control-Allow-Headers", "Authorization, Content-Type")
+               w.Header().Set("Access-Control-Allow-Headers", "Authorization, Content-Type, X-Http-Method-Override")
                w.Header().Set("Access-Control-Max-Age", "86486400")
        }
        if r.Method == "OPTIONS" {

Workbench 2 PAM Login feature is (almost?) ready at commit: a308a27833843d90405b927ac491fea7c853b91c - branch 16212-login-form
I'm getting Authentication failure errors even if I started a fresh arvbox instance with the following config override file:

Clusters:
  x3sgo:
    Login:
      PAM: true
      PAMService: arvados
      ProviderAppID: "" 

Working on a missing case on WB2: The ability to display the login form when selecting remote clusters with Login.PAM: true

• This endpoint can safely accept CORS requests (cross-origin GET /login reqs can't be allowed because they might be forwarded to RailsAPI, which returns a token based on the request cookies/session -- and changing CORS based on auth config would be too fragile).
• No _method=GET hack needed
• No dual-personality LoginOptions struct

• Docker test: build an arvados-controller binary to use in the container, in case there are changes since the last time "install cmd/arvados-server" ran
• Return 401 (or other suitable code) instead of 200 when authentication fails
• More detail in failure messages. We don't get much information to convey, but at least we can mention the word PAM, and if the password is never even requested, we can mention that in case it's a useful clue.
• If needed, pull the ldap server docker image explicitly before calling docker-run. Perhaps this will help avoid the timeout encountered in note-10.

The case for HTTP authentication doesn't sound compelling to me. It invariably results in terrible UX. If we need something better than linking WB1 to WB2's login, I think it would make more sense to add a form on WB1.

Admin docs do need to be added but I don't think it's a blocker for merging the backend.

16212-pam-login @ 16b5f7275ffa2bd4347134f7269744f4cd4baa2a -- developer-run-tests: #1793

Thanks for the fixes and more verbose messaging.

Using arvbox I'm having an issue on WB2 that I've just was able to reproduce using curl.

The following (as per you docker test) fails:

$ curl -s --include -d username=foo -d password=bar -k https://controller:8000/arvados/v1/users/authenticate
HTTP/1.1 401 Unauthorized
Server: nginx/1.10.3
Date: Tue, 31 Mar 2020 21:58:09 GMT
Content-Type: application/json
Content-Length: 80
Connection: keep-alive
Access-Control-Allow-Headers: Authorization, Content-Type, X-Http-Method-Override
Access-Control-Allow-Methods: GET, HEAD, PUT, POST, PATCH, DELETE
Access-Control-Allow-Origin: *
Access-Control-Max-Age: 86486400
X-Content-Type-Options: nosniff

{"errors":["PAM: Authentication failure (with username \"foo\" and password)"]}

Then, if using curl with -F, it fails the same way than on WB2:

curl -s --include -X POST -F username=foo -F password=bar -k https://controller:8000/arvados/v1/users/authenticate
HTTP/1.1 401 Unauthorized
Server: nginx/1.10.3
Date: Tue, 31 Mar 2020 22:01:08 GMT
Content-Type: application/json
Content-Length: 77
Connection: keep-alive
Access-Control-Allow-Headers: Authorization, Content-Type, X-Http-Method-Override
Access-Control-Allow-Methods: GET, HEAD, PUT, POST, PATCH, DELETE
Access-Control-Allow-Origin: *
Access-Control-Max-Age: 86486400
X-Content-Type-Options: nosniff

{"errors":["PAM: Authentication failure (with username \"\" and password)"]}

Just for the record, I've run arvbox shell and created the foo user with password bar, it should be enough for the test to work, right?

Commit c217a294 at WB2 branch updates the login form to use the new endpoint and data url-encoding.

Still not able to fully test it as the PAM feature isn't working for me on arvbox, what I may be missing?

Figured out why it isn’t working on arvbox: because the process should be running as root, or else the auth process only works for the running process’ user… so I got into arvbox shell, set arvbox user a password, et voilá! :)

Taken from: https://pkg.go.dev/github.com/msteinert/pam?tab=doc#example-package-Authenticate

So on arvbox (and anywhere else?), arvados-controller should be running as root.

developer-run-tests: #1799

• return a "u/p auth not available" error (instead of forwarding the request to Rails and getting a "not logged in" error) if the arvados/v1/users/authenticate endpoint is used when PAM is not enabled
• handle the arvados/v1/users/authenticate endpoint in controller, even when ForceLegacyAPI14 mode is enabled (there is no legacy API for this)

I still can't seem to get this to work:

$ echo '{"username": "foo", "password": "bar"}' | curl -k -d- https://172.17.0.2:8000/arvados/v1/users/authenticate
{"errors":["PAM: Authentication failure (with username \"\" and password)"]}

$ curl -k -F username=foo -F password=bar  https://172.17.0.2:8000/arvados/v1/users/authenticate
{"errors":["PAM: Authentication failure (with username \"\" and password)"]}

The fact that username is being returned as empty doesn't inspire confidence. It should probably specifically check and specifically send an error on blank username and/or password.

We determined this works with curl -d username=foo ... instead of -F

Now it works. However, the "scopes" and "uuid" fields ought to have values:

$ curl -k -d username=foo -d password=bar https://172.17.0.2:8000/arvados/v1/users/authenticate {"api_token":"v2/x1u39-gj3su-s3ehqzevde7h9tz/5fpfi2ldbvl9hwd5ixwaoloujk8wpt1aifecc135qgtkmuw9da","expires_at":"","kind":"arvados#aPIClientAuthorization","scopes":null,"uuid":""}

Updated to return the full record. But now that it's a real api_client_authorization record, the api_token field only has the "secret" part, not the v2/$uuid/ part, so the wb2 code will need to change accordingly.

16212-pam-login @ 5a1b5b69bbd4aa6995164eefab7d7cea52ee40ed -- developer-run-tests: #1803

The LDAP test still won't start for me. I don't know if it is because of arvbox docker-in-docker or something else.

I'm inclined to say merge it because unix PAM works but leave the LDAP story open until we have a reliable PAM-LDAP test running somewhere (could be on jenkins).

Commit 2c1a7eb9 at 16212-login-form (wb2 branch) assembles v2 token from controller's response.

16212-pam-install-docs @ 5792ec3a8ddfdba959da5c09dfa1be4ac7472c20 -- developer-run-tests: #1823

Feel like this could be expanded at least a little bit about what PAM is (I don't see "Portable Authentication Module" spelled out anywhere), why you might want to use it, and a link to somewhere like http://www.linux-pam.org/ more information.

reviewing 16212-login-form @ 2c1a7eb9248df217c86caf1685a05d5a2aaaac84

First couple of comments, I haven't actually tried it yet:

• is it possible to add some Cypress testing, now that it is merged -- need to figure out how to have an authenticate endpoint for testing
• This is looking at the Login.PAM flag. This requires a backend tweak but it would be better if the configuration just advertises whether to use 'login' or 'authenticate' and wb2 doesn't need to know whether the backend is SSO or Google or LDAP or PAM or something else.

I tried to load workbench2 and it failed:

TypeError: Object(...) is not a function
./src/views-components/login-form/login-form.tsx/LoginForm<
src/workbench2/src/views-components/login-form/login-form.tsx:54

  51 | 
  52 | export const LoginForm = withStyles(styles)(
  53 |     ({ handleSubmit, loginLabel, dispatch, classes }: LoginFormProps) => {
> 54 |         const userInput = useRef<HTMLInputElement>(null);
  55 |         const [username, setUsername] = useState('');
  56 |         const [password, setPassword] = useState('');
  57 |         const [isButtonDisabled, setIsButtonDisabled] = useState(true);

Peter Amstutz wrote:

I tried to load workbench2 and it failed:

[...]

I needed to update modules, after `yarn install` it worked.

From chat:

PAM only provides a username. Currently the PAM support also fills in the email address field, which is either username@domain (if configured) or just the username. First_name and last_name are blank.

As a result, there are places in the UI where the user display is blank.

• The top of the user menu is blank.
• The sharing dialog shows <email> in search but then the chip is blank

Proposed solution: add a generic "user display name" function and use that everywhere.

Display strategy is A: first/last names, B: email, C: username

Updates at commit 1866fb495 (wb2 repo) address the comments from the note above.
Test run: developer-tests-workbench2: #23

On the particular sharing dialog case, I had to do some refactoring to clean that up, and took the opportunity to properly name the PeopleSelect component that wasn't just offering 'people selection' but also groups.
One thing that I'm not sure is a good idea is that the component makes requests to the API server while the user types to do auto-completion, and it just asks for the first 5 items using limit. I left it that way but if we're going to remove that limit, will have to see how to avoid the UI covering the input field whenever too many items are listed.

Updates at 4d3cc2eb8 - branch 16212-login-endpoint-exported-config
Test run: developer-run-tests: #1829

• Unexports config Login.PAM
• Adds exported config Login.Endpoint with default value login

Updates at commit eda123a5 (wb2 repo) - branch 16212-login-form
Test run: developer-tests-workbench2: #24

• Uses Login.Endpoint exported config to decide whether to show the login button or form

Lucas Di Pentima wrote:

Updates at 4d3cc2eb8 - branch 16212-login-endpoint-exported-config
Test run: developer-run-tests: #1829

• Unexports config Login.PAM
• Adds exported config Login.Endpoint with default value login

Could the config loader set this automatically based on which login method is enabled? Then it doesn't need to be mentioned anywhere in config.default.yml.

The following discussion went on chat about adding a computed config knob about login endpoints:

[...]
Lucas Di Pentima @ldipenti 16:57
When I try to run the arvados boot, with a config that has a zzzzz cluster, xxxxx still appears, and errors out because all login options are disabled

Tom Clegg @tomclegg 16:58
Maybe it shouldn't become an error to have no login options enabled.
I know, that means no user can login via web, but there are other ways of getting tokens -- e.g., while you're setting up a new cluster and you don't need more strictly ordered sequences of install steps.

Lucas Di Pentima @ldipenti 17:00
Ok, will refine the check, thanks

Tom Clegg @tomclegg 17:00
yw

Lucas Di Pentima @ldipenti 17:01
Other thing I saw, if we start using this kind of computed configs, if they’re not listed on the default config they’ll be notified as deprecated or unknown

Tom Clegg @tomclegg 17:02
I see why you're adding it (because exported config is a subset of actual config), but don't love the idea of adding a config file entry that isn't actually a config knob.

Lucas Di Pentima @ldipenti 17:02
OTOH, adding those to the default config exposes them to the public

Tom Clegg @tomclegg 17:02
IOW, "computed config" isn't config.

Lucas Di Pentima @ldipenti 17:02
right :)
had a funny smell my approach
So, should I make exported config not a sctrict config subset?

Tom Clegg @tomclegg 17:03
So I wonder what our other options are. I had decided to just export PAM and leave the pam-awareness in wb/wb2 for now, even though ideally it should be abstracted out.

Lucas Di Pentima @ldipenti 17:04
Yes, that was the way it worked the branch some weeks go :D
the issue that I think talked about with @tetron is what would happen when ldap is added

Tom Clegg @tomclegg 17:06
Frankly that's still my preferred approach. It's simple, and relegated to about 2 LOC. The alternative (so far) touches lots of places and makes more weirdness.
Maybe we can fix it between now and LDAP (or expand it to "PAM || LDAP") instead of getting hung up on it here.
Or we can pause here and figure out the real answer to "server config as needed by an API client is not actually a strict subset of server config as seen by a human admin" 
e.g., is this something that really belongs in the discovery doc? (remembering that the DD already has lots of things that don't belong there)

Tom Clegg @tomclegg 17:11
The distinction between "exported config" and "api discovery doc" is fuzzy

Lucas Di Pentima @ldipenti 17:11
You mean, putting the Endpoint as a DD data instead of exported config?
I think it makes more sense to put it on the DD, as it’s a mixed bag of things, and not to put it on the exported config, avoiding making that another mixed bag of things in the process.

Tom Clegg @tomclegg 17:13
Hm. Maybe we should follow the crunch1 example. Advertise the endpoint(s) that can actually work.
So delete /login or /authenticate from the discovery doc, depending on pam. This is a bit sketchy given our discovery doc caching strategy, though.

Lucas Di Pentima @ldipenti 17:16
Ok, will go that way! Caching may not be an issue as it isn’t something that changes frequently

Tom Clegg @tomclegg 17:16
You know ... I think we should just leave it as PAM: true for now. Specifying one of two endpoints isn't a good solution anyway -- it can't support enabling multiple auth mechanisms, which surely we'll want soon enough.

So, branch 16212-login-endpoint-exported-config isn't needed anymore.

As for the wb2 branch 16212-login-form, I rebased it to drop the commit that changed config's use from Login.PAM to Login.Endpoint, so now its on commit 63ee9df09 (wb2 repo of course, is there a way to link commit from that repo here?), with a couple of new integration tests.
Test run: developer-tests-workbench2: #25

Lucas Di Pentima wrote:

The following discussion went on chat about adding a computed config knob about login endpoints:

[...]

I guess I've been outvoted, then.

So, branch 16212-login-endpoint-exported-config isn't needed anymore.

As for the wb2 branch 16212-login-form, I rebased it to drop the commit that changed config's use from Login.PAM to Login.Endpoint, so now its on commit 63ee9df09 (wb2 repo of course, is there a way to link commit from that repo here?), with a couple of new integration tests.
Test run: developer-tests-workbench2: #25

LGTM.

Peter Amstutz wrote:

Feel like this could be expanded at least a little bit about what PAM is (I don't see "Portable Authentication Module" spelled out anywhere), why you might want to use it, and a link to somewhere like http://www.linux-pam.org/ more information.

16212-pam-install-docs @ 44a02057129016d806b32cc5478bdffef1a565f8

Tom Clegg wrote:

Peter Amstutz wrote:

Feel like this could be expanded at least a little bit about what PAM is (I don't see "Portable Authentication Module" spelled out anywhere), why you might want to use it, and a link to somewhere like http://www.linux-pam.org/ more information.

16212-pam-install-docs @ 44a02057129016d806b32cc5478bdffef1a565f8

Thanks, that's exactly what I had in mind. LGTM.