diff --git a/authority.go b/authority.go index bd01a4d..11cd9e9 100644 --- a/authority.go +++ b/authority.go @@ -95,6 +95,9 @@ func ReadAuthority(ctx context.Context, names ...AuthorityValueName) (*Authority continue } value, err := query.workspaceValueLive(name.Name, name.Key) + if err != nil && !isWorkspaceValueAbsent(err) { + return nil, fmt.Errorf("%w: %s cannot be read: %w", ErrAuthorityValueMissing, name, err) + } if err != nil || strings.TrimSpace(value) == "" { return nil, fmt.Errorf("%w: %s", ErrAuthorityValueMissing, name) } @@ -164,6 +167,9 @@ func (a *Authority) Recheck(ctx context.Context) error { query := For(ctx) for _, name := range a.Names() { value, err := query.workspaceValueLive(name.Name, name.Key) + if err != nil && !isWorkspaceValueAbsent(err) { + return fmt.Errorf("%w: %s cannot be read: %w", ErrAuthorityValueChanged, name, err) + } if err != nil || strings.TrimSpace(value) == "" { return fmt.Errorf("%w: %s is no longer configured", ErrAuthorityValueChanged, name) } diff --git a/codefly.go b/codefly.go index d435e9c..e479e7f 100644 --- a/codefly.go +++ b/codefly.go @@ -269,12 +269,13 @@ func codeflyEnvironmentVariables() []string { return append([]string(nil), environmentVariablesSnapshot...) } -// deliveredNames lists, from the carriers the SDK already holds, the name of -// every value that is delivered non-empty: the snapshot (process values, the -// values file carriers delivered at load, and injected values) and the live -// process environment, where an inline value is listed by its own name and a -// file carrier by the key it carries. It reads no file: a carrier's path is -// evidence of delivery, and its content is read only by the per-key lookup, so +// deliveredNames lists the name of every carrier the SDK already holds with a +// non-empty value: the snapshot (process values, the values file carriers +// delivered at load, and injected values) and the live process environment, +// where an inline value is listed by its own name and a file carrier by the +// key it carries. For a live file carrier the non-empty value is its PATH: no +// file is opened, so a carrier naming an empty file is listed, and the per-key +// lookup then answers absent. Its content is read only by that lookup, so // listing a secret namespace neither materialises nor caches a secret value. func deliveredNames() []string { names := map[string]struct{}{} diff --git a/file_carrier_test.go b/file_carrier_test.go index 91a7bfe..3051879 100644 --- a/file_carrier_test.go +++ b/file_carrier_test.go @@ -129,3 +129,49 @@ func TestAnUnreadableFileCarrierFailsClosed(t *testing.T) { require.ErrorIs(t, err, resources.ErrFileCarrier) require.Error(t, codefly.LoadEnvironmentVariables()) } + +// A public delivery that cannot be read is refused with its reason. It is +// never answered from the secret namespace under the same name: that is a +// different delivery, and answering with it would silently replace the value +// the host meant this process to read (review r139 probed exactly this and got +// "secretvalue" back with no error). The same holds where the value is read as +// authority: ReadAuthority refuses, and Recheck refuses a pinned value whose +// public delivery broke even when the secret namespace holds the pinned value. +func TestAnUnreadablePublicValueIsNeverAnsweredFromTheSecretNamespace(t *testing.T) { + ctx := context.Background() + query := codefly.For(ctx) + public := resources.WorkspaceConfigurationPrefix + "__PROBE_FB__BINDING" + t.Setenv(resources.FileCarrierKey(public), filepath.Join(t.TempDir(), "absent")) + t.Setenv(resources.WorkspaceSecretConfigurationPrefix+"__PROBE_FB__BINDING", "secretvalue") + + // The premise: the public read fails, and the secret namespace answers. + _, err := query.WorkspaceConfiguration("probe-fb", "BINDING") + require.ErrorIs(t, err, resources.ErrFileCarrier) + secret, err := query.WorkspaceSecret("probe-fb", "BINDING") + require.NoError(t, err) + require.Equal(t, "secretvalue", secret) + + value, err := query.WorkspaceValue("probe-fb", "BINDING") + require.ErrorIs(t, err, resources.ErrFileCarrier, "the refusal names the public delivery's failure") + require.NotErrorIs(t, err, codefly.ErrWorkspaceConfigurationAbsent) + require.Empty(t, value) + require.NotContains(t, err.Error(), "secretvalue") + + _, err = codefly.ReadAuthority(ctx, codefly.AuthorityValueName{Name: "probe-fb", Key: "BINDING"}) + require.ErrorIs(t, err, codefly.ErrAuthorityValueMissing) + require.ErrorIs(t, err, resources.ErrFileCarrier) + + pinned := resources.WorkspaceConfigurationPrefix + "__PROBE_FB_PINNED__BINDING" + t.Setenv(pinned, "pinnedvalue") + authority, err := codefly.ReadAuthority(ctx, codefly.AuthorityValueName{Name: "probe-fb-pinned", Key: "BINDING"}) + require.NoError(t, err) + require.NoError(t, os.Unsetenv(pinned)) + t.Setenv(resources.FileCarrierKey(pinned), filepath.Join(t.TempDir(), "absent")) + t.Setenv(resources.WorkspaceSecretConfigurationPrefix+"__PROBE_FB_PINNED__BINDING", "pinnedvalue") + + err = authority.Recheck(ctx) + require.ErrorIs(t, err, codefly.ErrAuthorityValueChanged) + require.ErrorIs(t, err, resources.ErrFileCarrier) + _, err = query.WorkspaceValue("probe-fb-pinned", "BINDING") + require.ErrorIs(t, err, resources.ErrFileCarrier) +} diff --git a/for.go b/for.go index d6d66d8..aa1e04f 100644 --- a/for.go +++ b/for.go @@ -406,14 +406,17 @@ var ErrWorkspaceConfigurationAbsent = errors.New("no workspace configuration val // Codefly's environment variable encoding. // // Each key is spelled so that WorkspaceConfiguration(name, key) reads its -// value, and only keys a carrier delivers non-empty are listed. A group no -// carrier holds returns an empty list and an error wrapping +// value. A key is listed when a carrier names it: an inline value that is +// non-empty, or a file carrier whose path is non-empty. A group no carrier +// holds returns an empty list and an error wrapping // ErrWorkspaceConfigurationAbsent. // // Listing reads names, never values: no file a carrier names is opened, and -// no authority pin is read. A value is read through the per-key accessors, -// which apply their own rules, so a listed key whose delivery cannot be read -// fails there, naming its reason, rather than being dropped from the list. +// no authority pin is read. A file's content is judged only by the per-key +// accessors, which apply their own rules, so a listed key can still read as +// absent (its file is empty) or fail with a named reason (its file is missing, +// or it is delivered both inline and by file). It is never dropped from the +// list. func (q *Query) WorkspaceConfigurationKeys(name string) ([]string, error) { return workspaceConfigurationKeys(resources.WorkspaceConfigurationPrefix, name) } @@ -458,6 +461,11 @@ func workspaceConfigurationKeys(prefix string, name string) ([]string, error) { // then the secret namespace. It is intended for settings whose sensitivity is // deployment-defined while preserving a single SDK-only lookup boundary. // +// Only an absent public value moves the lookup on to the secret namespace. A +// public delivery that cannot be read (a missing file, a value delivered both +// inline and by file) is refused with that reason, and the secret namespace +// is not consulted. +// // A value this process read as authority-bearing (see ReadAuthority) answers // from what was read at boot, and a live value that has since drifted is // refused with ErrAuthorityValueChanged rather than returned. Without that, a @@ -467,6 +475,9 @@ func (q *Query) WorkspaceValue(name string, key string) (string, error) { addressed := AuthorityValueName{Name: name, Key: key}.canonical() pinned, isPinned := authorityPin(addressed) value, err := q.workspaceValueLive(name, key) + if err != nil && !isWorkspaceValueAbsent(err) { + return "", err + } if !isPinned { return value, err } @@ -479,16 +490,16 @@ func (q *Query) WorkspaceValue(name string, key string) (string, error) { return pinned, nil } -// WorkspaceValueIfSet resolves a workspace value and distinguishes the three -// answers WorkspaceValue flattens into two: the value is set, the value is -// genuinely not configured, or the lookup FAILED. +// WorkspaceValueIfSet resolves a workspace value and answers absence without +// an error: the value is set, the value is genuinely not configured, or the +// lookup FAILED. // -// WorkspaceValue reports "not configured" and "the delivery could not be read" -// with the same error, because its callers want a value or nothing. A caller -// deciding a posture question cannot live with that: a setting that must be -// asserted before something is permitted has to treat an unreadable delivery, -// and an authority-bearing value that has drifted, as a refusal rather than as -// "the operator did not assert it". Absence is an answer; a failed read is not. +// WorkspaceValue answers absence with an error, because its callers want a +// value or nothing. A caller deciding a posture question needs absence as an +// answer: a setting that must be asserted before something is permitted has to +// treat an unreadable delivery, and an authority-bearing value that has +// drifted, as a refusal rather than as "the operator did not assert it". +// Absence is an answer; a failed read is not. // // So an unreadable file carrier, a value delivered both inline and by file, and // ErrAuthorityValueChanged all come back as errors here, and only a value no @@ -540,14 +551,20 @@ func isWorkspaceValueAbsent(err error) bool { return errors.Is(err, ErrWorkspaceConfigurationAbsent) } +// workspaceValueLive is workspaceValueIfSetLive with absence as an error. A +// public delivery that cannot be read is returned as the failure it is, never +// stepped over to the secret namespace: a value under the same name there is a +// different delivery, and answering with it would let an unreadable public +// value be silently replaced. func (q *Query) workspaceValueLive(name string, key string) (string, error) { - if value, err := q.WorkspaceConfiguration(name, key); err == nil && value != "" { - return value, nil + value, found, err := q.workspaceValueIfSetLive(name, key) + if err != nil { + return "", err } - if value, err := q.WorkspaceSecret(name, key); err == nil && value != "" { - return value, nil + if !found { + return "", fmt.Errorf("%w for %s/%s", ErrWorkspaceConfigurationAbsent, name, key) } - return "", fmt.Errorf("%w for %s/%s", ErrWorkspaceConfigurationAbsent, name, key) + return value, nil } func (q *Query) workspaceConfigurationValue(prefix string, name string, key string) (string, error) { diff --git a/workspace_configuration_keys_test.go b/workspace_configuration_keys_test.go index 0c410d2..49027eb 100644 --- a/workspace_configuration_keys_test.go +++ b/workspace_configuration_keys_test.go @@ -176,4 +176,36 @@ func TestWorkspaceConfigurationKeysSeparatesAbsenceFromFailure(t *testing.T) { assert.NotErrorIs(t, err, codefly.ErrWorkspaceConfigurationAbsent, "an unreadable delivery is not absence") assert.ErrorIs(t, err, resources.ErrFileCarrier) }) + + t.Run("a file carrier naming an empty file", func(t *testing.T) { + key := "CODEFLY__WORKSPACE_CONFIGURATION__KEYS_EMPTY_FILE__BINDING" + path := filepath.Join(t.TempDir(), "empty") + require.NoError(t, os.WriteFile(path, nil, 0o600)) + t.Setenv(resources.FileCarrierKey(key), path) + + keys, err := codefly.For(ctx).WorkspaceConfigurationKeys("keys-empty-file") + + require.NoError(t, err) + assert.Equal(t, []string{"BINDING"}, keys, "listing opens no file, so the carrier's path is what it sees") + _, err = codefly.For(ctx).WorkspaceConfiguration("keys-empty-file", "BINDING") + assert.ErrorIs(t, err, codefly.ErrWorkspaceConfigurationAbsent, "an empty file reads as absent") + }) + + t.Run("a value delivered both inline and by file", func(t *testing.T) { + key := "CODEFLY__WORKSPACE_CONFIGURATION__KEYS_TWICE__BINDING" + path := filepath.Join(t.TempDir(), "binding") + require.NoError(t, os.WriteFile(path, []byte("by file"), 0o600)) + t.Setenv(key, "inline") + t.Setenv(resources.FileCarrierKey(key), path) + + keys, err := codefly.For(ctx).WorkspaceConfigurationKeys("keys-twice") + + require.NoError(t, err) + assert.Equal(t, []string{"BINDING"}, keys) + _, err = codefly.For(ctx).WorkspaceConfiguration("keys-twice", "BINDING") + require.Error(t, err) + assert.NotErrorIs(t, err, codefly.ErrWorkspaceConfigurationAbsent, "two sources for one fact is not absence") + assert.ErrorIs(t, err, resources.ErrFileCarrier) + assert.ErrorContains(t, err, "delivered both inline and by file") + }) }