diff --git a/cmd/create_test.go b/cmd/create_test.go new file mode 100644 index 0000000..c8fb9b3 --- /dev/null +++ b/cmd/create_test.go @@ -0,0 +1,140 @@ +package cmd + +import ( + "strings" + "testing" + + "github.com/robbycochran/harness-openshell/internal/gateway" + "github.com/robbycochran/harness-openshell/internal/preflight" + "github.com/robbycochran/harness-openshell/internal/profile" +) + +func TestActiveGatewayInfo_ListError(t *testing.T) { + gw := &mockGW{} + + _, err := activeGatewayInfo(gw) + if err == nil { + t.Fatal("expected error when no active gateway") + } + if !strings.Contains(err.Error(), "no active gateway") { + t.Errorf("error = %q, want 'no active gateway'", err) + } +} + +func TestActiveGatewayInfo_RemoteGateway(t *testing.T) { + gw := &mockGW{ + gatewayListResult: []gateway.GatewayInfo{ + {Name: "openshell-remote-ocp", Endpoint: "https://gateway.apps.ocp.example.com:443", Active: true}, + }, + } + + info, err := activeGatewayInfo(gw) + if err != nil { + t.Fatalf("activeGatewayInfo: %v", err) + } + if info.Name != "openshell-remote-ocp" { + t.Errorf("Name = %q, want openshell-remote-ocp", info.Name) + } +} + +func TestCreateDirect_NoProviders(t *testing.T) { + dir := setupTestProfile(t) + gw := &mockGW{ + providers: map[string]bool{}, + } + + cfg, err := profile.Parse(dir, "default") + if err != nil { + t.Fatalf("parse profile: %v", err) + } + + err = createDirect(dir, gw, "default", cfg, nil) + if err != nil { + t.Fatalf("createDirect: %v", err) + } + if gw.createCalls != 1 { + t.Errorf("createCalls = %d, want 1", gw.createCalls) + } + opts := gw.createOpts[0] + if len(opts.Providers) != 0 { + t.Errorf("Providers = %v, want empty", opts.Providers) + } +} + +func TestCreateDirect_WithProviders(t *testing.T) { + dir := setupTestProfile(t) + gw := &mockGW{ + providers: map[string]bool{"github": true}, + } + + cfg, err := profile.Parse(dir, "default") + if err != nil { + t.Fatalf("parse profile: %v", err) + } + + err = createDirect(dir, gw, "default", cfg, []string{"github"}) + if err != nil { + t.Fatalf("createDirect: %v", err) + } + opts := gw.createOpts[0] + if len(opts.Providers) != 1 || opts.Providers[0] != "github" { + t.Errorf("Providers = %v, want [github]", opts.Providers) + } + if opts.TTY { + t.Error("TTY should be false for create (non-interactive)") + } +} + +func TestCreateDirect_SandboxName(t *testing.T) { + dir := setupTestProfile(t) + gw := &mockGW{ + providers: map[string]bool{}, + } + + cfg, err := profile.Parse(dir, "default") + if err != nil { + t.Fatalf("parse profile: %v", err) + } + cfg.Name = "custom-sandbox" + + err = createDirect(dir, gw, "default", cfg, nil) + if err != nil { + t.Fatalf("createDirect: %v", err) + } + opts := gw.createOpts[0] + if opts.Name != "custom-sandbox" { + t.Errorf("Name = %q, want custom-sandbox", opts.Name) + } +} + +func TestProfileHasCustomProviders_NoCustom(t *testing.T) { + allProviders := []preflight.Provider{ + {Name: "github", Type: "openshell"}, + {Name: "vertex-local", Type: "openshell"}, + } + if profileHasCustomProviders([]string{"github", "vertex-local"}, allProviders) { + t.Error("no custom providers, should return false") + } +} + +func TestProfileHasCustomProviders_WithCustom(t *testing.T) { + allProviders := []preflight.Provider{ + {Name: "github", Type: "openshell"}, + {Name: "gws", Type: "custom"}, + } + if !profileHasCustomProviders([]string{"github", "gws"}, allProviders) { + t.Error("gws is custom, should return true") + } +} + +func TestProviderInList(t *testing.T) { + if !providerInList("github", []string{"github", "vertex-local"}) { + t.Error("github should be in list") + } + if providerInList("atlassian", []string{"github", "vertex-local"}) { + t.Error("atlassian should not be in list") + } + if providerInList("github", nil) { + t.Error("nil list should return false") + } +} diff --git a/cmd/deploy.go b/cmd/deploy.go index 103e89e..1b609c0 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -181,9 +181,8 @@ func deployFromConfig(harnessDir string, gwCfg *gateway.GatewayConfig, gw gatewa for _, sa := range gwCfg.OCP.SCCAnyuid { kc.RunOC(ctx, "adm", "policy", "add-scc-to-user", "anyuid", "-z", sa, "-n", namespace) } - clusterRunner.RunKubectl(ctx, "create", "clusterrolebinding", "agent-sandbox-admin", - "--clusterrole=cluster-admin", - "--serviceaccount=agent-sandbox-system:agent-sandbox-controller") + // The sandbox controller ClusterRole and ClusterRoleBinding are + // included in the upstream manifest.yaml applied in step 2. } // Addon manifests (RBAC, etc.) @@ -249,7 +248,10 @@ func deployFromConfig(harnessDir string, gwCfg *gateway.GatewayConfig, gw gatewa return fmt.Errorf("service type %q endpoint resolution not yet implemented", gwCfg.Gateway.Service) } - existing, _ := gw.GatewayList() + existing, err := gw.GatewayList() + if err != nil { + return fmt.Errorf("listing existing gateways: %w", err) + } for _, g := range existing { if routeHost != "" && strings.Contains(g.Endpoint, routeHost) { gw.GatewayRemove(g.Name) diff --git a/cmd/providers.go b/cmd/providers.go index c952206..33f51b8 100644 --- a/cmd/providers.go +++ b/cmd/providers.go @@ -53,7 +53,10 @@ func registerProviders(harnessDir string, gw gateway.Gateway, force bool, gwCfg // Force mode: require no running sandboxes if force { - sandboxes, _ := gw.SandboxList() + sandboxes, err := gw.SandboxList() + if err != nil { + return fmt.Errorf("listing sandboxes: %w", err) + } if len(sandboxes) > 0 { return fmt.Errorf("cannot --force with running sandboxes — delete them first") } @@ -94,7 +97,10 @@ func registerProviders(harnessDir string, gw gateway.Gateway, force bool, gwCfg // Show results status.Section("Providers") - names, _ := gw.ProviderList() + names, err := gw.ProviderList() + if err != nil { + return fmt.Errorf("listing providers: %w", err) + } for _, n := range names { status.OK(n) } diff --git a/cmd/providers_test.go b/cmd/providers_test.go new file mode 100644 index 0000000..fdf8fd1 --- /dev/null +++ b/cmd/providers_test.go @@ -0,0 +1,142 @@ +package cmd + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/robbycochran/harness-openshell/internal/gateway" +) + +func setupProvidersTest(t *testing.T) string { + t.Helper() + dir := t.TempDir() + os.MkdirAll(filepath.Join(dir, "sandbox", "profiles"), 0o755) + return dir +} + +func TestRegisterProviders_GitHubWhenTokenSet(t *testing.T) { + dir := setupProvidersTest(t) + t.Setenv("GITHUB_TOKEN", "ghp_test123") + t.Setenv("JIRA_API_TOKEN", "") + + gw := &mockGW{ + providers: map[string]bool{}, + } + + err := registerProviders(dir, gw, false, nil) + if err != nil { + t.Fatalf("registerProviders: %v", err) + } +} + +func TestRegisterProviders_SkipsWhenTokenMissing(t *testing.T) { + dir := setupProvidersTest(t) + t.Setenv("GITHUB_TOKEN", "") + t.Setenv("JIRA_API_TOKEN", "") + + gw := &mockGW{ + providers: map[string]bool{}, + } + + err := registerProviders(dir, gw, false, nil) + if err != nil { + t.Fatalf("registerProviders: %v", err) + } +} + +func TestRegisterProviders_SkipsExistingProvider(t *testing.T) { + dir := setupProvidersTest(t) + t.Setenv("GITHUB_TOKEN", "ghp_test123") + t.Setenv("JIRA_API_TOKEN", "") + + gw := &mockGW{ + providers: map[string]bool{"github": true}, + } + + err := registerProviders(dir, gw, false, nil) + if err != nil { + t.Fatalf("registerProviders: %v", err) + } +} + +func TestRegisterProviders_ForceWithRunningSandboxes(t *testing.T) { + dir := setupProvidersTest(t) + + gw := &mockGWWithSandboxes{ + mockGW: &mockGW{ + providers: map[string]bool{"github": true}, + }, + sandboxes: []string{"test-sandbox"}, + } + + err := registerProviders(dir, gw, true, nil) + if err == nil { + t.Fatal("expected error with --force and running sandboxes") + } + if !strings.Contains(err.Error(), "cannot --force") { + t.Errorf("error = %q, want 'cannot --force'", err) + } +} + +func TestRegisterProviders_ForceDeletesAndRecreates(t *testing.T) { + dir := setupProvidersTest(t) + t.Setenv("GITHUB_TOKEN", "ghp_test123") + t.Setenv("JIRA_API_TOKEN", "") + + gw := &mockGW{ + providers: map[string]bool{}, + } + + err := registerProviders(dir, gw, true, nil) + if err != nil { + t.Fatalf("registerProviders: %v", err) + } +} + +func TestRegisterProviders_RespectsGatewayConfig(t *testing.T) { + dir := setupProvidersTest(t) + t.Setenv("GITHUB_TOKEN", "ghp_test123") + t.Setenv("JIRA_API_TOKEN", "token") + + gw := &mockGW{ + providers: map[string]bool{}, + } + + gwCfg := &gateway.GatewayConfig{} + gwCfg.Providers.Enabled = []string{"github"} + + err := registerProviders(dir, gw, false, gwCfg) + if err != nil { + t.Fatalf("registerProviders: %v", err) + } +} + +func TestRegisterProviders_ListError(t *testing.T) { + dir := setupProvidersTest(t) + + gw := &mockGW{ + providers: map[string]bool{}, + providerErr: fmt.Errorf("gateway unreachable"), + } + + err := registerProviders(dir, gw, false, nil) + if err == nil { + t.Fatal("expected error when provider list fails") + } + if !strings.Contains(err.Error(), "listing providers") { + t.Errorf("error = %q, want 'listing providers'", err) + } +} + +// mockGWWithSandboxes wraps mockGW to return a non-empty sandbox list. +type mockGWWithSandboxes struct { + *mockGW + sandboxes []string +} + +func (m *mockGWWithSandboxes) SandboxList() ([]string, error) { + return m.sandboxes, nil +} diff --git a/cmd/teardown.go b/cmd/teardown.go index 76eb75e..d2cffb0 100644 --- a/cmd/teardown.go +++ b/cmd/teardown.go @@ -108,7 +108,10 @@ func teardownProviders(gw gateway.Gateway, activeGW string) error { if len(remaining) > 0 { // Sandbox may be mid-deletion — wait briefly and retry time.Sleep(2 * time.Second) - remaining, _ = gw.SandboxList() + remaining, err = gw.SandboxList() + if err != nil { + return fmt.Errorf("rechecking sandboxes: %w", err) + } if len(remaining) > 0 { return fmt.Errorf("cannot delete providers with running sandboxes — run: harness teardown --sandboxes") } @@ -191,6 +194,7 @@ func teardownK8s(gw gateway.Gateway, gwCfg *gateway.GatewayConfig, kc, clusterRu for _, sa := range sccAnyuid { kc.RunOC(ctx, "adm", "policy", "remove-scc-from-user", "anyuid", "-z", sa, "-n", namespace) } + // Clean up legacy cluster-admin binding (no longer created, but may exist from earlier deploys) clusterRunner.RunKubectl(ctx, "delete", "clusterrolebinding", "agent-sandbox-admin") status.Info("Cleared") @@ -217,7 +221,11 @@ func teardownK8s(gw gateway.Gateway, gwCfg *gateway.GatewayConfig, kc, clusterRu // Gateway config cleanup fmt.Println() status.Section("Gateway config") - gateways, _ := gw.GatewayList() + gateways, err := gw.GatewayList() + if err != nil { + status.Failf("listing gateways: %v", err) + return + } for _, g := range gateways { if !strings.Contains(g.Endpoint, "127.0.0.1") { if err := gw.GatewayRemove(g.Name); err == nil { diff --git a/cmd/up.go b/cmd/up.go index 459478c..17b70ae 100644 --- a/cmd/up.go +++ b/cmd/up.go @@ -102,7 +102,10 @@ func upRemote(harnessDir string, gwCfg *gateway.GatewayConfig, gw gateway.Gatewa } // 2. Ensure providers - providers, _ := gw.ProviderList() + providers, err := gw.ProviderList() + if err != nil { + return fmt.Errorf("listing providers: %w", err) + } if len(providers) == 0 { status.Section("Registering providers") if err := registerProviders(harnessDir, gw, false, gwCfg); err != nil { @@ -243,8 +246,11 @@ func upRemote(harnessDir string, gwCfg *gateway.GatewayConfig, gw gateway.Gatewa var jobStatus string deadline := time.Now().Add(10 * time.Minute) for time.Now().Before(deadline) { - jobStatus, _ = kc.RunKubectl(ctx, "get", "job", jobName, + jobStatus, err = kc.RunKubectl(ctx, "get", "job", jobName, "-o", "jsonpath={.status.conditions[0].type}") + if err != nil { + return fmt.Errorf("checking launcher job status: %w", err) + } if jobStatus == "Complete" || jobStatus == "Failed" || jobStatus == "SuccessCriteriaMet" { break } @@ -283,7 +289,10 @@ func upLocal(opts upLocalOpts) error { } // 2. Ensure providers - providers, _ := gw.ProviderList() + providers, err := gw.ProviderList() + if err != nil { + return fmt.Errorf("listing providers: %w", err) + } if len(providers) == 0 { status.Section("Registering providers") if err := registerProviders(opts.harnessDir, gw, false, opts.gwCfg); err != nil { diff --git a/gateways/ocp/gateway.toml b/gateways/ocp/gateway.toml index 9c45bce..cbf6b62 100644 --- a/gateways/ocp/gateway.toml +++ b/gateways/ocp/gateway.toml @@ -36,7 +36,7 @@ manifests = ["addons/rbac.yaml", "addons/route.yaml"] launcher = "ghcr.io/robbycochran/harness-openshell:launcher" [ocp] -scc-privileged = ["openshell", "openshell-sandbox", "default"] +scc-privileged = ["openshell", "openshell-sandbox"] scc-anyuid = ["openshell"] [secrets] diff --git a/internal/status/status.go b/internal/status/status.go index 42a936d..7e54175 100644 --- a/internal/status/status.go +++ b/internal/status/status.go @@ -3,6 +3,7 @@ package status import ( "fmt" "os" + "strings" ) var Verbose bool @@ -12,12 +13,57 @@ func Cmd(name string, args ...string) { return } fmt.Fprintf(os.Stderr, " $ %s", name) + redactNext := false for _, a := range args { + if redactNext { + fmt.Fprintf(os.Stderr, " %s", redactValue(a)) + redactNext = false + continue + } + if a == "--credential" { + redactNext = true + fmt.Fprintf(os.Stderr, " %s", a) + continue + } + if strings.HasPrefix(a, "--from-literal=") && isSensitiveLiteral(a) { + fmt.Fprintf(os.Stderr, " %s", redactFromLiteral(a)) + continue + } fmt.Fprintf(os.Stderr, " %s", a) } fmt.Fprintln(os.Stderr) } +// redactValue replaces the value portion of KEY=VALUE with ***. +func redactValue(s string) string { + if i := strings.IndexByte(s, '='); i >= 0 { + return s[:i+1] + "***" + } + return s +} + +// isSensitiveLiteral checks if a --from-literal=KEY=VALUE arg contains a secret key. +func isSensitiveLiteral(s string) bool { + upper := strings.ToUpper(s) + for _, keyword := range []string{"TOKEN", "SECRET", "PASSWORD", "KEY", "CREDENTIAL"} { + if strings.Contains(upper, keyword) { + return true + } + } + return false +} + +// redactFromLiteral redacts the value in --from-literal=KEY=VALUE. +func redactFromLiteral(s string) string { + // s is "--from-literal=KEY=VALUE", find the second '=' + prefix := "--from-literal=" + rest := s[len(prefix):] + if i := strings.IndexByte(rest, '='); i >= 0 { + return prefix + rest[:i+1] + "***" + } + return s +} + func OK(msg string) { fmt.Println(" ✓ " + msg) } func OKf(format string, a ...any) { fmt.Printf(" ✓ "+format+"\n", a...) } func Fail(msg string) { fmt.Println(" ✗ " + msg) } diff --git a/internal/status/status_test.go b/internal/status/status_test.go new file mode 100644 index 0000000..f0b3135 --- /dev/null +++ b/internal/status/status_test.go @@ -0,0 +1,115 @@ +package status + +import ( + "bytes" + "os" + "strings" + "testing" +) + +func captureCmd(name string, args ...string) string { + old := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + Verbose = true + Cmd(name, args...) + w.Close() + os.Stderr = old + var buf bytes.Buffer + buf.ReadFrom(r) + return buf.String() +} + +func TestCmdRedactsCredential(t *testing.T) { + out := captureCmd("openshell", "provider", "create", "github", "--credential", "GITHUB_TOKEN=ghp_secret123") + if got := out; got == "" { + t.Fatal("expected output") + } + if contains(out, "ghp_secret123") { + t.Errorf("credential value leaked: %s", out) + } + if !contains(out, "GITHUB_TOKEN=***") { + t.Errorf("expected redacted credential, got: %s", out) + } +} + +func TestCmdRedactsMultipleCredentials(t *testing.T) { + out := captureCmd("openshell", "provider", "create", "atlassian", + "--credential", "JIRA_API_TOKEN=secret1", + "--credential", "JIRA_URL=https://example.com") + if contains(out, "secret1") { + t.Errorf("first credential leaked: %s", out) + } + if contains(out, "https://example.com") { + t.Errorf("second credential leaked: %s", out) + } + if !contains(out, "JIRA_API_TOKEN=***") { + t.Errorf("expected redacted JIRA_API_TOKEN, got: %s", out) + } + if !contains(out, "JIRA_URL=***") { + t.Errorf("expected redacted JIRA_URL, got: %s", out) + } +} + +func TestCmdRedactsFromLiteral(t *testing.T) { + out := captureCmd("kubectl", "create", "secret", "generic", "openshell-atlassian", + "--from-literal=JIRA_API_TOKEN=mytoken", + "--from-literal=JIRA_URL=https://example.com") + if contains(out, "mytoken") { + t.Errorf("token leaked: %s", out) + } + if !contains(out, "--from-literal=JIRA_API_TOKEN=***") { + t.Errorf("expected redacted token, got: %s", out) + } + // JIRA_URL doesn't match sensitive keywords, should pass through + if !contains(out, "--from-literal=JIRA_URL=https://example.com") { + t.Errorf("non-sensitive literal should not be redacted, got: %s", out) + } +} + +func TestCmdDoesNotRedactNonSensitiveLiteral(t *testing.T) { + out := captureCmd("kubectl", "create", "configmap", "test", + "--from-literal=JIRA_URL=https://example.com", + "--from-literal=NAMESPACE=openshell") + if !contains(out, "JIRA_URL=https://example.com") { + t.Errorf("non-sensitive literal was redacted: %s", out) + } + if !contains(out, "NAMESPACE=openshell") { + t.Errorf("non-sensitive literal was redacted: %s", out) + } +} + +func TestCmdCredentialKeyOnly(t *testing.T) { + // --credential KEY (no =VALUE) should pass through as-is + out := captureCmd("openshell", "provider", "create", "github", "--credential", "GITHUB_TOKEN") + if !contains(out, "GITHUB_TOKEN") { + t.Errorf("credential key should be preserved: %s", out) + } +} + +func TestCmdNotVerbose(t *testing.T) { + old := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + Verbose = false + Cmd("openshell", "--credential", "TOKEN=secret") + w.Close() + os.Stderr = old + var buf bytes.Buffer + buf.ReadFrom(r) + if buf.Len() > 0 { + t.Errorf("expected no output when not verbose, got: %s", buf.String()) + } +} + +func TestCmdNormalArgs(t *testing.T) { + out := captureCmd("openshell", "sandbox", "create", "--from", "image:latest", "--provider", "github") + expected := " $ openshell sandbox create --from image:latest --provider github\n" + if out != expected { + t.Errorf("expected %q, got %q", expected, out) + } +} + +func contains(s, substr string) bool { + return strings.Contains(s, substr) +}