Merge pull request #7 from jcrood/cfg-session-salt

Add sessionSalt to override hard-coded salt

CHANGELOG.md

@@ -27,6 +27,12 @@ when the IDP uses a self-signed CA. Merged from https://github.com/vmware-archiv
 Adds the `customAssetsDir` config option to override the contents of /assets/ for use in 
 custom templates.
 
+
+### Override hard-coded session salt
+
+Adds the `sessionSalt` config option to override the hard-coded salt. Min. length is 8 characters.
+Fixes https://github.com/vmware-archive/gangway/issues/71
+
 ### todo
 
 ...

cmd/gangway/handlers_test.go

@@ -36,7 +36,7 @@ import (
 )
 
 func testInit() {
-	gangwayUserSession = session.New("test")
+	gangwayUserSession = session.New("test", "0123456789")
 	transportConfig = config.NewTransportConfig("")
 
 	oauth2Cfg = &oauth2.Config{
@@ -434,7 +434,7 @@ func TestUnauthedCommandlineHandlerRedirect(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	session.New("test")
+	session.New("test", "0123456789")
 
 	rr := httptest.NewRecorder()
 	handler := http.HandlerFunc(commandlineHandler)

cmd/gangway/main.go

@@ -86,7 +86,7 @@ func main() {
 	}
 
 	transportConfig = config.NewTransportConfig(cfg.TrustedCAPath)
-	gangwayUserSession = session.New(cfg.SessionSecurityKey)
+	gangwayUserSession = session.New(cfg.SessionSecurityKey, cfg.SessionSalt)
 
 	var assetFs http.FileSystem
 	if cfg.CustomAssetsDir != "" {

docs/README.md

@@ -16,14 +16,17 @@ While this is called a secret, based on the way that OAuth2 works with command l
 This will be divulged to any client that is configured through gangway.
 As such, it is probably acceptable to keep that secret in the config file and not worry about managing it as a true secret.
 
-We also have a secret string that is used to as a way to encrypt the cookies that are returned to the users.
-If using the example YAML, create a secret to hold this value with the following command line:
+We also have a secret string and salt that is used to as a way to encrypt the cookies that are returned 
+to the users. If using the example YAML, create a secret to hold these values with the following command line:
 
 ```
 kubectl -n gangway create secret generic gangway-key \
-  --from-literal=sessionkey=$(openssl rand -base64 32)
+  --from-literal=sessionkey=$(openssl rand -base64 32) \
+  --from-literal=sessionsalt=$(openssl rand -base64 32)
 ```
 
+Setting a salt is optional; if the config variable is not set a default baked in salt will be used.
+
 ## Path Prefix
 
 Gangway takes an optional path prefix if you want to host it at a url other than '/' (e.g. `https://example.com/gangway`).

docs/yaml/03-deployment.yaml

@@ -28,6 +28,11 @@ spec:
             secretKeyRef:
               name: gangway-key
               key: sessionkey
+        - name: GANGWAY_SESSION_SALT
+          valueFrom:
+            secretKeyRef:
+              name: gangway-key
+              key: sessionsalt
         ports:
         - name: http
           containerPort: 8080

internal/config/config.go

@@ -23,6 +23,8 @@ import (
 	"sigs.k8s.io/yaml"
 )
 
+const hardCodedDefaultSalt = "MkmfuPNHnZBBivy0L0aW"
+
 // Config the configuration field for gangway
 type Config struct {
 	Host string `yaml:"host"`
@@ -48,6 +50,7 @@ type Config struct {
 	HTTPPath               string   `yaml:"httpPath" envconfig:"http_path"`
 	ShowClaims             bool     `yaml:"showClaims" envconfig:"show_claims"`
 	SessionSecurityKey     string   `yaml:"sessionSecurityKey" envconfig:"SESSION_SECURITY_KEY"`
+	SessionSalt            string   `yaml:"sessionSalt" envconfig:"SESSION_SALT"`
 	CustomHTMLTemplatesDir string   `yaml:"customHTMLTemplatesDir" envconfig:"custom_html_templates_dir"`
 	CustomAssetsDir        string   `yaml:"customAssetsDir" envconfig:"custom_assets_dir"`
 }
@@ -68,6 +71,7 @@ func NewConfig(configFile string) (*Config, error) {
 		ClusterCAPath:          "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt",
 		HTTPPath:               "",
 		ShowClaims:             false,
+		SessionSalt:            hardCodedDefaultSalt,
 	}
 
 	if configFile != "" {
@@ -111,6 +115,7 @@ func (cfg *Config) Validate() error {
 		{cfg.RedirectURL == "", "no redirectURL specified"},
 		{cfg.SessionSecurityKey == "", "no SessionSecurityKey specified"},
 		{cfg.APIServerURL == "", "no apiServerURL specified"},
+		{len(cfg.SessionSalt) < 8, "salt needs to be min. 8 characters"},
 	}
 
 	for _, check := range checks {

internal/config/config_test.go

@@ -27,6 +27,7 @@ func TestConfigNotFound(t *testing.T) {
 }
 
 func TestEnvionmentOverrides(t *testing.T) {
+	salt := "randombanana"
 	os.Setenv("GANGWAY_AUTHORIZE_URL", "https://foo.bar/authorize")
 	os.Setenv("GANGWAY_APISERVER_URL", "https://k8s-api.foo.baz")
 	os.Setenv("GANGWAY_CLIENT_ID", "foo")
@@ -39,6 +40,7 @@ func TestEnvionmentOverrides(t *testing.T) {
 	os.Setenv("GANGWAY_AUDIENCE", "foo")
 	os.Setenv("GANGWAY_SCOPES", "groups,sub")
 	os.Setenv("GANGWAY_SHOW_CLAIMS", "false")
+	os.Setenv("GANGWAY_SESSION_SALT", salt)
 	cfg, err := NewConfig("")
 	if err != nil {
 		t.Errorf("Failed to test config overrides with error: %s", err)
@@ -59,6 +61,28 @@ func TestEnvionmentOverrides(t *testing.T) {
 	if cfg.ShowClaims != false {
 		t.Errorf("Failed to disable showing of claims. Expected %t but got %t", false, cfg.ShowClaims)
 	}
+	if cfg.SessionSalt != salt {
+		t.Errorf("Failed to override session salt. Expected %s but got %s", salt, cfg.SessionSalt)
+	}
+}
+
+func TestSessionSaltLength(t *testing.T) {
+	salt := "2short"
+	os.Setenv("GANGWAY_AUTHORIZE_URL", "https://foo.bar/authorize")
+	os.Setenv("GANGWAY_APISERVER_URL", "https://k8s-api.foo.baz")
+	os.Setenv("GANGWAY_CLIENT_ID", "foo")
+	os.Setenv("GANGWAY_CLIENT_SECRET", "bar")
+	os.Setenv("GANGWAY_REDIRECT_URL", "https://foo.baz/callback")
+	os.Setenv("GANGWAY_SESSION_SECURITY_KEY", "testing")
+	os.Setenv("GANGWAY_TOKEN_URL", "https://foo.bar/token")
+	os.Setenv("GANGWAY_SESSION_SALT", salt)
+	_, err := NewConfig("")
+	if err == nil {
+		t.Errorf("Expected error but got none")
+	}
+	if err.Error() != "invalid config: salt needs to be min. 8 characters" {
+		t.Errorf("Wrong error. Expected %v but got %v", "salt needs to be min. 8 characters", err)
+	}
 }
 
 func TestGetRootPathPrefix(t *testing.T) {

internal/session/session.go

@@ -22,22 +22,20 @@ import (
 	"golang.org/x/crypto/pbkdf2"
 )
 
-const salt = "MkmfuPNHnZBBivy0L0aW"
-
 // Session defines a Gangway session
 type Session struct {
 	Session *CustomCookieStore
 }
 
 // New inits a Session with CookieStore
-func New(sessionSecurityKey string) *Session {
+func New(sessionSecurityKey string, sessionSalt string) *Session {
 	return &Session{
-		Session: NewCustomCookieStore(generateSessionKeys(sessionSecurityKey)),
+		Session: NewCustomCookieStore(generateSessionKeys(sessionSecurityKey, sessionSalt)),
 	}
 }
 
 // generateSessionKeys creates a signed encryption key for the cookie store
-func generateSessionKeys(sessionSecurityKey string) ([]byte, []byte) {
+func generateSessionKeys(sessionSecurityKey string, salt string) ([]byte, []byte) {
 	// Take the configured security key and generate 96 bytes of data. This is
 	// used as the signing and encryption keys for the cookie store.  For details
 	// on the PBKDF2 function: https://en.wikipedia.org/wiki/PBKDF2

internal/session/session_test.go

@@ -24,7 +24,7 @@ import (
 )
 
 func TestGenerateSessionKeys(t *testing.T) {
-	b1, b2 := generateSessionKeys("testing")
+	b1, b2 := generateSessionKeys("testing", "0123456789")
 
 	if len(b1) != 64 || len(b2) != 32 {
 		t.Errorf("Wrong byte length's returned")
@@ -33,7 +33,7 @@ func TestGenerateSessionKeys(t *testing.T) {
 }
 
 func TestInitSessionStore(t *testing.T) {
-	s := New("testing")
+	s := New("testing", "0123456789")
 	if s.Session == nil {
 		t.Errorf("Session Store is nil. Did not get initialized")
 		return
@@ -42,7 +42,7 @@ func TestInitSessionStore(t *testing.T) {
 }
 
 func TestCleanupSession(t *testing.T) {
-	s := New("testing")
+	s := New("testing", "0123456789")
 	session := &sessions.Session{}
 	// create a test http server
 	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {