From 521e8ecf4ac93ac27c7bec97601c246e391daf43 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Mon, 30 Mar 2020 17:26:13 -0400 Subject: [PATCH] 16212: Move user/pass authentication to its own endpoint. Overloading the /login endpoint turns out to be too awkward: method, CORS permissions, and response type are all different. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- lib/controller/federation/conn.go | 4 ++++ lib/controller/federation/login_test.go | 3 +++ lib/controller/handler.go | 1 + lib/controller/localdb/conn.go | 4 ++++ lib/controller/localdb/login.go | 4 ++++ lib/controller/localdb/login_google.go | 6 +++++ lib/controller/localdb/login_pam.go | 22 ++++++++++--------- .../localdb/login_pam_docker_test.sh | 6 ++--- lib/controller/localdb/login_pam_test.go | 15 ++++--------- lib/controller/router/router.go | 7 ++++++ lib/controller/rpc/conn.go | 9 +++++++- sdk/go/arvados/api.go | 13 +++++++---- sdk/go/arvados/login.go | 9 -------- sdk/go/arvadostest/api.go | 4 ++++ 14 files changed, 69 insertions(+), 38 deletions(-) diff --git a/lib/controller/federation/conn.go b/lib/controller/federation/conn.go index 279b7a51d5..3e37a5c618 100644 --- a/lib/controller/federation/conn.go +++ b/lib/controller/federation/conn.go @@ -478,6 +478,10 @@ func (conn *Conn) UserBatchUpdate(ctx context.Context, options arvados.UserBatch return conn.local.UserBatchUpdate(ctx, options) } +func (conn *Conn) UserAuthenticate(ctx context.Context, options arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) { + return conn.local.UserAuthenticate(ctx, options) +} + func (conn *Conn) APIClientAuthorizationCurrent(ctx context.Context, options arvados.GetOptions) (arvados.APIClientAuthorization, error) { return conn.chooseBackend(options.UUID).APIClientAuthorizationCurrent(ctx, options) } diff --git a/lib/controller/federation/login_test.go b/lib/controller/federation/login_test.go index 1d6e12e015..2de260fdc2 100644 --- a/lib/controller/federation/login_test.go +++ b/lib/controller/federation/login_test.go @@ -46,6 +46,9 @@ func (s *LoginSuite) TestLogout(c *check.C) { s.cluster.Login.GoogleClientID = "zzzzzzzzzzzzzz" s.addHTTPRemote(c, "zhome", &arvadostest.APIStub{}) s.cluster.Login.LoginCluster = "zhome" + // s.fed is already set by SetUpTest, but we need to + // reinitialize with the above config changes. + s.fed = New(s.cluster) returnTo := "https://app.example.com/foo?bar" for _, trial := range []struct { diff --git a/lib/controller/handler.go b/lib/controller/handler.go index e3869261a1..d7bc9bd9a2 100644 --- a/lib/controller/handler.go +++ b/lib/controller/handler.go @@ -85,6 +85,7 @@ func (h *Handler) setup() { mux.Handle("/arvados/v1/collections/", rtr) mux.Handle("/arvados/v1/users", rtr) mux.Handle("/arvados/v1/users/", rtr) + mux.Handle("/"+arvados.EndpointUserAuthenticate.Path, rtr) mux.Handle("/login", rtr) mux.Handle("/logout", rtr) } diff --git a/lib/controller/localdb/conn.go b/lib/controller/localdb/conn.go index 909b6e1ff3..60263455bd 100644 --- a/lib/controller/localdb/conn.go +++ b/lib/controller/localdb/conn.go @@ -36,3 +36,7 @@ func (conn *Conn) Logout(ctx context.Context, opts arvados.LogoutOptions) (arvad func (conn *Conn) Login(ctx context.Context, opts arvados.LoginOptions) (arvados.LoginResponse, error) { return conn.loginController.Login(ctx, opts) } + +func (conn *Conn) UserAuthenticate(ctx context.Context, opts arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) { + return conn.loginController.UserAuthenticate(ctx, opts) +} diff --git a/lib/controller/localdb/login.go b/lib/controller/localdb/login.go index af9a034827..2d20531714 100644 --- a/lib/controller/localdb/login.go +++ b/lib/controller/localdb/login.go @@ -14,6 +14,7 @@ import ( type loginController interface { Login(ctx context.Context, opts arvados.LoginOptions) (arvados.LoginResponse, error) Logout(ctx context.Context, opts arvados.LogoutOptions) (arvados.LogoutResponse, error) + UserAuthenticate(ctx context.Context, options arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) } func chooseLoginController(cluster *arvados.Cluster, railsProxy *railsProxy) loginController { @@ -42,6 +43,9 @@ func (ctrl errorLoginController) Login(context.Context, arvados.LoginOptions) (a func (ctrl errorLoginController) Logout(context.Context, arvados.LogoutOptions) (arvados.LogoutResponse, error) { return arvados.LogoutResponse{}, ctrl.error } +func (ctrl errorLoginController) UserAuthenticate(context.Context, arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) { + return arvados.APIClientAuthorization{}, ctrl.error +} func noopLogout(cluster *arvados.Cluster, opts arvados.LogoutOptions) (arvados.LogoutResponse, error) { target := opts.ReturnTo diff --git a/lib/controller/localdb/login_google.go b/lib/controller/localdb/login_google.go index 61bbaf01b1..bf1754c158 100644 --- a/lib/controller/localdb/login_google.go +++ b/lib/controller/localdb/login_google.go @@ -12,6 +12,7 @@ import ( "encoding/base64" "errors" "fmt" + "net/http" "net/url" "strings" "sync" @@ -22,6 +23,7 @@ import ( "git.arvados.org/arvados.git/sdk/go/arvados" "git.arvados.org/arvados.git/sdk/go/auth" "git.arvados.org/arvados.git/sdk/go/ctxlog" + "git.arvados.org/arvados.git/sdk/go/httpserver" "github.com/coreos/go-oidc" "golang.org/x/oauth2" "google.golang.org/api/option" @@ -129,6 +131,10 @@ func (ctrl *googleLoginController) Login(ctx context.Context, opts arvados.Login } } +func (ctrl *googleLoginController) UserAuthenticate(ctx context.Context, opts arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) { + return arvados.APIClientAuthorization{}, httpserver.ErrorWithStatus(errors.New("username/password authentication is not available"), http.StatusBadRequest) +} + // Use a person's token to get all of their email addresses, with the // primary address at index 0. The provided defaultAddr is always // included in the returned slice, and is used as the primary if the diff --git a/lib/controller/localdb/login_pam.go b/lib/controller/localdb/login_pam.go index 059e27fa26..be90cb3d14 100644 --- a/lib/controller/localdb/login_pam.go +++ b/lib/controller/localdb/login_pam.go @@ -31,6 +31,10 @@ func (ctrl *pamLoginController) Logout(ctx context.Context, opts arvados.LogoutO } func (ctrl *pamLoginController) Login(ctx context.Context, opts arvados.LoginOptions) (arvados.LoginResponse, error) { + return arvados.LoginResponse{}, errors.New("interactive login is not available") +} + +func (ctrl *pamLoginController) UserAuthenticate(ctx context.Context, opts arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) { errorMessage := "" tx, err := pam.StartFunc(ctrl.Cluster.Login.PAMService, opts.Username, func(style pam.Style, message string) (string, error) { ctxlog.FromContext(ctx).Debugf("pam conversation: style=%v message=%q", style, message) @@ -49,18 +53,18 @@ func (ctrl *pamLoginController) Login(ctx context.Context, opts arvados.LoginOpt } }) if err != nil { - return arvados.LoginResponse{}, err + return arvados.APIClientAuthorization{}, err } err = tx.Authenticate(pam.DisallowNullAuthtok) if err != nil { - return arvados.LoginResponse{}, httpserver.ErrorWithStatus(err, http.StatusUnauthorized) + return arvados.APIClientAuthorization{}, httpserver.ErrorWithStatus(err, http.StatusUnauthorized) } if errorMessage != "" { - return arvados.LoginResponse{}, httpserver.ErrorWithStatus(errors.New(errorMessage), http.StatusUnauthorized) + return arvados.APIClientAuthorization{}, httpserver.ErrorWithStatus(errors.New(errorMessage), http.StatusUnauthorized) } user, err := tx.GetItem(pam.User) if err != nil { - return arvados.LoginResponse{}, err + return arvados.APIClientAuthorization{}, err } email := user if domain := ctrl.Cluster.Login.PAMDefaultEmailDomain; domain != "" && !strings.Contains(email, "@") { @@ -72,20 +76,18 @@ func (ctrl *pamLoginController) Login(ctx context.Context, opts arvados.LoginOpt // Send a fake ReturnTo value instead of the caller's // opts.ReturnTo. We won't follow the resulting // redirect target anyway. - ReturnTo: opts.Remote + ",https://none.invalid", + ReturnTo: ",https://none.invalid", AuthInfo: rpc.UserSessionAuthInfo{ Username: user, Email: email, }, }) if err != nil { - return arvados.LoginResponse{}, err + return arvados.APIClientAuthorization{}, err } target, err := url.Parse(resp.RedirectLocation) if err != nil { - return arvados.LoginResponse{}, err + return arvados.APIClientAuthorization{}, err } - resp.Token = target.Query().Get("api_token") - resp.RedirectLocation = "" - return resp, err + return arvados.APIClientAuthorization{APIToken: target.Query().Get("api_token")}, err } diff --git a/lib/controller/localdb/login_pam_docker_test.sh b/lib/controller/localdb/login_pam_docker_test.sh index 1d37f5c1c9..c71de47a45 100755 --- a/lib/controller/localdb/login_pam_docker_test.sh +++ b/lib/controller/localdb/login_pam_docker_test.sh @@ -170,13 +170,13 @@ check_contains() { } echo >&2 "Testing authentication failure" -resp="$(curl -s --include -H "X-Http-Method-Override: GET" -d username=foo -d password=nosecret "http://${ctrlhostport}/login" | tee $debug)" +resp="$(curl -s --include -d username=foo -d password=nosecret "http://${ctrlhostport}/arvados/v1/users/authenticate" | tee $debug)" check_contains "${resp}" "HTTP/1.1 401" check_contains "${resp}" '{"errors":["Authentication failure"]}' echo >&2 "Testing authentication success" -resp="$(curl -s --include -H "X-Http-Method-Override: GET" -d username=foo -d password=secret "http://${ctrlhostport}/login" | tee $debug)" +resp="$(curl -s --include -d username=foo -d password=secret "http://${ctrlhostport}/arvados/v1/users/authenticate" | tee $debug)" check_contains "${resp}" "HTTP/1.1 200" -check_contains "${resp}" '{"token":"v2/zzzzz-gj3su-' +check_contains "${resp}" '{"api_token":"v2/zzzzz-gj3su-' cleanup diff --git a/lib/controller/localdb/login_pam_test.go b/lib/controller/localdb/login_pam_test.go index 4af40f8d38..4ecead8b15 100644 --- a/lib/controller/localdb/login_pam_test.go +++ b/lib/controller/localdb/login_pam_test.go @@ -42,20 +42,16 @@ func (s *PamSuite) SetUpSuite(c *check.C) { } func (s *PamSuite) TestLoginFailure(c *check.C) { - resp, err := s.ctrl.Login(context.Background(), arvados.LoginOptions{ + resp, err := s.ctrl.UserAuthenticate(context.Background(), arvados.UserAuthenticateOptions{ Username: "bogususername", Password: "boguspassword", - ReturnTo: "https://example.com/foo", }) c.Check(err, check.ErrorMatches, "Authentication failure") hs, ok := err.(interface{ HTTPStatus() int }) if c.Check(ok, check.Equals, true) { c.Check(hs.HTTPStatus(), check.Equals, http.StatusUnauthorized) } - c.Check(resp.RedirectLocation, check.Equals, "") - c.Check(resp.Token, check.Equals, "") - c.Check(resp.Message, check.Equals, "") - c.Check(resp.HTML.String(), check.Equals, "") + c.Check(resp.APIToken, check.Equals, "") } // This test only runs if the ARVADOS_TEST_PAM_CREDENTIALS_FILE env @@ -73,15 +69,12 @@ func (s *PamSuite) TestLoginSuccess(c *check.C) { c.Assert(len(lines), check.Equals, 2, check.Commentf("credentials file %s should contain \"username\\npassword\"", testCredsFile)) u, p := lines[0], lines[1] - resp, err := s.ctrl.Login(context.Background(), arvados.LoginOptions{ + resp, err := s.ctrl.UserAuthenticate(context.Background(), arvados.UserAuthenticateOptions{ Username: u, Password: p, - ReturnTo: "https://example.com/foo", }) c.Check(err, check.IsNil) - c.Check(resp.RedirectLocation, check.Equals, "") - c.Check(resp.Token, check.Matches, `v2/zzzzz-gj3su-.*/.*`) - c.Check(resp.HTML.String(), check.Equals, "") + c.Check(resp.APIToken, check.Matches, `v2/zzzzz-gj3su-.*/.*`) authinfo := getCallbackAuthInfo(c, s.railsSpy) c.Check(authinfo.Email, check.Equals, u+"@"+s.cluster.Login.PAMDefaultEmailDomain) diff --git a/lib/controller/router/router.go b/lib/controller/router/router.go index bb6d905585..59205788dd 100644 --- a/lib/controller/router/router.go +++ b/lib/controller/router/router.go @@ -303,6 +303,13 @@ func (rtr *router) addRoutes() { return rtr.fed.UserDelete(ctx, *opts.(*arvados.DeleteOptions)) }, }, + { + arvados.EndpointUserAuthenticate, + func() interface{} { return &arvados.UserAuthenticateOptions{} }, + func(ctx context.Context, opts interface{}) (interface{}, error) { + return rtr.fed.UserAuthenticate(ctx, *opts.(*arvados.UserAuthenticateOptions)) + }, + }, } { rtr.addRoute(route.endpoint, route.defaultOpts, route.exec) } diff --git a/lib/controller/rpc/conn.go b/lib/controller/rpc/conn.go index 4b143b770b..c3c66d00a3 100644 --- a/lib/controller/rpc/conn.go +++ b/lib/controller/rpc/conn.go @@ -417,8 +417,15 @@ func (conn *Conn) UserSessionCreate(ctx context.Context, options UserSessionCrea } func (conn *Conn) UserBatchUpdate(ctx context.Context, options arvados.UserBatchUpdateOptions) (arvados.UserList, error) { - ep := arvados.APIEndpoint{Method: "PATCH", Path: "arvados/v1/users/batch_update"} + ep := arvados.EndpointUserBatchUpdate var resp arvados.UserList err := conn.requestAndDecode(ctx, &resp, ep, nil, options) return resp, err } + +func (conn *Conn) UserAuthenticate(ctx context.Context, options arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) { + ep := arvados.EndpointUserAuthenticate + var resp arvados.APIClientAuthorization + err := conn.requestAndDecode(ctx, &resp, ep, nil, options) + return resp, err +} diff --git a/sdk/go/arvados/api.go b/sdk/go/arvados/api.go index ff0dcf75a7..cec6e26b6d 100644 --- a/sdk/go/arvados/api.go +++ b/sdk/go/arvados/api.go @@ -56,6 +56,7 @@ var ( EndpointUserUpdate = APIEndpoint{"PATCH", "arvados/v1/users/{uuid}", "user"} EndpointUserUpdateUUID = APIEndpoint{"POST", "arvados/v1/users/{uuid}/update_uuid", ""} EndpointUserBatchUpdate = APIEndpoint{"PATCH", "arvados/v1/users/batch", ""} + EndpointUserAuthenticate = APIEndpoint{"POST", "arvados/v1/users/authenticate", ""} EndpointAPIClientAuthorizationCurrent = APIEndpoint{"GET", "arvados/v1/api_client_authorizations/current", ""} ) @@ -136,10 +137,13 @@ type DeleteOptions struct { } type LoginOptions struct { - ReturnTo string `json:"return_to"` // On success, redirect to this target with api_token=xxx query param - Remote string `json:"remote,omitempty"` // Salt token for remote Cluster ID - Code string `json:"code,omitempty"` // OAuth2 callback code - State string `json:"state,omitempty"` // OAuth2 callback state + ReturnTo string `json:"return_to"` // On success, redirect to this target with api_token=xxx query param + Remote string `json:"remote,omitempty"` // Salt token for remote Cluster ID + Code string `json:"code,omitempty"` // OAuth2 callback code + State string `json:"state,omitempty"` // OAuth2 callback state +} + +type UserAuthenticateOptions struct { Username string `json:"username,omitempty"` // PAM username Password string `json:"password,omitempty"` // PAM password } @@ -186,5 +190,6 @@ type API interface { UserList(ctx context.Context, options ListOptions) (UserList, error) UserDelete(ctx context.Context, options DeleteOptions) (User, error) UserBatchUpdate(context.Context, UserBatchUpdateOptions) (UserList, error) + UserAuthenticate(ctx context.Context, options UserAuthenticateOptions) (APIClientAuthorization, error) APIClientAuthorizationCurrent(ctx context.Context, options GetOptions) (APIClientAuthorization, error) } diff --git a/sdk/go/arvados/login.go b/sdk/go/arvados/login.go index 26c04b2c13..9178cc6a27 100644 --- a/sdk/go/arvados/login.go +++ b/sdk/go/arvados/login.go @@ -6,14 +6,11 @@ package arvados import ( "bytes" - "encoding/json" "net/http" ) type LoginResponse struct { RedirectLocation string `json:"redirect_location,omitempty"` - Token string `json:"token,omitempty"` - Message string `json:"message,omitempty"` HTML bytes.Buffer `json:"-"` } @@ -22,12 +19,6 @@ func (resp LoginResponse) ServeHTTP(w http.ResponseWriter, req *http.Request) { if resp.RedirectLocation != "" { w.Header().Set("Location", resp.RedirectLocation) w.WriteHeader(http.StatusFound) - } else if resp.Token != "" || resp.Message != "" { - w.Header().Set("Content-Type", "application/json") - if resp.Token == "" { - w.WriteHeader(http.StatusUnauthorized) - } - json.NewEncoder(w).Encode(resp) } else { w.Header().Set("Content-Type", "text/html") w.Write(resp.HTML.Bytes()) diff --git a/sdk/go/arvadostest/api.go b/sdk/go/arvadostest/api.go index 9019d33cfb..fa5f539360 100644 --- a/sdk/go/arvadostest/api.go +++ b/sdk/go/arvadostest/api.go @@ -177,6 +177,10 @@ func (as *APIStub) UserBatchUpdate(ctx context.Context, options arvados.UserBatc as.appendCall(as.UserBatchUpdate, ctx, options) return arvados.UserList{}, as.Error } +func (as *APIStub) UserAuthenticate(ctx context.Context, options arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) { + as.appendCall(as.UserAuthenticate, ctx, options) + return arvados.APIClientAuthorization{}, as.Error +} func (as *APIStub) APIClientAuthorizationCurrent(ctx context.Context, options arvados.GetOptions) (arvados.APIClientAuthorization, error) { as.appendCall(as.APIClientAuthorizationCurrent, ctx, options) return arvados.APIClientAuthorization{}, as.Error -- 2.30.2