Skip to content

fix incorrect return value from ensureOIDCProvider #63

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
travisbcotton wants to merge 2 commits into
base: main
Choose a base branch
from
+226 −17
Open

fix incorrect return value from ensureOIDCProvider #63

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f243609
fix incorrect return value from ensureOIDCProvider
travisbcotton Sep 24, 2026
0808ac2
fix lint
travisbcotton Sep 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 31 additions & 10 deletions internal/vault/client_vault.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,13 +338,26 @@ func (c *vaultClient) EnsureOIDCConfig(ctx context.Context, clusterName string,
return creds, nil
}

// expectedOIDCProviderIssuer derives the effective OIDC issuer Vault reports for
// the shared named provider from the configured base issuer. The operator writes
// the base (scheme+host, no path) as the provider's `issuer`, and Vault appends
// `/v1/identity/oidc/provider/<name>` when serving the provider and minting the
// `iss` claim. Comparisons against Vault's read-back MUST use this effective
// value, not the base, or a provider the operator just created would be
// misreported as a conflict (issue #62).
func expectedOIDCProviderIssuer(base string) string {
return strings.TrimRight(base, "/") +
"/v1/identity/oidc/provider/" +
vaultOIDCProviderName
}

// ensureOIDCProvider creates or reconciles the shared named OIDC provider,
// treating its issuer as an invariant that all control planes sharing the Vault
// must agree on.
//
// - Provider does not exist: create it with the requested issuer, then read it
// back and verify the issuer landed (guards against a concurrent creator
// having won with a different issuer).
// - Provider does not exist: create it with the requested base issuer, then
// read it back and verify the effective issuer landed (guards against a
// concurrent creator having won with a different issuer).
// - Provider exists, issuer matches: reconcile the fields we manage
// (allowed_client_ids wildcard + scopes). allowed_client_ids is always the
// fixed wildcard ["*"], never a per-control-plane read-modify-write, so
Expand All @@ -353,9 +366,13 @@ func (c *vaultClient) EnsureOIDCConfig(ctx context.Context, clusterName string,
// NOT modify the provider. Silently overwriting would let two control planes
// flip the shared issuer back and forth on every reconcile (issue #58).
//
// The issuer is scheme+host(+port) with no path; Vault appends
// `/v1/identity/oidc/provider/<name>` itself when minting the `iss` claim, and
// tokensmith validates against that exact URL (see helpers.go).
// The configured issuer is scheme+host(+port) with no path; Vault appends
// `/v1/identity/oidc/provider/<name>` itself when serving the provider and
// minting the `iss` claim, and tokensmith validates against that exact URL (see
// helpers.go). We therefore write the base issuer but compare Vault's read-back
// against the effective issuer produced by expectedOIDCProviderIssuer — comparing
// the raw base directly would misreport a provider the operator just created as a
// conflict (issue #62).
//
// Caveat: read-before-create does not make concurrent initial creation atomic
// (both callers may GET 404 then POST). Vault exposes only create-or-update
Expand All @@ -365,13 +382,17 @@ func (c *vaultClient) EnsureOIDCConfig(ctx context.Context, clusterName string,
func (c *vaultClient) ensureOIDCProvider(ctx context.Context, cfg OIDCConfig) error {
path := "identity/oidc/provider/" + vaultOIDCProviderName

// Vault reports the effective provider issuer (base + provider path), so
// compare against that rather than the configured base issuer (issue #62).
expected := expectedOIDCProviderIssuer(cfg.IssuerURL)

existing, err := c.api.Logical().ReadWithContext(ctx, path)
if err != nil {
return fmt.Errorf("reading oidc provider %q: %w", vaultOIDCProviderName, err)
}
if existing != nil && existing.Data != nil {
if current, _ := existing.Data["issuer"].(string); current != "" && current != cfg.IssuerURL {
return &OIDCIssuerConflictError{Existing: current, Requested: cfg.IssuerURL}
if current, _ := existing.Data["issuer"].(string); current != "" && current != expected {
return &OIDCIssuerConflictError{Existing: current, Requested: expected}
}
}

Expand All @@ -395,8 +416,8 @@ func (c *vaultClient) ensureOIDCProvider(ctx context.Context, cfg OIDCConfig) er
if !ok || current == "" {
return fmt.Errorf("oidc provider %q returned no issuer after write", vaultOIDCProviderName)
}
if current != cfg.IssuerURL {
return &OIDCIssuerConflictError{Existing: current, Requested: cfg.IssuerURL}
if current != expected {
return &OIDCIssuerConflictError{Existing: current, Requested: expected}
}
return nil
}
Expand Down
202 changes: 195 additions & 7 deletions internal/vault/client_vault_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"context"
"encoding/json"
"errors"
"maps"
"net/http"
"net/http/httptest"
"testing"
Expand Down Expand Up @@ -51,9 +52,17 @@ func TestVaultClient_EnsureOIDCConfigCreatesConfidentialAndPublicClients(t *test
if r.Method == http.MethodGet && r.URL.Path == testProviderPath {
// Reflect whatever was last written so the read-before-write and
// post-create read-back see a consistent issuer. 404 until created.
// Real Vault returns the EFFECTIVE provider issuer (base + provider
// path), not the base the operator wrote, so mirror that here
// (issue #62).
if prev, ok := writes[testProviderPath]; ok {
w.Header().Set("Content-Type", "application/json")
resp := map[string]any{"data": prev}
data := map[string]any{}
maps.Copy(data, prev)
if base, ok := data["issuer"].(string); ok {
data["issuer"] = base + testProviderPath
}
resp := map[string]any{"data": data}
_ = json.NewEncoder(w).Encode(resp)
return
}
Expand Down Expand Up @@ -130,9 +139,10 @@ func TestVaultClient_EnsureOIDCConfigIssuerConflict(t *testing.T) {
providerWrites := 0
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.Method == http.MethodGet && r.URL.Path == testProviderPath {
// Provider already pinned to a different issuer by another CP.
// Provider already pinned to a different issuer by another CP. Vault
// reports the EFFECTIVE issuer (base + provider path) (issue #62).
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{"data":{"issuer":"https://other.example.test","allowed_client_ids":["*"]}}`))
_, _ = w.Write([]byte(`{"data":{"issuer":"https://other.example.test/v1/identity/oidc/provider/openchami","allowed_client_ids":["*"]}}`))
return
}
if r.Method == http.MethodPut && r.URL.Path == testProviderPath {
Expand Down Expand Up @@ -165,7 +175,8 @@ func TestVaultClient_EnsureOIDCConfigIssuerConflict(t *testing.T) {
if !errors.As(err, &conflict) {
t.Fatalf("expected OIDCIssuerConflictError, got %v", err)
}
if conflict.Existing != "https://other.example.test" || conflict.Requested != testAlphaIssuer {
if conflict.Existing != "https://other.example.test/v1/identity/oidc/provider/openchami" ||
conflict.Requested != testAlphaIssuer+testProviderPath {
t.Errorf("unexpected conflict detail: %+v", conflict)
}
if providerWrites != 0 {
Expand Down Expand Up @@ -193,9 +204,10 @@ func TestVaultClient_EnsureOIDCConfigReadBackConflict(t *testing.T) {
return
}
// Post-write read-back: a concurrent creator won with a
// different issuer.
// different issuer. Vault reports the EFFECTIVE issuer
// (base + provider path) (issue #62).
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{"data":{"issuer":"https://winner.example.test","allowed_client_ids":["*"]}}`))
_, _ = w.Write([]byte(`{"data":{"issuer":"https://winner.example.test/v1/identity/oidc/provider/openchami","allowed_client_ids":["*"]}}`))
return
case http.MethodPut:
w.Header().Set("Content-Type", "application/json")
Expand Down Expand Up @@ -227,7 +239,8 @@ func TestVaultClient_EnsureOIDCConfigReadBackConflict(t *testing.T) {
if !errors.As(err, &conflict) {
t.Fatalf("expected OIDCIssuerConflictError from read-back, got %v", err)
}
if conflict.Existing != "https://winner.example.test" || conflict.Requested != testAlphaIssuer {
if conflict.Existing != "https://winner.example.test/v1/identity/oidc/provider/openchami" ||
conflict.Requested != testAlphaIssuer+testProviderPath {
t.Errorf("unexpected conflict detail: %+v", conflict)
}
}
Expand Down Expand Up @@ -286,6 +299,181 @@ func TestVaultClient_EnsureOIDCConfigReadBackMissingIssuer(t *testing.T) {
}
}

// TestExpectedOIDCProviderIssuer verifies the base issuer is turned into the
// effective provider issuer Vault reports, tolerating a trailing slash on the
// configured base.
func TestExpectedOIDCProviderIssuer(t *testing.T) {
t.Parallel()

cases := []struct {
name string
base string
want string
}{
{
name: "no trailing slash",
base: "http://vault.vault.svc.cluster.local:8200",
want: "http://vault.vault.svc.cluster.local:8200/v1/identity/oidc/provider/openchami",
},
{
name: "trailing slash trimmed",
base: "https://vault.example.com/",
want: "https://vault.example.com/v1/identity/oidc/provider/openchami",
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
if got := expectedOIDCProviderIssuer(tc.base); got != tc.want {
t.Errorf("expectedOIDCProviderIssuer(%q) = %q, want %q", tc.base, got, tc.want)
}
})
}
}

// TestVaultClient_EnsureOIDCProviderIdempotentOnEffectiveIssuer is the core
// regression test for issue #62: a provider previously created by the operator
// with the configured BASE issuer is read back by Vault as the EFFECTIVE issuer
// (base + provider path). The operator must treat these as equivalent and NOT
// report a conflict on the next reconcile.
func TestVaultClient_EnsureOIDCProviderIdempotentOnEffectiveIssuer(t *testing.T) {
t.Parallel()

const base = "https://vault.example.com"
providerWrites := 0
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path == testProviderPath {
switch r.Method {
case http.MethodGet:
// Provider already exists, created earlier by the operator with
// the base issuer; Vault serves the effective issuer.
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{"data":{"issuer":"https://vault.example.com/v1/identity/oidc/provider/openchami","allowed_client_ids":["*"]}}`))
return
case http.MethodPut:
providerWrites++
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{"data":{}}`))
return
}
}
w.WriteHeader(http.StatusNotFound)
}))
defer server.Close()

apiConfig := vaultapi.DefaultConfig()
apiConfig.Address = server.URL
api, err := vaultapi.NewClient(apiConfig)
if err != nil {
t.Fatalf("new Vault API client: %v", err)
}
client := &vaultClient{api: api}

if err := client.ensureOIDCProvider(context.Background(), OIDCConfig{IssuerURL: base}); err != nil {
t.Fatalf("ensureOIDCProvider on operator-created provider must succeed, got: %v", err)
}
if providerWrites == 0 {
t.Error("expected the provider fields to be reconciled (at least one write)")
}
}

// TestVaultClient_EnsureOIDCProviderRealConflict asserts a genuine cross-Vault
// conflict is still detected: the effective issuer served by Vault has a
// DIFFERENT host than the configured base, so it must be an
// OIDCIssuerConflictError.
func TestVaultClient_EnsureOIDCProviderRealConflict(t *testing.T) {
t.Parallel()

const base = "https://vault-a.example.com"
providerWrites := 0
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path == testProviderPath {
switch r.Method {
case http.MethodGet:
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{"data":{"issuer":"https://vault-b.example.com/v1/identity/oidc/provider/openchami","allowed_client_ids":["*"]}}`))
return
case http.MethodPut:
providerWrites++
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{"data":{}}`))
return
}
}
w.WriteHeader(http.StatusNotFound)
}))
defer server.Close()

apiConfig := vaultapi.DefaultConfig()
apiConfig.Address = server.URL
api, err := vaultapi.NewClient(apiConfig)
if err != nil {
t.Fatalf("new Vault API client: %v", err)
}
client := &vaultClient{api: api}

err = client.ensureOIDCProvider(context.Background(), OIDCConfig{IssuerURL: base})
var conflict *OIDCIssuerConflictError
if !errors.As(err, &conflict) {
t.Fatalf("expected OIDCIssuerConflictError, got %v", err)
}
if conflict.Existing != "https://vault-b.example.com/v1/identity/oidc/provider/openchami" ||
conflict.Requested != base+testProviderPath {
t.Errorf("unexpected conflict detail: %+v", conflict)
}
if providerWrites != 0 {
t.Errorf("provider must not be written on issuer conflict, got %d writes", providerWrites)
}
}

// TestVaultClient_EnsureOIDCProviderCreateReconcileReconcile exercises the
// full lifecycle from issue #62: the provider does not exist, is created by the
// operator, and two subsequent reconciles must both accept the provider Vault
// now serves under its effective issuer.
func TestVaultClient_EnsureOIDCProviderCreateReconcileReconcile(t *testing.T) {
t.Parallel()

const base = "http://vault.vault.svc.cluster.local:8200"
created := false
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path == testProviderPath {
switch r.Method {
case http.MethodGet:
if !created {
w.WriteHeader(http.StatusNotFound)
return
}
// Once created, Vault serves the effective issuer.
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{"data":{"issuer":"http://vault.vault.svc.cluster.local:8200/v1/identity/oidc/provider/openchami","allowed_client_ids":["*"]}}`))
return
case http.MethodPut:
created = true
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{"data":{}}`))
return
}
}
w.WriteHeader(http.StatusNotFound)
}))
defer server.Close()

apiConfig := vaultapi.DefaultConfig()
apiConfig.Address = server.URL
api, err := vaultapi.NewClient(apiConfig)
if err != nil {
t.Fatalf("new Vault API client: %v", err)
}
client := &vaultClient{api: api}

// Create, then reconcile twice; all three must succeed without a conflict.
for i := range 3 {
if err := client.ensureOIDCProvider(context.Background(), OIDCConfig{IssuerURL: base}); err != nil {
t.Fatalf("ensureOIDCProvider iteration %d must succeed, got: %v", i, err)
}
}
}

func assertJSONStrings(t *testing.T, got any, want []string) {
t.Helper()
values, ok := got.([]any)
Expand Down
Loading