Skip to content

CBG-4687 have sgcollect_info use a token #7596

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

Closed
wants to merge 1 commit into from
Closed

CBG-4687 have sgcollect_info use a token #7596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 5 additions & 9 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,17 +173,13 @@ jobs:
os: [macos-latest, windows-latest, ubuntu-latest]
steps:
- uses: actions/checkout@v4
- name: Set up python
uses: actions/setup-python@v5
with:
python-version: 3.9
- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install pytest pytest-github-actions-annotate-failures pytest-httpserver trustme
- uses: astral-sh/setup-uv@v6
- name: Run test
run: |
pytest
uv run -- pytest
- name: Run mypy
run: |
uv run -- mypy tools/sgcollect_info tools
Copy link
Member

Choose a reason for hiding this comment

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

I am not super keen on changing dependency management for the sake of it but if we do want to do it - again we should do it independent of feature PRs.


test-stats-definition-exporter:
runs-on: ${{ matrix.os }}
Expand Down
17 changes: 17 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,22 @@
[project]
name = "sgcollect_info"
version = "0.0.1"
requires-python = ">3.9,<3.10"

[mypy]
files = ["tools-tests/*.py"]
mypy_path = "$MYPY_CONFIG_FILE_DIR/tools"

[tool.ruff]
extend-include = ["tools/sgcollect_info"]

[tool.ruff.lint]
extend-select = ["I","B006"] # isort, mutable default argument

[dependency-groups]
dev = [
"mypy>=1.16.1",
"pytest>=8.4.0",
"pytest-httpserver>=1.1.3",
"trustme>=1.2.1",
]
14 changes: 11 additions & 3 deletions rest/admin_api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1673,14 +1673,22 @@ func (h *handler) handleSGCollect() error {
return base.HTTPErrorf(http.StatusBadRequest, "Invalid options used for sgcollect_info: %v", multiError)
}

// Populate username and password used by sgcollect_info script for talking to Sync Gateway.
params.syncGatewayUsername, params.syncGatewayPassword = h.getBasicAuth()
addr, err := h.server.getServerAddr(adminServer)
if err != nil {
return base.HTTPErrorf(http.StatusInternalServerError, "Error getting admin server address: %v", err)
}
if h.server.Config.API.HTTPS.TLSCertPath != "" {
addr = "https://" + addr
} else {
params.adminURL = "http://" + addr
}
params.adminURL = addr
Comment on lines +1676 to +1685
Copy link
Member

Choose a reason for hiding this comment

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

Whilst I agree this might be useful to do to avoid the sgcollect_info script iterating to find a working localhost URL as it does today, this is not related to the token feature and we should evaluate and implement each separately.

Copy link
Member

Choose a reason for hiding this comment

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

Let's try this URL and fall back to localhost

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved this into #7601 to be able to write the tests


zipFilename := sgcollectFilename()

logFilePath := h.server.Config.Logging.LogFilePath

if err := sgcollectInstance.Start(logFilePath, h.serialNumber, zipFilename, params); err != nil {
if err := sgcollectInstance.Start(logFilePath, h.serialNumber, zipFilename, params, h.server.sgcollectCookies); err != nil {
return base.HTTPErrorf(http.StatusInternalServerError, "Error running sgcollect_info: %v", err)
}

Expand Down
18 changes: 17 additions & 1 deletion rest/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ type handler struct {
httpLogLevel *base.LogLevel // if set, always log HTTP information at this level, instead of the default
rqCtx context.Context
serverType serverType
sgcollect bool // is called by sgcollect
}

type authScopeFunc func(context.Context, *handler) (string, error)
Expand Down Expand Up @@ -183,6 +184,7 @@ type handlerOptions struct {
skipLogDuration bool // if true, will skip logging HTTP response status/duration
authScopeFunc authScopeFunc // if set, this callback function will be used to set the auth scope for a given request body
httpLogLevel *base.LogLevel // if set, log HTTP requests to this handler at this level, instead of the usual info level
sgcollect bool // if true, this handler is being invoked as part of sgcollect
}

func newHandler(server *ServerContext, privs handlerPrivs, serverType serverType, r http.ResponseWriter, rq *http.Request, options handlerOptions) *handler {
Expand All @@ -199,6 +201,7 @@ func newHandler(server *ServerContext, privs handlerPrivs, serverType serverType
authScopeFunc: options.authScopeFunc,
httpLogLevel: options.httpLogLevel,
serverType: serverType,
sgcollect: options.sgcollect,
}

// initialize h.rqCtx
Expand Down Expand Up @@ -289,6 +292,19 @@ func (h *handler) invoke(method handlerMethod, accessPermissions []Permission, r
return method(h) // Call the actual handler code
}

// shouldCheckAdminRBAC returns true if the request needs to check the server for permissions to run
func (h *handler) shouldCheckAdminRBAC() bool {
adminAPIToken := h.getBearerToken()
if h.sgcollect && h.server.sgcollectCookies.isValidToken(adminAPIToken) {
return false
} else if h.privs == adminPrivs && *h.server.Config.API.AdminInterfaceAuthentication {
return true
} else if h.privs == metricsPrivs && *h.server.Config.API.MetricsInterfaceAuthentication {
return true
}
return false
}

// validateAndWriteHeaders sets up handler.db and validates the permission of the user and returns an error if there is not permission.
func (h *handler) validateAndWriteHeaders(method handlerMethod, accessPermissions []Permission, responsePermissions []Permission) (err error) {
var isRequestLogged bool
Expand Down Expand Up @@ -380,7 +396,7 @@ func (h *handler) validateAndWriteHeaders(method handlerMethod, accessPermission

// If an Admin Request and admin auth enabled or a metrics request with metrics auth enabled we need to check the
// user credentials
shouldCheckAdminAuth := (h.privs == adminPrivs && *h.server.Config.API.AdminInterfaceAuthentication) || (h.privs == metricsPrivs && *h.server.Config.API.MetricsInterfaceAuthentication)
shouldCheckAdminAuth := h.shouldCheckAdminRBAC()

// If admin/metrics endpoint but auth not enabled, set admin_noauth log ctx
if !shouldCheckAdminAuth && (h.serverType == adminServer || h.serverType == metricsServer) {
Expand Down
40 changes: 22 additions & 18 deletions rest/routing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"strconv"
"strings"

"github.com/couchbase/sync_gateway/base"
"github.com/gorilla/mux"
)

Expand Down Expand Up @@ -216,7 +217,7 @@ func CreateAdminRouter(sc *ServerContext) *mux.Router {
dbr.Handle("/_replicationStatus/{replicationID}",
makeHandler(sc, adminPrivs, []Permission{PermWriteReplications}, nil, (*handler).putReplicationStatus)).Methods("PUT")
dbr.Handle("/_config",
makeOfflineHandler(sc, adminPrivs, []Permission{PermUpdateDb}, nil, (*handler).handleGetDbConfig)).Methods("GET")
makeHandlerWithOptions(sc, adminPrivs, []Permission{PermUpdateDb}, nil, (*handler).handleGetDbConfig, handlerOptions{runOffline: true, sgcollect: true})).Methods("GET")
dbr.Handle("/_config",
makeOfflineHandler(sc, adminPrivs, []Permission{PermUpdateDb, PermConfigureSyncFn, PermConfigureAuth}, []Permission{PermUpdateDb, PermConfigureSyncFn, PermConfigureAuth}, (*handler).handlePutDbConfig)).Methods("PUT", "POST")

Expand Down Expand Up @@ -260,15 +261,15 @@ func CreateAdminRouter(sc *ServerContext) *mux.Router {
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handleSetLogging)).Methods("PUT", "POST")

r.Handle("/_config",
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handleGetConfig)).Methods("GET")
makeHandlerWithOptions(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handleGetConfig, handlerOptions{sgcollect: true})).Methods("GET")
r.Handle("/_config",
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePutConfig)).Methods("PUT")

r.Handle("/_cluster_info",
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handleGetClusterInfo)).Methods("GET")
makeHandlerWithOptions(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handleGetClusterInfo, handlerOptions{sgcollect: true})).Methods("GET")

r.Handle("/_status",
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handleGetStatus)).Methods("GET")
makeHandlerWithOptions(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handleGetStatus, handlerOptions{sgcollect: true})).Methods("GET")

r.Handle("/_sgcollect_info",
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handleSGCollectStatus)).Methods("GET")
Expand All @@ -281,33 +282,36 @@ func CreateAdminRouter(sc *ServerContext) *mux.Router {
r.Handle("/_stats",
makeHandler(sc, adminPrivs, []Permission{PermStatsExport}, nil, (*handler).handleStats)).Methods("GET")
r.Handle(kDebugURLPathPrefix,
makeSilentHandler(sc, adminPrivs, []Permission{PermStatsExport}, nil, (*handler).handleExpvar)).Methods("GET")
makeHandlerWithOptions(sc, adminPrivs, []Permission{PermStatsExport}, nil, (*handler).handleExpvar, handlerOptions{
httpLogLevel: base.Ptr(base.LevelDebug), // silent handler
sgcollect: true,
})).Methods("GET")
r.Handle("/_profile/{profilename}",
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handleProfiling)).Methods("POST")
makeHandlerWithOptions(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handleProfiling, handlerOptions{sgcollect: true})).Methods("POST")
r.Handle("/_profile",
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handleProfiling)).Methods("POST")
makeHandlerWithOptions(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handleProfiling, handlerOptions{sgcollect: true})).Methods("POST")
r.Handle("/_heap",
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handleHeapProfiling)).Methods("POST")
makeHandlerWithOptions(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handleHeapProfiling, handlerOptions{sgcollect: true})).Methods("POST")
r.Handle("/_debug/pprof/goroutine",
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePprofGoroutine)).Methods("GET", "POST")
makeHandlerWithOptions(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePprofGoroutine, handlerOptions{sgcollect: true})).Methods("GET", "POST")
r.Handle("/_debug/pprof/cmdline",
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePprofCmdline)).Methods("GET", "POST")
makeHandlerWithOptions(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePprofCmdline, handlerOptions{sgcollect: true})).Methods("GET", "POST")
r.Handle("/_debug/pprof/symbol",
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePprofSymbol)).Methods("GET", "POST")
makeHandlerWithOptions(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePprofSymbol, handlerOptions{sgcollect: true})).Methods("GET", "POST")
r.Handle("/_debug/pprof/heap",
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePprofHeap)).Methods("GET", "POST")
makeHandlerWithOptions(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePprofHeap, handlerOptions{sgcollect: true})).Methods("GET", "POST")
r.Handle("/_debug/pprof/profile",
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePprofProfile)).Methods("GET", "POST")
makeHandlerWithOptions(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePprofProfile, handlerOptions{sgcollect: true})).Methods("GET", "POST")
r.Handle("/_debug/pprof/block",
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePprofBlock)).Methods("GET", "POST")
makeHandlerWithOptions(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePprofBlock, handlerOptions{sgcollect: true})).Methods("GET", "POST")
r.Handle("/_debug/pprof/threadcreate",
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePprofThreadcreate)).Methods("GET", "POST")
makeHandlerWithOptions(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePprofThreadcreate, handlerOptions{sgcollect: true})).Methods("GET", "POST")
r.Handle("/_debug/pprof/mutex",
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePprofMutex)).Methods("GET", "POST")
makeHandlerWithOptions(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePprofMutex, handlerOptions{sgcollect: true})).Methods("GET", "POST")
r.Handle("/_debug/pprof/trace",
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePprofTrace)).Methods("GET", "POST")
makeHandlerWithOptions(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePprofTrace, handlerOptions{sgcollect: true})).Methods("GET", "POST")
r.Handle("/_debug/fgprof",
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handleFgprof)).Methods("GET", "POST")
makeHandlerWithOptions(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handleFgprof, handlerOptions{sgcollect: true})).Methods("GET", "POST")

r.Handle("/_post_upgrade",
makeHandler(sc, adminPrivs, []Permission{PermDevOps}, nil, (*handler).handlePostUpgrade)).Methods("POST")
Expand Down
37 changes: 34 additions & 3 deletions rest/server_context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"github.com/KimMachineGun/automemlimit/memlimit"
"github.com/couchbase/sync_gateway/auth"
"github.com/couchbase/sync_gateway/db/functions"
"github.com/google/uuid"
"github.com/shirou/gopsutil/mem"

"github.com/couchbase/gocbcore/v10"
Expand Down Expand Up @@ -86,6 +87,7 @@ type ServerContext struct {
DatabaseInitManager *DatabaseInitManager // Manages database initialization (index creation and readiness) independent of database stop/start/reload, when using persistent config
ActiveReplicationsCounter
invalidDatabaseConfigTracking invalidDatabaseConfigs
sgcollectCookies *sgcollectCookieManager
}

type ActiveReplicationsCounter struct {
Expand Down Expand Up @@ -163,9 +165,10 @@ func NewServerContext(ctx context.Context, config *StartupConfig, persistentConf
BootstrapContext: &bootstrapContext{sgVersion: *base.ProductVersion},
hasStarted: make(chan struct{}),
_httpServers: map[serverType]*serverInfo{},
}
sc.invalidDatabaseConfigTracking = invalidDatabaseConfigs{
dbNames: map[string]*invalidConfigInfo{},
invalidDatabaseConfigTracking: invalidDatabaseConfigs{
dbNames: map[string]*invalidConfigInfo{},
},
sgcollectCookies: newSgcollectCookieManager(),
}

if base.ServerIsWalrus(sc.Config.Bootstrap.Server) {
Expand Down Expand Up @@ -2277,3 +2280,31 @@ func (sc *ServerContext) getClusterUUID(ctx context.Context) (string, error) {
}
return base.ParseClusterUUID(output)
}

type sgcollectCookieManager struct {
tokens map[string]struct{}
Copy link
Member

Choose a reason for hiding this comment

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

Do we ever expect to support more than 1 valid token at any one time?

AFAIK since /_sgcollect_info is a singleton and has sync mechanisms in place to avoid multiple runs, this isn't and won't be required.

I think this is adding complexity by maintaining a map, since it's more likely that we can unintentionally build up a set of valid/usable tokens.

It might be better with a single token string that has a timestamp to also allow expiry after a period of time. This covers the case where the sgcollect info script hangs indefinitely and keeps the token alive, or any situations where we may be unintentionally not deleting the token from the map.

}

func newSgcollectCookieManager() *sgcollectCookieManager {
return &sgcollectCookieManager{
tokens: make(map[string]struct{}),
}
}

func (m *sgcollectCookieManager) createToken() (string, error) {
u, err := uuid.NewUUID()
if err != nil {
return "", fmt.Errorf("error generating UUID for sgcollect token: %w", err)
}
m.tokens[u.String()] = struct{}{}
return u.String(), nil
}

func (m *sgcollectCookieManager) deleteToken(token string) {
delete(m.tokens, token)
}

func (m *sgcollectCookieManager) isValidToken(token string) bool {
_, exists := m.tokens[token]
return exists
}
Loading
Loading