artin/sourcegraph
1
0
Fork 0
mirror of https://github.com/sourcegraph/sourcegraph.git synced 2026-02-06 19:21:50 +00:00
Code Issues Packages Projects Releases Wiki Activity

Don't require a specific username for executors auth (#25786)

Browse Source 
There is no added security by also requiring a specific user for the already secret token. This reduces complexity in setup as there is one value less to mess up.
This commit is contained in:
Erik Seliger 2021-10-07 16:15:43 +02:00 committed by GitHub
parent 116359a5a9
commit 2b767a9455
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 30 additions and 71 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
doc/admin/deploy_executors.md
View File

@ -17,13 +17,11 @@ In order for the executors to dequeue and perform work, they must be able to rea
The `frontend` service must define the following environment variables:
- `EXECUTOR_FRONTEND_USERNAME`
- `EXECUTOR_FRONTEND_PASSWORD`
The `executor` service must define the following environment variables:
- `EXECUTOR_FRONTEND_URL`
- `EXECUTOR_FRONTEND_USERNAME`
- `EXECUTOR_FRONTEND_PASSWORD`
When using a Sourcegraph Terraform module to provision executors, the required executor environment variables can be set via:

3
enterprise/cmd/executor/config.go
View File

@ -18,7 +18,6 @@ type Config struct {
env.BaseConfig
FrontendURL string
FrontendUsername string
FrontendPassword string
QueueName string
QueuePollInterval time.Duration
@ -38,7 +37,6 @@ type Config struct {
func (c *Config) Load() {
c.FrontendURL = c.Get("EXECUTOR_FRONTEND_URL", "", "The external URL of the sourcegraph instance.")
c.FrontendUsername = c.Get("EXECUTOR_FRONTEND_USERNAME", "", "The username supplied to the frontend.")
c.FrontendPassword = c.Get("EXECUTOR_FRONTEND_PASSWORD", "", "The password supplied to the frontend.")
c.QueueName = c.Get("EXECUTOR_QUEUE_NAME", "", "The name of the queue to listen to.")
c.QueuePollInterval = c.GetInterval("EXECUTOR_QUEUE_POLL_INTERVAL", "1s", "Interval between dequeue requests.")
@ -131,7 +129,6 @@ func (c *Config) BaseClientOptions() apiclient.BaseClientOptions {
func (c *Config) EndpointOptions() apiclient.EndpointOptions {
return apiclient.EndpointOptions{
URL: c.FrontendURL,
Username: c.FrontendUsername,
Password: c.FrontendPassword,
}
}

8
enterprise/cmd/executor/internal/apiclient/client.go
View File

@ -44,9 +44,6 @@ type EndpointOptions struct {
// URL is the target request URL.
URL string
// Username is the basic-auth username to include with all requests.
Username string
// Password is the basic-auth password to include with all requests.
Password string
}
@ -224,7 +221,6 @@ func (c *Client) Heartbeat(ctx context.Context, queueName string, jobIDs []int)
func (c *Client) makeRequest(method, path string, payload interface{}) (*http.Request, error) {
u, err := makeURL(
c.options.EndpointOptions.URL,
c.options.EndpointOptions.Username,
c.options.EndpointOptions.Password,
c.options.PathPrefix,
path,
@ -236,13 +232,13 @@ func (c *Client) makeRequest(method, path string, payload interface{}) (*http.Re
return MakeJSONRequest(method, u, payload)
}
func makeURL(base, username, password string, path ...string) (*url.URL, error) {
func makeURL(base, password string, path ...string) (*url.URL, error) {
u, err := makeRelativeURL(base, path...)
if err != nil {
return nil, err
}
u.User = url.UserPassword(username, password)
u.User = url.UserPassword("sourcegraph", password)
return u, nil
}

6
enterprise/cmd/executor/internal/apiclient/client_test.go
View File

@ -402,7 +402,6 @@ func testRoute(t *testing.T, spec routeSpec, f func(client *Client)) {
PathPrefix: "/.executors/queue",
EndpointOptions: EndpointOptions{
URL: ts.URL,
Username: "test",
Password: "hunter2",
},
}
@ -418,10 +417,7 @@ func testServer(t *testing.T, spec routeSpec) *httptest.Server {
t.Errorf("unexpected method. want=%s have=%s", spec.expectedPath, r.URL.Path)
}
username, password, _ := r.BasicAuth()
if username != spec.expectedUsername {
t.Errorf("unexpected username. want=%s have=%s", spec.expectedUsername, username)
}
_, password, _ := r.BasicAuth()
if password != spec.expectedPassword {
t.Errorf("unexpected password. want=%s have=%s", spec.expectedPassword, password)
}

5
enterprise/cmd/executor/internal/worker/workspace.go
View File

@ -30,7 +30,6 @@ func (h *handler) prepareWorkspace(ctx context.Context, commandRunner command.Ru
if repositoryName != "" {
cloneURL, err := makeURL(
h.options.ClientOptions.EndpointOptions.URL,
h.options.ClientOptions.EndpointOptions.Username,
h.options.ClientOptions.EndpointOptions.Password,
h.options.GitServicePath,
repositoryName,
@ -59,13 +58,13 @@ func (h *handler) prepareWorkspace(ctx context.Context, commandRunner command.Ru
return tempDir, nil
}
func makeURL(base, username, password string, path ...string) (*url.URL, error) {
func makeURL(base, password string, path ...string) (*url.URL, error) {
u, err := makeRelativeURL(base, path...)
if err != nil {
return nil, err
}
u.User = url.UserPassword(username, password)
u.User = url.UserPassword("sourcegraph", password)
return u, nil
}

3
enterprise/cmd/executor/internal/worker/workspace_test.go
View File

@ -17,7 +17,6 @@ func TestPrepareWorkspace(t *testing.T) {
ClientOptions: apiclient.Options{
EndpointOptions: apiclient.EndpointOptions{
URL: "https://test.io",
Username: "test",
Password: "hunter2",
},
},
@ -46,7 +45,7 @@ func TestPrepareWorkspace(t *testing.T) {
expectedCommands := [][]string{
{"git", "-C", dir, "init"},
{"git", "-C", dir, "-c", "protocol.version=2", "fetch", "https://test:hunter2@test.io/internal/git/torvalds/linux", "-t", "deadbeef"},
{"git", "-C", dir, "-c", "protocol.version=2", "fetch", "https://sourcegraph:hunter2@test.io/internal/git/torvalds/linux", "-t", "deadbeef"},
{"git", "-C", dir, "remote", "add", "origin", "torvalds/linux"},
{"git", "-C", dir, "checkout", "deadbeef"},
}

2
enterprise/cmd/frontend/internal/executorqueue/config/shared.go
View File

@ -8,11 +8,9 @@ import (
type SharedConfig struct {
env.BaseConfig
FrontendUsername string
FrontendPassword string
}
func (c *SharedConfig) Load() {
c.FrontendUsername = c.GetOptional("EXECUTOR_FRONTEND_USERNAME", "The username supplied to the frontend.")
c.FrontendPassword = c.GetOptional("EXECUTOR_FRONTEND_PASSWORD", "The password supplied to the frontend.")
}

10
enterprise/cmd/frontend/internal/executorqueue/queuehandler.go
View File

@ -50,7 +50,8 @@ func newExecutorQueueHandler(queueOptions map[string]handler.QueueOptions, uploa
// in which a shared key exchange can be done so safely.
func basicAuthMiddleware(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
username, password, ok := r.BasicAuth()
// We don't care about the username. Only the password matters here.
_, password, ok := r.BasicAuth()
if !ok {
// This header is required to be present with 401 responses in order to prompt the client
// to retry the request with basic auth credentials. If we do not send this header, the
@ -59,17 +60,12 @@ func basicAuthMiddleware(next http.Handler) http.Handler {
w.WriteHeader(http.StatusUnauthorized)
return
}
if sharedConfig.FrontendUsername == "" {
w.WriteHeader(http.StatusInternalServerError)
log15.Error("invalid value for EXECUTOR_FRONTEND_USERNAME: no value supplied")
return
}
if sharedConfig.FrontendPassword == "" {
w.WriteHeader(http.StatusInternalServerError)
log15.Error("invalid value for EXECUTOR_FRONTEND_PASSWORD: no value supplied")
return
}
if username != sharedConfig.FrontendUsername || password != sharedConfig.FrontendPassword {
if password != sharedConfig.FrontendPassword {
w.WriteHeader(http.StatusForbidden)
return
}

15
enterprise/cmd/frontend/internal/executorqueue/queuehandler_test.go
View File

@ -8,7 +8,6 @@ import (
)
func init() {
sharedConfig.FrontendUsername = "test"
sharedConfig.FrontendPassword = "hunter2"
}
@ -37,18 +36,8 @@ func TestInternalProxyAuthTokenMiddleware(t *testing.T) {
t.Errorf("unexpected www-authenticate header. want=%q have=%q", `Basic realm="Sourcegraph"`, value)
}
// wrong username
req.SetBasicAuth(strings.ToUpper(sharedConfig.FrontendUsername), sharedConfig.FrontendPassword)
resp, err = http.DefaultClient.Do(req)
if err != nil {
t.Fatalf("unexpected error performing request: %s", err)
}
if resp.StatusCode != http.StatusForbidden {
t.Errorf("unexpected status code. want=%d have=%d", http.StatusForbidden, resp.StatusCode)
}
// wrong password
req.SetBasicAuth(sharedConfig.FrontendUsername, strings.ToUpper(sharedConfig.FrontendPassword))
req.SetBasicAuth("anything", strings.ToUpper(sharedConfig.FrontendPassword))
resp, err = http.DefaultClient.Do(req)
if err != nil {
t.Fatalf("unexpected error performing request: %s", err)
@ -58,7 +47,7 @@ func TestInternalProxyAuthTokenMiddleware(t *testing.T) {
}
// correct token
req.SetBasicAuth(sharedConfig.FrontendUsername, sharedConfig.FrontendPassword)
req.SetBasicAuth("anything", sharedConfig.FrontendPassword)
resp, err = http.DefaultClient.Do(req)
if err != nil {
t.Fatalf("unexpected error performing request: %s", err)

9
enterprise/cmd/frontend/internal/executorqueue/queues/batches/transform.go
View File

@ -35,13 +35,13 @@ func createAndAttachInternalAccessToken(ctx context.Context, s batchesStore, job
return token, nil
}
func makeURL(base, username, password string) (string, error) {
func makeURL(base, password string) (string, error) {
u, err := url.Parse(base)
if err != nil {
return "", err
}
u.User = url.UserPassword(username, password)
u.User = url.UserPassword("sourcegraph", password)
return u.String(), nil
}
@ -106,12 +106,12 @@ func transformBatchSpecWorkspaceExecutionJobRecord(ctx context.Context, s batche
frontendURL := conf.Get().ExternalURL
srcEndpoint, err := makeURL(frontendURL, config.Shared.FrontendUsername, config.Shared.FrontendPassword)
srcEndpoint, err := makeURL(frontendURL, config.Shared.FrontendPassword)
if err != nil {
return apiclient.Job{}, err
}
redactedSrcEndpoint, err := makeURL(frontendURL, "USERNAME_REMOVED", "PASSWORD_REMOVED")
redactedSrcEndpoint, err := makeURL(frontendURL, "PASSWORD_REMOVED")
if err != nil {
return apiclient.Job{}, err
}
@ -151,7 +151,6 @@ func transformBatchSpecWorkspaceExecutionJobRecord(ctx context.Context, s batche
// (in src-cli). We only pass the constructed URL to src-cli, which we trust not to
// ship the values to a third party, but not to trust to ensure the values are absent
// from the command's stdout or stderr streams.
config.Shared.FrontendUsername: "USERNAME_REMOVED",
config.Shared.FrontendPassword: "PASSWORD_REMOVED",
// 🚨 SECURITY: Redact the access token used for src-cli to talk to

10
enterprise/cmd/frontend/internal/executorqueue/queues/batches/transform_test.go
View File

@ -41,7 +41,6 @@ func TestTransformBatchSpecWorkspaceExecutionJobRecord(t *testing.T) {
})
config := &Config{
Shared: &config.SharedConfig{
FrontendUsername: "test*",
FrontendPassword: "hunter2",
},
}
@ -109,16 +108,15 @@ func TestTransformBatchSpecWorkspaceExecutionJobRecord(t *testing.T) {
},
Dir: ".",
Env: []string{
"SRC_ENDPOINT=https://test%2A:hunter2@test.io",
"SRC_ENDPOINT=https://sourcegraph:hunter2@test.io",
"SRC_ACCESS_TOKEN=" + accessToken,
},
},
},
RedactedValues: map[string]string{
"https://test%2A:hunter2@test.io": "https://USERNAME_REMOVED:PASSWORD_REMOVED@test.io",
"test*": "USERNAME_REMOVED",
"hunter2": "PASSWORD_REMOVED",
accessToken: "SRC_ACCESS_TOKEN_REMOVED",
"https://sourcegraph:hunter2@test.io": "https://sourcegraph:PASSWORD_REMOVED@test.io",
"hunter2": "PASSWORD_REMOVED",
accessToken: "SRC_ACCESS_TOKEN_REMOVED",
},
}
if diff := cmp.Diff(expected, job); diff != "" {

9
enterprise/cmd/frontend/internal/executorqueue/queues/codeintel/transform.go
View File

@ -36,12 +36,12 @@ func transformRecord(index store.Index, config *Config) (apiclient.Job, error) {
frontendURL := conf.Get().ExternalURL
srcEndpoint, err := makeURL(frontendURL, config.Shared.FrontendUsername, config.Shared.FrontendPassword)
srcEndpoint, err := makeURL(frontendURL, config.Shared.FrontendPassword)
if err != nil {
return apiclient.Job{}, err
}
redactedSrcEndpoint, err := makeURL(frontendURL, "USERNAME_REMOVED", "PASSWORD_REMOVED")
redactedSrcEndpoint, err := makeURL(frontendURL, "PASSWORD_REMOVED")
if err != nil {
return apiclient.Job{}, err
}
@ -89,18 +89,17 @@ func transformRecord(index store.Index, config *Config) (apiclient.Job, error) {
// (in src-cli). We only pass the constructed URL to src-cli, which we trust not to
// ship the values to a third party, but not to trust to ensure the values are absent
// from the command's stdout or stderr streams.
config.Shared.FrontendUsername: "USERNAME_REMOVED",
config.Shared.FrontendPassword: "PASSWORD_REMOVED",
},
}, nil
}
func makeURL(base, username, password string) (string, error) {
func makeURL(base, password string) (string, error) {
u, err := url.Parse(base)
if err != nil {
return "", err
}
u.User = url.UserPassword(username, password)
u.User = url.UserPassword("sourcegraph", password)
return u.String(), nil
}

16
enterprise/cmd/frontend/internal/executorqueue/queues/codeintel/transform_test.go
View File

@ -35,7 +35,6 @@ func TestTransformRecord(t *testing.T) {
})
config := &Config{
Shared: &config.SharedConfig{
FrontendUsername: "test*",
FrontendPassword: "hunter2",
},
}
@ -75,13 +74,12 @@ func TestTransformRecord(t *testing.T) {
"-associated-index-id", "42",
},
Dir: "web",
Env: []string{"SRC_ENDPOINT=https://test%2A:hunter2@test.io"},
Env: []string{"SRC_ENDPOINT=https://sourcegraph:hunter2@test.io"},
},
},
RedactedValues: map[string]string{
"https://test%2A:hunter2@test.io": "https://USERNAME_REMOVED:PASSWORD_REMOVED@test.io",
"test*": "USERNAME_REMOVED",
"hunter2": "PASSWORD_REMOVED",
"https://sourcegraph:hunter2@test.io": "https://sourcegraph:PASSWORD_REMOVED@test.io",
"hunter2": "PASSWORD_REMOVED",
},
}
if diff := cmp.Diff(expected, job); diff != "" {
@ -117,7 +115,6 @@ func TestTransformRecordWithoutIndexer(t *testing.T) {
})
config := &Config{
Shared: &config.SharedConfig{
FrontendUsername: "test*",
FrontendPassword: "hunter2",
},
}
@ -157,13 +154,12 @@ func TestTransformRecordWithoutIndexer(t *testing.T) {
"-associated-index-id", "42",
},
Dir: "",
Env: []string{"SRC_ENDPOINT=https://test%2A:hunter2@test.io"},
Env: []string{"SRC_ENDPOINT=https://sourcegraph:hunter2@test.io"},
},
},
RedactedValues: map[string]string{
"https://test%2A:hunter2@test.io": "https://USERNAME_REMOVED:PASSWORD_REMOVED@test.io",
"test*": "USERNAME_REMOVED",
"hunter2": "PASSWORD_REMOVED",
"https://sourcegraph:hunter2@test.io": "https://sourcegraph:PASSWORD_REMOVED@test.io",
"hunter2": "PASSWORD_REMOVED",
},
}
if diff := cmp.Diff(expected, job); diff != "" {

3
sg.config.yaml
View File

@ -89,7 +89,6 @@ env:
# Required for frontend and executor to communicate
EXECUTOR_FRONTEND_URL: http://localhost:3080
EXECUTOR_FRONTEND_USERNAME: executor
EXECUTOR_FRONTEND_PASSWORD: hunter2
# Disable firecracker inside executor in dev
@ -444,7 +443,7 @@ commands:
<<: *executor_template
cmd: |
env TMPDIR="$HOME/.sourcegraph/batches-executor-temp" \
sudo --preserve-env=TMPDIR,EXECUTOR_QUEUE_NAME,SRC_PROF_HTTP,EXECUTOR_FRONTEND_URL,EXECUTOR_FRONTEND_USERNAME,EXECUTOR_FRONTEND_PASSWORD,EXECUTOR_USE_FIRECRACKER,EXECUTOR_IMAGE_ARCHIVE_PATH \
sudo --preserve-env=TMPDIR,EXECUTOR_QUEUE_NAME,SRC_PROF_HTTP,EXECUTOR_FRONTEND_URL,EXECUTOR_FRONTEND_PASSWORD,EXECUTOR_USE_FIRECRACKER,EXECUTOR_IMAGE_ARCHIVE_PATH \
.bin/executor
env:
EXECUTOR_USE_FIRECRACKER: true