From 9c798823dfad9232b4bde9d8ec545f82c49cdbcd Mon Sep 17 00:00:00 2001 From: Gregory Newman-Smith Date: Mon, 26 Feb 2024 10:00:47 +0000 Subject: [PATCH 1/4] CBG-3791: change getAuthScopeHandleCreateDB method to not expand environment variables --- rest/admin_api.go | 13 +++++-- rest/adminapitest/admin_api_test.go | 58 +++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 3 deletions(-) diff --git a/rest/admin_api.go b/rest/admin_api.go index 780ebfaa5c..47e5a0ad62 100644 --- a/rest/admin_api.go +++ b/rest/admin_api.go @@ -169,8 +169,8 @@ func (h *handler) handleCreateDB() error { func getAuthScopeHandleCreateDB(ctx context.Context, h *handler) (bucketName string, err error) { // grab a copy of the request body and restore body buffer for later handlers - bodyJSON, err := h.readBody() - if err != nil { + bodyJSON, readErr := h.readBody() + if readErr != nil { return "", base.HTTPErrorf(http.StatusInternalServerError, "Unable to read body: %v", err) } // mark the body as already read to avoid double-counting bytes for stats once it gets read again @@ -181,7 +181,14 @@ func getAuthScopeHandleCreateDB(ctx context.Context, h *handler) (bucketName str Bucket string `json:"bucket"` } reader := bytes.NewReader(bodyJSON) - err = DecodeAndSanitiseConfig(ctx, reader, &dbConfigBody, false) + + // read and decode json to pull bucket name from the config + b, err := io.ReadAll(reader) + if err != nil { + return "", err + } + d := base.JSONDecoder(bytes.NewBuffer(b)) + err = d.Decode(&dbConfigBody) if err != nil { return "", err } diff --git a/rest/adminapitest/admin_api_test.go b/rest/adminapitest/admin_api_test.go index 58561c050a..e73fbea331 100644 --- a/rest/adminapitest/admin_api_test.go +++ b/rest/adminapitest/admin_api_test.go @@ -4015,3 +4015,61 @@ func TestDatabaseCreationErrorCode(t *testing.T) { rest.RequireStatus(t, resp, http.StatusPreconditionFailed) } } + +// TestDatabaseCreationWithEnvVariable: +// - Create rest tester that enables admin auth and disallows db config env vars +// - Create CBS user to authenticate with over admin port to force auth scope callback call +// - Create db with sync function that calls env variable +// - Assert that db is created +func TestDatabaseCreationWithEnvVariable(t *testing.T) { + if base.UnitTestUrlIsWalrus() { + t.Skip("This test only works against Couchbase Server") + } + + tb := base.GetTestBucket(t) + ctx := base.TestCtx(t) + defer tb.Close(ctx) + + // disable AllowDbConfigEnvVars to avoid attempting to expand variables + enable admin auth + rt := rest.NewRestTester(t, &rest.RestTesterConfig{PersistentConfig: true, MutateStartupConfig: func(config *rest.StartupConfig) { + config.Unsupported.AllowDbConfigEnvVars = base.BoolPtr(false) + }, + AdminInterfaceAuthentication: true, + }) + defer rt.Close() + + // create a role to authenticate with in admin endpoint + eps, httpClient, err := rt.ServerContext().ObtainManagementEndpointsAndHTTPClient() + require.NoError(t, err) + rest.MakeUser(t, httpClient, eps[0], rest.MobileSyncGatewayRole.RoleName, "password", []string{fmt.Sprintf("%s[%s]", rest.MobileSyncGatewayRole.RoleName, tb.GetName())}) + defer rest.DeleteUser(t, httpClient, eps[0], rest.MobileSyncGatewayRole.RoleName) + + dataStore1, err := tb.GetNamedDataStore(0) + require.NoError(t, err) + dataStore1Name, ok := base.AsDataStoreName(dataStore1) + require.True(t, ok) + syncFunction := `function (doc) { console.log(\"${environment}\"); return true }` + + // create db config with sync function that has env variable in it + bucketConfig := fmt.Sprintf( + `{"bucket": "%s", + "scopes": { + "%s": { + "collections": { + "%s": { + "sync": "%s" + } + } + } + }, + "num_index_replicas": 0, + "enable_shared_bucket_access": true, + "use_views": false}`, + tb.GetName(), dataStore1Name.ScopeName(), dataStore1Name.CollectionName(), + syncFunction, + ) + + // create db with config and assert it is successful + resp := rt.SendAdminRequestWithAuth("PUT", "/db/", bucketConfig, rest.MobileSyncGatewayRole.RoleName, "password") + rest.RequireStatus(t, resp, http.StatusCreated) +} From fd5f2a98e4110901988997dddb2f2b9739ceea73 Mon Sep 17 00:00:00 2001 From: Gregory Newman-Smith Date: Mon, 26 Feb 2024 16:48:01 +0000 Subject: [PATCH 2/4] updates after review --- rest/admin_api.go | 12 ++++++- rest/adminapitest/admin_api_test.go | 55 +++++++++++++++++++++++++++-- rest/api_test.go | 2 +- rest/config.go | 4 +-- rest/config_legacy.go | 2 +- rest/config_startup.go | 2 +- 6 files changed, 69 insertions(+), 8 deletions(-) diff --git a/rest/admin_api.go b/rest/admin_api.go index 47e5a0ad62..cc31b2e3e2 100644 --- a/rest/admin_api.go +++ b/rest/admin_api.go @@ -171,7 +171,7 @@ func getAuthScopeHandleCreateDB(ctx context.Context, h *handler) (bucketName str // grab a copy of the request body and restore body buffer for later handlers bodyJSON, readErr := h.readBody() if readErr != nil { - return "", base.HTTPErrorf(http.StatusInternalServerError, "Unable to read body: %v", err) + return "", base.HTTPErrorf(http.StatusInternalServerError, "Unable to read body: %v", readErr) } // mark the body as already read to avoid double-counting bytes for stats once it gets read again h.requestBody.bodyRead = true @@ -187,6 +187,16 @@ func getAuthScopeHandleCreateDB(ctx context.Context, h *handler) (bucketName str if err != nil { return "", err } + + // Expand environment variables if needed + if base.BoolDefault(h.server.Config.Unsupported.AllowDbConfigEnvVars, true) { + b, err = expandEnv(h.ctx(), b) + if err != nil { + return "", err + } + } + b = base.ConvertBackQuotedStrings(b) + d := base.JSONDecoder(bytes.NewBuffer(b)) err = d.Decode(&dbConfigBody) if err != nil { diff --git a/rest/adminapitest/admin_api_test.go b/rest/adminapitest/admin_api_test.go index e73fbea331..6727bfc8d3 100644 --- a/rest/adminapitest/admin_api_test.go +++ b/rest/adminapitest/admin_api_test.go @@ -4052,7 +4052,7 @@ func TestDatabaseCreationWithEnvVariable(t *testing.T) { // create db config with sync function that has env variable in it bucketConfig := fmt.Sprintf( - `{"bucket": "%s", + `{"bucket": `+"`"+"%s"+"`"+`, "scopes": { "%s": { "collections": { @@ -4070,6 +4070,57 @@ func TestDatabaseCreationWithEnvVariable(t *testing.T) { ) // create db with config and assert it is successful - resp := rt.SendAdminRequestWithAuth("PUT", "/db/", bucketConfig, rest.MobileSyncGatewayRole.RoleName, "password") + resp := rt.SendAdminRequestWithAuth(http.MethodPut, "/db/", bucketConfig, rest.MobileSyncGatewayRole.RoleName, "password") + rest.RequireStatus(t, resp, http.StatusCreated) +} + +func TestDatabaseCreationWithEnvVariableWithBackticks(t *testing.T) { + if base.UnitTestUrlIsWalrus() { + t.Skip("This test only works against Couchbase Server") + } + + tb := base.GetTestBucket(t) + ctx := base.TestCtx(t) + defer tb.Close(ctx) + + // disable AllowDbConfigEnvVars to avoid attempting to expand variables + enable admin auth + rt := rest.NewRestTester(t, &rest.RestTesterConfig{PersistentConfig: true, MutateStartupConfig: func(config *rest.StartupConfig) { + config.Unsupported.AllowDbConfigEnvVars = base.BoolPtr(false) + }, + AdminInterfaceAuthentication: true, + }) + defer rt.Close() + + // create a role to authenticate with in admin endpoint + eps, httpClient, err := rt.ServerContext().ObtainManagementEndpointsAndHTTPClient() + require.NoError(t, err) + rest.MakeUser(t, httpClient, eps[0], rest.MobileSyncGatewayRole.RoleName, "password", []string{fmt.Sprintf("%s[%s]", rest.MobileSyncGatewayRole.RoleName, tb.GetName())}) + defer rest.DeleteUser(t, httpClient, eps[0], rest.MobileSyncGatewayRole.RoleName) + + dataStore1, err := tb.GetNamedDataStore(0) + require.NoError(t, err) + dataStore1Name, ok := base.AsDataStoreName(dataStore1) + require.True(t, ok) + syncFunction := `function (doc) { console.log(\"${environment}\"); return true }` + + bucketConfig := fmt.Sprintf( + `{"bucket": "%s", + "scopes": { + "%s": { + "collections": { + "%s": { + "sync": "%s" + } + } + } + }, + "num_index_replicas": 0, + "enable_shared_bucket_access": true, + "use_views": false}`, + tb.GetName(), dataStore1Name.ScopeName(), dataStore1Name.CollectionName(), + syncFunction, + ) + // create db with config and assert it is successful + resp := rt.SendAdminRequestWithAuth(http.MethodPut, "/backticks/", bucketConfig, rest.MobileSyncGatewayRole.RoleName, "password") rest.RequireStatus(t, resp, http.StatusCreated) } diff --git a/rest/api_test.go b/rest/api_test.go index 7d9fe5c3c8..727613c5dd 100644 --- a/rest/api_test.go +++ b/rest/api_test.go @@ -1437,7 +1437,7 @@ func TestEventConfigValidationInvalid(t *testing.T) { buf := bytes.NewBufferString(dbConfigJSON) var dbConfig DbConfig - err := DecodeAndSanitiseConfig(base.TestCtx(t), buf, &dbConfig, true) + err := DecodeAndSanitiseStartupConfig(base.TestCtx(t), buf, &dbConfig, true) require.Error(t, err) assert.Contains(t, err.Error(), "document_scribbled_on") } diff --git a/rest/config.go b/rest/config.go index f39347d30a..1d6e00f4ab 100644 --- a/rest/config.go +++ b/rest/config.go @@ -1134,8 +1134,8 @@ func (config *DbConfig) redactInPlace(ctx context.Context) error { return nil } -// DecodeAndSanitiseConfig will sanitise a config from an io.Reader and unmarshal it into the given config parameter. -func DecodeAndSanitiseConfig(ctx context.Context, r io.Reader, config interface{}, disallowUnknownFields bool) (err error) { +// DecodeAndSanitiseStartupConfig will sanitise a config from an io.Reader and unmarshal it into the given config parameter. +func DecodeAndSanitiseStartupConfig(ctx context.Context, r io.Reader, config interface{}, disallowUnknownFields bool) (err error) { b, err := io.ReadAll(r) if err != nil { return err diff --git a/rest/config_legacy.go b/rest/config_legacy.go index 7c9dda2efb..71a8cad27c 100644 --- a/rest/config_legacy.go +++ b/rest/config_legacy.go @@ -370,7 +370,7 @@ func LoadLegacyServerConfig(ctx context.Context, path string) (config *LegacySer // readLegacyServerConfig returns a validated LegacyServerConfig from an io.Reader func readLegacyServerConfig(ctx context.Context, r io.Reader) (config *LegacyServerConfig, err error) { - err = DecodeAndSanitiseConfig(ctx, r, &config, true) + err = DecodeAndSanitiseStartupConfig(ctx, r, &config, true) if err != nil { return config, err } diff --git a/rest/config_startup.go b/rest/config_startup.go index 1d97118dc6..b2868d1d9d 100644 --- a/rest/config_startup.go +++ b/rest/config_startup.go @@ -217,7 +217,7 @@ func LoadStartupConfigFromPath(ctx context.Context, path string) (*StartupConfig defer func() { _ = rc.Close() }() var sc StartupConfig - err = DecodeAndSanitiseConfig(ctx, rc, &sc, true) + err = DecodeAndSanitiseStartupConfig(ctx, rc, &sc, true) return &sc, err } From 6ecb17fed3974149c3618611b516ed778b704f8e Mon Sep 17 00:00:00 2001 From: Gregory Newman-Smith Date: Thu, 29 Feb 2024 16:02:34 +0000 Subject: [PATCH 3/4] updates off review --- rest/admin_api.go | 15 ++------------- rest/config.go | 20 ++++++++++++++++++-- rest/handler.go | 13 +++---------- 3 files changed, 23 insertions(+), 25 deletions(-) diff --git a/rest/admin_api.go b/rest/admin_api.go index cc31b2e3e2..c36c22b163 100644 --- a/rest/admin_api.go +++ b/rest/admin_api.go @@ -180,24 +180,13 @@ func getAuthScopeHandleCreateDB(ctx context.Context, h *handler) (bucketName str var dbConfigBody struct { Bucket string `json:"bucket"` } - reader := bytes.NewReader(bodyJSON) - // read and decode json to pull bucket name from the config - b, err := io.ReadAll(reader) + bodyJSON, err = sanitiseConfig(ctx, bodyJSON, h.server.Config.Unsupported.AllowDbConfigEnvVars) if err != nil { return "", err } - // Expand environment variables if needed - if base.BoolDefault(h.server.Config.Unsupported.AllowDbConfigEnvVars, true) { - b, err = expandEnv(h.ctx(), b) - if err != nil { - return "", err - } - } - b = base.ConvertBackQuotedStrings(b) - - d := base.JSONDecoder(bytes.NewBuffer(b)) + d := base.JSONDecoder(bytes.NewBuffer(bodyJSON)) err = d.Decode(&dbConfigBody) if err != nil { return "", err diff --git a/rest/config.go b/rest/config.go index 1d6e00f4ab..b3372a63bd 100644 --- a/rest/config.go +++ b/rest/config.go @@ -1142,11 +1142,10 @@ func DecodeAndSanitiseStartupConfig(ctx context.Context, r io.Reader, config int } // Expand environment variables. - b, err = expandEnv(ctx, b) + b, err = sanitiseConfig(ctx, b, base.BoolPtr(true)) if err != nil { return err } - b = base.ConvertBackQuotedStrings(b) d := base.JSONDecoder(bytes.NewBuffer(b)) if disallowUnknownFields { @@ -1156,6 +1155,23 @@ func DecodeAndSanitiseStartupConfig(ctx context.Context, r io.Reader, config int return base.WrapJSONUnknownFieldErr(err) } +// sanitiseConfig will expand environment variables if needed and will convert any back quotes in the config +func sanitiseConfig(ctx context.Context, configBytes []byte, allowEnvVars *bool) ([]byte, error) { + var err error + // Expand environment variables if needed + if base.BoolDefault(allowEnvVars, true) { + configBytes, err = expandEnv(ctx, configBytes) + if err != nil { + return nil, err + } + } + // Convert the back quotes into double-quotes, escapes literal + // backslashes, newlines or double-quotes with backslashes. + configBytes = base.ConvertBackQuotedStrings(configBytes) + + return configBytes, nil +} + // expandEnv replaces $var or ${var} in config according to the values of the // current environment variables. The replacement is case-sensitive. References // to undefined variables will result in an error. A default value can diff --git a/rest/handler.go b/rest/handler.go index e197c1eb6d..792e3a5bf3 100644 --- a/rest/handler.go +++ b/rest/handler.go @@ -1090,18 +1090,11 @@ func (h *handler) readSanitizeJSON(val interface{}) error { return err } - // Expand environment variables. - if base.BoolDefault(h.server.Config.Unsupported.AllowDbConfigEnvVars, true) { - content, err = expandEnv(h.ctx(), content) - if err != nil { - return err - } + content, err = sanitiseConfig(h.ctx(), content, h.server.Config.Unsupported.AllowDbConfigEnvVars) + if err != nil { + return err } - // Convert the back quotes into double-quotes, escapes literal - // backslashes, newlines or double-quotes with backslashes. - content = base.ConvertBackQuotedStrings(content) - // Decode the body bytes into target structure. decoder := base.JSONDecoder(bytes.NewReader(content)) decoder.DisallowUnknownFields() From 4dcb4a3c3cc88876d9cf4a49004cecf1a48842bd Mon Sep 17 00:00:00 2001 From: Gregory Newman-Smith Date: Fri, 1 Mar 2024 14:53:35 +0000 Subject: [PATCH 4/4] fix test for default collection --- rest/adminapitest/admin_api_test.go | 77 +++++++++-------------------- 1 file changed, 24 insertions(+), 53 deletions(-) diff --git a/rest/adminapitest/admin_api_test.go b/rest/adminapitest/admin_api_test.go index 6727bfc8d3..63576a829c 100644 --- a/rest/adminapitest/admin_api_test.go +++ b/rest/adminapitest/admin_api_test.go @@ -4031,10 +4031,14 @@ func TestDatabaseCreationWithEnvVariable(t *testing.T) { defer tb.Close(ctx) // disable AllowDbConfigEnvVars to avoid attempting to expand variables + enable admin auth - rt := rest.NewRestTester(t, &rest.RestTesterConfig{PersistentConfig: true, MutateStartupConfig: func(config *rest.StartupConfig) { - config.Unsupported.AllowDbConfigEnvVars = base.BoolPtr(false) - }, + rt := rest.NewRestTester(t, &rest.RestTesterConfig{ + PersistentConfig: true, + MutateStartupConfig: func(config *rest.StartupConfig) { + config.Unsupported.AllowDbConfigEnvVars = base.BoolPtr(false) + }, AdminInterfaceAuthentication: true, + SyncFn: `function (doc) { console.log("${environment}"); return true }`, + CustomTestBucket: tb, }) defer rt.Close() @@ -4044,33 +4048,12 @@ func TestDatabaseCreationWithEnvVariable(t *testing.T) { rest.MakeUser(t, httpClient, eps[0], rest.MobileSyncGatewayRole.RoleName, "password", []string{fmt.Sprintf("%s[%s]", rest.MobileSyncGatewayRole.RoleName, tb.GetName())}) defer rest.DeleteUser(t, httpClient, eps[0], rest.MobileSyncGatewayRole.RoleName) - dataStore1, err := tb.GetNamedDataStore(0) + cfg := rt.NewDbConfig() + input, err := base.JSONMarshal(&cfg) require.NoError(t, err) - dataStore1Name, ok := base.AsDataStoreName(dataStore1) - require.True(t, ok) - syncFunction := `function (doc) { console.log(\"${environment}\"); return true }` - - // create db config with sync function that has env variable in it - bucketConfig := fmt.Sprintf( - `{"bucket": `+"`"+"%s"+"`"+`, - "scopes": { - "%s": { - "collections": { - "%s": { - "sync": "%s" - } - } - } - }, - "num_index_replicas": 0, - "enable_shared_bucket_access": true, - "use_views": false}`, - tb.GetName(), dataStore1Name.ScopeName(), dataStore1Name.CollectionName(), - syncFunction, - ) // create db with config and assert it is successful - resp := rt.SendAdminRequestWithAuth(http.MethodPut, "/db/", bucketConfig, rest.MobileSyncGatewayRole.RoleName, "password") + resp := rt.SendAdminRequestWithAuth(http.MethodPut, "/db/", string(input), rest.MobileSyncGatewayRole.RoleName, "password") rest.RequireStatus(t, resp, http.StatusCreated) } @@ -4084,10 +4067,14 @@ func TestDatabaseCreationWithEnvVariableWithBackticks(t *testing.T) { defer tb.Close(ctx) // disable AllowDbConfigEnvVars to avoid attempting to expand variables + enable admin auth - rt := rest.NewRestTester(t, &rest.RestTesterConfig{PersistentConfig: true, MutateStartupConfig: func(config *rest.StartupConfig) { - config.Unsupported.AllowDbConfigEnvVars = base.BoolPtr(false) - }, + rt := rest.NewRestTester(t, &rest.RestTesterConfig{ + PersistentConfig: true, + MutateStartupConfig: func(config *rest.StartupConfig) { + config.Unsupported.AllowDbConfigEnvVars = base.BoolPtr(false) + }, AdminInterfaceAuthentication: true, + SyncFn: `function (doc) { console.log("${environment}"); return true }`, + CustomTestBucket: tb, }) defer rt.Close() @@ -4097,30 +4084,14 @@ func TestDatabaseCreationWithEnvVariableWithBackticks(t *testing.T) { rest.MakeUser(t, httpClient, eps[0], rest.MobileSyncGatewayRole.RoleName, "password", []string{fmt.Sprintf("%s[%s]", rest.MobileSyncGatewayRole.RoleName, tb.GetName())}) defer rest.DeleteUser(t, httpClient, eps[0], rest.MobileSyncGatewayRole.RoleName) - dataStore1, err := tb.GetNamedDataStore(0) + cfg := rt.NewDbConfig() + input, err := base.JSONMarshal(&cfg) require.NoError(t, err) - dataStore1Name, ok := base.AsDataStoreName(dataStore1) - require.True(t, ok) - syncFunction := `function (doc) { console.log(\"${environment}\"); return true }` - - bucketConfig := fmt.Sprintf( - `{"bucket": "%s", - "scopes": { - "%s": { - "collections": { - "%s": { - "sync": "%s" - } - } - } - }, - "num_index_replicas": 0, - "enable_shared_bucket_access": true, - "use_views": false}`, - tb.GetName(), dataStore1Name.ScopeName(), dataStore1Name.CollectionName(), - syncFunction, - ) + + // change config to include backticks + cfg.Bucket = base.StringPtr(fmt.Sprintf("`"+"%s"+"`", tb.GetName())) + // create db with config and assert it is successful - resp := rt.SendAdminRequestWithAuth(http.MethodPut, "/backticks/", bucketConfig, rest.MobileSyncGatewayRole.RoleName, "password") + resp := rt.SendAdminRequestWithAuth(http.MethodPut, "/backticks/", string(input), rest.MobileSyncGatewayRole.RoleName, "password") rest.RequireStatus(t, resp, http.StatusCreated) }