From fedd301104ee5109630840d520bea03d44300392 Mon Sep 17 00:00:00 2001 From: Kian Parvin Date: Thu, 11 Jul 2024 13:52:06 +0200 Subject: [PATCH 1/5] Add device flow login test --- internal/jimmhttp/websocket_test.go | 7 ++- internal/jimmtest/auth.go | 94 +++++++++++++++++++++++++---- internal/jimmtest/suite.go | 17 ++++-- internal/jujuapi/websocket_test.go | 89 +++++++++++++++++++++------ internal/rpc/proxy.go | 12 ++-- internal/rpc/proxy_test.go | 4 +- 6 files changed, 175 insertions(+), 48 deletions(-) diff --git a/internal/jimmhttp/websocket_test.go b/internal/jimmhttp/websocket_test.go index 8079a0b5b..d95554358 100644 --- a/internal/jimmhttp/websocket_test.go +++ b/internal/jimmhttp/websocket_test.go @@ -111,11 +111,12 @@ func TestWSHandlerNilServer(t *testing.T) { c.Assert(err, qt.ErrorMatches, `websocket: close 1000 \(normal\)`) } -type authFailServer struct{} +type authFailServer struct{ c jimmtest.SimpleTester } // GetAuthenticationService returns JIMM's oauth authentication service. func (s authFailServer) GetAuthenticationService() jimm.OAuthAuthenticator { - return jimmtest.NewMockOAuthAuthenticator("") + authenticator := jimmtest.NewMockOAuthAuthenticator(s.c, nil) + return &authenticator } func (s authFailServer) ServeWS(ctx context.Context, conn *websocket.Conn) {} @@ -124,7 +125,7 @@ func TestWSHandlerAuthFailsServer(t *testing.T) { c := qt.New(t) hnd := &jimmhttp.WSHandler{ - Server: authFailServer{}, + Server: authFailServer{c: c}, } srv := httptest.NewServer(hnd) diff --git a/internal/jimmtest/auth.go b/internal/jimmtest/auth.go index 1d44dcdf7..e0f98632f 100644 --- a/internal/jimmtest/auth.go +++ b/internal/jimmtest/auth.go @@ -21,11 +21,12 @@ import ( "github.com/coreos/go-oidc/v3/oidc" "github.com/go-chi/chi/v5" + "github.com/google/uuid" "github.com/gorilla/sessions" "github.com/juju/juju/api" jujuparams "github.com/juju/juju/rpc/params" - "github.com/lestrrat-go/jwx/v2/jwa" "github.com/lestrrat-go/jwx/v2/jwt" + "golang.org/x/oauth2" "github.com/canonical/jimm/internal/auth" "github.com/canonical/jimm/internal/db" @@ -63,16 +64,51 @@ func (a Authenticator) Authenticate(_ context.Context, _ *jujuparams.LoginReques type MockOAuthAuthenticator struct { jimm.OAuthAuthenticator + c SimpleTester + // PollingChan is used to simulate polling an OIDC server during the device flow. + // It expects a username to be received that will be used to generate the user's access token. + PollingChan <-chan string + polledUsername string + mockAccessToken string } -func NewMockOAuthAuthenticator(secretKey string) MockOAuthAuthenticator { - return MockOAuthAuthenticator{} +func NewMockOAuthAuthenticator(c SimpleTester, testChan <-chan string) MockOAuthAuthenticator { + return MockOAuthAuthenticator{c: c, PollingChan: testChan} +} + +// Device is a mock implementation for the start of the device flow, returning dummy polling data. +func (m *MockOAuthAuthenticator) Device(ctx context.Context) (*oauth2.DeviceAuthResponse, error) { + return &oauth2.DeviceAuthResponse{ + DeviceCode: "test-device-code", + UserCode: "test-user-code", + VerificationURI: "http://no-such-uri.canonical.com", + VerificationURIComplete: "http://no-such-uri.canonical.com", + Expiry: time.Now().Add(time.Minute), + Interval: int64(time.Minute.Seconds()), + }, nil +} + +// DeviceAccessToken is a mock implementation of the second step in the device flow where JIMM +// polls an OIDC server for the device code. +func (m *MockOAuthAuthenticator) DeviceAccessToken(ctx context.Context, res *oauth2.DeviceAuthResponse) (*oauth2.Token, error) { + select { + case username := <-m.PollingChan: + m.polledUsername = username + uuid, err := uuid.NewRandom() + if err != nil { + m.c.Fatalf("failed to generate UUID for device access token") + } + m.mockAccessToken = uuid.String() + case <-ctx.Done(): + return &oauth2.Token{}, ctx.Err() + } + return &oauth2.Token{AccessToken: m.mockAccessToken}, nil } // VerifySessionToken provides the mock implementation for verifying session tokens. // Allowing JIMM tests to create their own session tokens that will always be accepted. // Notice the use of jwt.ParseInsecure to skip JWT signature verification. -func (m MockOAuthAuthenticator) VerifySessionToken(token string) (jwt.Token, error) { +func (m *MockOAuthAuthenticator) VerifySessionToken(token string) (jwt.Token, error) { decodedToken, err := base64.StdEncoding.DecodeString(token) if err != nil { return nil, errors.New("authentication failed, failed to decode token") @@ -89,14 +125,41 @@ func (m MockOAuthAuthenticator) VerifySessionToken(token string) (jwt.Token, err return parsedToken, nil } -func (m MockOAuthAuthenticator) AuthenticateBrowserSession(ctx context.Context, w http.ResponseWriter, req *http.Request) (context.Context, error) { +// ExtractAndVerifyIDToken return an ID token where the subject matches the subject obtained during the device flow. +// The auth token must match the one returned during the device flow. +func (m *MockOAuthAuthenticator) ExtractAndVerifyIDToken(ctx context.Context, oauth2Token *oauth2.Token) (*oidc.IDToken, error) { + if m.polledUsername == "" { + return &oidc.IDToken{}, errors.New("unknown user for mock auth login") + } + if m.mockAccessToken != oauth2Token.AccessToken { + return &oidc.IDToken{}, errors.New("access token does not match the generated access token") + } + return &oidc.IDToken{Subject: m.polledUsername}, nil +} + +// Email returns the subject from an ID token. +func (m *MockOAuthAuthenticator) Email(idToken *oidc.IDToken) (string, error) { + return idToken.Subject, nil +} + +// UpdateIdentity is a no-op mock. +func (m *MockOAuthAuthenticator) UpdateIdentity(ctx context.Context, email string, token *oauth2.Token) error { + return nil +} + +// MintSessionToken creates an unsigned session token with the email provided. +func (m *MockOAuthAuthenticator) MintSessionToken(email string) (string, error) { + return newUnsignedSessionToken(m.c, email), nil +} + +// AuthenticateBrowserSession always returns an error. +func (m *MockOAuthAuthenticator) AuthenticateBrowserSession(ctx context.Context, w http.ResponseWriter, req *http.Request) (context.Context, error) { return ctx, errors.New("authentication failed") } -// NewUserSessionLogin returns a login provider than be used with Juju Dial Opts -// to define how login will take place. In this case we login using a session token -// that the JIMM server should verify with the same test secret. -func NewUserSessionLogin(c SimpleTester, username string) api.LoginProvider { +// Below are helper functions used for handling auth in tests. + +func newUnsignedSessionToken(c SimpleTester, username string) string { email := convertUsernameToEmail(username) token, err := jwt.NewBuilder(). Subject(email). @@ -105,13 +168,18 @@ func NewUserSessionLogin(c SimpleTester, username string) api.LoginProvider { if err != nil { c.Fatalf("failed to generate test session token") } - - freshToken, err := jwt.Sign(token, jwt.WithKey(jwa.HS256, []byte(JWTTestSecret))) + serialisedToken, err := jwt.NewSerializer().Serialize(token) if err != nil { - c.Fatalf("failed to sign test session token") + c.Fatalf("failed to serialise token") } + return base64.StdEncoding.EncodeToString(serialisedToken) +} - b64Token := base64.StdEncoding.EncodeToString(freshToken) +// NewUserSessionLogin returns a login provider than be used with Juju Dial Opts +// to define how login will take place. In this case we login using a session token +// that the JIMM server should verify with the same test secret. +func NewUserSessionLogin(c SimpleTester, username string) api.LoginProvider { + b64Token := newUnsignedSessionToken(c, username) return api.NewSessionTokenLoginProvider(b64Token, nil, nil) } diff --git a/internal/jimmtest/suite.go b/internal/jimmtest/suite.go index 564826c42..abf6bca0c 100644 --- a/internal/jimmtest/suite.go +++ b/internal/jimmtest/suite.go @@ -60,8 +60,9 @@ type JIMMSuite struct { COFGAClient *cofga.Client COFGAParams *cofga.OpenFGAParams - Server *httptest.Server - cancel context.CancelFunc + Server *httptest.Server + cancel context.CancelFunc + deviceFlowChan chan string } func (s *JIMMSuite) SetUpTest(c *gc.C) { @@ -85,8 +86,9 @@ func (s *JIMMSuite) SetUpTest(c *gc.C) { ctx, cancel := context.WithCancel(context.Background()) s.cancel = cancel - // Note that the secret key here must match what is used in tests. - s.JIMM.OAuthAuthenticator = NewMockOAuthAuthenticator(JWTTestSecret) + s.deviceFlowChan = make(chan string, 1) + authenticator := NewMockOAuthAuthenticator(c, s.deviceFlowChan) + s.JIMM.OAuthAuthenticator = &authenticator err = s.JIMM.Database.Migrate(ctx, false) c.Assert(err, gc.Equals, nil) @@ -247,6 +249,13 @@ func (s *JIMMSuite) AddModel(c *gc.C, owner names.UserTag, name string, cloud na return names.NewModelTag(mi.UUID) } +// Call this non-blocking function before login to ensure device flow won't block. +// +// This is necessary as the mock authenticator simulates polling an external OIDC server. +func (s *JIMMSuite) ContinueDeviceFlow(username string) { + s.deviceFlowChan <- username +} + // A JujuSuite is a suite that intialises a JIMM and adds the testing juju // controller. type JujuSuite struct { diff --git a/internal/jujuapi/websocket_test.go b/internal/jujuapi/websocket_test.go index 7a6da625a..627bcb210 100644 --- a/internal/jujuapi/websocket_test.go +++ b/internal/jujuapi/websocket_test.go @@ -104,13 +104,11 @@ func (s *websocketSuite) TearDownTest(c *gc.C) { // If info is nil then default values will be used. func (s *websocketSuite) openNoAssert( c *gc.C, - info *api.Info, - username string, - dialWebsocket func(ctx context.Context, urlStr string, tlsConfig *tls.Config, ipAddr string) (jsoncodec.JSONConn, error), + d loginDetails, ) (api.Connection, error) { var inf api.Info - if info != nil { - inf = *info + if d.info != nil { + inf = *d.info } u, err := url.Parse(s.HTTP.URL) c.Assert(err, gc.Equals, nil) @@ -125,33 +123,49 @@ func (s *websocketSuite) openNoAssert( c.Assert(err, gc.Equals, nil) inf.CACert = w.String() - lp := jimmtest.NewUserSessionLogin(c, username) + if d.lp == nil { + d.lp = jimmtest.NewUserSessionLogin(c, d.username) + } dialOpts := api.DialOpts{ InsecureSkipVerify: true, - LoginProvider: lp, + LoginProvider: d.lp, } - if dialWebsocket != nil { - dialOpts.DialWebsocket = dialWebsocket + if d.dialWebsocket != nil { + dialOpts.DialWebsocket = d.dialWebsocket } return api.Open(&inf, dialOpts) } +type loginDetails struct { + info *api.Info + username string + lp api.LoginProvider + dialWebsocket func(ctx context.Context, urlStr string, tlsConfig *tls.Config, ipAddr string) (jsoncodec.JSONConn, error) +} + func (s *websocketSuite) open(c *gc.C, info *api.Info, username string) api.Connection { - conn, err := s.openNoAssert(c, info, username, nil) + ld := loginDetails{info: info, username: username} + conn, err := s.openNoAssert(c, ld) c.Assert(err, gc.Equals, nil) return conn } +func (s *websocketSuite) openCustomLP(c *gc.C, info *api.Info, username string, lp api.LoginProvider) (api.Connection, error) { + ld := loginDetails{info: info, username: username, lp: lp} + return s.openNoAssert(c, ld) +} + func (s *websocketSuite) openWithDialWebsocket( c *gc.C, info *api.Info, username string, dialWebsocket func(ctx context.Context, urlStr string, tlsConfig *tls.Config, ipAddr string) (jsoncodec.JSONConn, error), ) api.Connection { - conn, err := s.openNoAssert(c, info, username, dialWebsocket) + ld := loginDetails{info: info, username: username, dialWebsocket: dialWebsocket} + conn, err := s.openNoAssert(c, ld) c.Assert(err, gc.Equals, nil) return conn } @@ -173,20 +187,57 @@ func (s *proxySuite) TestConnectToModel(c *gc.C) { c.Assert(err, gc.ErrorMatches, `no such request - method Admin.TestMethod is not implemented \(not implemented\)`) } -// TODO(CSS-7331) Refactor model proxy for new login methods -// func (s *proxySuite) TestConnectToModelAndLogin(c *gc.C) { +func (s *proxySuite) TestDeviceFlowLogin(c *gc.C) { + ctx := context.Background() + alice := names.NewUserTag("alice@canonical.com") + aliceUser := openfga.NewUser(&dbmodel.Identity{Name: alice.Id()}, s.JIMM.OpenFGAClient) + err := aliceUser.SetControllerAccess(ctx, s.Model.Controller.ResourceTag(), ofganames.AdministratorRelation) + c.Assert(err, gc.IsNil) + var cliOutput string + outputFunc := func(format string, a ...any) error { + cliOutput = fmt.Sprintf(format, a...) + return nil + } + s.JIMMSuite.ContinueDeviceFlow(aliceUser.Name) + conn, err := s.openCustomLP(c, &api.Info{ + ModelTag: s.Model.ResourceTag(), + SkipLogin: false, + }, "alice", api.NewSessionTokenLoginProvider("", outputFunc, func(s string) error { return nil })) + c.Assert(err, gc.IsNil) + defer conn.Close() + c.Check(err, gc.Equals, nil) + c.Check(cliOutput, gc.Matches, "Please visit .* and enter code.*") +} + +// TODO(Kian): This test aims to verify that JIMM gracefully handles clients that end their connection +// during the login flow after JIMM starts polling the OIDC server. +// After https://github.com/juju/juju/pull/17606 lands We can refactor +// the API connection login method to use the loging provider on the state struct. +// func (s *proxySuite) TestDeviceFlowCancelDuringPolling(c *gc.C) { // ctx := context.Background() // alice := names.NewUserTag("alice@canonical.com") // aliceUser := openfga.NewUser(&dbmodel.Identity{Name: alice.Id()}, s.JIMM.OpenFGAClient) // err := aliceUser.SetControllerAccess(ctx, s.Model.Controller.ResourceTag(), ofganames.AdministratorRelation) // c.Assert(err, gc.IsNil) -// conn, err := s.openNoAssert(c, &api.Info{ -// ModelTag: s.Model.ResourceTag(), -// SkipLogin: false, -// }, "alice") -// if err == nil { -// defer conn.Close() +// var cliOutput string +// _ = cliOutput +// outputFunc := func(format string, a ...any) error { +// cliOutput = fmt.Sprintf(format, a) +// return nil // } +// var wg sync.WaitGroup +// var conn api.Connection +// wg.Add(1) +// go func() { +// defer wg.Done() +// conn, err = s.openCustomLP(c, &api.Info{ +// ModelTag: s.Model.ResourceTag(), +// SkipLogin: true, +// }, "alice", api.NewSessionTokenLoginProvider("", outputFunc, func(s string) error { return nil })) +// c.Assert(err, gc.IsNil) +// }() +// conn.Login() +// // Close the connection after the cliOutput is filled. // c.Assert(err, gc.Equals, nil) // } diff --git a/internal/rpc/proxy.go b/internal/rpc/proxy.go index b43996c74..ae6a081b3 100644 --- a/internal/rpc/proxy.go +++ b/internal/rpc/proxy.go @@ -337,19 +337,17 @@ func (p *clientProxy) start(ctx context.Context) error { p.sendError(p.src, msg, err) continue } + // If there is a response for the client, send it to the client and continue. + // If there is a message for the controller instead, use the normal path. + // We can't send the client a response from JIMM and send the client message to the controller. if toClient != nil { p.src.sendMessage(nil, toClient) + continue } else if toController != nil { msg = toController p.msgs.addLoginMessage(toController) } } - if msg.RequestID == 0 { - zapctx.Error(ctx, "Invalid request ID 0") - err := errors.E(op, "Invalid request ID 0") - p.sendError(p.src, msg, err) - continue - } p.msgs.addMessage(msg) zapctx.Debug(ctx, "Writing to controller") if err := p.dst.writeJson(msg); err != nil { @@ -585,7 +583,7 @@ func modifyControllerResponse(msg *message) error { // an error func (p *clientProxy) handleAdminFacade(ctx context.Context, msg *message) (clientResponse *message, controllerMessage *message, err error) { errorFnc := func(err error) (*message, *message, error) { - return nil, nil, err + return nil, nil, errors.E(err, errors.CodeUnauthorized) } controllerLoginMessageFnc := func(data []byte) (*message, *message, error) { m := *msg diff --git a/internal/rpc/proxy_test.go b/internal/rpc/proxy_test.go index e72639823..0a3810731 100644 --- a/internal/rpc/proxy_test.go +++ b/internal/rpc/proxy_test.go @@ -310,8 +310,8 @@ func (m *mockOAuthAuthenticator) Device(ctx context.Context) (*oauth2.DeviceAuth return nil, m.err } return &oauth2.DeviceAuthResponse{ - DeviceCode: "test device code", - UserCode: "test user code", + DeviceCode: "test-device-code", + UserCode: "test-user-code", VerificationURI: "http://no-such-uri.canonical.com", VerificationURIComplete: "http://no-such-uri.canonical.com", Expiry: time.Now().Add(time.Minute), From 67a12d5b3450e96ba386d53686216909ae250633 Mon Sep 17 00:00:00 2001 From: Kian Parvin Date: Fri, 12 Jul 2024 10:13:23 +0200 Subject: [PATCH 2/5] more integration tests tweaks --- internal/jimmtest/auth.go | 5 +-- internal/jimmtest/suite.go | 2 +- internal/jujuapi/websocket_test.go | 60 +++++++++++++++++++++++------- internal/rpc/proxy.go | 2 +- 4 files changed, 51 insertions(+), 18 deletions(-) diff --git a/internal/jimmtest/auth.go b/internal/jimmtest/auth.go index e0f98632f..49a79c063 100644 --- a/internal/jimmtest/auth.go +++ b/internal/jimmtest/auth.go @@ -125,8 +125,9 @@ func (m *MockOAuthAuthenticator) VerifySessionToken(token string) (jwt.Token, er return parsedToken, nil } -// ExtractAndVerifyIDToken return an ID token where the subject matches the subject obtained during the device flow. +// ExtractAndVerifyIDToken returns an ID token where the subject is equal to the username obtained during the device flow. // The auth token must match the one returned during the device flow. +// If the polled username is empty it indicates an error that the device flow was not run prior to calling this function. func (m *MockOAuthAuthenticator) ExtractAndVerifyIDToken(ctx context.Context, oauth2Token *oauth2.Token) (*oidc.IDToken, error) { if m.polledUsername == "" { return &oidc.IDToken{}, errors.New("unknown user for mock auth login") @@ -157,8 +158,6 @@ func (m *MockOAuthAuthenticator) AuthenticateBrowserSession(ctx context.Context, return ctx, errors.New("authentication failed") } -// Below are helper functions used for handling auth in tests. - func newUnsignedSessionToken(c SimpleTester, username string) string { email := convertUsernameToEmail(username) token, err := jwt.NewBuilder(). diff --git a/internal/jimmtest/suite.go b/internal/jimmtest/suite.go index abf6bca0c..b325f0f68 100644 --- a/internal/jimmtest/suite.go +++ b/internal/jimmtest/suite.go @@ -249,7 +249,7 @@ func (s *JIMMSuite) AddModel(c *gc.C, owner names.UserTag, name string, cloud na return names.NewModelTag(mi.UUID) } -// Call this non-blocking function before login to ensure device flow won't block. +// Call this non-blocking function before login to ensure the device flow won't block. // // This is necessary as the mock authenticator simulates polling an external OIDC server. func (s *JIMMSuite) ContinueDeviceFlow(username string) { diff --git a/internal/jujuapi/websocket_test.go b/internal/jujuapi/websocket_test.go index 627bcb210..95def8b69 100644 --- a/internal/jujuapi/websocket_test.go +++ b/internal/jujuapi/websocket_test.go @@ -13,6 +13,7 @@ import ( "net/url" "github.com/juju/juju/api" + "github.com/juju/juju/api/client/client" "github.com/juju/juju/rpc/jsoncodec" jujuparams "github.com/juju/juju/rpc/params" "github.com/juju/names/v5" @@ -99,13 +100,17 @@ func (s *websocketSuite) TearDownTest(c *gc.C) { s.BootstrapSuite.TearDownTest(c) } +type loginDetails struct { + info *api.Info + username string + lp api.LoginProvider + dialWebsocket func(ctx context.Context, urlStr string, tlsConfig *tls.Config, ipAddr string) (jsoncodec.JSONConn, error) +} + // openNoAssert creates a new websocket connection to the test server, using the // connection info specified in info, authenticating as the given user. // If info is nil then default values will be used. -func (s *websocketSuite) openNoAssert( - c *gc.C, - d loginDetails, -) (api.Connection, error) { +func (s *websocketSuite) openNoAssert(c *gc.C, d loginDetails) (api.Connection, error) { var inf api.Info if d.info != nil { inf = *d.info @@ -139,13 +144,6 @@ func (s *websocketSuite) openNoAssert( return api.Open(&inf, dialOpts) } -type loginDetails struct { - info *api.Info - username string - lp api.LoginProvider - dialWebsocket func(ctx context.Context, urlStr string, tlsConfig *tls.Config, ipAddr string) (jsoncodec.JSONConn, error) -} - func (s *websocketSuite) open(c *gc.C, info *api.Info, username string) api.Connection { ld := loginDetails{info: info, username: username} conn, err := s.openNoAssert(c, ld) @@ -209,10 +207,46 @@ func (s *proxySuite) TestDeviceFlowLogin(c *gc.C) { c.Check(cliOutput, gc.Matches, "Please visit .* and enter code.*") } +type logger struct{} + +func (l logger) Errorf(string, ...interface{}) {} + +func (s *proxySuite) TestModelStatus(c *gc.C) { + conn := s.open(c, &api.Info{ + ModelTag: s.Model.ResourceTag(), + SkipLogin: false, + }, "alice@canonical.com") + defer conn.Close() + jujuClient := client.NewClient(conn, logger{}) + status, err := jujuClient.Status(nil) + c.Check(err, gc.IsNil) + c.Check(status, gc.Not(gc.IsNil)) + c.Check(status.Model.Name, gc.Equals, s.Model.Name) +} + +func (s *proxySuite) TestModelStatusWithoutPermission(c *gc.C) { + fooUser := openfga.NewUser(&dbmodel.Identity{Name: "foo@canonical.com"}, s.JIMM.OpenFGAClient) + var cliOutput string + outputFunc := func(format string, a ...any) error { + cliOutput = fmt.Sprintf(format, a...) + return nil + } + s.JIMMSuite.ContinueDeviceFlow(fooUser.Name) + conn, err := s.openCustomLP(c, &api.Info{ + ModelTag: s.Model.ResourceTag(), + SkipLogin: false, + }, "foo", api.NewSessionTokenLoginProvider("", outputFunc, func(s string) error { return nil })) + c.Check(err, gc.ErrorMatches, "permission denied .*") + if conn != nil { + defer conn.Close() + } + c.Check(cliOutput, gc.Matches, "Please visit .* and enter code.*") +} + // TODO(Kian): This test aims to verify that JIMM gracefully handles clients that end their connection // during the login flow after JIMM starts polling the OIDC server. -// After https://github.com/juju/juju/pull/17606 lands We can refactor -// the API connection login method to use the loging provider on the state struct. +// After https://github.com/juju/juju/pull/17606 lands we can begin work on this. +// The API connection's login method should be refactored use the login provider stored on the state struct. // func (s *proxySuite) TestDeviceFlowCancelDuringPolling(c *gc.C) { // ctx := context.Background() // alice := names.NewUserTag("alice@canonical.com") diff --git a/internal/rpc/proxy.go b/internal/rpc/proxy.go index ae6a081b3..23e561c71 100644 --- a/internal/rpc/proxy.go +++ b/internal/rpc/proxy.go @@ -339,7 +339,7 @@ func (p *clientProxy) start(ctx context.Context) error { } // If there is a response for the client, send it to the client and continue. // If there is a message for the controller instead, use the normal path. - // We can't send the client a response from JIMM and send the client message to the controller. + // We can't send the client a response from JIMM and send a message to the controller. if toClient != nil { p.src.sendMessage(nil, toClient) continue From 0d48f629a9e894566b071596af045218e0a81558 Mon Sep 17 00:00:00 2001 From: Kian Parvin Date: Fri, 12 Jul 2024 14:11:06 +0200 Subject: [PATCH 3/5] fix cli tests --- internal/jimmtest/auth.go | 26 ++++++++++++++++++-------- internal/jimmtest/jimm.go | 2 +- internal/rpc/proxy_test.go | 2 +- 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/internal/jimmtest/auth.go b/internal/jimmtest/auth.go index 49a79c063..b2042e04d 100644 --- a/internal/jimmtest/auth.go +++ b/internal/jimmtest/auth.go @@ -25,6 +25,7 @@ import ( "github.com/gorilla/sessions" "github.com/juju/juju/api" jujuparams "github.com/juju/juju/rpc/params" + "github.com/lestrrat-go/jwx/v2/jwa" "github.com/lestrrat-go/jwx/v2/jwt" "golang.org/x/oauth2" @@ -36,9 +37,10 @@ import ( ) const ( - // Note that these values are deliberately different to make sure we're not - // reusing/misusing them. - JWTTestSecret = "test-secret" + // Secrets used for auth in tests. + // + // Note that these values are deliberately different to make sure we're not reusing/misusing them. + JWTTestSecret = "jwt-test-secret" SessionStoreSecret = "another-test-secret" ) @@ -150,7 +152,7 @@ func (m *MockOAuthAuthenticator) UpdateIdentity(ctx context.Context, email strin // MintSessionToken creates an unsigned session token with the email provided. func (m *MockOAuthAuthenticator) MintSessionToken(email string) (string, error) { - return newUnsignedSessionToken(m.c, email), nil + return newSessionToken(m.c, email, ""), nil } // AuthenticateBrowserSession always returns an error. @@ -158,7 +160,10 @@ func (m *MockOAuthAuthenticator) AuthenticateBrowserSession(ctx context.Context, return ctx, errors.New("authentication failed") } -func newUnsignedSessionToken(c SimpleTester, username string) string { +// newSessionToken returns a serialised JWT that can be used in tests. +// Tests using a mock authenticator can provide an empty signatureSecret +// while integration tests must provide the same secret used when verifying JWTs. +func newSessionToken(c SimpleTester, username string, signatureSecret string) string { email := convertUsernameToEmail(username) token, err := jwt.NewBuilder(). Subject(email). @@ -167,9 +172,14 @@ func newUnsignedSessionToken(c SimpleTester, username string) string { if err != nil { c.Fatalf("failed to generate test session token") } - serialisedToken, err := jwt.NewSerializer().Serialize(token) + var serialisedToken []byte + if signatureSecret != "" { + serialisedToken, err = jwt.Sign(token, jwt.WithKey(jwa.HS256, []byte(JWTTestSecret))) + } else { + serialisedToken, err = jwt.NewSerializer().Serialize(token) + } if err != nil { - c.Fatalf("failed to serialise token") + c.Fatalf("failed to sign/serialise token") } return base64.StdEncoding.EncodeToString(serialisedToken) } @@ -178,7 +188,7 @@ func newUnsignedSessionToken(c SimpleTester, username string) string { // to define how login will take place. In this case we login using a session token // that the JIMM server should verify with the same test secret. func NewUserSessionLogin(c SimpleTester, username string) api.LoginProvider { - b64Token := newUnsignedSessionToken(c, username) + b64Token := newSessionToken(c, username, JWTTestSecret) return api.NewSessionTokenLoginProvider(b64Token, nil, nil) } diff --git a/internal/jimmtest/jimm.go b/internal/jimmtest/jimm.go index 942d2f65d..eebcc3ace 100644 --- a/internal/jimmtest/jimm.go +++ b/internal/jimmtest/jimm.go @@ -22,7 +22,7 @@ func NewTestJimmParams(t Tester) jimm.Params { Scopes: []string{oidc.ScopeOpenID, "profile", "email"}, SessionTokenExpiry: time.Duration(time.Hour), SessionCookieMaxAge: 60, - JWTSessionKey: "test-secret", + JWTSessionKey: JWTTestSecret, }, DashboardFinalRedirectURL: "dashboard-url", } diff --git a/internal/rpc/proxy_test.go b/internal/rpc/proxy_test.go index 0a3810731..7426c800b 100644 --- a/internal/rpc/proxy_test.go +++ b/internal/rpc/proxy_test.go @@ -83,7 +83,7 @@ func TestProxySocketsAdminFacade(t *testing.T) { }, expectedClientResponse: &message{ RequestID: 1, - Response: []byte(`{"verification-uri":"http://no-such-uri.canonical.com","user-code":"test user code"}`), + Response: []byte(`{"verification-uri":"http://no-such-uri.canonical.com","user-code":"test-user-code"}`), }, }, { about: "login device call, but the authenticator returns an error", From 2f717207fe4f9c43d4f2816c2dd0ca4cd7e5cd02 Mon Sep 17 00:00:00 2001 From: Kian Parvin Date: Mon, 15 Jul 2024 18:36:28 +0200 Subject: [PATCH 4/5] Ensure mock authenticator returns unauthorized error --- internal/auth/oauth2.go | 13 ++++++++----- internal/auth/oauth2_test.go | 18 ++++++++++++++++-- internal/jimmtest/auth.go | 10 +++++++--- internal/jujuapi/websocket_test.go | 8 ++++---- internal/rpc/proxy.go | 2 +- 5 files changed, 36 insertions(+), 15 deletions(-) diff --git a/internal/auth/oauth2.go b/internal/auth/oauth2.go index 113ce347a..ca007c9a4 100644 --- a/internal/auth/oauth2.go +++ b/internal/auth/oauth2.go @@ -326,6 +326,9 @@ func (as *AuthenticationService) MintSessionToken(email string) (string, error) // for user object creation func (as *AuthenticationService) VerifySessionToken(token string) (_ jwt.Token, err error) { const op = errors.Op("auth.AuthenticationService.VerifySessionToken") + errorFn := func(message string) error { + return errors.E(op, message, errors.CodeUnauthorized) + } defer func() { if err != nil { servermon.AuthenticationFailCount.WithLabelValues("VerifySessionToken").Inc() @@ -335,24 +338,24 @@ func (as *AuthenticationService) VerifySessionToken(token string) (_ jwt.Token, }() if len(token) == 0 { - return nil, errors.E(op, "authentication failed, no token presented") + return nil, errorFn("authentication failed, no token presented") } decodedToken, err := base64.StdEncoding.DecodeString(token) if err != nil { - return nil, errors.E(op, fmt.Sprintf("authentication failed, failed to decode token: %s", err)) + return nil, errorFn(fmt.Sprintf("authentication failed, failed to decode token: %s", err)) } parsedToken, err := jwt.Parse(decodedToken, jwt.WithKey(as.signingAlg, []byte(as.jwtSessionKey))) if err != nil { if stderrors.Is(err, jwt.ErrTokenExpired()) { - return nil, errors.E(op, errors.CodeUnauthorized, "JIMM session token expired") + return nil, errorFn("JIMM session token expired") } - return nil, errors.E(op, err) + return nil, errorFn(err.Error()) } if _, err = mail.ParseAddress(parsedToken.Subject()); err != nil { - return nil, errors.E(op, "failed to parse email") + return nil, errorFn("failed to parse email") } return parsedToken, nil diff --git a/internal/auth/oauth2_test.go b/internal/auth/oauth2_test.go index da94e73e5..86f7861f6 100644 --- a/internal/auth/oauth2_test.go +++ b/internal/auth/oauth2_test.go @@ -19,6 +19,7 @@ import ( "github.com/canonical/jimm/internal/auth" "github.com/canonical/jimm/internal/db" "github.com/canonical/jimm/internal/dbmodel" + "github.com/canonical/jimm/internal/errors" "github.com/canonical/jimm/internal/jimmtest" "github.com/coreos/go-oidc/v3/oidc" qt "github.com/frankban/quicktest" @@ -56,8 +57,6 @@ func setupTestAuthSvc(ctx context.Context, c *qt.C, expiry time.Duration) (*auth // This test requires the local docker compose to be running and keycloak // to be available. -// -// TODO(ale8k): Use a mock for this and also device below, but future work??? func TestAuthCodeURL(t *testing.T) { c := qt.New(t) ctx := context.Background() @@ -205,6 +204,20 @@ func TestSessionTokenRejectsExpiredToken(t *testing.T) { _, err = authSvc.VerifySessionToken(token) c.Assert(err, qt.ErrorMatches, `JIMM session token expired`) + c.Assert(errors.ErrorCode(err), qt.Equals, errors.CodeUnauthorized) +} + +func TestSessionTokenRejectsEmptyToken(t *testing.T) { + c := qt.New(t) + + ctx := context.Background() + + noDuration := time.Duration(0) + authSvc, _, _ := setupTestAuthSvc(ctx, c, noDuration) + + _, err := authSvc.VerifySessionToken("") + c.Assert(err, qt.ErrorMatches, `authentication failed, no token presented`) + c.Assert(errors.ErrorCode(err), qt.Equals, errors.CodeUnauthorized) } func TestSessionTokenValidatesEmail(t *testing.T) { @@ -220,6 +233,7 @@ func TestSessionTokenValidatesEmail(t *testing.T) { _, err = authSvc.VerifySessionToken(token) c.Assert(err, qt.ErrorMatches, "failed to parse email") + c.Assert(errors.ErrorCode(err), qt.Equals, errors.CodeUnauthorized) } func TestVerifyClientCredentials(t *testing.T) { diff --git a/internal/jimmtest/auth.go b/internal/jimmtest/auth.go index b2042e04d..80019f28b 100644 --- a/internal/jimmtest/auth.go +++ b/internal/jimmtest/auth.go @@ -31,6 +31,7 @@ import ( "github.com/canonical/jimm/internal/auth" "github.com/canonical/jimm/internal/db" + jimmerrors "github.com/canonical/jimm/internal/errors" "github.com/canonical/jimm/internal/jimm" "github.com/canonical/jimm/internal/jimmhttp" "github.com/canonical/jimm/internal/openfga" @@ -111,18 +112,21 @@ func (m *MockOAuthAuthenticator) DeviceAccessToken(ctx context.Context, res *oau // Allowing JIMM tests to create their own session tokens that will always be accepted. // Notice the use of jwt.ParseInsecure to skip JWT signature verification. func (m *MockOAuthAuthenticator) VerifySessionToken(token string) (jwt.Token, error) { + errorFn := func(err error) error { + return jimmerrors.E(err, jimmerrors.CodeUnauthorized) + } decodedToken, err := base64.StdEncoding.DecodeString(token) if err != nil { - return nil, errors.New("authentication failed, failed to decode token") + return nil, errorFn(errors.New("authentication failed, failed to decode token")) } parsedToken, err := jwt.ParseInsecure(decodedToken) if err != nil { - return nil, err + return nil, errorFn(err) } if _, err = mail.ParseAddress(parsedToken.Subject()); err != nil { - return nil, errors.New("failed to parse email") + return nil, errorFn(errors.New("failed to parse email")) } return parsedToken, nil } diff --git a/internal/jujuapi/websocket_test.go b/internal/jujuapi/websocket_test.go index 95def8b69..306b36d0f 100644 --- a/internal/jujuapi/websocket_test.go +++ b/internal/jujuapi/websocket_test.go @@ -151,7 +151,7 @@ func (s *websocketSuite) open(c *gc.C, info *api.Info, username string) api.Conn return conn } -func (s *websocketSuite) openCustomLP(c *gc.C, info *api.Info, username string, lp api.LoginProvider) (api.Connection, error) { +func (s *websocketSuite) openCustomLoginProvider(c *gc.C, info *api.Info, username string, lp api.LoginProvider) (api.Connection, error) { ld := loginDetails{info: info, username: username, lp: lp} return s.openNoAssert(c, ld) } @@ -185,7 +185,7 @@ func (s *proxySuite) TestConnectToModel(c *gc.C) { c.Assert(err, gc.ErrorMatches, `no such request - method Admin.TestMethod is not implemented \(not implemented\)`) } -func (s *proxySuite) TestDeviceFlowLogin(c *gc.C) { +func (s *proxySuite) TestSessionTokenLoginProvider(c *gc.C) { ctx := context.Background() alice := names.NewUserTag("alice@canonical.com") aliceUser := openfga.NewUser(&dbmodel.Identity{Name: alice.Id()}, s.JIMM.OpenFGAClient) @@ -197,7 +197,7 @@ func (s *proxySuite) TestDeviceFlowLogin(c *gc.C) { return nil } s.JIMMSuite.ContinueDeviceFlow(aliceUser.Name) - conn, err := s.openCustomLP(c, &api.Info{ + conn, err := s.openCustomLoginProvider(c, &api.Info{ ModelTag: s.Model.ResourceTag(), SkipLogin: false, }, "alice", api.NewSessionTokenLoginProvider("", outputFunc, func(s string) error { return nil })) @@ -232,7 +232,7 @@ func (s *proxySuite) TestModelStatusWithoutPermission(c *gc.C) { return nil } s.JIMMSuite.ContinueDeviceFlow(fooUser.Name) - conn, err := s.openCustomLP(c, &api.Info{ + conn, err := s.openCustomLoginProvider(c, &api.Info{ ModelTag: s.Model.ResourceTag(), SkipLogin: false, }, "foo", api.NewSessionTokenLoginProvider("", outputFunc, func(s string) error { return nil })) diff --git a/internal/rpc/proxy.go b/internal/rpc/proxy.go index 23e561c71..5fde08f40 100644 --- a/internal/rpc/proxy.go +++ b/internal/rpc/proxy.go @@ -583,7 +583,7 @@ func modifyControllerResponse(msg *message) error { // an error func (p *clientProxy) handleAdminFacade(ctx context.Context, msg *message) (clientResponse *message, controllerMessage *message, err error) { errorFnc := func(err error) (*message, *message, error) { - return nil, nil, errors.E(err, errors.CodeUnauthorized) + return nil, nil, err } controllerLoginMessageFnc := func(data []byte) (*message, *message, error) { m := *msg From 3097cf3816563e29ea8077e7729e79c67d0cd31e Mon Sep 17 00:00:00 2001 From: Kian Parvin Date: Tue, 16 Jul 2024 12:57:30 +0200 Subject: [PATCH 5/5] PR comments --- internal/auth/oauth2.go | 4 ++-- internal/auth/oauth2_test.go | 2 +- internal/jimmtest/auth.go | 2 +- internal/jimmtest/suite.go | 3 ++- internal/jujuapi/admin_test.go | 4 ++-- internal/jujuapi/websocket_test.go | 4 ++-- 6 files changed, 10 insertions(+), 9 deletions(-) diff --git a/internal/auth/oauth2.go b/internal/auth/oauth2.go index ca007c9a4..eb9800fbd 100644 --- a/internal/auth/oauth2.go +++ b/internal/auth/oauth2.go @@ -338,12 +338,12 @@ func (as *AuthenticationService) VerifySessionToken(token string) (_ jwt.Token, }() if len(token) == 0 { - return nil, errorFn("authentication failed, no token presented") + return nil, errorFn("no token presented") } decodedToken, err := base64.StdEncoding.DecodeString(token) if err != nil { - return nil, errorFn(fmt.Sprintf("authentication failed, failed to decode token: %s", err)) + return nil, errorFn(fmt.Sprintf("failed to decode token: %s", err)) } parsedToken, err := jwt.Parse(decodedToken, jwt.WithKey(as.signingAlg, []byte(as.jwtSessionKey))) diff --git a/internal/auth/oauth2_test.go b/internal/auth/oauth2_test.go index 86f7861f6..bb559ca68 100644 --- a/internal/auth/oauth2_test.go +++ b/internal/auth/oauth2_test.go @@ -216,7 +216,7 @@ func TestSessionTokenRejectsEmptyToken(t *testing.T) { authSvc, _, _ := setupTestAuthSvc(ctx, c, noDuration) _, err := authSvc.VerifySessionToken("") - c.Assert(err, qt.ErrorMatches, `authentication failed, no token presented`) + c.Assert(err, qt.ErrorMatches, `no token presented`) c.Assert(errors.ErrorCode(err), qt.Equals, errors.CodeUnauthorized) } diff --git a/internal/jimmtest/auth.go b/internal/jimmtest/auth.go index 80019f28b..99143e5df 100644 --- a/internal/jimmtest/auth.go +++ b/internal/jimmtest/auth.go @@ -117,7 +117,7 @@ func (m *MockOAuthAuthenticator) VerifySessionToken(token string) (jwt.Token, er } decodedToken, err := base64.StdEncoding.DecodeString(token) if err != nil { - return nil, errorFn(errors.New("authentication failed, failed to decode token")) + return nil, errorFn(errors.New("failed to decode token")) } parsedToken, err := jwt.ParseInsecure(decodedToken) diff --git a/internal/jimmtest/suite.go b/internal/jimmtest/suite.go index b325f0f68..c2fb26a39 100644 --- a/internal/jimmtest/suite.go +++ b/internal/jimmtest/suite.go @@ -249,10 +249,11 @@ func (s *JIMMSuite) AddModel(c *gc.C, owner names.UserTag, name string, cloud na return names.NewModelTag(mi.UUID) } +// EnableDeviceFlow allows a test to use the device flow. // Call this non-blocking function before login to ensure the device flow won't block. // // This is necessary as the mock authenticator simulates polling an external OIDC server. -func (s *JIMMSuite) ContinueDeviceFlow(username string) { +func (s *JIMMSuite) EnableDeviceFlow(username string) { s.deviceFlowChan <- username } diff --git a/internal/jujuapi/admin_test.go b/internal/jujuapi/admin_test.go index cd3e69a88..4073cd8da 100644 --- a/internal/jujuapi/admin_test.go +++ b/internal/jujuapi/admin_test.go @@ -255,11 +255,11 @@ func (s *adminSuite) TestDeviceLogin(c *gc.C) { // Test no token present var loginResult jujuparams.LoginResult err = conn.APICall("Admin", 4, "", "LoginWithSessionToken", nil, &loginResult) - c.Assert(err, gc.ErrorMatches, "authentication failed, no token presented.*") + c.Assert(err, gc.ErrorMatches, "no token presented.*") // Test token not base64 encoded err = conn.APICall("Admin", 4, "", "LoginWithSessionToken", params.LoginWithSessionTokenRequest{SessionToken: string(decodedToken)}, &loginResult) - c.Assert(err, gc.ErrorMatches, "authentication failed, failed to decode token.*") + c.Assert(err, gc.ErrorMatches, "failed to decode token.*") // Test token base64 encoded passes authentication err = conn.APICall("Admin", 4, "", "LoginWithSessionToken", params.LoginWithSessionTokenRequest{SessionToken: sessionTokenResp.SessionToken}, &loginResult) diff --git a/internal/jujuapi/websocket_test.go b/internal/jujuapi/websocket_test.go index 306b36d0f..5da00d9df 100644 --- a/internal/jujuapi/websocket_test.go +++ b/internal/jujuapi/websocket_test.go @@ -196,7 +196,7 @@ func (s *proxySuite) TestSessionTokenLoginProvider(c *gc.C) { cliOutput = fmt.Sprintf(format, a...) return nil } - s.JIMMSuite.ContinueDeviceFlow(aliceUser.Name) + s.JIMMSuite.EnableDeviceFlow(aliceUser.Name) conn, err := s.openCustomLoginProvider(c, &api.Info{ ModelTag: s.Model.ResourceTag(), SkipLogin: false, @@ -231,7 +231,7 @@ func (s *proxySuite) TestModelStatusWithoutPermission(c *gc.C) { cliOutput = fmt.Sprintf(format, a...) return nil } - s.JIMMSuite.ContinueDeviceFlow(fooUser.Name) + s.JIMMSuite.EnableDeviceFlow(fooUser.Name) conn, err := s.openCustomLoginProvider(c, &api.Info{ ModelTag: s.Model.ResourceTag(), SkipLogin: false,