From 710eeeaf8b1140b4df6e695de4ddafceff6f4344 Mon Sep 17 00:00:00 2001
From: gak <gak@gak0.com>
Date: Wed, 22 May 2024 06:24:49 +1000
Subject: [PATCH 01/10] wip: 1p

---
 buildengine/testdata/projects/another/go.sum  |   4 +-
 buildengine/testdata/projects/other/go.sum    |   4 +-
 common/configuration/1password_provider.go    | 238 ++++++++----------
 .../configuration/1password_provider_test.go  |   6 +-
 4 files changed, 117 insertions(+), 135 deletions(-)

diff --git a/buildengine/testdata/projects/another/go.sum b/buildengine/testdata/projects/another/go.sum
index ef44aba8ee..a13ddbc3e4 100644
--- a/buildengine/testdata/projects/another/go.sum
+++ b/buildengine/testdata/projects/another/go.sum
@@ -128,8 +128,8 @@ modernc.org/mathutil v1.6.0 h1:fRe9+AmYlaej+64JsEEhoWuAYBkOtQiMEU7n/XgfYi4=
 modernc.org/mathutil v1.6.0/go.mod h1:Ui5Q9q1TR2gFm0AQRqQUaBWFLAhQpCwNcuhBOSedWPo=
 modernc.org/memory v1.8.0 h1:IqGTL6eFMaDZZhEWwcREgeMXYwmW83LYW8cROZYkg+E=
 modernc.org/memory v1.8.0/go.mod h1:XPZ936zp5OMKGWPqbD3JShgd/ZoQ7899TUuQqxY+peU=
-modernc.org/sqlite v1.29.9 h1:9RhNMklxJs+1596GNuAX+O/6040bvOwacTxuFcRuQow=
-modernc.org/sqlite v1.29.9/go.mod h1:ItX2a1OVGgNsFh6Dv60JQvGfJfTPHPVpV6DF59akYOA=
+modernc.org/sqlite v1.29.10 h1:3u93dz83myFnMilBGCOLbr+HjklS6+5rJLx4q86RDAg=
+modernc.org/sqlite v1.29.10/go.mod h1:ItX2a1OVGgNsFh6Dv60JQvGfJfTPHPVpV6DF59akYOA=
 modernc.org/strutil v1.2.0 h1:agBi9dp1I+eOnxXeiZawM8F4LawKv4NzGWSaLfyeNZA=
 modernc.org/strutil v1.2.0/go.mod h1:/mdcBmfOibveCTBxUl5B5l6W+TTH1FXPLHZE6bTosX0=
 modernc.org/token v1.1.0 h1:Xl7Ap9dKaEs5kLoOQeQmPWevfnk/DM5qcLcYlA8ys6Y=
diff --git a/buildengine/testdata/projects/other/go.sum b/buildengine/testdata/projects/other/go.sum
index ef44aba8ee..a13ddbc3e4 100644
--- a/buildengine/testdata/projects/other/go.sum
+++ b/buildengine/testdata/projects/other/go.sum
@@ -128,8 +128,8 @@ modernc.org/mathutil v1.6.0 h1:fRe9+AmYlaej+64JsEEhoWuAYBkOtQiMEU7n/XgfYi4=
 modernc.org/mathutil v1.6.0/go.mod h1:Ui5Q9q1TR2gFm0AQRqQUaBWFLAhQpCwNcuhBOSedWPo=
 modernc.org/memory v1.8.0 h1:IqGTL6eFMaDZZhEWwcREgeMXYwmW83LYW8cROZYkg+E=
 modernc.org/memory v1.8.0/go.mod h1:XPZ936zp5OMKGWPqbD3JShgd/ZoQ7899TUuQqxY+peU=
-modernc.org/sqlite v1.29.9 h1:9RhNMklxJs+1596GNuAX+O/6040bvOwacTxuFcRuQow=
-modernc.org/sqlite v1.29.9/go.mod h1:ItX2a1OVGgNsFh6Dv60JQvGfJfTPHPVpV6DF59akYOA=
+modernc.org/sqlite v1.29.10 h1:3u93dz83myFnMilBGCOLbr+HjklS6+5rJLx4q86RDAg=
+modernc.org/sqlite v1.29.10/go.mod h1:ItX2a1OVGgNsFh6Dv60JQvGfJfTPHPVpV6DF59akYOA=
 modernc.org/strutil v1.2.0 h1:agBi9dp1I+eOnxXeiZawM8F4LawKv4NzGWSaLfyeNZA=
 modernc.org/strutil v1.2.0/go.mod h1:/mdcBmfOibveCTBxUl5B5l6W+TTH1FXPLHZE6bTosX0=
 modernc.org/token v1.1.0 h1:Xl7Ap9dKaEs5kLoOQeQmPWevfnk/DM5qcLcYlA8ys6Y=
diff --git a/common/configuration/1password_provider.go b/common/configuration/1password_provider.go
index 45eaacda82..fbe7da4b4c 100644
--- a/common/configuration/1password_provider.go
+++ b/common/configuration/1password_provider.go
@@ -2,23 +2,21 @@ package configuration
 
 import (
 	"context"
-	"encoding/base64"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/url"
-	"regexp"
 	"strings"
 
-	"github.com/alecthomas/types/optional"
-
 	"github.com/TBD54566975/ftl/internal/exec"
+	"github.com/TBD54566975/ftl/internal/log"
 	"github.com/TBD54566975/ftl/internal/slices"
 )
 
 // OnePasswordProvider is a configuration provider that reads passwords from
 // 1Password vaults via the "op" command line tool.
 type OnePasswordProvider struct {
-	OnePassword bool `name:"op" help:"Write 1Password secret references - does not write to 1Password." group:"Provider:" xor:"configwriter"`
+	Vault string `name:"op" help:"Store a secret in this 1Password vault." group:"Provider:" xor:"configwriter" placeholder:"VAULT"`
 }
 
 var _ MutableProvider[Secrets] = OnePasswordProvider{}
@@ -27,172 +25,156 @@ func (OnePasswordProvider) Role() Secrets                               { return
 func (o OnePasswordProvider) Key() string                               { return "op" }
 func (o OnePasswordProvider) Delete(ctx context.Context, ref Ref) error { return nil }
 
-// Load returns either a single field if the op:// reference specifies a field, or all fields if not.
-//
-// A single value/password:
-// op://Personal/With Spaces/username
-// op --format json item get --vault Personal "With Spaces" --fields=username
-// { id, value, ... }
-// "value"
-//
-// All fields:
-// op://Personal/With Spaces
-// op --format json item get --vault Personal "With Spaces"
-// { fields: [ { id, value, ... } ], ... }
-// { id: value, ... }
+// Load returns the secret stored in 1password, quoted as valid JSON.
 func (o OnePasswordProvider) Load(ctx context.Context, ref Ref, key *url.URL) ([]byte, error) {
-	_, err := exec.LookPath("op")
-	if err != nil {
-		return nil, fmt.Errorf("1Password CLI tool \"op\" not found: %w", err)
+	if err := checkOpBinary(); err != nil {
+		return nil, err
 	}
 
-	decoded, err := base64.RawURLEncoding.DecodeString(key.Host)
+	vault := key.Host
+	full, err := getItem(ctx, vault, ref)
 	if err != nil {
-		return nil, fmt.Errorf("1Password secret reference must be a base64 encoded string: %w", err)
+		return nil, fmt.Errorf("get item failed: %w", err)
 	}
 
-	parsedRef, err := decodeSecretRef(string(decoded))
+	secret, ok := slices.Find(full.Fields, func(item entry) bool {
+		return item.ID == "password"
+	})
+	if !ok {
+		return nil, fmt.Errorf("password field not found in item %q", ref)
+	}
+
+	jsonSecret, err := json.Marshal(secret.Value)
 	if err != nil {
-		return nil, fmt.Errorf("1Password secret reference invalid: %w", err)
+		return nil, fmt.Errorf("json marshal failed: %w", err)
 	}
 
-	args := []string{"--format", "json", "item", "get", "--vault", parsedRef.Vault, parsedRef.Item}
-	v, fieldSpecified := parsedRef.Field.Get()
-	if fieldSpecified {
-		args = append(args, "--fields", v)
+	return jsonSecret, nil
+}
+
+// Store will save the given secret in 1Password via the `op` command.
+//
+// op does not support "create or update" as a single command. Neither does it support specifying an ID on create.
+// Because of this, we need check if the item exists before creating it, and update it if it does.
+func (o OnePasswordProvider) Store(ctx context.Context, ref Ref, value []byte) (*url.URL, error) {
+	if err := checkOpBinary(); err != nil {
+		return nil, err
 	}
-	output, err := exec.Capture(ctx, ".", "op", args...)
+
+	// value is json encoded
+	var secret string
+	err := json.Unmarshal(value, &secret)
 	if err != nil {
-		return nil, fmt.Errorf("run `op` with args %v: %w", args, err)
+		return nil, fmt.Errorf("json unmarshal failed: %w", err)
 	}
 
-	if fieldSpecified {
-		v, err := decodeSingle(output)
+	logger := log.FromContext(ctx)
+	logger.Debugf("Storing password: %s", value)
+	logger.Debugf("module/title name: %s", ref)
+
+	url := &url.URL{Scheme: "op", Host: o.Vault}
+
+	_, err = getItem(ctx, o.Vault, ref)
+	var notFound notFoundError
+	if errors.As(err, &notFound) {
+		err = createItem(ctx, o.Vault, ref, secret)
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("create item failed: %w", err)
 		}
+		return url, nil
 
-		return json.Marshal(v.Value)
+	} else if err != nil {
+		return nil, fmt.Errorf("get item failed: %w", err)
 	}
 
-	full, err := decodeFull(output)
+	err = editItem(ctx, o.Vault, ref, secret)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("edit item failed: %w", err)
 	}
+	return url, nil
+}
 
-	// Filter out anything without a value
-	filtered := slices.Filter(full, func(e entry) bool {
-		return e.Value != ""
-	})
-	// Map to id: value
-	var mapped = make(map[string]string, len(filtered))
-	for _, e := range filtered {
-		mapped[e.ID] = e.Value
+func (o OnePasswordProvider) Writer() bool { return o.Vault != "" }
+
+func checkOpBinary() error {
+	_, err := exec.LookPath("op")
+	if err != nil {
+		return fmt.Errorf("1Password CLI tool \"op\" not found: %w", err)
 	}
+	return nil
+}
 
-	return json.Marshal(mapped)
+type notFoundError struct {
+	vault string
+	ref   Ref
 }
 
-func (o OnePasswordProvider) Store(ctx context.Context, ref Ref, value []byte) (*url.URL, error) {
-	var opref string
-	if err := json.Unmarshal(value, &opref); err != nil {
-		return nil, fmt.Errorf("1Password value must be a JSON string containing a 1Password secret refererence: %w", err)
-	}
-	if !strings.HasPrefix(opref, "op://") {
-		return nil, fmt.Errorf("1Password secret reference must start with \"op://\"")
-	}
-	encoded := base64.RawURLEncoding.EncodeToString([]byte(opref))
-	return &url.URL{Scheme: "op", Host: encoded}, nil
+func (e notFoundError) Error() string {
+	return fmt.Sprintf("item %q not found in vault %q", e.ref, e.vault)
 }
 
-func (o OnePasswordProvider) Writer() bool { return o.OnePassword }
+// item is the JSON response from `op item get`.
+type item struct {
+	Fields []entry `json:"fields"`
+}
 
 type entry struct {
 	ID    string `json:"id"`
 	Value string `json:"value"`
 }
 
-type fullResponse struct {
-	Fields []entry `json:"fields"`
-}
+// op --format json item get --vault Personal "With Spaces"
+func getItem(ctx context.Context, vault string, ref Ref) (*item, error) {
+	logger := log.FromContext(ctx)
+
+	args := []string{"--format", "json", "item", "get", "--vault", vault, ref.String()}
+	output, err := exec.Capture(ctx, ".", "op", args...)
+	logger.Debugf("Getting item with args %v", args)
+	logger.Debugf("Output: %s", output)
+	if err != nil {
+		if strings.Contains(string(output), "isn't a vault") {
+			return nil, fmt.Errorf("vault %q not found: %w", vault, err)
+		}
 
-// Decode a full item response from op
-func decodeFull(output []byte) ([]entry, error) {
-	var full fullResponse
+		// Item not found, seen two ways of reporting this:
+		if strings.Contains(string(output), "not found in vault") {
+			return nil, notFoundError{vault, ref}
+		}
+		if strings.Contains(string(output), "isn't an item") {
+			return nil, notFoundError{vault, ref}
+		}
+
+		// Don't show the output in the error message because it may contain secrets.
+		return nil, fmt.Errorf("run `op` with args %v: %w", args, err)
+	}
+
+	var full item
 	if err := json.Unmarshal(output, &full); err != nil {
 		return nil, fmt.Errorf("error decoding op full response: %w", err)
 	}
-	return full.Fields, nil
+	return &full, nil
 }
 
-// Decode a single field from op
-func decodeSingle(output []byte) (*entry, error) {
-	var single entry
-	if err := json.Unmarshal(output, &single); err != nil {
-		return nil, fmt.Errorf("error decoding op single response: %w", err)
+// op item create --category Password --vault FTL --title mod.ule "password=val ue"
+func createItem(ctx context.Context, vault string, ref Ref, secret string) error {
+	logger := log.FromContext(ctx)
+	args := []string{"item", "create", "--category", "Password", "--vault", vault, "--title", ref.String(), "password=" + secret}
+	logger.Debugf("Creating item with args %v", args)
+	_, err := exec.Capture(ctx, ".", "op", args...)
+	if err != nil {
+		return fmt.Errorf("create item failed in vault %q, ref %q: %w", vault, ref, err)
 	}
-	return &single, nil
-}
 
-// Custom parser for 1Password secret references because the format is not a standard URL, and we also need to
-// allow users to omit the field name so that we can support secrets with multiple fields.
-//
-// Does not support "section-name".
-//
-//	op://<vault-name>/<item-name>[/<field-name>]
-//
-// Secret references are case-insensitive and support the following characters:
-//
-//	alphanumeric characters (a-z, A-Z, 0-9), -, _, . and the whitespace character
-//
-// If an item or field name includes a / or an unsupported character, use the item
-// or field's unique identifier (ID) instead of its name.
-//
-// See https://developer.1password.com/docs/cli/secrets-reference-syntax/
-type secretRef struct {
-	Vault string
-	Item  string
-	Field optional.Option[string]
+	return nil
 }
 
-var validCharsRegex = regexp.MustCompile(`^[a-zA-Z0-9\-_\. ]+$`)
-
-func decodeSecretRef(ref string) (*secretRef, error) {
-
-	// Take out and check the "op://" prefix
-	const prefix = "op://"
-	if !strings.HasPrefix(ref, prefix) {
-		return nil, fmt.Errorf("must start with \"op://\"")
-	}
-	ref = ref[len(prefix):]
-
-	parts := strings.Split(ref, "/")
-
-	if len(parts) < 2 {
-		return nil, fmt.Errorf("must have at least 2 parts")
-	}
-	if len(parts) > 3 {
-		return nil, fmt.Errorf("must have at most 3 parts")
-	}
-
-	for _, part := range parts {
-		if part == "" {
-			return nil, fmt.Errorf("url parts must not be empty")
-		}
-
-		if !validCharsRegex.MatchString(part) {
-			return nil, fmt.Errorf("url part %q contains unsupported characters. regex: %q", part, validCharsRegex)
-		}
-	}
-
-	secret := secretRef{
-		Vault: parts[0],
-		Item:  parts[1],
-		Field: optional.None[string](),
-	}
-	if len(parts) == 3 {
-		secret.Field = optional.Some(parts[2])
+// op item edit --vault ftl test "password=with space"
+func editItem(ctx context.Context, vault string, ref Ref, secret string) error {
+	args := []string{"item", "edit", "--vault", vault, ref.String(), "password=" + secret}
+	_, err := exec.Capture(ctx, ".", "op", args...)
+	if err != nil {
+		return fmt.Errorf("edit item failed in vault %q, ref %q: %w", vault, ref, err)
 	}
 
-	return &secret, nil
+	return nil
 }
diff --git a/common/configuration/1password_provider_test.go b/common/configuration/1password_provider_test.go
index 52185ae77f..24b1ae03d8 100644
--- a/common/configuration/1password_provider_test.go
+++ b/common/configuration/1password_provider_test.go
@@ -69,13 +69,13 @@ func TestDecodeSecretRef(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := decodeSecretRef(tt.ref)
+			got, err := vaultRef(tt.ref)
 			if (err != nil) != tt.wantErr {
-				t.Errorf("decodeSecretRef() error = %v, wantErr %v", err, tt.wantErr)
+				t.Errorf("vaultRef() error = %v, wantErr %v", err, tt.wantErr)
 				return
 			}
 			if !reflect.DeepEqual(got, tt.want) {
-				t.Errorf("decodeSecretRef() got = %v, want %v", got, tt.want)
+				t.Errorf("vaultRef() got = %v, want %v", got, tt.want)
 			}
 		})
 	}

From 0eacdb148d03ee06e6d3cd4bb54f325d99ea7258 Mon Sep 17 00:00:00 2001
From: gak <gak@gak0.com>
Date: Wed, 22 May 2024 06:30:58 +1000
Subject: [PATCH 02/10] chore: cleanups

---
 common/configuration/1password_provider.go | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/common/configuration/1password_provider.go b/common/configuration/1password_provider.go
index fbe7da4b4c..58809756cc 100644
--- a/common/configuration/1password_provider.go
+++ b/common/configuration/1password_provider.go
@@ -25,7 +25,7 @@ func (OnePasswordProvider) Role() Secrets                               { return
 func (o OnePasswordProvider) Key() string                               { return "op" }
 func (o OnePasswordProvider) Delete(ctx context.Context, ref Ref) error { return nil }
 
-// Load returns the secret stored in 1password, quoted as valid JSON.
+// Load returns the secret stored in 1password, quoted as a JSON string.
 func (o OnePasswordProvider) Load(ctx context.Context, ref Ref, key *url.URL) ([]byte, error) {
 	if err := checkOpBinary(); err != nil {
 		return nil, err
@@ -61,17 +61,12 @@ func (o OnePasswordProvider) Store(ctx context.Context, ref Ref, value []byte) (
 		return nil, err
 	}
 
-	// value is json encoded
 	var secret string
 	err := json.Unmarshal(value, &secret)
 	if err != nil {
 		return nil, fmt.Errorf("json unmarshal failed: %w", err)
 	}
 
-	logger := log.FromContext(ctx)
-	logger.Debugf("Storing password: %s", value)
-	logger.Debugf("module/title name: %s", ref)
-
 	url := &url.URL{Scheme: "op", Host: o.Vault}
 
 	_, err = getItem(ctx, o.Vault, ref)
@@ -91,6 +86,7 @@ func (o OnePasswordProvider) Store(ctx context.Context, ref Ref, value []byte) (
 	if err != nil {
 		return nil, fmt.Errorf("edit item failed: %w", err)
 	}
+
 	return url, nil
 }
 
@@ -130,7 +126,6 @@ func getItem(ctx context.Context, vault string, ref Ref) (*item, error) {
 	args := []string{"--format", "json", "item", "get", "--vault", vault, ref.String()}
 	output, err := exec.Capture(ctx, ".", "op", args...)
 	logger.Debugf("Getting item with args %v", args)
-	logger.Debugf("Output: %s", output)
 	if err != nil {
 		if strings.Contains(string(output), "isn't a vault") {
 			return nil, fmt.Errorf("vault %q not found: %w", vault, err)
@@ -144,7 +139,6 @@ func getItem(ctx context.Context, vault string, ref Ref) (*item, error) {
 			return nil, notFoundError{vault, ref}
 		}
 
-		// Don't show the output in the error message because it may contain secrets.
 		return nil, fmt.Errorf("run `op` with args %v: %w", args, err)
 	}
 
@@ -157,9 +151,7 @@ func getItem(ctx context.Context, vault string, ref Ref) (*item, error) {
 
 // op item create --category Password --vault FTL --title mod.ule "password=val ue"
 func createItem(ctx context.Context, vault string, ref Ref, secret string) error {
-	logger := log.FromContext(ctx)
 	args := []string{"item", "create", "--category", "Password", "--vault", vault, "--title", ref.String(), "password=" + secret}
-	logger.Debugf("Creating item with args %v", args)
 	_, err := exec.Capture(ctx, ".", "op", args...)
 	if err != nil {
 		return fmt.Errorf("create item failed in vault %q, ref %q: %w", vault, ref, err)

From b0363e0925cf909db4c42ce4e815ec38abb72c05 Mon Sep 17 00:00:00 2001
From: gak <gak@gak0.com>
Date: Wed, 22 May 2024 06:33:11 +1000
Subject: [PATCH 03/10] chore: remove old tests

---
 .../configuration/1password_provider_test.go  | 82 -------------------
 1 file changed, 82 deletions(-)
 delete mode 100644 common/configuration/1password_provider_test.go

diff --git a/common/configuration/1password_provider_test.go b/common/configuration/1password_provider_test.go
deleted file mode 100644
index 24b1ae03d8..0000000000
--- a/common/configuration/1password_provider_test.go
+++ /dev/null
@@ -1,82 +0,0 @@
-package configuration
-
-import (
-	"github.com/alecthomas/types/optional"
-	"reflect"
-	"testing"
-)
-
-func TestDecodeSecretRef(t *testing.T) {
-	tests := []struct {
-		name    string
-		ref     string
-		want    *secretRef
-		wantErr bool
-	}{
-		{
-			name: "simple with field",
-			ref:  "op://development/Access Keys/access_key_id",
-			want: &secretRef{
-				Vault: "development",
-				Item:  "Access Keys",
-				Field: optional.Some("access_key_id"),
-			},
-		},
-		{
-			name: "simple without field",
-			ref:  "op://vault/item",
-			want: &secretRef{
-				Vault: "vault",
-				Item:  "item",
-				Field: optional.None[string](),
-			},
-		},
-		{
-			name: "lots of spaces",
-			ref:  "op://My Awesome Vault/My Awesome Item/My Awesome Field",
-			want: &secretRef{
-				Vault: "My Awesome Vault",
-				Item:  "My Awesome Item",
-				Field: optional.Some("My Awesome Field"),
-			},
-		},
-		{
-			name:    "missing op://",
-			ref:     "development/Access Keys/access_key_id",
-			wantErr: true,
-		},
-		{
-			name:    "empty parts",
-			ref:     "op://development//access_key_id",
-			wantErr: true,
-		},
-		{
-			name:    "invalid characters",
-			ref:     "op://development/aws/acce$s",
-			wantErr: true,
-		},
-		{
-			name:    "too many parts",
-			ref:     "op://development/Access Keys/access_key_id/extra",
-			wantErr: true,
-		},
-		{
-			name:    "too few parts",
-			ref:     "op://development",
-			wantErr: true,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			got, err := vaultRef(tt.ref)
-			if (err != nil) != tt.wantErr {
-				t.Errorf("vaultRef() error = %v, wantErr %v", err, tt.wantErr)
-				return
-			}
-			if !reflect.DeepEqual(got, tt.want) {
-				t.Errorf("vaultRef() got = %v, want %v", got, tt.want)
-			}
-		})
-	}
-}

From 00cf30535a39c293b507afa35dda79bc86c538ad Mon Sep 17 00:00:00 2001
From: gak <gak@gak0.com>
Date: Wed, 22 May 2024 06:59:39 +1000
Subject: [PATCH 04/10] test: added simple test to interact with 1p

---
 .../configuration/1password_provider_test.go  | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 common/configuration/1password_provider_test.go

diff --git a/common/configuration/1password_provider_test.go b/common/configuration/1password_provider_test.go
new file mode 100644
index 0000000000..a8fdd86122
--- /dev/null
+++ b/common/configuration/1password_provider_test.go
@@ -0,0 +1,55 @@
+//go:build 1password
+
+// 1password needs to be running and have a vault named "ftl test".
+//
+// These tests will clean up before and after itself.
+
+package configuration
+
+import (
+	"context"
+	"testing"
+
+	"github.com/alecthomas/assert/v2"
+
+	"github.com/TBD54566975/ftl/internal/exec"
+	"github.com/TBD54566975/ftl/internal/log"
+)
+
+const vault = "ftl test"
+const module = "test.module"
+
+func clean(ctx context.Context) bool {
+	args := []string{"item", "delete", "--vault", vault, module}
+	_, err := exec.Capture(ctx, ".", "op", args...)
+	return err == nil
+}
+
+func Test1PasswordProvider(t *testing.T) {
+	ctx := log.ContextWithNewDefaultLogger(context.Background())
+
+	// OK to fail the initial clean.
+	_ = clean(ctx)
+	defer func() {
+		if !clean(ctx) {
+			t.Fatal("clean failed")
+		}
+	}()
+
+	_, err := getItem(ctx, vault, Ref{Name: module})
+	assert.Error(t, err)
+
+	err = createItem(ctx, vault, Ref{Name: module}, "hunter1")
+	assert.NoError(t, err)
+
+	value, err := getItem(ctx, vault, Ref{Name: module})
+	assert.NoError(t, err)
+	assert.Equal(t, "hunter1", value.Fields[0].Value)
+
+	err = editItem(ctx, vault, Ref{Name: module}, "hunter2")
+	assert.NoError(t, err)
+
+	value, err = getItem(ctx, vault, Ref{Name: module})
+	assert.NoError(t, err)
+	assert.Equal(t, "hunter2", value.Fields[0].Value)
+}

From a48a08efb48139472b3c6b94a265193bcaf04074 Mon Sep 17 00:00:00 2001
From: gak <gak@gak0.com>
Date: Wed, 22 May 2024 08:56:45 +1000
Subject: [PATCH 05/10] test: new items in 1p have multiple entries so dont
 assume one

---
 common/configuration/1password_provider.go      | 13 +++++++++----
 common/configuration/1password_provider_test.go |  8 ++++++--
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/common/configuration/1password_provider.go b/common/configuration/1password_provider.go
index 58809756cc..2321648f7a 100644
--- a/common/configuration/1password_provider.go
+++ b/common/configuration/1password_provider.go
@@ -37,14 +37,12 @@ func (o OnePasswordProvider) Load(ctx context.Context, ref Ref, key *url.URL) ([
 		return nil, fmt.Errorf("get item failed: %w", err)
 	}
 
-	secret, ok := slices.Find(full.Fields, func(item entry) bool {
-		return item.ID == "password"
-	})
+	secret, ok := full.value("password")
 	if !ok {
 		return nil, fmt.Errorf("password field not found in item %q", ref)
 	}
 
-	jsonSecret, err := json.Marshal(secret.Value)
+	jsonSecret, err := json.Marshal(secret)
 	if err != nil {
 		return nil, fmt.Errorf("json marshal failed: %w", err)
 	}
@@ -119,6 +117,13 @@ type entry struct {
 	Value string `json:"value"`
 }
 
+func (i item) value(field string) (string, bool) {
+	secret, ok := slices.Find(i.Fields, func(item entry) bool {
+		return item.ID == field
+	})
+	return secret.Value, ok
+}
+
 // op --format json item get --vault Personal "With Spaces"
 func getItem(ctx context.Context, vault string, ref Ref) (*item, error) {
 	logger := log.FromContext(ctx)
diff --git a/common/configuration/1password_provider_test.go b/common/configuration/1password_provider_test.go
index a8fdd86122..c267572bf7 100644
--- a/common/configuration/1password_provider_test.go
+++ b/common/configuration/1password_provider_test.go
@@ -44,12 +44,16 @@ func Test1PasswordProvider(t *testing.T) {
 
 	value, err := getItem(ctx, vault, Ref{Name: module})
 	assert.NoError(t, err)
-	assert.Equal(t, "hunter1", value.Fields[0].Value)
+	secret, ok := value.value("password")
+	assert.True(t, ok)
+	assert.Equal(t, "hunter1", secret)
 
 	err = editItem(ctx, vault, Ref{Name: module}, "hunter2")
 	assert.NoError(t, err)
 
 	value, err = getItem(ctx, vault, Ref{Name: module})
 	assert.NoError(t, err)
-	assert.Equal(t, "hunter2", value.Fields[0].Value)
+	secret, ok = value.value("password")
+	assert.True(t, ok)
+	assert.Equal(t, "hunter2", secret)
 }

From a7e5f2306f46431c8d3c4e9576e0fa34d112b497 Mon Sep 17 00:00:00 2001
From: gak <gak@gak0.com>
Date: Wed, 22 May 2024 09:28:06 +1000
Subject: [PATCH 06/10] fix: don't allow spaces in vault names

---
 common/configuration/1password_provider.go      | 3 +++
 common/configuration/1password_provider_test.go | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/common/configuration/1password_provider.go b/common/configuration/1password_provider.go
index 2321648f7a..16846ea0d1 100644
--- a/common/configuration/1password_provider.go
+++ b/common/configuration/1password_provider.go
@@ -58,6 +58,9 @@ func (o OnePasswordProvider) Store(ctx context.Context, ref Ref, value []byte) (
 	if err := checkOpBinary(); err != nil {
 		return nil, err
 	}
+	if strings.Contains(o.Vault, " ") {
+		return nil, fmt.Errorf("1Password vault name %q contains spaces, which is not supported", o.Vault)
+	}
 
 	var secret string
 	err := json.Unmarshal(value, &secret)
diff --git a/common/configuration/1password_provider_test.go b/common/configuration/1password_provider_test.go
index c267572bf7..94b00982e9 100644
--- a/common/configuration/1password_provider_test.go
+++ b/common/configuration/1password_provider_test.go
@@ -1,6 +1,6 @@
 //go:build 1password
 
-// 1password needs to be running and have a vault named "ftl test".
+// 1password needs to be running and have a vault named "ftl-test".
 //
 // These tests will clean up before and after itself.
 
@@ -16,7 +16,7 @@ import (
 	"github.com/TBD54566975/ftl/internal/log"
 )
 
-const vault = "ftl test"
+const vault = "ftl-test"
 const module = "test.module"
 
 func clean(ctx context.Context) bool {

From c44e912b00d32c4b7a55ab29195a908b661024fe Mon Sep 17 00:00:00 2001
From: gak <gak@gak0.com>
Date: Wed, 22 May 2024 14:39:37 +1000
Subject: [PATCH 07/10] fix: pr fixes (more to come)

---
 common/configuration/1password_provider.go    | 55 +++++++++----------
 .../configuration/1password_provider_test.go  | 35 ++++++++----
 common/configuration/aws_secrets_provider.go  | 43 +++++++++++++++
 3 files changed, 93 insertions(+), 40 deletions(-)
 create mode 100644 common/configuration/aws_secrets_provider.go

diff --git a/common/configuration/1password_provider.go b/common/configuration/1password_provider.go
index 16846ea0d1..ce564ee331 100644
--- a/common/configuration/1password_provider.go
+++ b/common/configuration/1password_provider.go
@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"github.com/kballard/go-shellquote"
 	"net/url"
 	"strings"
 
@@ -25,7 +26,7 @@ func (OnePasswordProvider) Role() Secrets                               { return
 func (o OnePasswordProvider) Key() string                               { return "op" }
 func (o OnePasswordProvider) Delete(ctx context.Context, ref Ref) error { return nil }
 
-// Load returns the secret stored in 1password, quoted as a JSON string.
+// Load returns the secret stored in 1password.
 func (o OnePasswordProvider) Load(ctx context.Context, ref Ref, key *url.URL) ([]byte, error) {
 	if err := checkOpBinary(); err != nil {
 		return nil, err
@@ -37,17 +38,19 @@ func (o OnePasswordProvider) Load(ctx context.Context, ref Ref, key *url.URL) ([
 		return nil, fmt.Errorf("get item failed: %w", err)
 	}
 
-	secret, ok := full.value("password")
+	secret, ok := full.password()
 	if !ok {
 		return nil, fmt.Errorf("password field not found in item %q", ref)
 	}
 
-	jsonSecret, err := json.Marshal(secret)
+	// Just to verify that it is JSON encoded.
+	var decoded interface{}
+	err = json.Unmarshal(secret, &decoded)
 	if err != nil {
-		return nil, fmt.Errorf("json marshal failed: %w", err)
+		return nil, fmt.Errorf("secret is not JSON encoded: %w", err)
 	}
 
-	return jsonSecret, nil
+	return secret, nil
 }
 
 // Store will save the given secret in 1Password via the `op` command.
@@ -62,18 +65,11 @@ func (o OnePasswordProvider) Store(ctx context.Context, ref Ref, value []byte) (
 		return nil, fmt.Errorf("1Password vault name %q contains spaces, which is not supported", o.Vault)
 	}
 
-	var secret string
-	err := json.Unmarshal(value, &secret)
-	if err != nil {
-		return nil, fmt.Errorf("json unmarshal failed: %w", err)
-	}
-
 	url := &url.URL{Scheme: "op", Host: o.Vault}
 
-	_, err = getItem(ctx, o.Vault, ref)
-	var notFound notFoundError
-	if errors.As(err, &notFound) {
-		err = createItem(ctx, o.Vault, ref, secret)
+	_, err := getItem(ctx, o.Vault, ref)
+	if errors.As(err, new(itemNotFoundError)) {
+		err = createItem(ctx, o.Vault, ref, value)
 		if err != nil {
 			return nil, fmt.Errorf("create item failed: %w", err)
 		}
@@ -83,7 +79,7 @@ func (o OnePasswordProvider) Store(ctx context.Context, ref Ref, value []byte) (
 		return nil, fmt.Errorf("get item failed: %w", err)
 	}
 
-	err = editItem(ctx, o.Vault, ref, secret)
+	err = editItem(ctx, o.Vault, ref, value)
 	if err != nil {
 		return nil, fmt.Errorf("edit item failed: %w", err)
 	}
@@ -101,12 +97,12 @@ func checkOpBinary() error {
 	return nil
 }
 
-type notFoundError struct {
+type itemNotFoundError struct {
 	vault string
 	ref   Ref
 }
 
-func (e notFoundError) Error() string {
+func (e itemNotFoundError) Error() string {
 	return fmt.Sprintf("item %q not found in vault %q", e.ref, e.vault)
 }
 
@@ -120,11 +116,11 @@ type entry struct {
 	Value string `json:"value"`
 }
 
-func (i item) value(field string) (string, bool) {
+func (i item) password() ([]byte, bool) {
 	secret, ok := slices.Find(i.Fields, func(item entry) bool {
-		return item.ID == field
+		return item.ID == "password"
 	})
-	return secret.Value, ok
+	return []byte(secret.Value), ok
 }
 
 // op --format json item get --vault Personal "With Spaces"
@@ -133,21 +129,22 @@ func getItem(ctx context.Context, vault string, ref Ref) (*item, error) {
 
 	args := []string{"--format", "json", "item", "get", "--vault", vault, ref.String()}
 	output, err := exec.Capture(ctx, ".", "op", args...)
-	logger.Debugf("Getting item with args %v", args)
+	logger.Debugf("Getting item with args %s", shellquote.Join(args...))
 	if err != nil {
+		// This is specifically not itemNotFoundError, ty distinguish between vault not found and item not found.
 		if strings.Contains(string(output), "isn't a vault") {
 			return nil, fmt.Errorf("vault %q not found: %w", vault, err)
 		}
 
 		// Item not found, seen two ways of reporting this:
 		if strings.Contains(string(output), "not found in vault") {
-			return nil, notFoundError{vault, ref}
+			return nil, itemNotFoundError{vault, ref}
 		}
 		if strings.Contains(string(output), "isn't an item") {
-			return nil, notFoundError{vault, ref}
+			return nil, itemNotFoundError{vault, ref}
 		}
 
-		return nil, fmt.Errorf("run `op` with args %v: %w", args, err)
+		return nil, fmt.Errorf("run `op` with args %s: %w", shellquote.Join(args...), err)
 	}
 
 	var full item
@@ -158,8 +155,8 @@ func getItem(ctx context.Context, vault string, ref Ref) (*item, error) {
 }
 
 // op item create --category Password --vault FTL --title mod.ule "password=val ue"
-func createItem(ctx context.Context, vault string, ref Ref, secret string) error {
-	args := []string{"item", "create", "--category", "Password", "--vault", vault, "--title", ref.String(), "password=" + secret}
+func createItem(ctx context.Context, vault string, ref Ref, secret []byte) error {
+	args := []string{"item", "create", "--category", "Password", "--vault", vault, "--title", ref.String(), "password=" + string(secret)}
 	_, err := exec.Capture(ctx, ".", "op", args...)
 	if err != nil {
 		return fmt.Errorf("create item failed in vault %q, ref %q: %w", vault, ref, err)
@@ -169,8 +166,8 @@ func createItem(ctx context.Context, vault string, ref Ref, secret string) error
 }
 
 // op item edit --vault ftl test "password=with space"
-func editItem(ctx context.Context, vault string, ref Ref, secret string) error {
-	args := []string{"item", "edit", "--vault", vault, ref.String(), "password=" + secret}
+func editItem(ctx context.Context, vault string, ref Ref, secret []byte) error {
+	args := []string{"item", "edit", "--vault", vault, ref.String(), "password=" + string(secret)}
 	_, err := exec.Capture(ctx, ".", "op", args...)
 	if err != nil {
 		return fmt.Errorf("edit item failed in vault %q, ref %q: %w", vault, ref, err)
diff --git a/common/configuration/1password_provider_test.go b/common/configuration/1password_provider_test.go
index 94b00982e9..99dde9a5b5 100644
--- a/common/configuration/1password_provider_test.go
+++ b/common/configuration/1password_provider_test.go
@@ -1,6 +1,6 @@
 //go:build 1password
 
-// 1password needs to be running and have a vault named "ftl-test".
+// 1password needs to be running and will temporarily make a "ftl-test" vault.
 //
 // These tests will clean up before and after itself.
 
@@ -19,8 +19,14 @@ import (
 const vault = "ftl-test"
 const module = "test.module"
 
+func createVault(ctx context.Context) error {
+	args := []string{"vault", "create", vault}
+	_, err := exec.Capture(ctx, ".", "op", args...)
+	return err
+}
+
 func clean(ctx context.Context) bool {
-	args := []string{"item", "delete", "--vault", vault, module}
+	args := []string{"vault", "delete", vault}
 	_, err := exec.Capture(ctx, ".", "op", args...)
 	return err == nil
 }
@@ -30,30 +36,37 @@ func Test1PasswordProvider(t *testing.T) {
 
 	// OK to fail the initial clean.
 	_ = clean(ctx)
-	defer func() {
+	t.Cleanup(func() {
 		if !clean(ctx) {
 			t.Fatal("clean failed")
 		}
-	}()
+	})
 
-	_, err := getItem(ctx, vault, Ref{Name: module})
+	err := createVault(ctx)
+	assert.NoError(t, err)
+
+	_, err = getItem(ctx, vault, Ref{Name: module})
 	assert.Error(t, err)
 
-	err = createItem(ctx, vault, Ref{Name: module}, "hunter1")
+	var pw1 = []byte("hunter1")
+	// {"user":"root", "password":"hunter2🪤"}
+	var pw2 = []byte("{\"user\":\"root\", \"password\":\"hunter🪤\"}")
+
+	err = createItem(ctx, vault, Ref{Name: module}, pw1)
 	assert.NoError(t, err)
 
 	value, err := getItem(ctx, vault, Ref{Name: module})
 	assert.NoError(t, err)
-	secret, ok := value.value("password")
+	secret, ok := value.password()
 	assert.True(t, ok)
-	assert.Equal(t, "hunter1", secret)
+	assert.Equal(t, pw1, secret)
 
-	err = editItem(ctx, vault, Ref{Name: module}, "hunter2")
+	err = editItem(ctx, vault, Ref{Name: module}, pw2)
 	assert.NoError(t, err)
 
 	value, err = getItem(ctx, vault, Ref{Name: module})
 	assert.NoError(t, err)
-	secret, ok = value.value("password")
+	secret, ok = value.password()
 	assert.True(t, ok)
-	assert.Equal(t, "hunter2", secret)
+	assert.Equal(t, pw2, secret)
 }
diff --git a/common/configuration/aws_secrets_provider.go b/common/configuration/aws_secrets_provider.go
new file mode 100644
index 0000000000..cd27546b23
--- /dev/null
+++ b/common/configuration/aws_secrets_provider.go
@@ -0,0 +1,43 @@
+package configuration
+
+import (
+	"context"
+	_ "github.com/aws/aws-sdk-go-v2/aws"
+	"github.com/aws/aws-sdk-go-v2/config"
+	"github.com/aws/aws-sdk-go-v2/service/secretsmanager"
+	_ "github.com/aws/aws-sdk-go-v2/service/secretsmanager"
+	"log"
+)
+
+func brainstorm() {
+	cfg, err := config.LoadDefaultConfig(context.TODO(),
+		config.WithRegion("us-west-2"),
+	)
+	if err != nil {
+		log.Fatalf("unable to load SDK config, %v", err)
+	}
+
+	svc := secretsmanager.NewFromConfig(cfg)
+
+	// create a secret
+	name := "test-secret"
+	secret := "hunter1"
+	_, err = svc.CreateSecret(context.TODO(), &secretsmanager.CreateSecretInput{
+		Name:         &name,
+		SecretString: &secret,
+	})
+	if err != nil {
+		log.Fatalf("failed to create secret, %v", err)
+	}
+
+	// get the secret
+	out, err := svc.GetSecretValue(context.TODO(), &secretsmanager.GetSecretValueInput{
+		SecretId: &name,
+	})
+	if err != nil {
+		log.Fatalf("failed to retrieve secret, %v", err)
+	}
+
+	log.Printf("retrieved secret: %s", *out.SecretString)
+
+}

From 3c1ce74c39e5d4f8b16a23f394fd7a7ed08b20b5 Mon Sep 17 00:00:00 2001
From: gak <gak@gak0.com>
Date: Thu, 23 May 2024 05:53:20 +1000
Subject: [PATCH 08/10] fix: oops

---
 common/configuration/aws_secrets_provider.go | 43 --------------------
 1 file changed, 43 deletions(-)
 delete mode 100644 common/configuration/aws_secrets_provider.go

diff --git a/common/configuration/aws_secrets_provider.go b/common/configuration/aws_secrets_provider.go
deleted file mode 100644
index cd27546b23..0000000000
--- a/common/configuration/aws_secrets_provider.go
+++ /dev/null
@@ -1,43 +0,0 @@
-package configuration
-
-import (
-	"context"
-	_ "github.com/aws/aws-sdk-go-v2/aws"
-	"github.com/aws/aws-sdk-go-v2/config"
-	"github.com/aws/aws-sdk-go-v2/service/secretsmanager"
-	_ "github.com/aws/aws-sdk-go-v2/service/secretsmanager"
-	"log"
-)
-
-func brainstorm() {
-	cfg, err := config.LoadDefaultConfig(context.TODO(),
-		config.WithRegion("us-west-2"),
-	)
-	if err != nil {
-		log.Fatalf("unable to load SDK config, %v", err)
-	}
-
-	svc := secretsmanager.NewFromConfig(cfg)
-
-	// create a secret
-	name := "test-secret"
-	secret := "hunter1"
-	_, err = svc.CreateSecret(context.TODO(), &secretsmanager.CreateSecretInput{
-		Name:         &name,
-		SecretString: &secret,
-	})
-	if err != nil {
-		log.Fatalf("failed to create secret, %v", err)
-	}
-
-	// get the secret
-	out, err := svc.GetSecretValue(context.TODO(), &secretsmanager.GetSecretValueInput{
-		SecretId: &name,
-	})
-	if err != nil {
-		log.Fatalf("failed to retrieve secret, %v", err)
-	}
-
-	log.Printf("retrieved secret: %s", *out.SecretString)
-
-}

From f47ac6c4f653c01610de72b99bc3514c0a086ad9 Mon Sep 17 00:00:00 2001
From: gak <gak@gak0.com>
Date: Thu, 23 May 2024 05:53:45 +1000
Subject: [PATCH 09/10] fix: use allowlist instead of just space. help tweak.

---
 common/configuration/1password_provider.go | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/common/configuration/1password_provider.go b/common/configuration/1password_provider.go
index ce564ee331..f699d05f65 100644
--- a/common/configuration/1password_provider.go
+++ b/common/configuration/1password_provider.go
@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"github.com/kballard/go-shellquote"
 	"net/url"
+	"regexp"
 	"strings"
 
 	"github.com/TBD54566975/ftl/internal/exec"
@@ -17,7 +18,7 @@ import (
 // OnePasswordProvider is a configuration provider that reads passwords from
 // 1Password vaults via the "op" command line tool.
 type OnePasswordProvider struct {
-	Vault string `name:"op" help:"Store a secret in this 1Password vault." group:"Provider:" xor:"configwriter" placeholder:"VAULT"`
+	Vault string `name:"op" help:"Store a secret in this 1Password vault. The name of the 1Password item will be the <ref> and the secret will be stored in the password field." group:"Provider:" xor:"configwriter" placeholder:"VAULT"`
 }
 
 var _ MutableProvider[Secrets] = OnePasswordProvider{}
@@ -53,6 +54,8 @@ func (o OnePasswordProvider) Load(ctx context.Context, ref Ref, key *url.URL) ([
 	return secret, nil
 }
 
+var vaultRegex = regexp.MustCompile(`^[a-zA-Z0-9_\-.]+$`)
+
 // Store will save the given secret in 1Password via the `op` command.
 //
 // op does not support "create or update" as a single command. Neither does it support specifying an ID on create.
@@ -61,8 +64,8 @@ func (o OnePasswordProvider) Store(ctx context.Context, ref Ref, value []byte) (
 	if err := checkOpBinary(); err != nil {
 		return nil, err
 	}
-	if strings.Contains(o.Vault, " ") {
-		return nil, fmt.Errorf("1Password vault name %q contains spaces, which is not supported", o.Vault)
+	if !vaultRegex.MatchString(o.Vault) {
+		return nil, fmt.Errorf("vault name %q contains invalid characters. a-z A-Z 0-9 _ . - are valid", o.Vault)
 	}
 
 	url := &url.URL{Scheme: "op", Host: o.Vault}
@@ -131,7 +134,7 @@ func getItem(ctx context.Context, vault string, ref Ref) (*item, error) {
 	output, err := exec.Capture(ctx, ".", "op", args...)
 	logger.Debugf("Getting item with args %s", shellquote.Join(args...))
 	if err != nil {
-		// This is specifically not itemNotFoundError, ty distinguish between vault not found and item not found.
+		// This is specifically not itemNotFoundError, to distinguish between vault not found and item not found.
 		if strings.Contains(string(output), "isn't a vault") {
 			return nil, fmt.Errorf("vault %q not found: %w", vault, err)
 		}

From 06ba1f7bb5d756baf4c1d35b4ec8b70076c0ce19 Mon Sep 17 00:00:00 2001
From: gak <gak@gak0.com>
Date: Thu, 23 May 2024 06:01:53 +1000
Subject: [PATCH 10/10] test: slighty more readable json

---
 common/configuration/1password_provider_test.go | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/common/configuration/1password_provider_test.go b/common/configuration/1password_provider_test.go
index 99dde9a5b5..51fd4976a9 100644
--- a/common/configuration/1password_provider_test.go
+++ b/common/configuration/1password_provider_test.go
@@ -49,8 +49,10 @@ func Test1PasswordProvider(t *testing.T) {
 	assert.Error(t, err)
 
 	var pw1 = []byte("hunter1")
-	// {"user":"root", "password":"hunter2🪤"}
-	var pw2 = []byte("{\"user\":\"root\", \"password\":\"hunter🪤\"}")
+	var pw2 = []byte(`{
+	  "user": "root",
+	  "password": "hunter🪤"
+	}`)
 
 	err = createItem(ctx, vault, Ref{Name: module}, pw1)
 	assert.NoError(t, err)