diff --git a/cmd/objgitd/ssh.go b/cmd/objgitd/ssh.go index 9f837e1..c2d6251 100644 --- a/cmd/objgitd/ssh.go +++ b/cmd/objgitd/ssh.go @@ -119,14 +119,19 @@ func newSSHServer(d *daemon, addr string) (*ssh.Server, error) { // handleSSH services one git-over-SSH exec request: parse the command, authorize, // resolve the repository, and hand the session to the matching go-git transport -// command. The session is the protocol stream (reader and writer). The one -// non-git command, sh, goes to handleShell. +// command. The session is the protocol stream (reader and writer). The non-git +// commands go to their own handlers: sh to handleShell, and the webhook +// settings commands to handleSSHWebhookSettings and handleSSHWebhookSet. func (d *daemon) handleSSH(s ssh.Session) { cmd := s.Command() if len(cmd) > 0 && cmd[0] == "sh" { d.handleShell(s, cmd[1:]) return } + if len(cmd) > 0 && cmd[0] == "objgit-webhook-set" { + d.handleSSHWebhookSet(s, cmd[1:]) + return + } if len(cmd) < 2 { fmt.Fprintln(s.Stderr(), "objgitd: this is a git SSH endpoint; interactive shells are not supported") if d.allowHooks { diff --git a/cmd/objgitd/webhook_settings.go b/cmd/objgitd/webhook_settings.go index 400eae0..c49975a 100644 --- a/cmd/objgitd/webhook_settings.go +++ b/cmd/objgitd/webhook_settings.go @@ -1,11 +1,15 @@ package main import ( + "errors" + "flag" "fmt" "log/slog" "net/http" + "strings" ssh "github.com/gliderlabs/ssh" + settingsv1 "github.com/tigrisdata/objgit/gen/tigrisdata/objgit/webhooks/v1" "github.com/tigrisdata/objgit/internal/auth" "github.com/tigrisdata/objgit/internal/repofs" "github.com/tigrisdata/objgit/internal/webhook" @@ -79,6 +83,104 @@ func (d *daemon) handleSSHWebhookSettings(s ssh.Session, repo string) { _, _ = s.Write(append(body, '\n')) } +const webhookSetUsage = "usage: objgit-webhook-set org/repo [-url URL] [-rotate-secret]" + +// handleSSHWebhookSet changes a repository's webhook settings. Each flag sets +// one non-secret setting; an omitted flag keeps the current value. The secret +// is kept, or generated when there is none or -rotate-secret is set. The +// command prints the stored settings, secret included, as ProtoJSON. +func (d *daemon) handleSSHWebhookSet(s ssh.Session, args []string) { + if len(args) == 0 || strings.HasPrefix(args[0], "-") { + fmt.Fprintln(s.Stderr(), "objgitd: "+webhookSetUsage) + _ = s.Exit(1) + return + } + fset := flag.NewFlagSet("objgit-webhook-set", flag.ContinueOnError) + fset.SetOutput(s.Stderr()) + fset.Usage = func() { + fmt.Fprintln(fset.Output(), webhookSetUsage) + fset.PrintDefaults() + } + url := fset.String("url", "", "webhook destination URL; HTTPS, or HTTP to loopback only") + rotate := fset.Bool("rotate-secret", false, "replace the signing secret with a new random one") + if err := fset.Parse(args[1:]); err != nil { + _ = s.Exit(1) + return + } + if fset.NArg() != 0 { + fset.Usage() + _ = s.Exit(1) + return + } + + ref, err := repofs.Parse(args[0]) + if err != nil { + fmt.Fprintln(s.Stderr(), "objgitd: invalid repository") + _ = s.Exit(1) + return + } + var cred auth.Credential = auth.Anonymous{} + if key := s.PublicKey(); key != nil { + cred = auth.PublicKey{Key: key} + } + if d.authorize(s.Context(), auth.Request{ + Repo: ref.Path(), Operation: auth.Admin, Cred: cred, Transport: "ssh", + }) != auth.Allow { + fmt.Fprintln(s.Stderr(), "objgitd: admin access denied") + _ = s.Exit(1) + return + } + + // Unreadable settings block a partial update, because the kept values are + // unknown. Setting both -url and -rotate-secret replaces them. + current, err := webhook.ReadSettings(d.sysFS, ref.Path()) + if err != nil { + if *url == "" || !*rotate { + slog.Error("read webhook settings", "repo", ref.Path(), "err", err) + fmt.Fprintln(s.Stderr(), "objgitd: cannot read webhook settings; set -url and -rotate-secret to replace them") + _ = s.Exit(1) + return + } + current = nil + } + settings := &settingsv1.Settings{Url: current.GetUrl(), Secret: current.GetSecret()} + if *url != "" { + settings.Url = *url + } + if settings.GetUrl() == "" { + fmt.Fprintln(s.Stderr(), "objgitd: no webhook URL is set; use -url") + _ = s.Exit(1) + return + } + rotated := *rotate || settings.GetSecret() == "" + if rotated { + settings.Secret = webhook.NewSecret() + } + if err := webhook.WriteSettings(d.sysFS, ref.Path(), settings); err != nil { + slog.Error("write webhook settings", "repo", ref.Path(), "err", err) + fmt.Fprintf(s.Stderr(), "objgitd: cannot write webhook settings: %v\n", err) + _ = s.Exit(1) + return + } + slog.Info("webhook settings updated", + "repo", ref.Path(), + "remote", s.RemoteAddr().String(), + "secret_rotated", rotated, + ) + + body, found, err := d.webhookSettingsJSON(ref) + if err != nil || !found { + if err == nil { + err = errors.New("settings missing after write") + } + slog.Error("read webhook settings", "repo", ref.Path(), "err", err) + fmt.Fprintln(s.Stderr(), "objgitd: webhook settings were written but cannot be read back") + _ = s.Exit(1) + return + } + _, _ = s.Write(append(body, '\n')) +} + func (d *daemon) webhookSettingsJSON(ref repofs.RepoRef) ([]byte, bool, error) { settings, err := webhook.ReadSettings(d.sysFS, ref.Path()) if err != nil || settings == nil { diff --git a/cmd/objgitd/webhook_settings_test.go b/cmd/objgitd/webhook_settings_test.go index a9e6d58..d0e64b9 100644 --- a/cmd/objgitd/webhook_settings_test.go +++ b/cmd/objgitd/webhook_settings_test.go @@ -9,12 +9,17 @@ import ( "os" "os/exec" "path/filepath" + "strings" "testing" "github.com/go-git/go-billy/v6/memfs" + "github.com/go-git/go-billy/v6/util" + settingsv1 "github.com/tigrisdata/objgit/gen/tigrisdata/objgit/webhooks/v1" "github.com/tigrisdata/objgit/internal/auth" "github.com/tigrisdata/objgit/internal/webhook" gossh "golang.org/x/crypto/ssh" + "google.golang.org/protobuf/encoding/protojson" + "google.golang.org/protobuf/proto" ) type authorizeFunc func(context.Context, auth.Request) auth.Decision @@ -52,7 +57,7 @@ func TestSSHWebhookSettingsAuthorization(t *testing.T) { authz auth.Authorizer wantSecret bool }{ - {"default denies", auth.AllowAnonymous{AllowWrite: true}, false}, + {"default allows", auth.AllowAnonymous{AllowWrite: true}, true}, {"authorizer allows", authz, true}, } for _, tt := range tests { @@ -113,8 +118,8 @@ func TestHTTPWebhookSettingsAuthorization(t *testing.T) { wantStatus int wantSecret bool }{ - {"anonymous default", auth.AllowAnonymous{AllowWrite: true}, "", "", "/_objgit/webhooks/acme/widgets/settings", http.StatusForbidden, false}, - {"basic default", auth.AllowAnonymous{AllowWrite: true}, "operator", "admin-password", "/_objgit/webhooks/acme/widgets/settings", http.StatusForbidden, false}, + {"anonymous default", auth.AllowAnonymous{AllowWrite: true}, "", "", "/_objgit/webhooks/acme/widgets/settings", http.StatusOK, true}, + {"basic default", auth.AllowAnonymous{AllowWrite: true}, "operator", "admin-password", "/_objgit/webhooks/acme/widgets/settings", http.StatusOK, true}, {"wrong credential", allowed, "operator", "wrong", "/_objgit/webhooks/acme/widgets/settings", http.StatusUnauthorized, false}, {"admin credential", allowed, "operator", "admin-password", "/_objgit/webhooks/acme/widgets/settings", http.StatusOK, true}, {"missing settings", allowed, "operator", "admin-password", "/_objgit/webhooks/acme/missing/settings", http.StatusNotFound, false}, @@ -140,3 +145,89 @@ func TestHTTPWebhookSettingsAuthorization(t *testing.T) { }) } } + +func TestSSHWebhookSet(t *testing.T) { + checkSSHBinaries(t) + deny := authorizeFunc(func(context.Context, auth.Request) auth.Decision { return auth.Deny }) + tests := []struct { + name string + existing string // "", "valid", or "corrupt" + authz auth.Authorizer + args []string + wantErr string // stderr substring; empty means success + wantURL string + keepSecret bool + }{ + {name: "create", args: []string{"acme/widgets", "-url", "https://example.com/new"}, wantURL: "https://example.com/new"}, + {name: "change URL keeps secret", existing: "valid", args: []string{"acme/widgets", "-url", "https://example.com/new"}, wantURL: "https://example.com/new", keepSecret: true}, + {name: "rotate keeps URL", existing: "valid", args: []string{"acme/widgets", "-rotate-secret"}, wantURL: "https://example.com/events"}, + {name: "replace unreadable", existing: "corrupt", args: []string{"acme/widgets", "-url", "https://example.com/new", "-rotate-secret"}, wantURL: "https://example.com/new"}, + {name: "partial update of unreadable", existing: "corrupt", args: []string{"acme/widgets", "-url", "https://example.com/new"}, wantErr: "cannot read webhook settings"}, + {name: "no URL", args: []string{"acme/widgets"}, wantErr: "no webhook URL"}, + {name: "plain HTTP", existing: "valid", args: []string{"acme/widgets", "-url", "http://example.com/new"}, wantErr: "must use HTTPS"}, + {name: "unknown flag", existing: "valid", args: []string{"acme/widgets", "-secret", "x"}, wantErr: "flag provided but not defined"}, + {name: "extra argument", existing: "valid", args: []string{"acme/widgets", "-url", "https://example.com/new", "extra"}, wantErr: "usage:"}, + {name: "no repository", args: []string{"-url", "https://example.com/new"}, wantErr: "usage:"}, + {name: "invalid repository", args: []string{"../widgets", "-url", "https://example.com/new"}, wantErr: "invalid repository"}, + {name: "denied", existing: "valid", authz: deny, args: []string{"acme/widgets", "-url", "https://example.com/new"}, wantErr: "admin access denied"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fs := memfs.New() + switch tt.existing { + case "valid": + writeWebhookSettings(t, fs, "acme/widgets", "https://example.com/events") + case "corrupt": + settingsPath, _ := webhook.SettingsPath("acme/widgets") + if err := util.WriteFile(fs, settingsPath, []byte("{not json"), 0o600); err != nil { + t.Fatal(err) + } + } + before, _ := util.ReadFile(fs, ".objgit/webhooks/acme/widgets/settings.json") + addr, _ := startSSHServer(t, false, false, func(d *daemon) { + d.sysFS = fs + if tt.authz != nil { + d.authz = tt.authz + } + }) + + stdout, stderr, code := runSSHCommand(t, addr, append([]string{"objgit-webhook-set"}, tt.args...)...) + if tt.wantErr != "" { + if code == 0 { + t.Fatalf("command succeeded, want error %q\nstdout: %s", tt.wantErr, stdout) + } + if !strings.Contains(stderr, tt.wantErr) { + t.Errorf("stderr = %q, want substring %q", stderr, tt.wantErr) + } + if after, _ := util.ReadFile(fs, ".objgit/webhooks/acme/widgets/settings.json"); !bytes.Equal(before, after) { + t.Errorf("failed command changed stored settings to %q", after) + } + return + } + if code != 0 { + t.Fatalf("exit %d\nstderr: %s", code, stderr) + } + + var printed settingsv1.Settings + if err := protojson.Unmarshal([]byte(stdout), &printed); err != nil { + t.Fatalf("stdout is not settings JSON: %v\n%s", err, stdout) + } + stored, err := webhook.ReadSettings(fs, "acme/widgets") + if err != nil || stored == nil { + t.Fatalf("ReadSettings = %v, %v", stored, err) + } + if !proto.Equal(&printed, stored) { + t.Errorf("printed %v, stored %v", &printed, stored) + } + if stored.GetUrl() != tt.wantURL { + t.Errorf("url = %q, want %q", stored.GetUrl(), tt.wantURL) + } + if gotKept := stored.GetSecret() == "test-secret"; gotKept != tt.keepSecret { + t.Errorf("secret = %q, keepSecret = %v", stored.GetSecret(), tt.keepSecret) + } + if !tt.keepSecret && len(stored.GetSecret()) != 64 { + t.Errorf("new secret %q is not 32 hex-encoded bytes", stored.GetSecret()) + } + }) + } +} diff --git a/docs/architecture/auth.md b/docs/architecture/auth.md index 14d478e..ccc8ac9 100644 --- a/docs/architecture/auth.md +++ b/docs/architecture/auth.md @@ -61,9 +61,11 @@ and stderr plus a non-zero exit status for SSH. ## The one implementation today `auth.AllowAnonymous{AllowWrite}` allows read for everyone. It allows write -only when `AllowWrite` is set. It always denies `Admin`. The webhook-settings -queries over HTTP and SSH request `Admin`; an ACL-backed authorizer must -explicitly grant it before they reveal the raw signing secret. +only when `AllowWrite` is set. For now, it also allows `Admin` for everyone. +The webhook-settings commands request `Admin`: the queries over HTTP and SSH, +and `objgit-webhook-set` over SSH. Thus, any client can read the raw signing +secret of every repository, and change its settings. This is a +temporary measure until an ACL-backed authorizer replaces it. `main.go` wires it as `AllowAnonymous{AllowWrite: *allowPush}`. The `-allow-push` flag is therefore configuration for this default, and not a diff --git a/docs/usage/webhooks.md b/docs/usage/webhooks.md index b92829f..02f3771 100644 --- a/docs/usage/webhooks.md +++ b/docs/usage/webhooks.md @@ -23,8 +23,8 @@ logged and the push continues without webhook delivery. ## Query settings -The Git HTTP and SSH listeners expose admin-only queries for the raw settings, -including the signing secret: +The Git HTTP and SSH listeners expose admin-only queries for the raw settings. +The output includes the signing secret: ```sh curl --user 'operator:' \ @@ -38,11 +38,40 @@ absent; the SSH command exits with an error. Use HTTPS for the HTTP query because it carries a credential and a raw secret. Both queries ask the configured `internal/auth.Authorizer` for the `Admin` -operation on that repository. The default `AllowAnonymous` authorizer denies -`Admin`, even when `-allow-push` is set. Deploy an authorizer that checks your -admin ACL and grants `Admin` only to eligible HTTP or SSH credentials before -using these queries. Ordinary repository read or write permission does not -grant access to the secret. +operation on that repository. For now, the default `AllowAnonymous` authorizer +allows `Admin` for all clients. Thus, any client that can reach the daemon can +read the signing secret. To restrict access, deploy an authorizer that grants +`Admin` only to the credentials in your admin ACL. + +## Change settings + +To change the settings of a repository, use the `objgit-webhook-set` SSH +command: + +```sh +ssh git@git.example.com objgit-webhook-set acme/widgets -url https://events.example.com/objgit +``` + +The command has these flags: + +| Flag | Function | +| ---------------- | ----------------------------------------------------------------- | +| `-url` | Sets the destination URL. The same URL rules as above apply. | +| `-rotate-secret` | Replaces the signing secret with a new random secret of 32 bytes. | + +Put the repository first, then the flags. If you do not give a flag, the +command keeps the current value of that setting. If the repository has no +settings, you must give `-url`. If no secret exists, the command generates +one. + +The command writes the settings object and then prints it as ProtoJSON. The +output includes the signing secret. Give this secret to the webhook receiver. +The next push uses the new settings. + +If the current settings object is not valid, the command cannot keep its +values. To replace such an object, give both `-url` and `-rotate-secret`. + +The command asks the `Authorizer` for the `Admin` operation, as the queries do. The settings schema is [settings.proto](../../proto/tigrisdata/objgit/webhooks/v1/settings.proto), diff --git a/internal/auth/auth.go b/internal/auth/auth.go index a3afe88..ab5ed53 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -87,10 +87,13 @@ type Authorizer interface { Authorize(ctx context.Context, req Request) Decision } -// AllowAnonymous is the permissive default: read for everyone, write only when -// AllowWrite is set. "Dangerously allow everything the server is configured to -// allow" — never more open than the -allow-push gate. It ignores the credential -// entirely and never returns Unauthenticated. +// AllowAnonymous is the permissive default: read and admin for everyone, write +// only when AllowWrite is set. It ignores the credential entirely and never +// returns Unauthenticated. +// +// TODO: Admin is granted to everyone as a temporary measure. This exposes +// every repository's webhook signing secret to any client. Replace it with an +// ACL-backed authorizer. type AllowAnonymous struct{ AllowWrite bool } func (a AllowAnonymous) Authorize(_ context.Context, req Request) Decision { @@ -102,6 +105,8 @@ func (a AllowAnonymous) Authorize(_ context.Context, req Request) Decision { return Allow } return Deny + case Admin: + return Allow default: return Deny } diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 50d3852..d8e2155 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -16,7 +16,8 @@ func TestAllowAnonymousAuthorize(t *testing.T) { {name: "no-write/write", allowWrite: false, op: Write, want: Deny}, {name: "allow-write/read", allowWrite: true, op: Read, want: Allow}, {name: "allow-write/write", allowWrite: true, op: Write, want: Allow}, - {name: "allow-write/admin", allowWrite: true, op: Admin, want: Deny}, + {name: "no-write/admin", allowWrite: false, op: Admin, want: Allow}, + {name: "allow-write/admin", allowWrite: true, op: Admin, want: Allow}, } for _, tt := range tests { diff --git a/internal/webhook/webhook.go b/internal/webhook/webhook.go index 7164b39..4d8f486 100644 --- a/internal/webhook/webhook.go +++ b/internal/webhook/webhook.go @@ -6,6 +6,7 @@ import ( "bytes" "context" "crypto/hmac" + "crypto/rand" "crypto/sha256" "encoding/hex" "errors" @@ -124,6 +125,47 @@ func ReadSettings(fs billy.Filesystem, repo string) (*settingsv1.Settings, error return &settings, nil } +// NewSecret returns a random signing secret: 32 bytes, hex encoded. +func NewSecret() string { + b := make([]byte, 32) + _, _ = rand.Read(b) // crypto/rand.Read never returns an error. + return hex.EncodeToString(b) +} + +// WriteSettings validates settings and stores them as repo's ProtoJSON +// settings object, replacing any earlier one. The next push reads them. +func WriteSettings(fs billy.Filesystem, repo string, settings *settingsv1.Settings) error { + settingsPath, err := SettingsPath(repo) + if err != nil { + return err + } + if settings.GetSecret() == "" { + return errors.New("webhook settings have no secret") + } + if _, _, err := validateURL(settings.GetUrl()); err != nil { + return err + } + body, err := protojson.Marshal(settings) + if err != nil { + return fmt.Errorf("encode webhook settings: %w", err) + } + if err := fs.MkdirAll(path.Dir(settingsPath), 0o700); err != nil && !errors.Is(err, os.ErrExist) { + return fmt.Errorf("create webhook settings directory: %w", err) + } + f, err := fs.Create(settingsPath) + if err != nil { + return fmt.Errorf("create webhook settings %s: %w", settingsPath, err) + } + if _, err := f.Write(body); err != nil { + f.Close() + return fmt.Errorf("write webhook settings %s: %w", settingsPath, err) + } + if err := f.Close(); err != nil { + return fmt.Errorf("write webhook settings %s: %w", settingsPath, err) + } + return nil +} + // Enabled reports whether repo has a configured webhook destination. func (c *Client) Enabled(repo string) bool { if c == nil { diff --git a/internal/webhook/webhook_test.go b/internal/webhook/webhook_test.go index f8eb666..4e1bcec 100644 --- a/internal/webhook/webhook_test.go +++ b/internal/webhook/webhook_test.go @@ -19,6 +19,7 @@ import ( "github.com/go-git/go-billy/v6" "github.com/go-git/go-billy/v6/memfs" pushv1 "github.com/tigrisdata/objgit/gen/tigrisdata/objgit/events/push/v1" + settingsv1 "github.com/tigrisdata/objgit/gen/tigrisdata/objgit/webhooks/v1" "google.golang.org/protobuf/encoding/protojson" "google.golang.org/protobuf/proto" ) @@ -289,3 +290,70 @@ func TestDeliverDoesNotExposeURLQuery(t *testing.T) { t.Errorf("error exposed URL query: %v", err) } } + +func TestWriteSettings(t *testing.T) { + tests := []struct { + name string + repo string + url string + secret string + wantError string + }{ + {name: "public HTTPS", url: "https://example.com/hook", secret: "key"}, + {name: "loopback HTTP", url: "http://127.0.0.1:1234/hook", secret: "key"}, + {name: "bad repository", repo: "../widgets", url: "https://example.com/hook", secret: "key", wantError: "invalid repository"}, + {name: "missing secret", url: "https://example.com/hook", wantError: "no secret"}, + {name: "plain HTTP", url: "http://example.com/hook", secret: "key", wantError: "must use HTTPS"}, + {name: "private IP", url: "https://10.0.0.1/hook", secret: "key", wantError: "nonpublic IP"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + repo := tt.repo + if repo == "" { + repo = "acme/widgets" + } + fs := memfs.New() + want := &settingsv1.Settings{Url: tt.url, Secret: tt.secret} + err := WriteSettings(fs, repo, want) + if tt.wantError != "" { + if err == nil || !strings.Contains(err.Error(), tt.wantError) { + t.Fatalf("error = %v, want substring %q", err, tt.wantError) + } + if _, statErr := fs.Stat(".objgit/webhooks/acme/widgets/settings.json"); statErr == nil { + t.Error("rejected settings were written") + } + return + } + if err != nil { + t.Fatal(err) + } + got, err := ReadSettings(fs, repo) + if err != nil { + t.Fatal(err) + } + if !proto.Equal(got, want) { + t.Errorf("ReadSettings = %v, want %v", got, want) + } + c, err := Load(fs, repo) + if err != nil { + t.Fatal(err) + } + if !c.Enabled(repo) { + t.Error("written settings did not enable delivery") + } + }) + } +} + +func TestNewSecret(t *testing.T) { + a, b := NewSecret(), NewSecret() + if len(a) != 64 { + t.Errorf("len(NewSecret()) = %d, want 64", len(a)) + } + if _, err := hex.DecodeString(a); err != nil { + t.Errorf("NewSecret() = %q is not hex: %v", a, err) + } + if a == b { + t.Error("two secrets are equal") + } +}