From 1b9ee3d7633ff06b26da9856d7792fd08246e164 Mon Sep 17 00:00:00 2001 From: Umputun Date: Wed, 8 Aug 2018 14:33:18 -0500 Subject: [PATCH] feature/keystore (#185) * add keyStore and switch all secret usage to this store - implement static ks - add shared secret * lint: shadow in jwt and missing comment * rename static key group to shared and use top-level shared secret * move siteID extraction (from token) to internal func * lint: merge var and assign for key test --- backend/app/main.go | 50 +++++++++++++++------- backend/app/migrator/disqus_test.go | 5 ++- backend/app/migrator/migrator_test.go | 7 +-- backend/app/migrator/remark_test.go | 7 +-- backend/app/migrator/wordpress_test.go | 5 ++- backend/app/rest/api/migrator.go | 28 +++++++++--- backend/app/rest/api/migrator_test.go | 7 +-- backend/app/rest/api/rest.go | 3 +- backend/app/rest/api/rest_test.go | 5 ++- backend/app/rest/auth/auth_test.go | 9 ++-- backend/app/rest/auth/dev_provider_test.go | 5 ++- backend/app/rest/auth/jwt.go | 44 ++++++++++++++++--- backend/app/rest/auth/jwt_test.go | 19 ++++---- backend/app/rest/auth/provider.go | 2 +- backend/app/rest/auth/provider_test.go | 18 +++++--- backend/app/store/keys/keys.go | 27 ++++++++++++ backend/app/store/keys/keys_test.go | 20 +++++++++ backend/app/store/service/service.go | 12 ++++-- backend/app/store/service/service_test.go | 21 +++++---- 19 files changed, 217 insertions(+), 77 deletions(-) create mode 100644 backend/app/store/keys/keys.go create mode 100644 backend/app/store/keys/keys_test.go diff --git a/backend/app/main.go b/backend/app/main.go index a88b5759..483ed189 100644 --- a/backend/app/main.go +++ b/backend/app/main.go @@ -16,6 +16,7 @@ import ( "github.com/hashicorp/logutils" "github.com/jessevdk/go-flags" "github.com/pkg/errors" + "github.com/umputun/remark/backend/app/store/keys" "github.com/umputun/remark/backend/app/migrator" "github.com/umputun/remark/backend/app/rest/api" @@ -29,13 +30,14 @@ import ( // Opts with command line flags and env type Opts struct { - SecretKey string `long:"secret" env:"SECRET" required:"true" description:"secret key"` - RemarkURL string `long:"url" env:"REMARK_URL" required:"true" description:"url to remark"` + RemarkURL string `long:"url" env:"REMARK_URL" required:"true" description:"url to remark"` + SharedSecret string `long:"secret" env:"SECRET" required:"true" description:"shared secret key"` Store StoreGroup `group:"store" namespace:"store" env-namespace:"STORE"` Avatar AvatarGroup `group:"avatar" namespace:"avatar" env-namespace:"AVATAR"` Cache CacheGroup `group:"cache" namespace:"cache" env-namespace:"CACHE"` Mongo MongoGroup `group:"mongo" namespace:"mongo" env-namespace:"MONGO"` + Key KeyGroup `group:"key" namespace:"key" env-namespace:"KEY"` Sites []string `long:"site" env:"SITE" default:"remark" description:"site names" env-delim:","` Admins []string `long:"admin" env:"ADMIN" description:"admin(s) names" env-delim:","` @@ -106,6 +108,11 @@ type MongoGroup struct { DB string `long:"db" env:"DB" default:"remark42" description:"mongo database"` } +// KeyGroup defines options group for key params +type KeyGroup struct { + Type string `long:"type" env:"TYPE" description:"type of key store" choice:"shared" choice:"mongo" default:"shared"` +} + var revision = "unknown" // Application holds all active objects @@ -166,10 +173,15 @@ func New(opts Opts) (*Application, error) { return nil, err } + keyStore, err := makeKeyStore(opts.Key, opts.SharedSecret) + if err != nil { + return nil, err + } + dataService := &service.DataStore{ Interface: storeEngine, EditDuration: opts.EditDuration, - Secret: opts.SecretKey, + KeyStore: keyStore, MaxCommentSize: opts.MaxCommentSize, Admins: opts.Admins, } @@ -180,8 +192,7 @@ func New(opts Opts) (*Application, error) { } // token TTL is 5 minutes, inactivity interval 7+ days by default - jwtService := auth.NewJWT(opts.SecretKey, strings.HasPrefix(opts.RemarkURL, "https://"), - opts.Auth.TTL.JWT, opts.Auth.TTL.Cookie) + jwtService := auth.NewJWT(keyStore, strings.HasPrefix(opts.RemarkURL, "https://"), opts.Auth.TTL.JWT, opts.Auth.TTL.Cookie) avatarStore, err := makeAvatarStore(opts.Avatar, opts.Mongo) if err != nil { @@ -202,20 +213,21 @@ func New(opts Opts) (*Application, error) { DisqusImporter: &migrator.Disqus{DataStore: dataService}, WordPressImporter: &migrator.WordPress{DataStore: dataService}, NativeExported: &migrator.Remark{DataStore: dataService}, - SecretKey: opts.SecretKey, + KeyStore: keyStore, } authProviders := makeAuthProviders(jwtService, avatarProxy, dataService, opts) srv := &api.Rest{ - Version: revision, - DataService: dataService, - Exporter: exporter, - WebRoot: opts.WebRoot, - RemarkURL: opts.RemarkURL, - ImageProxy: &proxy.Image{Enabled: opts.ImageProxy, RoutePath: "/api/v1/img", RemarkURL: opts.RemarkURL}, - AvatarProxy: avatarProxy, - ReadOnlyAge: opts.ReadOnlyAge, + Version: revision, + DataService: dataService, + Exporter: exporter, + WebRoot: opts.WebRoot, + RemarkURL: opts.RemarkURL, + ImageProxy: &proxy.Image{Enabled: opts.ImageProxy, RoutePath: "/api/v1/img", RemarkURL: opts.RemarkURL}, + AvatarProxy: avatarProxy, + ReadOnlyAge: opts.ReadOnlyAge, + SharedSecret: opts.SharedSecret, Authenticator: auth.Authenticator{ JWTService: jwtService, AdminEmail: opts.AdminEmail, @@ -336,6 +348,15 @@ func makeAvatarStore(group AvatarGroup, mg MongoGroup) (avatar.Store, error) { return nil, errors.Errorf("unsupported avatar store type %s", group.Type) } +func makeKeyStore(group KeyGroup, sharedSecret string) (keys.Store, error) { + switch group.Type { + case "shared": + return keys.NewStaticStore(sharedSecret), nil + default: + return nil, errors.Errorf("unsupported key store type %s", group.Type) + } +} + func makeCache(group CacheGroup, mg MongoGroup) (cache.LoadingCache, error) { switch group.Type { case "mem": @@ -398,7 +419,6 @@ func makeAuthProviders(jwtService *auth.JWT, avatarProxy *proxy.Avatar, ds *serv RemarkURL: opts.RemarkURL, Cid: cid, Csecret: secret, - SecretKey: opts.SecretKey, PermissionChecker: ds, } } diff --git a/backend/app/migrator/disqus_test.go b/backend/app/migrator/disqus_test.go index af7a55ba..a8e435e3 100644 --- a/backend/app/migrator/disqus_test.go +++ b/backend/app/migrator/disqus_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/require" "github.com/umputun/remark/backend/app/store" "github.com/umputun/remark/backend/app/store/engine" + "github.com/umputun/remark/backend/app/store/keys" "github.com/umputun/remark/backend/app/store/service" "github.com/stretchr/testify/assert" @@ -19,7 +20,7 @@ func TestDisqus_Import(t *testing.T) { defer os.Remove("/tmp/remark-test.db") b, err := engine.NewBoltDB(bolt.Options{}, engine.BoltSite{FileName: "/tmp/remark-test.db", SiteID: "test"}) require.Nil(t, err, "create store") - dataStore := service.DataStore{Interface: b} + dataStore := service.DataStore{Interface: b, KeyStore: keys.NewStaticStore("12345")} d := Disqus{DataStore: &dataStore} size, err := d.Import(strings.NewReader(xmlTestDisqus), "test") assert.Nil(t, err) @@ -36,7 +37,7 @@ func TestDisqus_Import(t *testing.T) { assert.Equal(t, store.Locator{SiteID: "test", URL: "http://radio-t.umputun.com/2011/03/229_8880.html"}, c.Locator) assert.Equal(t, "Dmitry Noname", c.User.Name) assert.Equal(t, "disqus_8799342cdf328253e03313958ffc6a433659d7ff", c.User.ID) - assert.Equal(t, "96243f024cf6ad42b66f0c72709ae20b5d10ec14", c.User.IP) + assert.Equal(t, "7001968ea3f6c9013a9f0a3650f200c10c927638", c.User.IP) posts, err := dataStore.List("test", 0, 0) assert.Nil(t, err) diff --git a/backend/app/migrator/migrator_test.go b/backend/app/migrator/migrator_test.go index 120b1185..2ec70199 100644 --- a/backend/app/migrator/migrator_test.go +++ b/backend/app/migrator/migrator_test.go @@ -8,6 +8,7 @@ import ( "github.com/coreos/bbolt" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/umputun/remark/backend/app/store/keys" "github.com/umputun/remark/backend/app/store/service" "github.com/umputun/remark/backend/app/store/engine" @@ -24,7 +25,7 @@ func TestMigrator_ImportDisqus(t *testing.T) { b, err := engine.NewBoltDB(bolt.Options{}, engine.BoltSite{FileName: "/tmp/remark-test.db", SiteID: "test"}) require.Nil(t, err, "create store") - dataStore := &service.DataStore{Interface: b} + dataStore := &service.DataStore{Interface: b, KeyStore: keys.NewStaticStore("12345")} size, err := ImportComments(ImportParams{ DataStore: dataStore, InputFile: "/tmp/disqus-test.xml", @@ -50,7 +51,7 @@ func TestMigrator_ImportWordPress(t *testing.T) { b, err := engine.NewBoltDB(bolt.Options{}, engine.BoltSite{FileName: "/tmp/remark-test.db", SiteID: "test"}) require.Nil(t, err, "create store") - dataStore := &service.DataStore{Interface: b} + dataStore := &service.DataStore{Interface: b, KeyStore: keys.NewStaticStore("12345")} size, err := ImportComments(ImportParams{ DataStore: dataStore, InputFile: "/tmp/wordpress-test.xml", @@ -79,7 +80,7 @@ func TestMigrator_ImportRemark(t *testing.T) { b, err := engine.NewBoltDB(bolt.Options{}, engine.BoltSite{FileName: "/tmp/remark-test.db", SiteID: "radio-t"}) require.Nil(t, err, "create store") - dataStore := &service.DataStore{Interface: b} + dataStore := &service.DataStore{Interface: b, KeyStore: keys.NewStaticStore("12345")} size, err := ImportComments(ImportParams{ DataStore: dataStore, diff --git a/backend/app/migrator/remark_test.go b/backend/app/migrator/remark_test.go index 414ec90f..572060d5 100644 --- a/backend/app/migrator/remark_test.go +++ b/backend/app/migrator/remark_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/umputun/remark/backend/app/store" "github.com/umputun/remark/backend/app/store/engine" + "github.com/umputun/remark/backend/app/store/keys" "github.com/umputun/remark/backend/app/store/service" ) @@ -49,7 +50,7 @@ func TestRemark_Import(t *testing.T) { os.Remove(testDb) b, err := engine.NewBoltDB(bolt.Options{}, engine.BoltSite{SiteID: "radio-t", FileName: testDb}) assert.Nil(t, err) - r := Remark{DataStore: &service.DataStore{Interface: b}} + r := Remark{DataStore: &service.DataStore{Interface: b, KeyStore: keys.NewStaticStore("12345")}} size, err := r.Import(buf, "radio-t") assert.Nil(t, err) assert.Equal(t, 2, size) @@ -77,7 +78,7 @@ func TestRemark_ImportManyWithError(t *testing.T) { os.Remove(testDb) b, err := engine.NewBoltDB(bolt.Options{}, engine.BoltSite{SiteID: "radio-t", FileName: testDb}) assert.Nil(t, err) - r := Remark{DataStore: &service.DataStore{Interface: b}} + r := Remark{DataStore: &service.DataStore{Interface: b, KeyStore: keys.NewStaticStore("12345")}} n, err := r.Import(buf, "radio-t") assert.EqualError(t, err, "failed to save 2 comments") assert.Equal(t, 1200, n) @@ -93,7 +94,7 @@ func prep(t *testing.T) *service.DataStore { boltStore, err := engine.NewBoltDB(bolt.Options{}, engine.BoltSite{SiteID: "radio-t", FileName: testDb}) assert.Nil(t, err) - b := &service.DataStore{Interface: boltStore} + b := &service.DataStore{Interface: boltStore, KeyStore: keys.NewStaticStore("12345")} comment := store.Comment{ ID: "efbc17f177ee1a1c0ee6e1e025749966ec071adc", diff --git a/backend/app/migrator/wordpress_test.go b/backend/app/migrator/wordpress_test.go index c915956a..d1cef135 100644 --- a/backend/app/migrator/wordpress_test.go +++ b/backend/app/migrator/wordpress_test.go @@ -8,6 +8,7 @@ import ( "github.com/coreos/bbolt" "github.com/stretchr/testify/assert" + "github.com/umputun/remark/backend/app/store/keys" "github.com/umputun/remark/backend/app/store" "github.com/umputun/remark/backend/app/store/engine" @@ -20,7 +21,7 @@ func TestWordPress_Import(t *testing.T) { b, err := engine.NewBoltDB(bolt.Options{}, engine.BoltSite{FileName: "/tmp/remark-test.db", SiteID: siteID}) assert.Nil(t, err, "create store") - dataStore := service.DataStore{Interface: b} + dataStore := service.DataStore{Interface: b, KeyStore: keys.NewStaticStore("12345")} wp := WordPress{DataStore: &dataStore} size, err := wp.Import(strings.NewReader(xmlTestWP), siteID) assert.Nil(t, err) @@ -35,7 +36,7 @@ func TestWordPress_Import(t *testing.T) { assert.Equal(t, store.Locator{URL: "https://realmenweardress.es/2010/07/do-you-rp/", SiteID: siteID}, c.Locator) assert.Equal(t, "wordpress_75b2b81081f82495d7af26759e67af6554ffda4a", c.User.ID) assert.Equal(t, "SuperUser3", c.User.Name) - assert.Equal(t, "b646e160768fbc1414d3b2c8f88a767bfbb00871", c.User.IP) + assert.Equal(t, "e8b1e92bbcf5b9bb88472f9bdb82d1b8c7ed39d6", c.User.IP) ts, _ := time.Parse(wpTimeLayout, "2010-08-18 15:19:14") assert.Equal(t, ts, c.Timestamp) assert.Equal(t, c.Text, "Mekkatorque was over in that tent up to the right") diff --git a/backend/app/rest/api/migrator.go b/backend/app/rest/api/migrator.go index bb891bd2..3fdc3d9d 100644 --- a/backend/app/rest/api/migrator.go +++ b/backend/app/rest/api/migrator.go @@ -15,6 +15,7 @@ import ( "github.com/go-chi/chi" "github.com/go-chi/chi/middleware" "github.com/go-chi/render" + "github.com/umputun/remark/backend/app/store/keys" "github.com/umputun/remark/backend/app/migrator" "github.com/umputun/remark/backend/app/rest" @@ -29,7 +30,7 @@ type Migrator struct { DisqusImporter migrator.Importer WordPressImporter migrator.Importer NativeExported migrator.Exporter - SecretKey string + KeyStore keys.Store httpServer *http.Server lock sync.Mutex @@ -81,15 +82,22 @@ func (m *Migrator) routes() chi.Router { // imports comments from post body. func (m *Migrator) importCtrl(w http.ResponseWriter, r *http.Request) { + siteID := r.URL.Query().Get("site") secret := r.URL.Query().Get("secret") - if strings.TrimSpace(secret) == "" || secret != m.SecretKey { + + skey, err := m.KeyStore.Get(siteID) + if err != nil { + render.Status(r, http.StatusForbidden) + render.JSON(w, r, JSON{"status": "error", "details": "secret key store"}) + return + } + + if strings.TrimSpace(secret) == "" || secret != skey { render.Status(r, http.StatusForbidden) render.JSON(w, r, JSON{"status": "error", "details": "secret key"}) return } - siteID := r.URL.Query().Get("site") - var importer migrator.Importer switch r.URL.Query().Get("provider") { case "disqus": @@ -116,14 +124,22 @@ func (m *Migrator) importCtrl(w http.ResponseWriter, r *http.Request) { // exports all comments for siteID as gz file func (m *Migrator) exportCtrl(w http.ResponseWriter, r *http.Request) { + siteID := r.URL.Query().Get("site") secret := r.URL.Query().Get("secret") - if strings.TrimSpace(secret) == "" || secret != m.SecretKey { + + skey, err := m.KeyStore.Get(siteID) + if err != nil { + render.Status(r, http.StatusForbidden) + render.JSON(w, r, JSON{"status": "error", "details": "secret key store"}) + return + } + + if strings.TrimSpace(secret) == "" || secret != skey { render.Status(r, http.StatusForbidden) render.JSON(w, r, JSON{"status": "error", "details": "secret key"}) return } - siteID := r.URL.Query().Get("site") exportFile := fmt.Sprintf("%s-%s.json.gz", siteID, time.Now().Format("20060102")) log.Printf("[DEBUG] import request for site=%s to %s", siteID, exportFile) diff --git a/backend/app/rest/api/migrator_test.go b/backend/app/rest/api/migrator_test.go index 43f6c0e4..6de6342b 100644 --- a/backend/app/rest/api/migrator_test.go +++ b/backend/app/rest/api/migrator_test.go @@ -13,6 +13,7 @@ import ( "github.com/coreos/bbolt" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/umputun/remark/backend/app/store/keys" "github.com/umputun/remark/backend/app/migrator" "github.com/umputun/remark/backend/app/rest/cache" @@ -127,14 +128,14 @@ func TestMigrator_Shutdown(t *testing.T) { func prepImportSrv(t *testing.T) (svc *Migrator, ts *httptest.Server) { b, err := engine.NewBoltDB(bolt.Options{}, engine.BoltSite{FileName: testDb, SiteID: "radio-t"}) require.Nil(t, err) - dataStore := &service.DataStore{Interface: b} + dataStore := &service.DataStore{Interface: b, KeyStore: keys.NewStaticStore("123456")} svc = &Migrator{ DisqusImporter: &migrator.Disqus{DataStore: dataStore}, WordPressImporter: &migrator.WordPress{DataStore: dataStore}, NativeImporter: &migrator.Remark{DataStore: dataStore}, NativeExported: &migrator.Remark{DataStore: dataStore}, Cache: &cache.Nop{}, - SecretKey: "123456", + KeyStore: keys.NewStaticStore("123456"), } routes := svc.routes() @@ -142,7 +143,7 @@ func prepImportSrv(t *testing.T) (svc *Migrator, ts *httptest.Server) { return svc, ts } -func cleanupImportSrv(srv *Migrator, ts *httptest.Server) { +func cleanupImportSrv(_ *Migrator, ts *httptest.Server) { ts.Close() os.Remove(testDb) } diff --git a/backend/app/rest/api/rest.go b/backend/app/rest/api/rest.go index ddef456d..c83db44c 100644 --- a/backend/app/rest/api/rest.go +++ b/backend/app/rest/api/rest.go @@ -43,6 +43,7 @@ type Rest struct { WebRoot string RemarkURL string ReadOnlyAge int + SharedSecret string ScoreThresholds struct { Low int Critical int @@ -129,7 +130,7 @@ func (s *Rest) routes() chi.Router { }) router.Use(corsMiddleware.Handler) - ipFn := func(ip string) string { return store.HashValue(ip, s.DataService.Secret)[:12] } // logger uses it for anonymization + ipFn := func(ip string) string { return store.HashValue(ip, s.SharedSecret)[:12] } // logger uses it for anonymization // auth routes for all providers router.Route("/auth", func(r chi.Router) { diff --git a/backend/app/rest/api/rest_test.go b/backend/app/rest/api/rest_test.go index 936e8df2..9a691c76 100644 --- a/backend/app/rest/api/rest_test.go +++ b/backend/app/rest/api/rest_test.go @@ -14,6 +14,7 @@ import ( "github.com/coreos/bbolt" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/umputun/remark/backend/app/store/keys" "github.com/umputun/remark/backend/app/migrator" "github.com/umputun/remark/backend/app/rest/auth" @@ -78,7 +79,7 @@ func prep(t *testing.T) (srv *Rest, ts *httptest.Server) { Interface: b, EditDuration: 5 * time.Minute, MaxCommentSize: 4000, - Secret: "123456", + KeyStore: keys.NewStaticStore("123456"), Admins: []string{"a1", "a2"}, } srv = &Rest{ @@ -88,7 +89,7 @@ func prep(t *testing.T) (srv *Rest, ts *httptest.Server) { Providers: nil, AdminEmail: "admin@remark-42.com", - JWTService: auth.NewJWT("12345", false, time.Minute, time.Hour), + JWTService: auth.NewJWT(keys.NewStaticStore("123456"), false, time.Minute, time.Hour), }, Exporter: &migrator.Remark{DataStore: dataStore}, Cache: &cache.Nop{}, diff --git a/backend/app/rest/auth/auth_test.go b/backend/app/rest/auth/auth_test.go index e7a50d71..33dc0327 100644 --- a/backend/app/rest/auth/auth_test.go +++ b/backend/app/rest/auth/auth_test.go @@ -11,6 +11,7 @@ import ( "github.com/go-chi/chi" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/umputun/remark/backend/app/store/keys" ) var testJwtUserBlocked = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjI3ODkxOTE4MjIsImp0aSI6InJhbmRvbSBpZCIsImlzcyI6InJlbWFyazQyIiwibmJmIjoxNTI2ODg0MjIyLCJ1c2VyIjp7Im5hbWUiOiJuYW1lMSIsImlkIjoiaWQxIiwicGljdHVyZSI6IiIsImFkbWluIjpmYWxzZSwiYmxvY2siOnRydWV9LCJzdGF0ZSI6IjEyMzQ1NiIsImZyb20iOiJmcm9tIn0.6P_OwGf8CUJRtvNSlW20GmaMb5pFvCNemP94fHCqb5Q" @@ -18,7 +19,7 @@ var testJwtUserBlocked = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjI3ODkxO var testJwtDeleteMe = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjI3ODkxOTE4MjIsImp0aSI6InJhbmRvbSBpZCIsImlzcyI6InJlbWFyazQyIiwibmJmIjoxNTI2ODg0MjIyLCJ1c2VyIjp7Im5hbWUiOiJuYW1lMSIsImlkIjoiaWQxIiwicGljdHVyZSI6IiIsImFkbWluIjpmYWxzZSwiYmxvY2siOmZhbHNlfSwiZmxhZ3MiOnsiZGVsZXRlbWUiOnRydWV9fQ.SLh1QpFytWZqcT99VgcdAOtgFKhvpKCcZwqWTvAd63g" func TestAuthJWTCookie(t *testing.T) { - a := Authenticator{DevPasswd: "123456", JWTService: NewJWT("xyz 12345", false, time.Hour, time.Hour), + a := Authenticator{DevPasswd: "123456", JWTService: NewJWT(keys.NewStaticStore("xyz 12345"), false, time.Hour, time.Hour), PermissionChecker: &mockUserPermissions{}} router := chi.NewRouter() router.With(a.Auth(true)).Get("/auth", func(w http.ResponseWriter, r *http.Request) { @@ -56,7 +57,7 @@ func TestAuthJWTCookie(t *testing.T) { } func TestAuthJWTHeader(t *testing.T) { - a := Authenticator{DevPasswd: "123456", JWTService: NewJWT("xyz 12345", false, time.Hour, time.Hour)} + a := Authenticator{DevPasswd: "123456", JWTService: NewJWT(keys.NewStaticStore("xyz 12345"), false, time.Hour, time.Hour)} router := chi.NewRouter() router.With(a.Auth(true)).Get("/auth", func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(201) @@ -83,7 +84,7 @@ func TestAuthJWTHeader(t *testing.T) { } func TestAuthJWtBlocked(t *testing.T) { - a := Authenticator{DevPasswd: "123456", JWTService: NewJWT("xyz 12345", false, time.Hour, time.Hour)} + a := Authenticator{DevPasswd: "123456", JWTService: NewJWT(keys.NewStaticStore("xyz 12345"), false, time.Hour, time.Hour)} router := chi.NewRouter() router.With(a.Auth(true)).Get("/auth", func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(201) @@ -103,7 +104,7 @@ func TestAuthJWtBlocked(t *testing.T) { } func TestAuthJWtFlags(t *testing.T) { - a := Authenticator{DevPasswd: "123456", JWTService: NewJWT("xyz 12345", false, time.Hour, time.Hour)} + a := Authenticator{DevPasswd: "123456", JWTService: NewJWT(keys.NewStaticStore("xyz 12345"), false, time.Hour, time.Hour)} router := chi.NewRouter() router.With(a.Auth(true)).Get("/auth", func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(201) diff --git a/backend/app/rest/auth/dev_provider_test.go b/backend/app/rest/auth/dev_provider_test.go index 448ac29c..8f768baa 100644 --- a/backend/app/rest/auth/dev_provider_test.go +++ b/backend/app/rest/auth/dev_provider_test.go @@ -13,11 +13,12 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/umputun/remark/backend/app/store" + "github.com/umputun/remark/backend/app/store/keys" ) func TestDevProvider(t *testing.T) { - params := Params{RemarkURL: "http://127.0.0.1:8080", SecretKey: "123456", Cid: "cid", Csecret: "csecret", - JwtService: NewJWT("12345", false, time.Hour, time.Hour*24*31), + params := Params{RemarkURL: "http://127.0.0.1:8080", Cid: "cid", Csecret: "csecret", + JwtService: NewJWT(keys.NewStaticStore("12345"), false, time.Hour, time.Hour*24*31), PermissionChecker: &mockUserPermissions{admin: "dev_user"}, } srv := DevAuthServer{Provider: NewDev(params), nonInteractive: true, username: "dev_user"} diff --git a/backend/app/rest/auth/jwt.go b/backend/app/rest/auth/jwt.go index 54855432..0269ad0f 100644 --- a/backend/app/rest/auth/jwt.go +++ b/backend/app/rest/auth/jwt.go @@ -4,8 +4,9 @@ import ( "net/http" "time" - jwt "github.com/dgrijalva/jwt-go" + "github.com/dgrijalva/jwt-go" "github.com/pkg/errors" + "github.com/umputun/remark/backend/app/store/keys" "github.com/umputun/remark/backend/app/store" ) @@ -13,7 +14,7 @@ import ( // JWT wraps jwt operations // supports both header and cookie jwt type JWT struct { - secret string + keyStore keys.Store secureCookies bool tokenDuration time.Duration cookieDuration time.Duration @@ -43,9 +44,9 @@ const xsrfCookieName = "XSRF-TOKEN" const xsrfHeaderKey = "X-XSRF-TOKEN" // NewJWT makes JWT service -func NewJWT(secret string, secureCookies bool, tokenDuration time.Duration, cookieDuration time.Duration) *JWT { +func NewJWT(keyStore keys.Store, secureCookies bool, tokenDuration time.Duration, cookieDuration time.Duration) *JWT { res := JWT{ - secret: secret, + keyStore: keyStore, secureCookies: secureCookies, tokenDuration: tokenDuration, cookieDuration: cookieDuration, @@ -56,14 +57,20 @@ func NewJWT(secret string, secureCookies bool, tokenDuration time.Duration, cook // Token makes jwt with claims func (j *JWT) Token(claims *CustomClaims) (string, error) { token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) - tokenString, err := token.SignedString([]byte(j.secret)) + + secret, err := j.keyStore.Get(claims.SiteID) + if err != nil { + return "", errors.Wrap(err, "can't get secret") + } + + tokenString, err := token.SignedString([]byte(secret)) if err != nil { return "", errors.Wrap(err, "can't sign jwt token") } return tokenString, nil } -// HasFlags indicates presense of special flags +// HasFlags indicates presence of special flags func (j *JWT) HasFlags(claims *CustomClaims) bool { return claims.Flags.DeleteMe || claims.Flags.Login } @@ -71,11 +78,34 @@ func (j *JWT) HasFlags(claims *CustomClaims) bool { // Parse token string and verify. Not checking for expiration func (j *JWT) Parse(tokenString string) (*CustomClaims, error) { parser := jwt.Parser{SkipClaimsValidation: true} // allow parsing of expired tokens + + getSiteID := func() (siteID string, err error) { // parse token without signature check to get siteID + preToken, _, err := parser.ParseUnverified(tokenString, &CustomClaims{}) + if err != nil { + return "", errors.Wrap(err, "can't pre-parse jwt") + } + preClaims, ok := preToken.Claims.(*CustomClaims) + if !ok { + return "", errors.New("invalid jwt") + } + return preClaims.SiteID, nil + } + + siteID, err := getSiteID() + if err != nil { + return nil, errors.Wrap(err, "failed to get siteID from jwt token") + } + + secret, err := j.keyStore.Get(siteID) + if err != nil { + return nil, errors.Wrap(err, "can't get secret") + } + token, err := parser.ParseWithClaims(tokenString, &CustomClaims{}, func(token *jwt.Token) (interface{}, error) { if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok { return nil, errors.Errorf("unexpected signing method: %v", token.Header["alg"]) } - return []byte(j.secret), nil + return []byte(secret), nil }) if err != nil { return nil, errors.Wrap(err, "can't parse jwt") diff --git a/backend/app/rest/auth/jwt_test.go b/backend/app/rest/auth/jwt_test.go index 0795809b..f1ce7efa 100644 --- a/backend/app/rest/auth/jwt_test.go +++ b/backend/app/rest/auth/jwt_test.go @@ -10,6 +10,7 @@ import ( "github.com/dgrijalva/jwt-go" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/umputun/remark/backend/app/store/keys" "github.com/umputun/remark/backend/app/store" ) @@ -29,7 +30,7 @@ var testJwtBadSign = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjI3ODkxOTE4M var days31 = time.Hour * 24 * 31 func TestJWT_Token(t *testing.T) { - j := NewJWT("xyz 12345", false, time.Hour, days31) + j := NewJWT(keys.NewStaticStore("xyz 12345"), false, time.Hour, days31) claims := &CustomClaims{ State: "123456", @@ -52,7 +53,7 @@ func TestJWT_Token(t *testing.T) { } func TestJWT_Parse(t *testing.T) { - j := NewJWT("xyz 12345", false, time.Hour, days31) + j := NewJWT(keys.NewStaticStore("xyz 12345"), false, time.Hour, days31) claims, err := j.Parse(testJwtValid) assert.NoError(t, err) assert.False(t, j.IsExpired(claims)) @@ -70,7 +71,7 @@ func TestJWT_Parse(t *testing.T) { } func TestJWT_Set(t *testing.T) { - j := NewJWT("xyz 12345", false, time.Hour, days31) + j := NewJWT(keys.NewStaticStore("xyz 12345"), false, time.Hour, days31) claims := &CustomClaims{ State: "123456", @@ -115,7 +116,7 @@ func TestJWT_Set(t *testing.T) { } func TestJWT_GetFromHeader(t *testing.T) { - j := NewJWT("xyz 12345", false, time.Hour, days31) + j := NewJWT(keys.NewStaticStore("xyz 12345"), false, time.Hour, days31) req := httptest.NewRequest("GET", "/", nil) req.Header.Add(jwtHeaderKey, testJwtValid) @@ -134,13 +135,13 @@ func TestJWT_GetFromHeader(t *testing.T) { req = httptest.NewRequest("GET", "/", nil) req.Header.Add(jwtHeaderKey, "bad bad token") _, err = j.Get(req) - assert.NotNil(t, err) - assert.True(t, strings.Contains(err.Error(), "can't parse jwt: token contains an invalid number of segments"), err.Error()) + require.NotNil(t, err) + assert.True(t, strings.Contains(err.Error(), "can't pre-parse jwt: token contains an invalid number of segments"), err.Error()) } func TestJWT_SetAndGetWithCookies(t *testing.T) { - j := NewJWT("xyz 12345", false, time.Hour, days31) + j := NewJWT(keys.NewStaticStore("xyz 12345"), false, time.Hour, days31) claims := &CustomClaims{ State: "123456", @@ -182,7 +183,7 @@ func TestJWT_SetAndGetWithCookies(t *testing.T) { } func TestJWT_SetAndGetWithXsrfMismatch(t *testing.T) { - j := NewJWT("xyz 12345", false, time.Hour, days31) + j := NewJWT(keys.NewStaticStore("xyz 12345"), false, time.Hour, days31) claims := &CustomClaims{ State: "123456", @@ -219,7 +220,7 @@ func TestJWT_SetAndGetWithXsrfMismatch(t *testing.T) { } func TestJWT_SetAndGetWithCookiesExpired(t *testing.T) { - j := NewJWT("xyz 12345", false, time.Hour, days31) + j := NewJWT(keys.NewStaticStore("xyz 12345"), false, time.Hour, days31) claims := &CustomClaims{ State: "123456", diff --git a/backend/app/rest/auth/provider.go b/backend/app/rest/auth/provider.go index 30f87739..1fe0588d 100644 --- a/backend/app/rest/auth/provider.go +++ b/backend/app/rest/auth/provider.go @@ -39,7 +39,6 @@ type Params struct { AvatarProxy *proxy.Avatar JwtService *JWT PermissionChecker PermissionChecker - SecretKey string Cid string Csecret string } @@ -171,6 +170,7 @@ func (p Provider) authHandler(w http.ResponseWriter, r *http.Request) { Issuer: "remark42", Id: p.randToken(), }, + SiteID: oauthClaims.SiteID, SessionOnly: oauthClaims.SessionOnly, } diff --git a/backend/app/rest/auth/provider_test.go b/backend/app/rest/auth/provider_test.go index 14e83a8f..c26c9087 100644 --- a/backend/app/rest/auth/provider_test.go +++ b/backend/app/rest/auth/provider_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/umputun/remark/backend/app/store/keys" "golang.org/x/oauth2" "github.com/umputun/remark/backend/app/store" @@ -52,6 +53,14 @@ func TestLogin(t *testing.T) { assert.Equal(t, store.User{Name: "blah", ID: "mock_myuser1", Picture: "http://exmple.com/pic1.png", Admin: false, Blocked: true, IP: ""}, u) + token := resp.Cookies()[0].Value + jwtSvc := NewJWT(keys.NewStaticStore("12345"), false, time.Hour, time.Hour*24*31) + + claims, err := jwtSvc.Parse(token) + require.NoError(t, err) + assert.Equal(t, "remark42", claims.Issuer) + assert.Equal(t, "remark", claims.SiteID) + // check admin user resp, err = client.Get("http://localhost:8981/login?site=remark") assert.Nil(t, err) @@ -94,7 +103,7 @@ func TestLoginSessionOnly(t *testing.T) { req.AddCookie(resp.Cookies()[1]) req.Header.Add("X-XSRF-TOKEN", resp.Cookies()[1].Value) - jwtService := NewJWT("12345", false, time.Hour, time.Hour) + jwtService := NewJWT(keys.NewStaticStore("12345"), false, time.Hour, time.Hour) res, err := jwtService.Get(req) require.Nil(t, err) assert.Equal(t, true, res.SessionOnly) @@ -129,13 +138,12 @@ func TestLogout(t *testing.T) { } func TestInitProvider(t *testing.T) { - params := Params{RemarkURL: "url", SecretKey: "123456", Cid: "cid", Csecret: "csecret"} + params := Params{RemarkURL: "url", Cid: "cid", Csecret: "csecret"} provider := Provider{Name: "test", RedirectURL: "redir"} res := initProvider(params, provider) assert.Equal(t, "cid", res.conf.ClientID) assert.Equal(t, "csecret", res.conf.ClientSecret) assert.Equal(t, "redir", res.RedirectURL) - assert.Equal(t, "123456", res.SecretKey) assert.Equal(t, "test", res.Name) } @@ -160,8 +168,8 @@ func mockProvider(t *testing.T, loginPort, authPort int) (*http.Server, *http.Se }, } - params := Params{RemarkURL: "url", SecretKey: "123456", Cid: "cid", Csecret: "csecret", - JwtService: NewJWT("12345", false, time.Hour, time.Hour*24*31), + params := Params{RemarkURL: "url", Cid: "cid", Csecret: "csecret", + JwtService: NewJWT(keys.NewStaticStore("12345"), false, time.Hour, time.Hour*24*31), // AvatarProxy: &proxy.Avatar{Store: &mockAvatarStore, RoutePath: "/v1/avatar"}, PermissionChecker: &mockUserPermissions{admin: "mock_myuser2", verified: "mock_myuser2", blocked: "mock_myuser1"}, } diff --git a/backend/app/store/keys/keys.go b/backend/app/store/keys/keys.go new file mode 100644 index 00000000..36353a6c --- /dev/null +++ b/backend/app/store/keys/keys.go @@ -0,0 +1,27 @@ +package keys + +import "github.com/pkg/errors" + +// Store defines interface returning key for given site +// this key used for JWT and HMAC hashes +type Store interface { + Get(siteID string) (key string, err error) +} + +// StaticStore implements keys.Store with a single, predefined key +type StaticStore struct { + key string +} + +// NewStaticStore makes StaticStore instance with given key +func NewStaticStore(key string) *StaticStore { + return &StaticStore{key: key} +} + +// Get returns static key for all sites, allows empty site +func (s *StaticStore) Get(siteID string) (key string, err error) { + if s.key == "" { + return "", errors.New("empty key for static key store") + } + return s.key, nil +} diff --git a/backend/app/store/keys/keys_test.go b/backend/app/store/keys/keys_test.go new file mode 100644 index 00000000..c4b47eb2 --- /dev/null +++ b/backend/app/store/keys/keys_test.go @@ -0,0 +1,20 @@ +package keys + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestStaticStore_Get(t *testing.T) { + var ks Store = NewStaticStore("key123") + + k, err := ks.Get("any") + assert.NoError(t, err, "valid store") + assert.Equal(t, "key123", k, "valid site") + + ks = NewStaticStore("") + + _, err = ks.Get("any") + assert.NotNil(t, err, "invalid (empty key) store") +} diff --git a/backend/app/store/service/service.go b/backend/app/store/service/service.go index c7fd97ad..d26d898b 100644 --- a/backend/app/store/service/service.go +++ b/backend/app/store/service/service.go @@ -6,6 +6,7 @@ import ( "github.com/google/uuid" "github.com/pkg/errors" + "github.com/umputun/remark/backend/app/store/keys" "github.com/umputun/remark/backend/app/store" "github.com/umputun/remark/backend/app/store/engine" @@ -15,7 +16,7 @@ import ( type DataStore struct { engine.Interface EditDuration time.Duration - Secret string + KeyStore keys.Store MaxCommentSize int Admins []string @@ -43,8 +44,13 @@ func (s *DataStore) Create(comment store.Comment) (commentID string, err error) comment.Votes = make(map[string]bool) } - comment.Sanitize() // clear potentially dangerous js from all parts of comment - comment.User.HashIP(s.Secret) // replace ip by hash + comment.Sanitize() // clear potentially dangerous js from all parts of comment + + secret, err := s.KeyStore.Get(comment.Locator.SiteID) + if err != nil { + return "", errors.Wrapf(err, "can't get secret for site %s", comment.Locator.SiteID) + } + comment.User.HashIP(secret) // replace ip by hash return s.Interface.Create(comment) } diff --git a/backend/app/store/service/service_test.go b/backend/app/store/service/service_test.go index f5a76f19..590a65ba 100644 --- a/backend/app/store/service/service_test.go +++ b/backend/app/store/service/service_test.go @@ -13,6 +13,7 @@ import ( "github.com/pkg/errors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/umputun/remark/backend/app/store/keys" "github.com/umputun/remark/backend/app/store" "github.com/umputun/remark/backend/app/store/engine" @@ -22,7 +23,8 @@ var testDb = "/tmp/test-remark.db" func TestService_CreateFromEmpty(t *testing.T) { defer os.Remove(testDb) - b := DataStore{Interface: prepStoreEngine(t), Secret: "secret 123"} + ks := keys.NewStaticStore("secret 123") + b := DataStore{Interface: prepStoreEngine(t), KeyStore: ks} comment := store.Comment{ Text: "text", User: store.User{IP: "192.168.1.1", ID: "user", Name: "name"}, @@ -45,7 +47,8 @@ func TestService_CreateFromEmpty(t *testing.T) { func TestService_CreateFromPartial(t *testing.T) { defer os.Remove(testDb) - b := DataStore{Interface: prepStoreEngine(t), Secret: "secret 123"} + ks := keys.NewStaticStore("secret 123") + b := DataStore{Interface: prepStoreEngine(t), KeyStore: ks} comment := store.Comment{ Text: "text", Timestamp: time.Date(2018, 3, 25, 16, 34, 33, 0, time.UTC), @@ -70,7 +73,7 @@ func TestService_CreateFromPartial(t *testing.T) { func TestService_Vote(t *testing.T) { defer os.Remove(testDb) - b := DataStore{Interface: prepStoreEngine(t)} + b := DataStore{Interface: prepStoreEngine(t), KeyStore: keys.NewStaticStore("secret 123")} comment := store.Comment{ Text: "text", @@ -115,7 +118,7 @@ func TestService_Vote(t *testing.T) { func TestService_VoteAggressive(t *testing.T) { defer os.Remove(testDb) - b := DataStore{Interface: prepStoreEngine(t)} + b := DataStore{Interface: prepStoreEngine(t), KeyStore: keys.NewStaticStore("secret 123")} comment := store.Comment{ Text: "text", @@ -175,7 +178,7 @@ func TestService_VoteAggressive(t *testing.T) { func TestService_VoteConcurrent(t *testing.T) { defer os.Remove(testDb) - b := DataStore{Interface: prepStoreEngine(t)} + b := DataStore{Interface: prepStoreEngine(t), KeyStore: keys.NewStaticStore("secret 123")} comment := store.Comment{ Text: "text", @@ -206,7 +209,7 @@ func TestService_VoteConcurrent(t *testing.T) { func TestService_Pin(t *testing.T) { defer os.Remove(testDb) - b := DataStore{Interface: prepStoreEngine(t)} + b := DataStore{Interface: prepStoreEngine(t), KeyStore: keys.NewStaticStore("secret 123")} res, err := b.Last("radio-t", 0) t.Logf("%+v", res[0]) @@ -230,7 +233,7 @@ func TestService_Pin(t *testing.T) { func TestService_EditComment(t *testing.T) { defer os.Remove(testDb) - b := DataStore{Interface: prepStoreEngine(t)} + b := DataStore{Interface: prepStoreEngine(t), KeyStore: keys.NewStaticStore("secret 123")} res, err := b.Last("radio-t", 0) t.Logf("%+v", res[0]) @@ -257,7 +260,7 @@ func TestService_EditComment(t *testing.T) { func TestService_EditCommentDurationFailed(t *testing.T) { defer os.Remove(testDb) - b := DataStore{Interface: prepStoreEngine(t), EditDuration: 100 * time.Millisecond} + b := DataStore{Interface: prepStoreEngine(t), EditDuration: 100 * time.Millisecond, KeyStore: keys.NewStaticStore("secret 123")} res, err := b.Last("radio-t", 0) t.Logf("%+v", res[0]) @@ -274,7 +277,7 @@ func TestService_EditCommentDurationFailed(t *testing.T) { func TestService_ValidateComment(t *testing.T) { - b := DataStore{MaxCommentSize: 2000} + b := DataStore{MaxCommentSize: 2000, KeyStore: keys.NewStaticStore("secret 123")} longText := fmt.Sprintf("%4000s", "X") tbl := []struct {