From c3d0733e27fe5a642602f0ebc22380c235e84541 Mon Sep 17 00:00:00 2001 From: Jan Dittberner Date: Thu, 26 May 2022 17:25:25 +0200 Subject: [PATCH] Replace gorilla/csrf with justinas/nosurf - replace dependency for less indirect dependencies - remove unused configuration options --- README.md | 2 - cmd/boardvoting/config.go | 23 ------- cmd/boardvoting/handlers.go | 106 ------------------------------ cmd/boardvoting/helpers.go | 109 +++++++++++++++++++++++++++++++ cmd/boardvoting/main.go | 2 - cmd/boardvoting/middleware.go | 14 ++++ cmd/boardvoting/routes.go | 9 +-- config.yaml.example | 2 - go.mod | 15 ++--- go.sum | 7 +- ui/html/pages/create_motion.html | 2 +- 11 files changed, 137 insertions(+), 154 deletions(-) diff --git a/README.md b/README.md index befcdc1..6d27faa 100644 --- a/README.md +++ b/README.md @@ -91,8 +91,6 @@ You can use the following table to find useful values for the parameters in `con | `client_ca_certificates` | File containing allowed client certificate CA certificates (production value is `cacert_class3.pem`) | use the shell code above | | `server_certificate` | X.509 certificate that is used to identify your server (i.e. `server.crt`) | use the filename used as `-out` parameter in the `openssl` invocation above | | `server_key` | PEM encoded private key file (i.e. `server.key`) | use the filename used as `-keyout` parameter in the `openssl` invocation above | -| `cookie_secret` | A base64 encoded random byte value of at least 32 bytes used to encrypt cookies | see [Generating random byte values](#generating-random-byte-values) below | -| `csrf_key` | A base64 encoded random byte value of at least 32 bytes used to encrypt [CSRF](https://en.wikipedia.org/wiki/Cross-site_request_forgery#Prevention) tokens | see [Generating random byte values](#generating-random-byte-values) below | | `mail_config.smtp_host` | Mail server host (production value is `localhost`) | `localhost` | | `mail_config.smtp_port` | Mail server TCP port (production value is `25` | see [how to setup a debugging SMTP server](#debugging-smtp-server) below and choose the port of that (default `8025`) | | `mail_config.base_url` | The base URL of your application instance (production value is https://motions.cacert.org) | use https://localhost:8443 | diff --git a/cmd/boardvoting/config.go b/cmd/boardvoting/config.go index b1d8895..f6ffdba 100644 --- a/cmd/boardvoting/config.go +++ b/cmd/boardvoting/config.go @@ -18,7 +18,6 @@ limitations under the License. package main import ( - "encoding/base64" "fmt" "io/ioutil" "time" @@ -27,8 +26,6 @@ import ( ) const ( - cookieSecretMinLen = 32 - csrfKeyLength = 32 httpIdleTimeout = time.Minute httpReadHeaderTimeout = 5 * time.Second httpReadTimeout = 5 * time.Second @@ -62,8 +59,6 @@ type Config struct { HTTPSAddress string `yaml:"https_address,omitempty"` MailConfig *mailConfig `yaml:"mail_config"` Timeouts *httpTimeoutConfig `yaml:"timeouts,omitempty"` - CookieSecret []byte `yaml:"-"` - CsrfKey []byte `yaml:"-"` } func parseConfig(configFile string) (*Config, error) { @@ -87,23 +82,5 @@ func parseConfig(configFile string) (*Config, error) { return nil, fmt.Errorf("could not parse configuration: %w", err) } - if config.CookieSecret, err = base64.StdEncoding.DecodeString(config.CookieSecretStr); err != nil { - return nil, fmt.Errorf("could not decode cookie secret: %w", err) - } - - if len(config.CookieSecret) < cookieSecretMinLen { - return nil, fmt.Errorf("cookie secret is less than the minimum require %d bytes long", cookieSecretMinLen) - } - - if config.CsrfKey, err = base64.StdEncoding.DecodeString(config.CsrfKeyStr); err != nil { - return nil, fmt.Errorf("could not decode CSRF key: %w", err) - } - - if len(config.CsrfKey) != csrfKeyLength { - return nil, fmt.Errorf( - "CSRF key must be exactly %d bytes long but is %d bytes long", csrfKeyLength, len(config.CsrfKey), - ) - } - return config, nil } diff --git a/cmd/boardvoting/handlers.go b/cmd/boardvoting/handlers.go index 5ba1e40..09e339f 100644 --- a/cmd/boardvoting/handlers.go +++ b/cmd/boardvoting/handlers.go @@ -18,24 +18,16 @@ limitations under the License. package main import ( - "bytes" "database/sql" "errors" "fmt" - "html/template" - "io/fs" "net/http" - "path/filepath" - "strings" "time" - "github.com/Masterminds/sprig/v3" - "github.com/gorilla/csrf" "github.com/julienschmidt/httprouter" "git.cacert.org/cacert-boardvoting/internal/models" "git.cacert.org/cacert-boardvoting/internal/validator" - "git.cacert.org/cacert-boardvoting/ui" ) func checkRole(v *models.User, roles []string) (bool, error) { @@ -47,49 +39,6 @@ func checkRole(v *models.User, roles []string) (bool, error) { return hasRole, nil } -func newTemplateCache() (map[string]*template.Template, error) { - cache := map[string]*template.Template{} - - pages, err := fs.Glob(ui.Files, "html/pages/*.html") - if err != nil { - return nil, fmt.Errorf("could not find page templates: %w", err) - } - - funcMaps := sprig.FuncMap() - funcMaps["nl2br"] = func(text string) template.HTML { - // #nosec G203 input is sanitized - return template.HTML(strings.ReplaceAll(template.HTMLEscapeString(text), "\n", "
")) - } - funcMaps["canManageUsers"] = func(v *models.User) (bool, error) { - return checkRole(v, []string{models.RoleSecretary, models.RoleAdmin}) - } - funcMaps["canVote"] = func(v *models.User) (bool, error) { - return checkRole(v, []string{models.RoleVoter}) - } - funcMaps["canStartVote"] = func(v *models.User) (bool, error) { - return checkRole(v, []string{models.RoleVoter}) - } - funcMaps[csrf.TemplateTag] = csrf.TemplateField - - for _, page := range pages { - name := filepath.Base(page) - - ts, err := template.New("").Funcs(funcMaps).ParseFS( - ui.Files, - "html/base.html", - "html/partials/*.html", - page, - ) - if err != nil { - return nil, fmt.Errorf("could not parse base template: %w", err) - } - - cache[name] = ts - } - - return cache, nil -} - type topLevelNavItem string type subLevelNavItem string @@ -102,61 +51,6 @@ const ( subLevelNavUsers subLevelNavItem = "users" ) -type templateData struct { - PrevPage string - NextPage string - Motion *models.MotionForDisplay - Motions []*models.MotionForDisplay - User *models.User - Users []*models.User - Request *http.Request - Flash string - Form any - ActiveNav topLevelNavItem - ActiveSubNav subLevelNavItem -} - -func (app *application) newTemplateData( - r *http.Request, - nav topLevelNavItem, - subNav subLevelNavItem, -) *templateData { - user, err := app.GetUser(r) - if err != nil { - app.errorLog.Printf("error getting user for template data: %v", err) - } - - return &templateData{ - Request: r, - User: user, - ActiveNav: nav, - ActiveSubNav: subNav, - Flash: app.sessionManager.PopString(r.Context(), "flash"), - } -} - -func (app *application) render(w http.ResponseWriter, status int, page string, data *templateData) { - ts, ok := app.templateCache[page] - if !ok { - app.serverError(w, fmt.Errorf("the template %s does not exist", page)) - - return - } - - buf := new(bytes.Buffer) - - err := ts.ExecuteTemplate(buf, "base", data) - if err != nil { - app.serverError(w, err) - - return - } - - w.WriteHeader(status) - - _, _ = buf.WriteTo(w) -} - func (m *templateData) motionPaginationOptions(limit int, first, last *time.Time) error { motions := m.Motions diff --git a/cmd/boardvoting/helpers.go b/cmd/boardvoting/helpers.go index ce919c3..23d7a24 100644 --- a/cmd/boardvoting/helpers.go +++ b/cmd/boardvoting/helpers.go @@ -18,12 +18,22 @@ limitations under the License. package main import ( + "bytes" "errors" "fmt" + "html/template" + "io/fs" "net/http" + "path/filepath" "runtime/debug" + "strings" + "github.com/Masterminds/sprig/v3" "github.com/go-playground/form/v4" + "github.com/justinas/nosurf" + + "git.cacert.org/cacert-boardvoting/internal/models" + "git.cacert.org/cacert-boardvoting/ui" ) func (app *application) serverError(w http.ResponseWriter, err error) { @@ -61,3 +71,102 @@ func (app *application) decodePostForm(r *http.Request, dst any) error { return nil } + +func newTemplateCache() (map[string]*template.Template, error) { + cache := map[string]*template.Template{} + + pages, err := fs.Glob(ui.Files, "html/pages/*.html") + if err != nil { + return nil, fmt.Errorf("could not find page templates: %w", err) + } + + funcMaps := sprig.FuncMap() + funcMaps["nl2br"] = func(text string) template.HTML { + // #nosec G203 input is sanitized + return template.HTML(strings.ReplaceAll(template.HTMLEscapeString(text), "\n", "
")) + } + funcMaps["canManageUsers"] = func(v *models.User) (bool, error) { + return checkRole(v, []string{models.RoleSecretary, models.RoleAdmin}) + } + funcMaps["canVote"] = func(v *models.User) (bool, error) { + return checkRole(v, []string{models.RoleVoter}) + } + funcMaps["canStartVote"] = func(v *models.User) (bool, error) { + return checkRole(v, []string{models.RoleVoter}) + } + + for _, page := range pages { + name := filepath.Base(page) + + ts, err := template.New("").Funcs(funcMaps).ParseFS( + ui.Files, + "html/base.html", + "html/partials/*.html", + page, + ) + if err != nil { + return nil, fmt.Errorf("could not parse base template: %w", err) + } + + cache[name] = ts + } + + return cache, nil +} + +type templateData struct { + PrevPage string + NextPage string + Motion *models.MotionForDisplay + Motions []*models.MotionForDisplay + User *models.User + Users []*models.User + Request *http.Request + Flash string + Form any + ActiveNav topLevelNavItem + ActiveSubNav subLevelNavItem + CSRFToken string +} + +func (app *application) newTemplateData( + r *http.Request, + nav topLevelNavItem, + subNav subLevelNavItem, +) *templateData { + user, err := app.GetUser(r) + if err != nil { + app.errorLog.Printf("error getting user for template data: %v", err) + } + + return &templateData{ + Request: r, + User: user, + ActiveNav: nav, + ActiveSubNav: subNav, + Flash: app.sessionManager.PopString(r.Context(), "flash"), + CSRFToken: nosurf.Token(r), + } +} + +func (app *application) render(w http.ResponseWriter, status int, page string, data *templateData) { + ts, ok := app.templateCache[page] + if !ok { + app.serverError(w, fmt.Errorf("the template %s does not exist", page)) + + return + } + + buf := new(bytes.Buffer) + + err := ts.ExecuteTemplate(buf, "base", data) + if err != nil { + app.serverError(w, err) + + return + } + + w.WriteHeader(status) + + _, _ = buf.WriteTo(w) +} diff --git a/cmd/boardvoting/main.go b/cmd/boardvoting/main.go index 1cd4bc6..1d28b47 100644 --- a/cmd/boardvoting/main.go +++ b/cmd/boardvoting/main.go @@ -61,7 +61,6 @@ type application struct { templateCache map[string]*template.Template sessionManager *scs.SessionManager formDecoder *form.Decoder - csrfKey []byte } func main() { @@ -110,7 +109,6 @@ func main() { mailConfig: config.MailConfig, templateCache: templateCache, sessionManager: sessionManager, - csrfKey: config.CsrfKey, formDecoder: setupFormDecoder(), } diff --git a/cmd/boardvoting/middleware.go b/cmd/boardvoting/middleware.go index 5326f05..9224ec2 100644 --- a/cmd/boardvoting/middleware.go +++ b/cmd/boardvoting/middleware.go @@ -24,6 +24,8 @@ import ( "net/http" "strings" + "github.com/justinas/nosurf" + "git.cacert.org/cacert-boardvoting/internal/models" ) @@ -172,3 +174,15 @@ func (app *application) userCanEditVote(next http.Handler) http.Handler { func (app *application) userCanChangeVoters(next http.Handler) http.Handler { return app.requireRole(next, []string{models.RoleSecretary, models.RoleAdmin}) } + +func noSurf(next http.Handler) http.Handler { + csrfHandler := nosurf.New(next) + csrfHandler.SetBaseCookie(http.Cookie{ + HttpOnly: true, + Path: "/", + Secure: true, + SameSite: http.SameSiteStrictMode, + }) + + return csrfHandler +} diff --git a/cmd/boardvoting/routes.go b/cmd/boardvoting/routes.go index aeed07a..42bc2d7 100644 --- a/cmd/boardvoting/routes.go +++ b/cmd/boardvoting/routes.go @@ -22,7 +22,6 @@ import ( "io/fs" "net/http" - "github.com/gorilla/csrf" "github.com/julienschmidt/httprouter" "github.com/justinas/alice" "github.com/vearutop/statigz" @@ -61,16 +60,14 @@ func (app *application) routes() http.Handler { ) router.Handler(http.MethodGet, "/static/*filepath", http.StripPrefix("/static", fileServer)) - csrfHandler := csrf.Protect(app.csrfKey, csrf.SameSite(csrf.SameSiteStrictMode)) - dynamic := alice.New( app.sessionManager.LoadAndSave, app.tryAuthenticate, ) - canVote := dynamic.Append(app.userCanVote, csrfHandler) - canEditVote := dynamic.Append(app.userCanEditVote, csrfHandler) - canManageUsers := dynamic.Append(app.userCanChangeVoters, csrfHandler) + canVote := dynamic.Append(app.userCanVote, noSurf) + canEditVote := dynamic.Append(app.userCanEditVote, noSurf) + canManageUsers := dynamic.Append(app.userCanChangeVoters, noSurf) router.Handler(http.MethodGet, "/motions/", dynamic.ThenFunc(app.motionList)) router.Handler(http.MethodGet, "/motions/:tag", dynamic.ThenFunc(app.motionDetails)) diff --git a/config.yaml.example b/config.yaml.example index 86b51f7..deeb81a 100644 --- a/config.yaml.example +++ b/config.yaml.example @@ -3,8 +3,6 @@ database_file: database.sqlite client_ca_certificates: cacert_class3.pem server_certificate: server.crt server_key: server.key -cookie_secret: base64encoded_random_byte_value_of_at_least_32_bytes -csrf_key: base64encoded_random_byte_value_of_at_least_32_bytes mail_server: smtp_host: localhost smtp_port: 25 diff --git a/go.mod b/go.mod index e406ea3..cf4ab1c 100644 --- a/go.mod +++ b/go.mod @@ -6,8 +6,6 @@ require ( github.com/Masterminds/sprig/v3 v3.2.2 github.com/golang-migrate/migrate/v4 v4.15.2 github.com/google/uuid v1.3.0 // indirect - github.com/gorilla/csrf v1.7.1 - github.com/gorilla/sessions v1.2.1 github.com/hashicorp/errwrap v1.1.0 // indirect github.com/hashicorp/go-multierror v1.1.1 // indirect github.com/huandu/xstrings v1.3.2 // indirect @@ -26,22 +24,23 @@ require ( ) require ( + github.com/alexedwards/scs/sqlite3store v0.0.0-20220216073957-c252878bcf5a + github.com/alexedwards/scs/v2 v2.5.0 + github.com/go-playground/form/v4 v4.2.0 + github.com/gorilla/csrf v1.7.1 + github.com/gorilla/sessions v1.2.1 + github.com/julienschmidt/httprouter v1.3.0 github.com/justinas/alice v1.2.0 + github.com/justinas/nosurf v1.1.1 github.com/stretchr/testify v1.7.0 ) require ( github.com/Masterminds/goutils v1.1.1 // indirect github.com/Masterminds/semver/v3 v3.1.1 // indirect - github.com/alexedwards/scs v1.4.1 // indirect - github.com/alexedwards/scs/sqlite3store v0.0.0-20220216073957-c252878bcf5a // indirect - github.com/alexedwards/scs/v2 v2.5.0 // indirect github.com/andybalholm/brotli v1.0.4 // indirect github.com/davecgh/go-spew v1.1.1 // indirect - github.com/go-playground/form/v4 v4.2.0 // indirect - github.com/gorilla/schema v1.2.0 // indirect github.com/gorilla/securecookie v1.1.1 // indirect - github.com/julienschmidt/httprouter v1.3.0 // indirect github.com/mitchellh/reflectwalk v1.0.2 // indirect github.com/pkg/errors v0.9.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect diff --git a/go.sum b/go.sum index 8b0558b..7df4665 100644 --- a/go.sum +++ b/go.sum @@ -121,8 +121,6 @@ github.com/alecthomas/template v0.0.0-20190718012654-fb15b899a751/go.mod h1:LOuy github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0= github.com/alecthomas/units v0.0.0-20190717042225-c3de453c63f4/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0= github.com/alecthomas/units v0.0.0-20190924025748-f65c72e2690d/go.mod h1:rBZYJk541a8SKzHPHnH3zbiI+7dagKZ0cgpgrD7Fyho= -github.com/alexedwards/scs v1.4.1 h1:/5L5a07IlqApODcEfZyMsu8Smd1S7Q4nBjEyKxIRTp0= -github.com/alexedwards/scs v1.4.1/go.mod h1:JRIFiXthhMSivuGbxpzUa0/hT5rz2hpyw61Bmd+S1bg= github.com/alexedwards/scs/sqlite3store v0.0.0-20220216073957-c252878bcf5a h1:5SCXvM8hruEAoNdKHVte0v3uVKqWLjDQeq4KIfFGqpM= github.com/alexedwards/scs/sqlite3store v0.0.0-20220216073957-c252878bcf5a/go.mod h1:Iyk7S76cxGaiEX/mSYmTZzYehp4KfyylcLaV3OnToss= github.com/alexedwards/scs/v2 v2.5.0 h1:zgxOfNFmiJyXG7UPIuw1g2b9LWBeRLh3PjfB9BDmfL4= @@ -462,6 +460,7 @@ github.com/go-openapi/swag v0.0.0-20160704191624-1d0bd113de87/go.mod h1:DXUve3Dp github.com/go-openapi/swag v0.19.2/go.mod h1:POnQmlKehdgb5mhVOsnJFsivZCEZ/vjK9gh66Z9tfKk= github.com/go-openapi/swag v0.19.5/go.mod h1:POnQmlKehdgb5mhVOsnJFsivZCEZ/vjK9gh66Z9tfKk= github.com/go-openapi/swag v0.19.14/go.mod h1:QYRuS/SOXUCsnplDa677K7+DxSOj6IPNl/eQntq43wQ= +github.com/go-playground/assert/v2 v2.0.1 h1:MsBgLAaY856+nPRTKrp3/OZK38U/wa0CcBYNjji3q3A= github.com/go-playground/assert/v2 v2.0.1/go.mod h1:VDjEfimB/XKnb+ZQfWdccd7VUvScMdVu0Titje2rxJ4= github.com/go-playground/form/v4 v4.2.0 h1:N1wh+Goz61e6w66vo8vJkQt+uwZSoLz50kZPJWR8eic= github.com/go-playground/form/v4 v4.2.0/go.mod h1:q1a2BY+AQUUzhl6xA/6hBetay6dEIhMHjgvJiGo6K7U= @@ -626,8 +625,6 @@ github.com/gorilla/mux v1.6.2/go.mod h1:1lud6UwP+6orDFRuTfBEV8e9/aOM/c4fVVCaMa2z github.com/gorilla/mux v1.7.2/go.mod h1:1lud6UwP+6orDFRuTfBEV8e9/aOM/c4fVVCaMa2zaAs= github.com/gorilla/mux v1.7.3/go.mod h1:1lud6UwP+6orDFRuTfBEV8e9/aOM/c4fVVCaMa2zaAs= github.com/gorilla/mux v1.7.4/go.mod h1:DVbg23sWSpFRCP0SfiEN6jmj59UnW/n46BH5rLB71So= -github.com/gorilla/schema v1.2.0 h1:YufUaxZYCKGFuAq3c96BOhjgd5nmXiOY9NGzF247Tsc= -github.com/gorilla/schema v1.2.0/go.mod h1:kgLaKoK1FELgZqMAVxx/5cbj0kT+57qxUrAlIO2eleU= github.com/gorilla/securecookie v1.1.1 h1:miw7JPhV+b/lAHSXz4qd/nN9jRiAFV5FwjeKyCS8BvQ= github.com/gorilla/securecookie v1.1.1/go.mod h1:ra0sb63/xPlUeL+yeDciTfxMRAA+MP+HVt/4epWDjd4= github.com/gorilla/sessions v1.2.1 h1:DHd3rPN5lE3Ts3D8rKkQ8x/0kqfeNmBAaiSi+o7FsgI= @@ -766,6 +763,8 @@ github.com/jung-kurt/gofpdf v1.0.0/go.mod h1:7Id9E/uU8ce6rXgefFLlgrJj/GYY22cpxn+ github.com/jung-kurt/gofpdf v1.0.3-0.20190309125859-24315acbbda5/go.mod h1:7Id9E/uU8ce6rXgefFLlgrJj/GYY22cpxn+r32jIOes= github.com/justinas/alice v1.2.0 h1:+MHSA/vccVCF4Uq37S42jwlkvI2Xzl7zTPCN5BnZNVo= github.com/justinas/alice v1.2.0/go.mod h1:fN5HRH/reO/zrUflLfTN43t3vXvKzvZIENsNEe7i7qA= +github.com/justinas/nosurf v1.1.1 h1:92Aw44hjSK4MxJeMSyDa7jwuI9GR2J/JCQiaKvXXSlk= +github.com/justinas/nosurf v1.1.1/go.mod h1:ALpWdSbuNGy2lZWtyXdjkYv4edL23oSEgfBT1gPJ5BQ= github.com/k0kubun/colorstring v0.0.0-20150214042306-9440f1994b88/go.mod h1:3w7q1U84EfirKl04SVQ/s7nPm1ZPhiXd34z40TNz36k= github.com/k0kubun/pp v2.3.0+incompatible/go.mod h1:GWse8YhT0p8pT4ir3ZgBbfZild3tgzSScAn6HmfYukg= github.com/kardianos/osext v0.0.0-20190222173326-2bc1f35cddc0/go.mod h1:1NbS8ALrpOvjt0rHPNLyCIeMtbizbir8U//inJ+zuB8= diff --git a/ui/html/pages/create_motion.html b/ui/html/pages/create_motion.html index 9093e51..9da5adc 100644 --- a/ui/html/pages/create_motion.html +++ b/ui/html/pages/create_motion.html @@ -3,7 +3,7 @@ {{ define "main" }}
- {{ csrfField .Request }} +