diff --git a/.vscode/settings.json b/.vscode/settings.json index 90fbfbc2b6..daee9de8e0 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -8,6 +8,7 @@ }, "go.lintTool": "golangci-lint", "go.lintFlags": ["--fast"], + "go.buildTags": "test", "eslint.workingDirectories": ["./web"], "prettier.ignorePath": "./web/.prettierignore", // Enable the ESlint flat config support diff --git a/cmd/server/server.go b/cmd/server/server.go index 76e73679b0..524b9f0af6 100644 --- a/cmd/server/server.go +++ b/cmd/server/server.go @@ -265,6 +265,13 @@ func run(c *cli.Context) error { } func setupEvilGlobals(c *cli.Context, s store.Store) error { + // secrets + var err error + server.Config.Server.JWTSecret, err = setupJWTSecret(s) + if err != nil { + return fmt.Errorf("could not setup jwt secret: %w", err) + } + // services server.Config.Services.Queue = setupQueue(c, s) server.Config.Services.Logs = logging.New() diff --git a/cmd/server/setup.go b/cmd/server/setup.go index b22e8e01ac..343b337d9f 100644 --- a/cmd/server/setup.go +++ b/cmd/server/setup.go @@ -17,10 +17,13 @@ package main import ( "context" + "encoding/base32" + "errors" "fmt" "os" "time" + "github.com/gorilla/securecookie" "github.com/prometheus/client_golang/prometheus" prometheus_auto "github.com/prometheus/client_golang/prometheus/promauto" "github.com/rs/zerolog/log" @@ -34,6 +37,7 @@ import ( "go.woodpecker-ci.org/woodpecker/v2/server/services/log/file" "go.woodpecker-ci.org/woodpecker/v2/server/store" "go.woodpecker-ci.org/woodpecker/v2/server/store/datastore" + "go.woodpecker-ci.org/woodpecker/v2/server/store/types" ) func setupStore(c *cli.Context) (store.Store, error) { @@ -165,3 +169,26 @@ func setupLogStore(c *cli.Context, s store.Store) (logService.Service, error) { return s, nil } } + +const jwtSecretID = "jwt-secret" + +func setupJWTSecret(_store store.Store) (string, error) { + jwtSecret, err := _store.ServerConfigGet(jwtSecretID) + if errors.Is(err, types.RecordNotExist) { + jwtSecret := base32.StdEncoding.EncodeToString( + securecookie.GenerateRandomKey(32), + ) + err = _store.ServerConfigSet(jwtSecretID, jwtSecret) + if err != nil { + return "", err + } + log.Debug().Msg("created jwt secret") + return jwtSecret, nil + } + + if err != nil { + return "", err + } + + return jwtSecret, nil +} diff --git a/server/api/hook.go b/server/api/hook.go index 0ed75a4f3f..0cbc100ead 100644 --- a/server/api/hook.go +++ b/server/api/hook.go @@ -104,7 +104,7 @@ func BlockTilQueueHasRunningItem(c *gin.Context) { func PostHook(c *gin.Context) { _store := store.FromContext(c) - _forge, err := server.Config.Services.Manager.ForgeMain() // TODO: get the forge for the specific repo somehow + _forge, err := server.Config.Services.Manager.ForgeByID(1) // TODO: get the forge for the specific repo somehow if err != nil { log.Error().Err(err).Msg("Cannot get main forge") c.AbortWithStatus(http.StatusInternalServerError) diff --git a/server/api/login.go b/server/api/login.go index 0cabbe887a..aa883b79d4 100644 --- a/server/api/login.go +++ b/server/api/login.go @@ -37,6 +37,8 @@ import ( "go.woodpecker-ci.org/woodpecker/v2/shared/token" ) +const stateTokenDuration = time.Minute * 5 + func HandleAuth(c *gin.Context) { // TODO: check if this is really needed c.Writer.Header().Del("Content-Type") @@ -56,17 +58,45 @@ func HandleAuth(c *gin.Context) { } _store := store.FromContext(c) + + code := c.Request.FormValue("code") + state := c.Request.FormValue("state") + isCallback := code != "" && state != "" forgeID := int64(1) // TODO: replace with forge id when multiple forges are supported - _forge, err := server.Config.Services.Manager.ForgeMain() + + if isCallback { // validate the state token + _, err := token.Parse([]token.Type{token.OAuthStateToken}, state, func(_ *token.Token) (string, error) { + return server.Config.Server.JWTSecret, nil + }) + if err != nil { + log.Error().Err(err).Msg("cannot verify state token") + c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=invalid_state") + return + } + } else { // only generate a state token if not a callback + var err error + jwtSecret := server.Config.Server.JWTSecret + exp := time.Now().Add(stateTokenDuration).Unix() + stateToken := token.New(token.OAuthStateToken) + stateToken.Set("forge-id", strconv.FormatInt(forgeID, 10)) + state, err = stateToken.SignExpires(jwtSecret, exp) + if err != nil { + log.Error().Err(err).Msg("cannot create state token") + c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error") + return + } + } + + _forge, err := server.Config.Services.Manager.ForgeByID(forgeID) if err != nil { - log.Error().Err(err).Msg("Cannot get main forge") + log.Error().Err(err).Msgf("Cannot get forge by id %d", forgeID) c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error") return } userFromForge, redirectURL, err := _forge.Login(c, &forge_types.OAuthRequest{ Code: c.Request.FormValue("code"), - State: "woodpecker", // TODO: use proper state + State: state, }) if err != nil { log.Error().Err(err).Msg("cannot authenticate user") @@ -250,7 +280,7 @@ func GetLogout(c *gin.Context) { func DeprecatedGetLoginToken(c *gin.Context) { _store := store.FromContext(c) - _forge, err := server.Config.Services.Manager.ForgeMain() // TODO: get selected forge from auth request + _forge, err := server.Config.Services.Manager.ForgeByID(1) // TODO: get selected forge from auth request if err != nil { log.Error().Err(err).Msg("Cannot get main forge") c.AbortWithStatus(http.StatusInternalServerError) diff --git a/server/api/login_test.go b/server/api/login_test.go index 5de2745098..f76dd721dd 100644 --- a/server/api/login_test.go +++ b/server/api/login_test.go @@ -1,6 +1,7 @@ package api_test import ( + "context" "fmt" "net/http" "net/http/httptest" @@ -16,11 +17,13 @@ import ( "go.woodpecker-ci.org/woodpecker/v2/server" "go.woodpecker-ci.org/woodpecker/v2/server/api" mocks_forge "go.woodpecker-ci.org/woodpecker/v2/server/forge/mocks" + forge_types "go.woodpecker-ci.org/woodpecker/v2/server/forge/types" "go.woodpecker-ci.org/woodpecker/v2/server/model" mocks_services "go.woodpecker-ci.org/woodpecker/v2/server/services/mocks" "go.woodpecker-ci.org/woodpecker/v2/server/services/permissions" mocks_store "go.woodpecker-ci.org/woodpecker/v2/server/store/mocks" "go.woodpecker-ci.org/woodpecker/v2/server/store/types" + "go.woodpecker-ci.org/woodpecker/v2/shared/token" ) func TestHandleAuth(t *testing.T) { @@ -69,12 +72,37 @@ func TestHandleAuth(t *testing.T) { assert.Equal(g, fmt.Sprintf("/login?%s", query.Encode()), c.Writer.Header().Get("Location")) }) - g.It("should fail if a code was provided and no state", func() { - // TODO: implement - }) - g.It("should fail if the state is wrong", func() { - // TODO: implement + _manager := mocks_services.NewManager(t) + _store := mocks_store.NewStore(t) + server.Config.Services.Manager = _manager + server.Config.Permissions.Open = true + server.Config.Permissions.Orgs = permissions.NewOrgs(nil) + server.Config.Permissions.Admins = permissions.NewAdmins(nil) + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Set("store", _store) + + query := url.Values{} + query.Set("code", "assumed_to_be_valid_code") + + wrongToken := token.New(token.OAuthStateToken) + wrongToken.Set("forge_id", "1") + signedWrongToken, _ := wrongToken.Sign("wrong_secret") + query.Set("state", signedWrongToken) + + c.Request = &http.Request{ + Header: make(http.Header), + URL: &url.URL{ + Scheme: "https", + RawQuery: query.Encode(), + }, + } + + api.HandleAuth(c) + + assert.Equal(g, http.StatusSeeOther, c.Writer.Status()) + assert.Equal(g, "/login?error=invalid_state", c.Writer.Header().Get("Location")) }) g.It("should redirect to forge login page", func() { @@ -95,10 +123,17 @@ func TestHandleAuth(t *testing.T) { }, } - forgeRedirectURL := "https://my-awesome-forge.com/oauth/authorize?client_id=client-id" + _manager.On("ForgeByID", int64(1)).Return(_forge, nil) - _manager.On("ForgeMain").Return(_forge, nil) - _forge.On("Login", mock.Anything, mock.Anything).Return(nil, forgeRedirectURL, nil) + forgeRedirectURL := "" + _forge.On("Login", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { + state, ok := args.Get(1).(*forge_types.OAuthRequest) + if ok { + forgeRedirectURL = fmt.Sprintf("https://my-awesome-forge.com/oauth/authorize?client_id=client-id&state=%s", state.State) + } + }).Return(nil, func(context.Context, *forge_types.OAuthRequest) string { + return forgeRedirectURL + }, nil) api.HandleAuth(c) @@ -124,7 +159,7 @@ func TestHandleAuth(t *testing.T) { }, } - _manager.On("ForgeMain").Return(_forge, nil) + _manager.On("ForgeByID", int64(1)).Return(_forge, nil) _forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil) _store.On("GetUserRemoteID", user.ForgeRemoteID, user.Login).Return(nil, types.RecordNotExist) _store.On("CreateUser", mock.Anything).Return(nil) @@ -158,7 +193,7 @@ func TestHandleAuth(t *testing.T) { }, } - _manager.On("ForgeMain").Return(_forge, nil) + _manager.On("ForgeByID", int64(1)).Return(_forge, nil) _forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil) _store.On("GetUserRemoteID", user.ForgeRemoteID, user.Login).Return(user, nil) _store.On("OrgGet", org.ID).Return(org, nil) @@ -190,7 +225,7 @@ func TestHandleAuth(t *testing.T) { }, } - _manager.On("ForgeMain").Return(_forge, nil) + _manager.On("ForgeByID", int64(1)).Return(_forge, nil) _forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil) _store.On("GetUserRemoteID", user.ForgeRemoteID, user.Login).Return(nil, types.RecordNotExist) @@ -218,7 +253,7 @@ func TestHandleAuth(t *testing.T) { }, } - _manager.On("ForgeMain").Return(_forge, nil) + _manager.On("ForgeByID", int64(1)).Return(_forge, nil) _forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil) _forge.On("Teams", mock.Anything, user).Return([]*model.Team{ { @@ -252,7 +287,7 @@ func TestHandleAuth(t *testing.T) { } user.OrgID = 0 - _manager.On("ForgeMain").Return(_forge, nil) + _manager.On("ForgeByID", int64(1)).Return(_forge, nil) _forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil) _store.On("GetUserRemoteID", user.ForgeRemoteID, user.Login).Return(user, nil) _store.On("OrgFindByName", user.Login).Return(nil, types.RecordNotExist) @@ -286,7 +321,7 @@ func TestHandleAuth(t *testing.T) { } user.OrgID = 0 - _manager.On("ForgeMain").Return(_forge, nil) + _manager.On("ForgeByID", int64(1)).Return(_forge, nil) _forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil) _store.On("GetUserRemoteID", user.ForgeRemoteID, user.Login).Return(user, nil) _store.On("OrgFindByName", user.Login).Return(org, nil) @@ -320,7 +355,7 @@ func TestHandleAuth(t *testing.T) { } org.Name = "not-the-user-name" - _manager.On("ForgeMain").Return(_forge, nil) + _manager.On("ForgeByID", int64(1)).Return(_forge, nil) _forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil) _store.On("GetUserRemoteID", user.ForgeRemoteID, user.Login).Return(user, nil) _store.On("OrgGet", user.OrgID).Return(org, nil) diff --git a/server/config.go b/server/config.go index 0ebcce5fc3..bb5ba721e5 100644 --- a/server/config.go +++ b/server/config.go @@ -38,6 +38,7 @@ var Config = struct { LogStore log.Service } Server struct { + JWTSecret string Key string Cert string OAuthHost string diff --git a/server/services/manager.go b/server/services/manager.go index 5526937f86..39ce07a488 100644 --- a/server/services/manager.go +++ b/server/services/manager.go @@ -46,7 +46,7 @@ type Manager interface { EnvironmentService() environment.Service ForgeFromRepo(repo *model.Repo) (forge.Forge, error) ForgeFromUser(user *model.User) (forge.Forge, error) - ForgeMain() (forge.Forge, error) + ForgeByID(id int64) (forge.Forge, error) } type manager struct { @@ -120,18 +120,14 @@ func (m *manager) EnvironmentService() environment.Service { } func (m *manager) ForgeFromRepo(repo *model.Repo) (forge.Forge, error) { - return m.getForgeByID(repo.ForgeID) + return m.ForgeByID(repo.ForgeID) } func (m *manager) ForgeFromUser(user *model.User) (forge.Forge, error) { - return m.getForgeByID(user.ForgeID) + return m.ForgeByID(user.ForgeID) } -func (m *manager) ForgeMain() (forge.Forge, error) { - return m.getForgeByID(1) // main forge is always 1 and is configured via environment variables -} - -func (m *manager) getForgeByID(id int64) (forge.Forge, error) { +func (m *manager) ForgeByID(id int64) (forge.Forge, error) { item := m.forgeCache.Get(id) if item != nil && !item.IsExpired() { return item.Value(), nil diff --git a/server/services/mocks/manager.go b/server/services/mocks/manager.go index b71d8ea4d5..0df118f818 100644 --- a/server/services/mocks/manager.go +++ b/server/services/mocks/manager.go @@ -68,29 +68,29 @@ func (_m *Manager) EnvironmentService() environment.Service { return r0 } -// ForgeFromRepo provides a mock function with given fields: repo -func (_m *Manager) ForgeFromRepo(repo *model.Repo) (forge.Forge, error) { - ret := _m.Called(repo) +// ForgeByID provides a mock function with given fields: id +func (_m *Manager) ForgeByID(id int64) (forge.Forge, error) { + ret := _m.Called(id) if len(ret) == 0 { - panic("no return value specified for ForgeFromRepo") + panic("no return value specified for ForgeByID") } var r0 forge.Forge var r1 error - if rf, ok := ret.Get(0).(func(*model.Repo) (forge.Forge, error)); ok { - return rf(repo) + if rf, ok := ret.Get(0).(func(int64) (forge.Forge, error)); ok { + return rf(id) } - if rf, ok := ret.Get(0).(func(*model.Repo) forge.Forge); ok { - r0 = rf(repo) + if rf, ok := ret.Get(0).(func(int64) forge.Forge); ok { + r0 = rf(id) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(forge.Forge) } } - if rf, ok := ret.Get(1).(func(*model.Repo) error); ok { - r1 = rf(repo) + if rf, ok := ret.Get(1).(func(int64) error); ok { + r1 = rf(id) } else { r1 = ret.Error(1) } @@ -98,29 +98,29 @@ func (_m *Manager) ForgeFromRepo(repo *model.Repo) (forge.Forge, error) { return r0, r1 } -// ForgeFromUser provides a mock function with given fields: user -func (_m *Manager) ForgeFromUser(user *model.User) (forge.Forge, error) { - ret := _m.Called(user) +// ForgeFromRepo provides a mock function with given fields: repo +func (_m *Manager) ForgeFromRepo(repo *model.Repo) (forge.Forge, error) { + ret := _m.Called(repo) if len(ret) == 0 { - panic("no return value specified for ForgeFromUser") + panic("no return value specified for ForgeFromRepo") } var r0 forge.Forge var r1 error - if rf, ok := ret.Get(0).(func(*model.User) (forge.Forge, error)); ok { - return rf(user) + if rf, ok := ret.Get(0).(func(*model.Repo) (forge.Forge, error)); ok { + return rf(repo) } - if rf, ok := ret.Get(0).(func(*model.User) forge.Forge); ok { - r0 = rf(user) + if rf, ok := ret.Get(0).(func(*model.Repo) forge.Forge); ok { + r0 = rf(repo) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(forge.Forge) } } - if rf, ok := ret.Get(1).(func(*model.User) error); ok { - r1 = rf(user) + if rf, ok := ret.Get(1).(func(*model.Repo) error); ok { + r1 = rf(repo) } else { r1 = ret.Error(1) } @@ -128,29 +128,29 @@ func (_m *Manager) ForgeFromUser(user *model.User) (forge.Forge, error) { return r0, r1 } -// ForgeMain provides a mock function with given fields: -func (_m *Manager) ForgeMain() (forge.Forge, error) { - ret := _m.Called() +// ForgeFromUser provides a mock function with given fields: user +func (_m *Manager) ForgeFromUser(user *model.User) (forge.Forge, error) { + ret := _m.Called(user) if len(ret) == 0 { - panic("no return value specified for ForgeMain") + panic("no return value specified for ForgeFromUser") } var r0 forge.Forge var r1 error - if rf, ok := ret.Get(0).(func() (forge.Forge, error)); ok { - return rf() + if rf, ok := ret.Get(0).(func(*model.User) (forge.Forge, error)); ok { + return rf(user) } - if rf, ok := ret.Get(0).(func() forge.Forge); ok { - r0 = rf() + if rf, ok := ret.Get(0).(func(*model.User) forge.Forge); ok { + r0 = rf(user) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(forge.Forge) } } - if rf, ok := ret.Get(1).(func() error); ok { - r1 = rf() + if rf, ok := ret.Get(1).(func(*model.User) error); ok { + r1 = rf(user) } else { r1 = ret.Error(1) } diff --git a/shared/token/token.go b/shared/token/token.go index f893babfed..1f3deaa127 100644 --- a/shared/token/token.go +++ b/shared/token/token.go @@ -27,11 +27,12 @@ type SecretFunc func(*Token) (string, error) type Type string const ( - UserToken Type = "user" // user token (exp cli) - SessToken Type = "sess" // session token (ui token requires csrf check) - HookToken Type = "hook" // repo hook token - CsrfToken Type = "csrf" - AgentToken Type = "agent" + UserToken Type = "user" // user token (exp cli) + SessToken Type = "sess" // session token (ui token requires csrf check) + HookToken Type = "hook" // repo hook token + CsrfToken Type = "csrf" + AgentToken Type = "agent" + OAuthStateToken Type = "oauth-state" ) // SignerAlgo id default algorithm used to sign JWT tokens. diff --git a/web/src/assets/locales/en.json b/web/src/assets/locales/en.json index 41ba7dd178..646b965bce 100644 --- a/web/src/assets/locales/en.json +++ b/web/src/assets/locales/en.json @@ -452,5 +452,6 @@ "oauth_error": "Error while authenticating against OAuth provider", "internal_error": "Some internal error occurred", "registration_closed": "The registration is closed", - "access_denied": "You are not allowed to access this instance" + "access_denied": "You are not allowed to access this instance", + "invalid_state": "The OAuth state is invalid" } diff --git a/web/src/views/Login.vue b/web/src/views/Login.vue index 65b25e769b..9f60c192bf 100644 --- a/web/src/views/Login.vue +++ b/web/src/views/Login.vue @@ -52,6 +52,7 @@ const authErrorMessages = { internal_error: i18n.t('internal_error'), registration_closed: i18n.t('registration_closed'), access_denied: i18n.t('access_denied'), + invalid_state: i18n.t('invalid_state'), }; const errorMessage = ref();