From 19165289ebe768865ecfc40ea662ef895836ca21 Mon Sep 17 00:00:00 2001 From: Natsu Kagami Date: Wed, 17 May 2023 01:29:03 +0200 Subject: [PATCH 1/2] Harden Admin Panel access - An ephemeral session key is generated every restart of kjudge. The cookie now stores this session key. This ensures the cookie never lives past kjudge restarting. - Access key is no longer stored on the cookie itself. - We only store the hash of the key in long-term memory. --- server/admin/admin.go | 6 ++-- server/admin/auth.go | 10 +++--- server/auth/admin.go | 76 ++++++++++++++++++++++++++----------------- server/server.go | 7 +++- 4 files changed, 62 insertions(+), 37 deletions(-) diff --git a/server/admin/admin.go b/server/admin/admin.go index e1d7c00d..9d2aeed6 100644 --- a/server/admin/admin.go +++ b/server/admin/admin.go @@ -11,19 +11,21 @@ import ( type Group struct { *echo.Group db *db.DB + au *auth.AdminAuth } // New creates a new group. -func New(db *db.DB, unauthed *echo.Group) (*Group, error) { +func New(db *db.DB, au *auth.AdminAuth, unauthed *echo.Group) (*Group, error) { grp := &Group{ Group: unauthed, db: db, + au: au, } // Authentication unauthed.GET("/login", grp.LoginGet) unauthed.POST("/login", grp.LoginPost) - g := unauthed.Group("", auth.MustAdmin) + g := unauthed.Group("", au.MustAdmin) g.GET("/logout", grp.LogoutPost) g.GET("", grp.Home) // Contest List diff --git a/server/admin/auth.go b/server/admin/auth.go index 5b474014..904d0ccc 100644 --- a/server/admin/auth.go +++ b/server/admin/auth.go @@ -23,7 +23,7 @@ func (l *LoginCtx) Render(c echo.Context) error { // LoginGet implements GET /admin/login. func (g *Group) LoginGet(c echo.Context) error { - if ok, err := alreadyLoggedIn(c); err != nil { + if ok, err := g.alreadyLoggedIn(c); err != nil { return err } else if ok { return nil @@ -34,14 +34,14 @@ func (g *Group) LoginGet(c echo.Context) error { // LoginPost implements POST /admin/login. func (g *Group) LoginPost(c echo.Context) error { - if ok, err := alreadyLoggedIn(c); err != nil { + if ok, err := g.alreadyLoggedIn(c); err != nil { return err } else if ok { return nil } key := c.FormValue("key") - if err := auth.SaveAdmin(key, c); err != nil { + if err := g.au.SaveAdmin(key, c); err != nil { return (&LoginCtx{Error: err}).Render(c) } last := c.QueryParam("last") @@ -64,8 +64,8 @@ func (g *Group) LogoutPost(c echo.Context) error { } // Redirect if already logged in. -func alreadyLoggedIn(c echo.Context) (bool, error) { - ok, err := auth.AuthenticateAdmin(c) +func (g *Group) alreadyLoggedIn(c echo.Context) (bool, error) { + ok, err := g.au.AuthenticateAdmin(c) if err != nil { return false, err } diff --git a/server/auth/admin.go b/server/auth/admin.go index 8ea648a7..ecba2975 100644 --- a/server/auth/admin.go +++ b/server/auth/admin.go @@ -13,38 +13,56 @@ import ( "github.com/pkg/errors" ) -// The string that will be used as the admin key. -var adminKey string +// AdminAuth hosts the authentication module for admin. +type AdminAuth struct { + // The string that will be used as the admin key. + hashedAdminKey []byte + // Random session key, will change every restart. Guarantees re-login of admin panel. + sessionKey string +} -// The admin key environment variable. -const adminKeyEnv = "ADMIN_KEY" +// AdminKeyEnv is the admin key environment variable to look for. +const AdminKeyEnv = "ADMIN_KEY" // The key name for the admin session. -const adminSessionName = "kjudge_admin" +const ( + adminSessionName = "kjudge_admin" + adminSessionKeyField = "key" +) -func init() { - val := os.Getenv(adminKeyEnv) +// NewAdmin creates a new AdminAuth. +func NewAdmin() (*AdminAuth, error) { + val := os.Getenv(AdminKeyEnv) if val != "" { if len(val) < 6 { - log.Fatalf("The admin key should be at least 6 characters long.") + return nil, errors.New("The admin key should be at least 6 characters long.") + } + } else { + log.Printf("%s variable not set. A random key will be generated and displayed.", AdminKeyEnv) + b := make([]byte, 16) + if _, err := rand.Read(b); err != nil { + log.Fatalf("%+v", errors.WithStack(err)) } - adminKey = val - return + val = fmt.Sprintf("%x", b) + log.Printf("The Admin Panel access key is `%s`\n", val) } - - log.Println("ADMIN_KEY variable not set. A random key will be generated and displayed.") - b := make([]byte, 16) - if _, err := rand.Read(b); err != nil { - log.Fatalf("%+v", errors.WithStack(err)) + // store the hashed version of it in memory + adminKey, err := PasswordHash(val) + if err != nil { + return nil, errors.Wrap(err, "error generating hashed key") } - adminKey = fmt.Sprintf("%x", b) - log.Printf("The Admin Panel access key is `%s`\n", adminKey) + // generate a session key + sessionKey := make([]byte, 64) + if _, err := rand.Read(sessionKey); err != nil { + return nil, errors.Wrap(err, "error generating session key") + } + return &AdminAuth{hashedAdminKey: adminKey, sessionKey: fmt.Sprintf("%x", sessionKey)}, nil } // MustAdmin is a middleware that ensures admin access. -func MustAdmin(h echo.HandlerFunc) echo.HandlerFunc { +func (au *AdminAuth) MustAdmin(h echo.HandlerFunc) echo.HandlerFunc { return func(c echo.Context) error { - res, err := AuthenticateAdmin(c) + res, err := au.AuthenticateAdmin(c) if err != nil { return nil } @@ -57,7 +75,7 @@ func MustAdmin(h echo.HandlerFunc) echo.HandlerFunc { } // AuthenticateAdmin returns whether the context has admin panel access. -func AuthenticateAdmin(c echo.Context) (bool, error) { +func (au *AdminAuth) AuthenticateAdmin(c echo.Context) (bool, error) { sess, err := session.Get(adminSessionName, c) if err != nil { return false, errors.Wrapf(RemoveAdmin(c), "handling err %v", err) @@ -65,26 +83,26 @@ func AuthenticateAdmin(c echo.Context) (bool, error) { if sess.IsNew { return false, nil } - hashedKey, ok := sess.Values["key"].(string) + key, ok := sess.Values[adminSessionKeyField].(string) if !ok { return false, nil } - return CheckPassword(adminKey, hashedKey) + return key == au.sessionKey, nil } // SaveAdmin saves the admin cookie. -func SaveAdmin(key string, c echo.Context) error { - if key != adminKey { +func (au *AdminAuth) SaveAdmin(key string, c echo.Context) error { + if ok, err := CheckPassword(key, string(au.hashedAdminKey)); err != nil { + return err + } else if !ok { return errors.New("Invalid admin key") } sess, _ := session.Get(adminSessionName, c) sess.Options.MaxAge = 0 - hashedKey, err := PasswordHash(adminKey) - if err != nil { - return err - } - sess.Values["key"] = string(hashedKey) + sess.Options.HttpOnly = true + sess.Options.SameSite = http.SameSiteStrictMode + sess.Values[adminSessionKeyField] = au.sessionKey return errors.WithStack(sess.Save(c.Request(), c.Response())) } diff --git a/server/server.go b/server/server.go index d79f2a87..eb2408a9 100644 --- a/server/server.go +++ b/server/server.go @@ -17,6 +17,7 @@ import ( "github.com/natsukagami/kjudge/models" "github.com/natsukagami/kjudge/models/verify" "github.com/natsukagami/kjudge/server/admin" + "github.com/natsukagami/kjudge/server/auth" "github.com/natsukagami/kjudge/server/contests" "github.com/natsukagami/kjudge/server/template" "github.com/natsukagami/kjudge/server/user" @@ -59,7 +60,11 @@ func New(db *db.DB) (*Server, error) { s.SetupProfiling() - if _, err := admin.New(s.db, s.echo.Group("/admin")); err != nil { + au, err := auth.NewAdmin() + if err != nil { + return nil, err + } + if _, err := admin.New(s.db, au, s.echo.Group("/admin")); err != nil { return nil, err } if _, err := user.New(s.db, s.echo.Group("/user")); err != nil { From de3ef1b15a7fae425a5a9c3bf3d17aef2e7db386 Mon Sep 17 00:00:00 2001 From: Natsu Kagami Date: Wed, 17 May 2023 01:48:58 +0200 Subject: [PATCH 2/2] Add some admin auth tests ... and some tools for writing admin panel tests --- server/admin/auth_test.go | 74 +++++++++++++++++++++++++++++++++++++++ server/auth/admin.go | 10 +++--- test/testing.go | 40 +++++++++++++++++++++ 3 files changed, 119 insertions(+), 5 deletions(-) create mode 100644 server/admin/auth_test.go diff --git a/server/admin/auth_test.go b/server/admin/auth_test.go new file mode 100644 index 00000000..aee6ea70 --- /dev/null +++ b/server/admin/auth_test.go @@ -0,0 +1,74 @@ +package admin_test + +import ( + "net/http" + "net/url" + "os" + "testing" + + "github.com/natsukagami/kjudge/server/auth" + "github.com/natsukagami/kjudge/test" +) + +func TestAdminLogin(t *testing.T) { + ts := test.NewServer(t) + + run := func(key string) *http.Response { + form := url.Values{} + form.Set("key", key) + + return ts.Serve(ts.PostForm(t, "/admin/login", form)) + } + + t.Run("success", func(t *testing.T) { + resp := run(os.Getenv(auth.AdminKeyEnv)) + + if resp.StatusCode >= 400 { + t.Errorf("Expected OK got %d", resp.StatusCode) + } + cookies := resp.Cookies() + if len(cookies) != 1 { + t.Errorf("Expected one cookie got %#v", cookies) + } + cookie := cookies[0] + if cookie.Name != auth.AdminSessionName { + t.Errorf("Expected cookie name `%s` got `%s`", auth.AdminSessionName, cookie.Name) + } + if !cookie.HttpOnly { + t.Error("Cookie is not Http-Only") + } + if cookie.SameSite != http.SameSiteStrictMode { + t.Error("Cookie is not strict same-site") + } + }) + + t.Run("fail", func(t *testing.T) { + resp := run("ababababababa") + + if resp.StatusCode < 400 { + t.Errorf("Expected error got %d", resp.StatusCode) + } + cookies := resp.Cookies() + if len(cookies) != 0 { + t.Errorf("Expected no cookie got %#v", cookies) + } + }) +} + +func TestAdminLoginCheck(t *testing.T) { + ts := test.NewServer(t) + withAdmin := ts.WithAdmin(t) + + t.Run("with admin cookie", func(t *testing.T) { + resp := ts.Serve(ts.Get(t, "/admin", nil), withAdmin) + if resp.StatusCode != http.StatusOK { + t.Errorf("Expected OK got %d", resp.StatusCode) + } + }) + t.Run("without admin cookie", func(t *testing.T) { + resp := ts.Serve(ts.Get(t, "/admin", nil)) + if resp.StatusCode == http.StatusOK { + t.Errorf("Expected redirect got %d", resp.StatusCode) + } + }) +} diff --git a/server/auth/admin.go b/server/auth/admin.go index ecba2975..4f53b2d8 100644 --- a/server/auth/admin.go +++ b/server/auth/admin.go @@ -24,9 +24,9 @@ type AdminAuth struct { // AdminKeyEnv is the admin key environment variable to look for. const AdminKeyEnv = "ADMIN_KEY" -// The key name for the admin session. const ( - adminSessionName = "kjudge_admin" + // AdminSessionName is the key name for the admin session cookie. + AdminSessionName = "kjudge_admin" adminSessionKeyField = "key" ) @@ -76,7 +76,7 @@ func (au *AdminAuth) MustAdmin(h echo.HandlerFunc) echo.HandlerFunc { // AuthenticateAdmin returns whether the context has admin panel access. func (au *AdminAuth) AuthenticateAdmin(c echo.Context) (bool, error) { - sess, err := session.Get(adminSessionName, c) + sess, err := session.Get(AdminSessionName, c) if err != nil { return false, errors.Wrapf(RemoveAdmin(c), "handling err %v", err) } @@ -98,7 +98,7 @@ func (au *AdminAuth) SaveAdmin(key string, c echo.Context) error { } else if !ok { return errors.New("Invalid admin key") } - sess, _ := session.Get(adminSessionName, c) + sess, _ := session.Get(AdminSessionName, c) sess.Options.MaxAge = 0 sess.Options.HttpOnly = true sess.Options.SameSite = http.SameSiteStrictMode @@ -108,7 +108,7 @@ func (au *AdminAuth) SaveAdmin(key string, c echo.Context) error { // RemoveAdmin removes the admin cookie session. func RemoveAdmin(c echo.Context) error { - sess, _ := session.Get(adminSessionName, c) + sess, _ := session.Get(AdminSessionName, c) sess.Options.MaxAge = -1 return errors.WithStack(sess.Save(c.Request(), c.Response())) } diff --git a/test/testing.go b/test/testing.go index 0912447e..f10c76fc 100644 --- a/test/testing.go +++ b/test/testing.go @@ -2,7 +2,9 @@ package test import ( + "crypto/rand" _ "embed" + "fmt" "log" "net/http" "net/http/httptest" @@ -15,6 +17,7 @@ import ( "github.com/labstack/echo/v4" "github.com/natsukagami/kjudge/db" "github.com/natsukagami/kjudge/server" + "github.com/natsukagami/kjudge/server/auth" "github.com/pkg/errors" ) @@ -72,6 +75,13 @@ func NewDB(t *testing.T) *db.DB { // NewServer creates a new kjudge server running on a test database. func NewServer(t *testing.T) *TestServer { + // generate an admin key + adminKey := make([]byte, 32) + if _, err := rand.Read(adminKey); err != nil { + t.Fatal("generating admin key:", err) + } + t.Setenv(auth.AdminKeyEnv, fmt.Sprintf("%x", adminKey)) + db := NewDB(t) s, err := server.New(db) if err != nil { @@ -88,6 +98,16 @@ func (ts *TestServer) PostForm(t *testing.T, path string, body url.Values) *http return req } +// Get fires a new GET request with URL queries. +func (ts *TestServer) Get(t *testing.T, path string, queries url.Values) *http.Request { + if len(queries) > 0 { + path = path + "?" + queries.Encode() + } + req := httptest.NewRequest(http.MethodGet, path, nil) + + return req +} + // Serve serves a HTTP request and returns its response. func (ts *TestServer) Serve(req *http.Request, opts ...ReqOpt) *http.Response { for _, opt := range opts { @@ -119,5 +139,25 @@ func (ts *TestServer) WithMisaka(t *testing.T) ReqOpt { } } +// WithAdmin logs in with the admin panel cookie for the next request. +func (ts *TestServer) WithAdmin(t *testing.T) ReqOpt { + // Perform the log in. + form := url.Values{} + form.Set("key", os.Getenv(auth.AdminKeyEnv)) + resp := ts.Serve(ts.PostForm(t, "/admin/login", form)) + + if resp.StatusCode >= 400 { + t.Fatalf("Cannot login to admin panel: got code %d", resp.StatusCode) + } + cookies := resp.Cookies() + if len(cookies) != 1 { + t.Fatalf("Cannot login to admin panel: expect one cookie, got %#v", cookies) + } + + return func(req *http.Request) { + req.AddCookie(cookies[0]) + } +} + // ReqOpt is an option for sending requests. type ReqOpt func(*http.Request)