Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CBG-3791: change getAuthScopeHandleCreateDB method to not expand environment variables #6703

Merged
merged 4 commits into from
Mar 5, 2024
Merged

CBG-3791: change getAuthScopeHandleCreateDB method to not expand environment variables #6703

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9c79882
CBG-3791: change getAuthScopeHandleCreateDB method to not expand envi…
gregns1 Feb 26, 2024
fd5f2a9
updates after review
gregns1 Feb 26, 2024
6ecb17f
updates off review
gregns1 Feb 29, 2024
4dcb4a3
fix test for default collection
gregns1 Mar 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions rest/admin_api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,9 @@ 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 {
return "", base.HTTPErrorf(http.StatusInternalServerError, "Unable to read body: %v", err)
bodyJSON, readErr := h.readBody()
if readErr != nil {
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
Expand All @@ -181,7 +181,24 @@ 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can call readSantizedJson here.

The thing that might happen is that you need to call ConvertBackQuotedStrings to make it valid json. The test should include a config with backquotes

b, err := io.ReadAll(reader)
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're doing bytes->reader->bytes for no reason here with NewReader->ReadAll

We should be able to pass bodyJSON straight into expandEnv and ConvertBackQuotedStrings, since they don't modify the slice, they return a modified copy of it.

It does feels like we need to push those two pieces (expandEnv/ConvertBackQuotedStrings) into a shared function called by this, DecodeAndSanitiseConfig, and readSanitizeJSON.

We can pass a bool in to that function control env var behaviour and just have it take bytes and return bytes.

if err != nil {
return "", err
}
}
b = base.ConvertBackQuotedStrings(b)

d := base.JSONDecoder(bytes.NewBuffer(b))
err = d.Decode(&dbConfigBody)
if err != nil {
return "", err
}
Expand Down
109 changes: 109 additions & 0 deletions rest/adminapitest/admin_api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4015,3 +4015,112 @@ 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't work for SG_TEST_USE_GSI=false or SG_TEST_USE_DEFAULT_COLLECTION=true

Will need to build the db config to accommodate those modes.

Some existing functions to consider using or adapting:

  • dbConfigForTestBucket
  • makeDbConfig

Should be able to include string values like "${VAR}" for sync function before marshalling it to pass into the request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow didn't even think about that, on it fixing it

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}`,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might fail if SG_TEST_USE_XATTRS=false is set, can you check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked and no failure.

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")
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)
}
2 changes: 1 addition & 1 deletion rest/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down
4 changes: 2 additions & 2 deletions rest/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion rest/config_legacy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion rest/config_startup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
Loading