From fb6a4c3b874114883a49aacdd408d0c17dfc0a20 Mon Sep 17 00:00:00 2001 From: Christoph Kluge Date: Wed, 9 Apr 2025 16:00:27 +0200 Subject: [PATCH] review and move api endpoints secured check --- internal/api/rest.go | 95 +++++-------------------------------------- internal/auth/auth.go | 80 ++++++++++++++++++++++++++++++++++-- pkg/schema/config.go | 5 ++- 3 files changed, 91 insertions(+), 89 deletions(-) diff --git a/internal/api/rest.go b/internal/api/rest.go index 712a0b3..352dd94 100644 --- a/internal/api/rest.go +++ b/internal/api/rest.go @@ -69,11 +69,10 @@ func New() *RestApi { func (api *RestApi) MountApiRoutes(r *mux.Router) { r.StrictSlash(true) - + // REST API Uses TokenAuth r.HandleFunc("/jobs/start_job/", api.startJob).Methods(http.MethodPost, http.MethodPut) r.HandleFunc("/jobs/stop_job/", api.stopJobByRequest).Methods(http.MethodPost, http.MethodPut) // r.HandleFunc("/jobs/import/", api.importJob).Methods(http.MethodPost, http.MethodPut) - r.HandleFunc("/jobs/", api.getJobs).Methods(http.MethodGet) r.HandleFunc("/jobs/{id}", api.getJobById).Methods(http.MethodPost) r.HandleFunc("/jobs/{id}", api.getCompleteJobById).Methods(http.MethodGet) @@ -83,7 +82,6 @@ func (api *RestApi) MountApiRoutes(r *mux.Router) { r.HandleFunc("/jobs/delete_job/", api.deleteJobByRequest).Methods(http.MethodDelete) r.HandleFunc("/jobs/delete_job/{id}", api.deleteJobById).Methods(http.MethodDelete) r.HandleFunc("/jobs/delete_job_before/{ts}", api.deleteJobBefore).Methods(http.MethodDelete) - r.HandleFunc("/clusters/", api.getClusters).Methods(http.MethodGet) if api.MachineStateDir != "" { @@ -94,7 +92,7 @@ func (api *RestApi) MountApiRoutes(r *mux.Router) { func (api *RestApi) MountUserApiRoutes(r *mux.Router) { r.StrictSlash(true) - + // REST API Uses TokenAuth r.HandleFunc("/jobs/", api.getJobs).Methods(http.MethodGet) r.HandleFunc("/jobs/{id}", api.getJobById).Methods(http.MethodPost) r.HandleFunc("/jobs/{id}", api.getCompleteJobById).Methods(http.MethodGet) @@ -103,7 +101,7 @@ func (api *RestApi) MountUserApiRoutes(r *mux.Router) { func (api *RestApi) MountConfigApiRoutes(r *mux.Router) { r.StrictSlash(true) - + // Settings Frontend Uses SessionAuth if api.Authentication != nil { r.HandleFunc("/roles/", api.getRoles).Methods(http.MethodGet) r.HandleFunc("/users/", api.createUser).Methods(http.MethodPost, http.MethodPut) @@ -116,7 +114,7 @@ func (api *RestApi) MountConfigApiRoutes(r *mux.Router) { func (api *RestApi) MountFrontendApiRoutes(r *mux.Router) { r.StrictSlash(true) - + // Settings Frontrend Uses SessionAuth if api.Authentication != nil { r.HandleFunc("/jwt/", api.getJWT).Methods(http.MethodGet) r.HandleFunc("/configuration/", api.updateConfiguration).Methods(http.MethodPost) @@ -221,44 +219,6 @@ func decode(r io.Reader, val interface{}) error { return dec.Decode(val) } -func securedCheck(r *http.Request) error { - user := repository.GetUserFromContext(r.Context()) - if user == nil { - return fmt.Errorf("no user in context") - } - - if user.AuthType == schema.AuthToken { - // If nothing declared in config: deny all request to this endpoint - if config.Keys.ApiAllowedIPs == nil || len(config.Keys.ApiAllowedIPs) == 0 { - return fmt.Errorf("missing configuration key ApiAllowedIPs") - } - - if config.Keys.ApiAllowedIPs[0] == "*" { - return nil - } - - // extract IP address - IPAddress := r.Header.Get("X-Real-Ip") - if IPAddress == "" { - IPAddress = r.Header.Get("X-Forwarded-For") - } - if IPAddress == "" { - IPAddress = r.RemoteAddr - } - - if strings.Contains(IPAddress, ":") { - IPAddress = strings.Split(IPAddress, ":")[0] - } - - // check if IP is allowed - if !util.Contains(config.Keys.ApiAllowedIPs, IPAddress) { - return fmt.Errorf("unknown ip: %v", IPAddress) - } - } - - return nil -} - // getClusters godoc // @summary Lists all cluster configs // @tags Cluster query @@ -1093,7 +1053,6 @@ func (api *RestApi) getJobMetrics(rw http.ResponseWriter, r *http.Request) { // @summary Adds a new user // @tags User // @description User specified in form data will be saved to database. -// @description Only accessible from IPs registered with apiAllowedIPs configuration option. // @accept mpfd // @produce plain // @param username formData string true "Unique user ID" @@ -1111,11 +1070,7 @@ func (api *RestApi) getJobMetrics(rw http.ResponseWriter, r *http.Request) { // @security ApiKeyAuth // @router /users/ [post] func (api *RestApi) createUser(rw http.ResponseWriter, r *http.Request) { - err := securedCheck(r) - if err != nil { - http.Error(rw, err.Error(), http.StatusForbidden) - return - } + // SecuredCheck() only worked with TokenAuth: Removed rw.Header().Set("Content-Type", "text/plain") me := repository.GetUserFromContext(r.Context()) @@ -1162,7 +1117,6 @@ func (api *RestApi) createUser(rw http.ResponseWriter, r *http.Request) { // @summary Deletes a user // @tags User // @description User defined by username in form data will be deleted from database. -// @description Only accessible from IPs registered with apiAllowedIPs configuration option. // @accept mpfd // @produce plain // @param username formData string true "User ID to delete" @@ -1175,11 +1129,7 @@ func (api *RestApi) createUser(rw http.ResponseWriter, r *http.Request) { // @security ApiKeyAuth // @router /users/ [delete] func (api *RestApi) deleteUser(rw http.ResponseWriter, r *http.Request) { - err := securedCheck(r) - if err != nil { - http.Error(rw, err.Error(), http.StatusForbidden) - return - } + // SecuredCheck() only worked with TokenAuth: Removed if user := repository.GetUserFromContext(r.Context()); !user.HasRole(schema.RoleAdmin) { http.Error(rw, "Only admins are allowed to delete a user", http.StatusForbidden) @@ -1200,7 +1150,6 @@ func (api *RestApi) deleteUser(rw http.ResponseWriter, r *http.Request) { // @tags User // @description Returns a JSON-encoded list of users. // @description Required query-parameter defines if all users or only users with additional special roles are returned. -// @description Only accessible from IPs registered with apiAllowedIPs configuration option. // @produce json // @param not-just-user query bool true "If returned list should contain all users or only users with additional special roles" // @success 200 {array} api.ApiReturnedUser "List of users returned successfully" @@ -1211,11 +1160,7 @@ func (api *RestApi) deleteUser(rw http.ResponseWriter, r *http.Request) { // @security ApiKeyAuth // @router /users/ [get] func (api *RestApi) getUsers(rw http.ResponseWriter, r *http.Request) { - err := securedCheck(r) - if err != nil { - http.Error(rw, err.Error(), http.StatusForbidden) - return - } + // SecuredCheck() only worked with TokenAuth: Removed if user := repository.GetUserFromContext(r.Context()); !user.HasRole(schema.RoleAdmin) { http.Error(rw, "Only admins are allowed to fetch a list of users", http.StatusForbidden) @@ -1236,7 +1181,6 @@ func (api *RestApi) getUsers(rw http.ResponseWriter, r *http.Request) { // @tags User // @description Modifies user defined by username (id) in one of four possible ways. // @description If more than one formValue is set then only the highest priority field is used. -// @description Only accessible from IPs registered with apiAllowedIPs configuration option. // @accept mpfd // @produce plain // @param id path string true "Database ID of User" @@ -1253,11 +1197,7 @@ func (api *RestApi) getUsers(rw http.ResponseWriter, r *http.Request) { // @security ApiKeyAuth // @router /user/{id} [post] func (api *RestApi) updateUser(rw http.ResponseWriter, r *http.Request) { - err := securedCheck(r) - if err != nil { - http.Error(rw, err.Error(), http.StatusForbidden) - return - } + // SecuredCheck() only worked with TokenAuth: Removed if user := repository.GetUserFromContext(r.Context()); !user.HasRole(schema.RoleAdmin) { http.Error(rw, "Only admins are allowed to update a user", http.StatusForbidden) @@ -1305,7 +1245,6 @@ func (api *RestApi) updateUser(rw http.ResponseWriter, r *http.Request) { // @tags User // @description Modifies the content of notice.txt, shown as notice box on the homepage. // @description If more than one formValue is set then only the highest priority field is used. -// @description Only accessible from IPs registered with apiAllowedIPs configuration option. // @accept mpfd // @produce plain // @param new-content formData string false "Priority 1: New content to display" @@ -1318,11 +1257,7 @@ func (api *RestApi) updateUser(rw http.ResponseWriter, r *http.Request) { // @security ApiKeyAuth // @router /notice/ [post] func (api *RestApi) editNotice(rw http.ResponseWriter, r *http.Request) { - err := securedCheck(r) - if err != nil { - http.Error(rw, err.Error(), http.StatusForbidden) - return - } + // SecuredCheck() only worked with TokenAuth: Removed if user := repository.GetUserFromContext(r.Context()); !user.HasRole(schema.RoleAdmin) { http.Error(rw, "Only admins are allowed to update the notice.txt file", http.StatusForbidden) @@ -1364,12 +1299,6 @@ func (api *RestApi) editNotice(rw http.ResponseWriter, r *http.Request) { } func (api *RestApi) getJWT(rw http.ResponseWriter, r *http.Request) { - err := securedCheck(r) - if err != nil { - http.Error(rw, err.Error(), http.StatusForbidden) - return - } - rw.Header().Set("Content-Type", "text/plain") username := r.FormValue("username") me := repository.GetUserFromContext(r.Context()) @@ -1398,11 +1327,7 @@ func (api *RestApi) getJWT(rw http.ResponseWriter, r *http.Request) { } func (api *RestApi) getRoles(rw http.ResponseWriter, r *http.Request) { - err := securedCheck(r) - if err != nil { - http.Error(rw, err.Error(), http.StatusForbidden) - return - } + // SecuredCheck() only worked with TokenAuth: Removed user := repository.GetUserFromContext(r.Context()) if !user.HasRole(schema.RoleAdmin) { diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 262204c..d5e48ac 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -10,9 +10,11 @@ import ( "database/sql" "encoding/base64" "errors" + "fmt" "net" "net/http" "os" + "strings" "sync" "time" @@ -20,6 +22,7 @@ import ( "github.com/ClusterCockpit/cc-backend/internal/config" "github.com/ClusterCockpit/cc-backend/internal/repository" + "github.com/ClusterCockpit/cc-backend/internal/util" "github.com/ClusterCockpit/cc-backend/pkg/log" "github.com/ClusterCockpit/cc-backend/pkg/schema" "github.com/gorilla/sessions" @@ -233,9 +236,9 @@ func (auth *Authentication) Login( limiter := getIPUserLimiter(ip, username) if !limiter.Allow() { - log.Warnf("AUTH/RATE > Too many login attempts for combination IP: %s, Username: %s", ip, username) - onfailure(rw, r, errors.New("Too many login attempts, try again in a few minutes.")) - return + log.Warnf("AUTH/RATE > Too many login attempts for combination IP: %s, Username: %s", ip, username) + onfailure(rw, r, errors.New("Too many login attempts, try again in a few minutes.")) + return } var dbUser *schema.User @@ -325,6 +328,14 @@ func (auth *Authentication) AuthApi( onfailure(rw, r, err) return } + + ipErr := securedCheck(user, "api", r) + if ipErr != nil { + log.Infof("auth api -> secured check failed: %s", err.Error()) + onfailure(rw, r, ipErr) + return + } + if user != nil { switch { case len(user.Roles) == 1: @@ -360,6 +371,14 @@ func (auth *Authentication) AuthUserApi( onfailure(rw, r, err) return } + + ipErr := securedCheck(user, "userapi", r) + if ipErr != nil { + log.Infof("auth user api -> secured check failed: %s", err.Error()) + onfailure(rw, r, ipErr) + return + } + if user != nil { switch { case len(user.Roles) == 1: @@ -445,3 +464,58 @@ func (auth *Authentication) Logout(onsuccess http.Handler) http.Handler { onsuccess.ServeHTTP(rw, r) }) } + +// Helper Moved To MiddleWare Auth Handlers +func securedCheck(user *schema.User, checkEndpoint string, r *http.Request) error { + if user == nil { + return fmt.Errorf("no user for secured check") + } + + // extract IP address for checking + IPAddress := r.Header.Get("X-Real-Ip") + if IPAddress == "" { + IPAddress = r.Header.Get("X-Forwarded-For") + } + if IPAddress == "" { + IPAddress = r.RemoteAddr + } + + if strings.Contains(IPAddress, ":") { + IPAddress = strings.Split(IPAddress, ":")[0] + } + + // Used for checking TokenAuth'd Requests Only: Remove '== schema.AuthToken'-Condition + if checkEndpoint == "api" { + // If nothing declared in config: deny all request to this api endpoint + if config.Keys.ApiAllowedIPs == nil || len(config.Keys.ApiAllowedIPs) == 0 { + return fmt.Errorf("missing configuration key ApiAllowedIPs") + } + // If wildcard declared in config: Continue + if config.Keys.ApiAllowedIPs[0] == "*" { + return nil + } + // check if IP is allowed + if !util.Contains(config.Keys.ApiAllowedIPs, IPAddress) { + return fmt.Errorf("unknown ip: %v", IPAddress) + } + + } else if checkEndpoint == "userapi" { + // If nothing declared in config: deny all request to this api endpoint + if config.Keys.UserApiAllowedIPs == nil || len(config.Keys.UserApiAllowedIPs) == 0 { + return fmt.Errorf("missing configuration key UserApiAllowedIPs") + } + // If wildcard declared in config: Continue + if config.Keys.UserApiAllowedIPs[0] == "*" { + return nil + } + // check if IP is allowed + if !util.Contains(config.Keys.UserApiAllowedIPs, IPAddress) { + return fmt.Errorf("unknown user ip: %v", IPAddress) + } + + } else { + return fmt.Errorf("unknown checkEndpoint for secured check") + } + + return nil +} diff --git a/pkg/schema/config.go b/pkg/schema/config.go index f9116cf..16b4219 100644 --- a/pkg/schema/config.go +++ b/pkg/schema/config.go @@ -100,9 +100,12 @@ type ProgramConfig struct { // Address where the http (or https) server will listen on (for example: 'localhost:80'). Addr string `json:"addr"` - // Addresses from which secured API endpoints can be reached + // Addresses from which secured admin API endpoints can be reached, can be wildcard "*" ApiAllowedIPs []string `json:"apiAllowedIPs"` + // Addresses from which secured admin API endpoints can be reached, can be wildcard "*" + UserApiAllowedIPs []string `json:"userApiAllowedIPs"` + // Drop root permissions once .env was read and the port was taken. User string `json:"user"` Group string `json:"group"`