Skip to content

Add OCI Vault: vaults, keys, secrets - #425

Open
arunesh-j wants to merge 1 commit into
developmentfrom
feat/oci-vault
Open

arunesh-j wants to merge 1 commit into
developmentfrom
feat/oci-vault

Conversation

@arunesh-j

Copy link
Copy Markdown
Collaborator

Summary

  • Implements OCI Vault against the existing portable secrets driver.
  • OCI Vault is a superset of that driver — it puts KMS key management behind the secrets service — so most of the surface lives in a consumer-side Extras interface. Nothing added to services/secrets/driver, per Move OCI-only capabilities out of shared driver packages #393.

Closes #413. Part of #376.

Changes

  • providers/oci/vault/ — Mock over memstore implementing driver.Secrets, one sync.RWMutex guarding all four stores: vaults, master encryption keys and key versions, OCI-shaped secrets, secret versions and stages.
  • server/oci/vault/ — /20180608 management and /20190301 retrieval, claimed separately in Matches.
  • Wiring is one line each in providers/oci/oci.go and server/oci/oci.go.

How DeleteSecret maps onto scheduled deletion

The interesting design question in this service. DeleteSecret(name) takes the same path as ScheduleSecretDeletion, with timeOfDeletion at OCI's soonest permitted value — one day out — rather than the 30-day default an unspecified wire call receives.

The secret moves to PENDING_DELETION, keeping its OCID and every version. All seven portable operations then treat it as gone (NotFound from Get/Put/GetValue/ListVersions/Delete; absent from ListSecrets), while the OCI surface still lists it as PENDING_DELETION and CancelSecretDeletion restores it — the same soft delete the AWS Secrets Manager mock exposes. Nothing reaps; a scheduled resource stays pending until cancelled.

One deliberate consequence: a pending secret releases its name, so a portable delete→recreate works. CancelSecretDeletion then fails with AlreadyExists if the name was retaken, rather than producing two live secrets with the same name.

Deletion windows are enforced — 7–30 days for vaults and keys, 1–30 for secrets; out-of-window or unparseable timeOfDeletion is a 400.

Judgement calls

  • Portable creates need a vault. OCI has no secret outside one, so the first portable CreateSecret mints cloudemu-default-vault and an AES-256 key. Tested that a second create reuses them, and that 16 concurrent creates mint exactly one vault.
  • Crypto endpoints are claimed, not faked. encrypt/decrypt/sign/verify/generateDataEncryptionKey/exportKey answer 501 naming the gap, because CloudEmu stores no key material. A fake ciphertext would be worse than an honest refusal.
  • Compartments: RequireCompartmentID on /vaults, /keys, /secrets; deliberately not on keyVersions, secrets/{id}/versions, getByName or any /20190301 path — the real API takes none there and the parent resource already names the compartment.
  • Status codes follow the SDK response shapes: 200 + body for vault/key mutations (ScheduleVaultDeletionResponse carries a Vault), 204 headers-only for secret and version deletions (those responses carry no body), 202 for changeCompartment.
  • Never accept-and-ignore — 400 naming the field for definedTags, restoreFromFile, restoreFromObjectStore, externalKeyManagerMetadata, autoKeyRotationDetails, externalKeyReference, desiredState, secretRules, rotationConfig, secretGenerationContext, a non-BASE64 contentType, and reshaping a key after create.

Provider Coverage

  • AWS
  • Azure
  • GCP
  • OCI

Checklist

  • All tests pass (go test ./...) — exit 0, 272 packages
  • Linter passes (golangci-lint run --timeout=9m) — 0 issues
  • Every provider the change applies to implements the same behavior — OCI-only, additive
  • Integration tests added to cloudemu_test.go — driver + handler tests instead
  • Unit tests added to provider test files

Test Plan

go build ./...                                              clean
go test ./...                                               exit 0, 272 packages
go test -race ./providers/oci/... ./server/oci/...          11/11 ok
golangci-lint run --timeout=9m ./providers/oci/... ./server/oci/...   0 issues
go generate ./...                                           docs/coverage committed

Coverage leak check clean: no OCI operation in docs/coverage/{aws,azure,gcp}/*.md; git diff development -- services/ empty. docs/coverage/oci/vault.md lists exactly the 7 portable operations, as expected with capabilities consumer-side.

Lock sweep over all 63 *Mock methods: every one of the 42 exported methods takes m.mu exactly once, no locking method calls another locking method, no unexported helper touches m.mu, and there is no store.Update anywhere — so no closure can re-enter its store.

End-to-end on a running server (port 4614):

create vault -> create key -> create secret        -> opc-work-request-id stamped
write version 2                                    -> 200
read CURRENT bundle                                -> "hunter3"
read PREVIOUS stage                                -> "hunter2"
version stages                                     -> [PREVIOUS] / [CURRENT,LATEST]
schedule secret deletion                           -> 204, PENDING_DELETION, timeOfDeletion 2026-09-20
cancel deletion                                    -> 204, ACTIVE
list without compartmentId                         -> 400
poll work request                                  -> CREATE_VAULT, SUCCEEDED
rotate key (new key version)                       -> 200
encrypt                                            -> 501, names the gap

OCIDs came out as ocid1.{vault,key,keyversion,vaultsecret}.oc1.iad.….

A note on parallel worktrees

The session first reported cmd/cloudemu TestServeOutOfProcess as a pre-existing failure. That was wrong and I have re-checked it: with nothing else running, the test passes on clean development in under 6s. The failure is contention on the shared ~/.cloudemu daemon lock between the six Wave 2 worktrees running suites in parallel. This branch's full suite is exit 0.

Left out

KMS crypto operations (disclosed as 501), OCI secret rules and rotation targets, tag namespaces, and vault backup/restore — each rejected by name rather than silently dropped.

@arunesh-j arunesh-j added the oci Oracle Cloud Infrastructure label Aug 21, 2026

@NitinKumar004 NitinKumar004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review notes

Real data-plane engine (per #427): N/A — OCI Vault is a secrets/KMS service.

CI: the Build failure is a flaky runner I/O error (fork/exec .../link: input/output error), not a code defect — a re-run should clear it. Findings below are the real items.

Findings

Medium · real-engine — Secret name uniqueness enforced globally across all vaults instead of per-vault
providers/oci/vault/secret.go:117
If a user creates a secret named db-password in vault A, then calls CreateSecret (CreateOCISecret) for db-password in vault B -> the handler returns 409 AlreadyExists, but real OCI creates it, because validateSecretSpecLocked (secret.go:117) calls secretByNameLocked (secret.go:398) which scans every secret in every vault and never compares spec.VaultID (OCI scopes secret-name uniqueness to the vault). CancelOCISecretDeletion (secret.go:339) reuses the same global check and can likewise wrongly refuse a restore when an unrelated vault reused the name.

Medium · docs — docs/services.md not updated for OCI Vault (Definition-of-done miss)
docs/services.md:1389
If a user or contributor consults docs/services.md (the human service reference) for OCI secrets -> the ~30-op Vault/KMS surface is invisible: the Secrets header (line 1389) still lists only AWS/Azure/GCP and there is no OCI Vault section, unlike every sibling OCI service (VCN line 407, Monitoring 623, Identity 749). oci-conventions.md Definition-of-done explicitly requires 'Operations added to docs/services.md'; only the generated coverage pages (7 portable ops) were updated, and those cannot describe the OCI-only Extras surface.

Low · docs — Garbled doc comment on VaultManagement interface
server/oci/vault/extras.go:7
If a maintainer reads the VaultManagement doc comment (extras.go:7-11) -> they get an out-of-order, incoherent sentence (a dangling 'container above a secret at all...' fragment and a //nolint:revive directive that runs on into prose), because the comment lines were reordered incorrectly in a tidy edit. The nolint still functions; cosmetic only.

Low · coverage — server/oci/vault wire handler coverage 79.7% (below 90% pillar)
server/oci/vault/handler.go:246
If a client sends an unsupported verb (e.g. DELETE /vaults/{id}) or walks a multi-page list -> those paths ship untested (notFound 0%, listKeys 37.5%, listSecretVersions 50%, getSecretVersion 54.5%, paginate next-page 70%), so a regression in the negative-verb or pagination branch would pass the suite silently.

Low · coverage — No oci-go-sdk SDK-compat test
server/oci/vault/handler_test.go:31
If the wire envelope subtly diverges from what the real oci-go-sdk KMS/Vaults/Secrets client expects (field casing, work-request waiter behavior) -> the hand-rolled map[string]any assertions may still pass while the actual SDK client would fail, because no test round-trips through the real SDK marshaller/waiter (convention calls this the strongest evidence, framed as 'ideally').

@NitinKumar004 NitinKumar004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — OCI Vault (KMS + Secrets)

Verdict: Request changes — one persistence blocker, one real state-name fidelity bug, and a rebase. State guards and scope are otherwise correct.

On the red CI

The failing "Build & Vet" job is an infrastructure flake, not a code defect — the runner hit a disk I/O error corrupting its build cache:

go: error obtaining buildID for go tool link: fork/exec .../tool/linux_amd64/link: input/output error

which cascaded into spurious no such file or directory import errors across unrelated packages (azureai, bedrock, cache, …). go build ./... and go vet ./... pass clean locally on the pushed SHA. Re-run the job. (Separately, the PR reports mergeable: CONFLICTING — a real rebase requirement, below.)

Blocking

Mock is not Snapshottable — providers/oci/vault/ (no snapshot.go).
vault.Mock holds four memstore.Store fields (vaults, keys, keyVersions, secrets) plus default IDs, but ships no snapshot.go and implements no snapshot.Snapshottable — the only providers/oci/* package missing it. On the merged tree:

oci: field Vault (*vault.Mock) holds a memstore.Store but is not Snapshottable — add persistence (see #582)

Consequence: CI Test goes red on rebase, and Vault state is silently dropped by every snapshot/restore. Fix: add providers/oci/vault/snapshot.go (+ test) implementing Snapshottable for all four stores (deep-copying key versions, secret versions, and tag maps), mirroring providers/oci/vcn/snapshot.go.

Should fix

KMS keys and key versions report lifecycleState: "ACTIVE" — real OCI reports ENABLED — providers/oci/vault/vault.go:31 (StateActive), key.go:142, key.go:445.
Per the OCI SDK KeyLifecycleStateEnum (CREATING, ENABLING, ENABLED, DISABLING, DISABLED, DELETING, DELETED, PENDING_DELETION, …), KMS keys/key-versions never report ACTIVE — that value belongs to Vaults and Secrets (both correctly ACTIVE here). Live-confirmed: POST /20180608/keys returned "lifecycleState":"ACTIVE". Cascade: terraform-provider-oci's oci_kms_key waits for ENABLED after create/rotate, so an ACTIVE key hangs or errors the apply; SDK consumers comparing against KeyLifecycleStateEnabled misclassify every key. Fix: use ENABLED/DISABLED for the key + key-version surface (keep ACTIVE for vault + secret).

Non-blocking

  • No enableKey/disableKey and no DISABLED state — providers/oci/vault/key.go. The key lifecycle is ACTIVE ⇄ PENDING_DELETION only; OCI's enable/disable management ops and DISABLING/DISABLED/ENABLING states are absent. Fits Track-1 scope (metadata only, no crypto) but is a genuine feature gap — recommend a follow-up ticket, provided the ENABLED naming above is fixed now.
  • scheduleSecretDeletion returns 204; sibling key/vault deletions return 200 + body — server/oci/vault/secret.go:184. Worth confirming the status code + body against the OCI spec for consistency with the key/vault paths.
  • Branch is behind development / CONFLICTING — rebase; only generated docs/coverage/README.md + oci/README.md conflict, resolved by re-running go generate.

Verified good

Full lifecycle over the wire (create vault → key ACTIVE → rotate a PENDING_DELETION key → 409 IncorrectState → schedule→PENDING_DELETION with timeOfDeletion → cancel→ACTIVE → create secret → get bundle round-trips content → schedule secret deletion): state guards present and correct (CreateKeyVersion/UpdateOCISecret/ScheduleSecretVersionDeletion all guard on live state; current secret version can't be scheduled). Scope correct — no real crypto: encrypt/decrypt/sign/verify/generateDataEncryptionKey/exportKey return 501 naming the gap; secrets store base64 content in-memory (matches the AWS KMS metadata-only precedent). Wiring complete; opc-request-id/opc-work-request-id headers + OCID formats correct; name check-then-set atomic under one mutex (-race clean); reads deep-copy.

@arunesh-j

Copy link
Copy Markdown
Collaborator Author

Rebased onto adb83d85 (789 commits) and addressed everything in 73c67190.

On the red CI — your diagnosis was right

Only the two generated coverage files conflicted, exactly as you predicted; took the upstream side and re-ran go generate. services/secrets/driver gained no methods, so there was no upstream drift to absorb. go build ./... and the whole suite pass on the rebased tree, consistent with the flaky-runner reading. The rebase push should give the job a clean run.

Blocking — snapshot.go

Added providers/oci/vault/snapshot.go, mirroring vcn/snapshot.go with the shared storeDump table so Snapshot and Restore cannot drift. All four stores; secret versions travel with their secret.

The non-obvious part: defaultVaultID/defaultKeyID had to go into the snapshot too. Without them a restored mock mints a second default vault on the next portable create and orphans every secret it just restored.

snapshot_test.go covers all four stores, vault→key→secret cross-references, version stages, scheduled deletion surviving restore and still being cancellable afterwards, portable-default continuity, deep-copy, and empty/malformed input. go test ./persist/... now passes.

Should fix — KMS ENABLED

Four sites in key.go (142, 269, 321, 445) now use a new StateEnabled. Worth noting: two existing tests asserted ACTIVE for keys — they encoded the bug — and were corrected. TestKeyStatesAreEnabledAndResourceStatesAreActive pins both vocabularies so the split cannot regress.

I added only ENABLED, not DISABLED: without enableKey/disableKey (which you scoped out) DISABLED is unreachable and would be dead vocabulary. Follow-up ticket wants enableKey/disableKey plus ENABLING/DISABLING/DISABLED together.

The 204 question — it is correct, and now pinned

Secrets management and KMS are separate OCI services that merely share the /20180608 prefix. The SDK's ScheduleSecretDeletionResponse carries only opc-request-id/opc-work-request-id — no entity, hence 204 — while ScheduleKeyDeletionResponse carries a Key, hence 200 + body. The asymmetry is real OCI's, not ours.

Documented in writeSecretAction and pinned by TestDeletionActionStatusCodesMatchTheService so it is not "tidied" into consistency later. Step 13 of the transcript below shows both live.

Per-vault secret names

Uniqueness is now scoped to the vault; CancelOCISecretDeletion no longer consults unrelated vaults. The portable lookup resolves across vaults but rejects an ambiguous name with InvalidArgument naming both vaults, rather than silently picking one — the same answer #423 settled on for log groups, so #423/#424/#425 stay consistent.

One addition worth flagging: portable CreateSecret now also refuses a name another vault already holds, otherwise it would happily create a secret its own reads would immediately call ambiguous.

Load-bearing — reverting the s.VaultID == vaultID clause fails four tests across both packages (TestSecretNamesAreUniquePerVault, TestCancelDeletionAllowsTheSameNameInAnotherVault, TestPortableRejectsAnAmbiguousName, TestSameSecretNameInTwoVaults).

Also

docs/services.md OCI Vault section (32 ops across both API versions, plus namespace semantics, the ENABLED/ACTIVE split, scheduled deletion, staging, rejected inputs, the 501 crypto surface). Garbled VaultManagement comment repaired.

Verification

go build ./...                                     clean
go test ./...                                      exit 0, 451 packages
go test ./persist/...                              ok      (the blocking guard)
go test -race ./providers/oci/... ./server/oci/... 11/11 ok
golangci-lint (both vault packages)                0 issues
go test -cover   provider 96.9%   wire 94.1%  (wire was 79.7%)
go generate ./...                                  docs/coverage committed
no OCI op in docs/coverage/{aws,azure,gcp}/*.md    confirmed

One lint note: hugeParam on CreateSecret's cfg appeared after the rebase because upstream grew driver.SecretConfig. The signature is fixed by the interface, so it carries a targeted //nolint:gocritic. azure/keyvault and gcp/secretmanager trip the identical issue and are currently unfixed — worth a separate sweep rather than a drive-by here.

E2E, port 4614

create key in vault A     -> "lifecycleState":"ENABLED"   (vault itself: ACTIVE)
bundles                   -> CURRENT from-vault-a-v2 ; PREVIOUS from-vault-a-v1
db-password in vault B    -> 200
getByName                 -> A ...0008 (vault ...0001) ; B ...0017 (vault ...0010)
duplicate within vault A  -> 409 "secret \"db-password\" already exists in vault ...0001"
schedule secret deletion  -> 204 + Opc-Work-Request-Id  -> PENDING_DELETION 2026-10-07
cancel                    -> 204 -> ACTIVE
rotate key                -> keyversion ...0023 ENABLED ; both versions ENABLED
encrypt                   -> 501 "stores no key material"
KEY schedule/cancel       -> 200 + body (PENDING_DELETION, then ENABLED)
list                      -> two db-password secrets, distinct OCIDs, distinct vaults

@NitinKumar004 NitinKumar004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The earlier points are mostly fixed. On the merged tree a real OCI SDK client still can't read secret versions, read a bundle by name or create a key. Details below.

Earlier review points

  • Mock not Snapshottable: fixed. providers/oci/vault/snapshot.go has the compile assertion. go test ./persist/... passes on the tree merged with current development, and /_cloudemu/snapshot on serve includes the vault secrets.
  • Keys reporting ACTIVE: fixed. Keys and key versions now report ENABLED, while vaults and secrets keep ACTIVE.
  • Secret names unique across all vaults: fixed. Uniqueness is now per vault, and the portable lookup rejects an ambiguous name.
  • docs/services.md missing OCI Vault: fixed.
  • Garbled VaultManagement comment: fixed.
  • Wire coverage below 90%: fixed. It is now 94.8% for the wire package and 98.0% for the provider.
  • No oci-go-sdk compat test: not addressed. The High items below are what such a test would have caught (see Low 3).
  • scheduleSecretDeletion returning 204: fine. The SDK's ScheduleSecretDeletionResponse has no body. It carries etag and opc-request-id but no opc-work-request-id, so stamping one is harmless but not real.
  • enableKey/disableKey: still absent, as agreed for a follow-up.
  • Rebase: needed again (see Medium 1).

High

1. Secret version paths don't match the SDK. GetSecretVersion and version deletion return 404.
server/oci/vault/secret.go:48 routes /secrets/{id}/versions/{n} and /secrets/{id}/versions/{n}/actions/{scheduleDeletion,cancelDeletion}. The real Vaults API (oci-go-sdk vault/vault_vaults_client.go) uses the singular form for these three calls:

  • GET /secrets/{secretId}/version/{secretVersionNumber}
  • POST /secrets/{secretId}/version/{secretVersionNumber}/actions/scheduleDeletion
  • POST /secrets/{secretId}/version/{secretVersionNumber}/actions/cancelDeletion

Only ListSecretVersions uses the plural /versions.

Repro on serve:

GET /20180608/secrets/<id>/version/1   -> 404 "no such vault resource"
GET /20180608/secrets/<id>/versions/1  -> 200

Fix: route version/{n} and version/{n}/actions/*, keep versions for the list only, and update the handler tests and docs/services.md.

2. GetSecretBundleByName must be POST. The handler answers 405.
server/oci/vault/bundle.go:21 rejects every non-GET under /20190301/secretbundles. The SDK sends POST /secretbundles/actions/getByName with secretName and vaultId as query params.

Repro:

POST /20190301/secretbundles/actions/getByName?secretName=db&vaultId=<v>  -> 405 MethodNotAllowed
GET  same URL                                                             -> 200

Fix: accept POST for actions/getByName and keep GET for /{secretId} and /{secretId}/versions.

3. A real KMS client can't create a key. The vault comes from a made-up query param, and the advertised endpoints point at Oracle.

  • server/oci/vault/key.go:120 reads the vault from ?vaultId=.
  • providers/oci/vault/key.go:110 rejects a create without it.
  • In real OCI the vault is implied by the per-vault host. KmsManagementClient is built with client.Host = vault.managementEndpoint, and CreateKey/ListKeys send no vaultId.
  • providers/oci/vault/vault.go:422-423 mints cryptoEndpoint/managementEndpoint as https://<full vault OCID>-management.kms.<region>.oraclecloud.com. A user who follows the vault's endpoint, as the SDK pattern and oci_kms_key.management_endpoint do, sends traffic to Oracle instead of the emulator.

Repro:

POST /20180608/vaults                       -> managementEndpoint "https://ocid1.vault.oc1.iad....-management.kms.us-ashburn-1.oraclecloud.com"
POST /20180608/keys  {compartmentId, displayName, keyShape:{AES,32}}   (SDK shape, no vaultId)
                                            -> 400 "vaultId is required"

Fix: mint both endpoints against the emulator's own address, derived from the request host. Carry the vault in something the client sends back. One option is a host label the handler parses, such as a *.localhost name. Keep ?vaultId= as a fallback. Resolve the vault from the host in CreateKey and ListKeys. The prefix also shouldn't be the full dotted OCID, since real endpoints use a short host label.

Medium

1. Branch conflicts with current development.
A trial merge onto origin/development (770204c) conflicts in docs/coverage/README.md and docs/coverage/oci/README.md. Code merges cleanly. Taking development's side and re-running go generate . gives the right rows (secrets gains [Vault](./oci/vault.md), and the OCI index gains Vault), and a second run is idempotent.

2. Vault, key and secret creates skip the compartment existence check the OCI server now does for VCN.
server/oci/oci.go (vault registration) wires vault.New(d.Vault, d.WorkRequests) with no compartment checker, while VCN gets one from Identity (#1295).

Repro on the same serve:

POST /20180608/vaults {"compartmentId":"ocid1.compartment.oc1..doesnotexist", ...}  -> 200 ACTIVE
POST /20160918/vcns   same compartment                                             -> 404 NotAuthorizedOrNotFound

Fix: add the same SetCompartmentChecker hook to the vault handler. Call it on CreateVault, CreateKey, CreateSecret and the three changeCompartment actions, and wire it in server/oci/oci.go like VCN.

Low

  1. /20180608/secrets/actions/getByName is not a real OCI operation (server/oci/vault/secret.go:36). The Vaults API has no GetSecretByName. Name lookup is ListSecrets?name= or the bundle-side getByName. Drop it, or label it clearly as an emulator extension in docs/services.md.
  2. VaultCompartment (providers/oci/vault/vault.go:336) and KeyCompartment (providers/oci/vault/key.go:298) are only called from tests. They are declared in Extras (server/oci/vault/extras.go:19,34), but the handler never calls them. Only SecretCompartment is used. Remove them from the interface and the mock.
  3. There is still no oci-go-sdk round-trip test. A httptest test driving vault.VaultsClient, secrets.SecretsClient and keymanagement.KmsManagementClient would have caught all three High items. Please add one covering create, get, list versions, get version and bundle by name.
  4. Known gaps, fine as a follow-up but worth a ticket: keys/{id}/actions/enable|disable currently answer 404 "unknown action". Key version schedule/cancel deletion (/keys/{keyId}/keyVersions/{id}/actions/*) is also not routed.

Checked

  • Build and vet pass on the PR merged into current development. go test passes on providers/oci, server/oci, persist and internal/coveragegen, and -race passes on the vault packages and server/oci. golangci-lint reports 0 issues on the touched packages.
  • go generate . is idempotent after the conflict resolution.
  • Live serve run: vault, key and secret create. GetSecret and GetSecretBundle return identical bytes on repeated reads. Schedule secret deletion returns 204 and the secret moves to PENDING_DELETION. Paging returns opc-next-page and the next page. opc-request-id is echoed. The crypto endpoints return 501.
  • Terraform: not run. The oracle/oci provider has no plain-HTTP per-service endpoint override, and oci_kms_key would follow the vault's management_endpoint anyway (High 3). Raw requests in the exact oci-go-sdk shapes were used instead.
  • --enforce-auth: OCI has no auth gate in this repo (AWS and Azure only), so there is nothing for this handler to declare.
  • Wiring: provider slot, from_provider.go, server registration, Snapshottable, and the docs/services.md section are all in place.

@NitinKumar004

Copy link
Copy Markdown
Collaborator

Please merge the latest development into this branch (a merge is fine, no need to rebase). It is well behind now, and docs/coverage/README.md conflicts. After merging, run go generate ./... and commit the regenerated docs/coverage, then push. I'll re-review once it's up to date with the findings above addressed.

@arunesh-j

Copy link
Copy Markdown
Collaborator Author

Thanks. Everything is addressed in the latest commit, rebased on current development with docs regenerated.

High

  1. Secret version paths — a single version is now /secrets/{id}/version/{n}, with /version/{n}/actions/{scheduleDeletion,cancelDeletion}. Only the list stays plural (/versions). The handler tests, the handler doc comment and docs/services.md are updated.
  2. GetSecretBundleByName — POST /20190301/secretbundles/actions/getByName is served. A GET there is 405, and /{secretId} and /{secretId}/versions stay GET.
  3. KMS create key / endpoints
    • Each vault gets a short endpoint label (the 13-char tail of its OCID suffix, not the whole OCID). managementEndpoint / cryptoEndpoint are rendered against the emulator's own address: http://<label>-management.kms.localhost:<port> and …-crypto…, on the port the request came in on.
    • CreateKey and ListKeys resolve the vault from the request's host label. If there is none they fall back to ?vaultId=, and then to the only active vault in the compartment. Otherwise they return 400 naming both options.
    • *.localhost resolves through the system resolver (macOS, most Linux desktops) but not Go's pure resolver. docs/services.md says so; the SDK test uses a dialer so it doesn't depend on DNS.

Medium

  1. Rebased; docs/coverage regenerated, and go generate is idempotent.
  2. Compartment checks — SetCompartmentChecker is wired in server/oci/oci.go like VCN, and checked on CreateVault, CreateKey, CreateSecret and the three changeCompartment actions. Test: TestCreatesAndMovesRequireAnExistingCompartment.

Low

  1. /secrets/actions/getByName is removed (404 now). GetOCISecretByName is gone from Extras and the mock, and the tests use the bundle getByName or ListSecrets?name=.
  2. VaultCompartment / KeyCompartment are removed from Extras and the mock; the tests read the compartment from GetVault / GetKey.
  3. oci-go-sdk round trip — added server/oci/vault_sdk_test.go. It drives the full OCI server with the real keymanagement.KmsVaultClient, KmsManagementClient (built on the vault's managementEndpoint, so CreateKey sends no vaultId), vault.VaultsClient and secrets.SecretsClient. It covers create vault/key/secret, ListKeys, GetSecret, ListSecretVersions, GetSecretVersion and GetSecretBundleByName with content decode. This adds github.com/oracle/oci-go-sdk/v65 v65.106.1 to the root go.mod as a test dependency, the same way the AWS, Azure and GCP SDK-compat tests do. go mod tidy is clean.
  4. Follow-ups are tracked in OCI Vault: key enable/disable and key version deletion #1550 (key enable/disable, key version deletion, etag/if-match).

Wire tests: TestKeysResolveTheirVaultFromTheEndpointHost (endpoint shape and short label, create and list via the host label, 400 with two vaults on the plain address) and TestKeysFallBackToTheOnlyVault.

Local: providers/oci/..., server/oci/..., persist/..., internal/coveragegen/... and services/secrets/... pass. -race passes on both vault packages. golangci-lint --new-from-rev=origin/development reports 0 issues.

@NitinKumar004 NitinKumar004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. The secret version paths and the bundle getByName are fixed, and so is the compartment check. The SDK round trip test is a good addition. I re-ran everything on 3125e4f merged with current development (58fdd8c). The merge is clean and go generate is idempotent.

One blocker remains. The KMS management endpoint now points back at the emulator, but *.kms.localhost does not resolve on macOS. A real SDK or Terraform client that follows the vault's managementEndpoint gets no such host. The new SDK test passes only because it swaps in a dialer that ignores the hostname. I also found a few lifecycle gaps against the OCI docs, listed below.

Earlier findings

# Finding Status
H1 Secret version paths (/version/{n} singular) Fixed. Verified live with the real SDK: GetSecretVersion, ScheduleSecretVersionDeletion and CancelSecretVersionDeletion all succeed
H2 GetSecretBundleByName must be POST Fixed. SDK GetSecretBundleByName returns the content
H3 KMS client can't create a key; endpoints point at Oracle Partly fixed. The endpoints now use the emulator's port and a short label, and CreateKey/ListKeys read the vault from the host. But the host doesn't resolve on macOS (High 1 below)
M1 Merge development Fixed. The branch is 1 commit behind, merges cleanly, and regenerated docs show no diff
M2 Compartment existence check Fixed. CreateVault into an unknown tenancy and changeCompartment to an unknown compartment both return 404 NotAuthorizedOrNotFound
L1 /secrets/actions/getByName is not a real op Fixed (404)
L2 VaultCompartment / KeyCompartment only used by tests Fixed
L3 No oci-go-sdk round trip Fixed, with one caveat: the custom dialer hides High 1
L4 enable/disable, key version deletion Not fixed. Tracked in #1550, which is fine. Live: EnableKey returns 404 "unknown action enable"
(r2) Snapshottable, ENABLED for keys, per-vault names, docs, comment, coverage Still fixed

High

1. *.kms.localhost doesn't resolve on macOS, so a client that follows managementEndpoint can't reach the emulator.
server/oci/vault/vault.go:199 (endpointDomain = "kms.localhost") and the claim in docs/services.md:1468.

On macOS (Go 1.27.1, darwin/arm64, cgo resolver):

CreateVault -> managementEndpoint http://0000000000008-management.kms.localhost:47425
keymanagement.NewKmsManagementClientWithConfigurationProvider(cp, managementEndpoint).CreateKey(...)
  -> dial tcp: lookup 0000000000008-management.kms.localhost: no such host
ping 0000000000008-management.kms.localhost  -> cannot resolve: Unknown host
curl  http://0000000000008-management.kms.localhost:47425/...  -> 200   (curl special-cases .localhost itself)

The docs say *.localhost "resolves through the system resolver on macOS". It doesn't. Only curl and browsers handle it internally. Linux without systemd-resolved or nss-myhostname fails the same way, and that covers most slim or Alpine client containers. This affects:

  • the oci-go-sdk KmsManagementClient and KmsCryptoClient built on the vault's endpoints, as the SDK docs show
  • Terraform's oci_kms_key, whose management_endpoint is normally oci_kms_vault.x.management_endpoint
  • the OCI CLI's --endpoint

TestVaultSDKRoundTrip passes only because loopbackClient dials the test server whatever the host is.

I checked whether a path-based label could work instead. The SDK drops any path on the client host: an endpoint of http://127.0.0.1:P/kms/abc sends /20180608/keys. So the label has to stay in the hostname, and that name has to resolve. Suggested fix:

  • Make the advertised KMS endpoint host configurable on serve, the same way --k8s-host is. Default it to something that resolves without extra setup.
  • When no label is present, keep the ?vaultId= and single-vault fallbacks so a client on the plain address keeps working.
  • Correct the docs. Explain how to add /etc/hosts entries on macOS, or how to point the KMS client at the plain address while only one vault exists.
  • Make the SDK test cover the default path: a client that only knows the plain address must work, and the docs must not promise resolution that the OS doesn't provide.

Medium

2. Scheduling a vault's deletion doesn't move its keys to PENDING_DELETION.
providers/oci/vault/vault.go:289. The SDK doc for ScheduleVaultDeletion says: "This sets the lifecycle state of the vault and all keys in it that are not already scheduled for deletion to PENDING_DELETION". It also says the call is rejected with 409 if any key's deletion is already scheduled later than the vault's. The vault deletion docs say the same: "the vault and all its associated keys go into a pending deletion state".
Live: after ScheduleVaultDeletion on v2, GetKey on its key still returns ENABLED. A user's teardown or cleanup test that checks key state gets the wrong answer.
Fix: cascade inside the same lock, setting each key that isn't already pending to PENDING_DELETION with the vault's timeOfDeletion. Return 409 when an already-pending key is due later than the vault. CancelVaultDeletion should restore the keys it moved.

3. A secret pending deletion frees its name, which real OCI doesn't do.
providers/oci/vault/secret.go:387 (&& s.LifecycleState == StateActive).
Live: ScheduleSecretDeletion on db, then CreateSecret db in the same vault returns 200 with a second OCID. Real OCI rejects this until the deletion window ends with 400 InvalidParameter ... name already exists (terraform-provider-oci#1765). It's a well-known pain point for anyone who destroys and re-applies oci_vault_secret. The AWS Secrets Manager mock in this repo also refuses to recreate a name that's scheduled for deletion (providers/aws/secretsmanager/secretsmanager.go:301), so the PR body's comparison to the AWS mock doesn't hold.
Fix: keep the name reserved while the secret is PENDING_DELETION. Then portable DeleteSecret followed by CreateSecret fails exactly as it does on AWS. That's the honest behavior, and it removes the need for the "recreated while pending" branch in CancelOCISecretDeletion.

4. Bundles are still served for a secret pending deletion.
providers/oci/vault/version.go:169-193, and secret.go:422-426, which deliberately falls back to a pending secret in getByName.
The secret deletion docs say: "When a secret is pending deletion, resources or services that rely on that secret immediately become inaccessible."
Live: after ScheduleSecretDeletion, both GetSecretBundle and GetSecretBundleByName still return the content. An app whose secret has been revoked keeps working against cloudemu.
Fix: refuse bundle reads (by OCID and by name) while the secret is PENDING_DELETION, the same way the portable reads already do.

5. KMS crypto returns 501 although every other KMS in the repo does real crypto.
server/oci/vault/handler.go:233. AWS KMS (providers/aws/kms/crypto.go), GCP KMS (providers/gcp/kms/crypto.go, with per-version material and the version id carried in the ciphertext) and Azure Key Vault keys (providers/azure/keyvault/keys_azure.go) all encrypt, decrypt, sign and verify for real. Here a key is a record with no material. Every encrypt, decrypt, generateDataEncryptionKey, sign and verify call is 501, confirmed live via KmsCryptoClient.
I accepted this in an earlier round. Seeing the other three clouds side by side, a key that can't encrypt misses the main reason to create one. Envelope encryption with OCI keys, which is how apps use OCI Vault, can't be tested at all.
Fix: generate material per key version at create and rotate time. Keep it out of the response and persist it via the snapshot. Serve encrypt/decrypt/generateDataEncryptionKey for AES and sign/verify for RSA and ECDSA on the crypto endpoint. Embed the key version in the ciphertext so decryption after a rotation still works, as the GCP mock does. Refuse crypto on keys that aren't ENABLED. If you'd rather land this PR without crypto, open a ticket for it and link it here. In that case the 501 can stay, since it names the gap honestly.

Low

  1. A key that's PENDING_DELETION can still be used for a new secret or a re-key (providers/oci/vault/secret.go:108-115, secret.go:255-262). Live: CreateSecret with a pending key returns 200. Real OCI requires an ENABLED key. Check k.LifecycleState == StateEnabled in both places.
  2. The provider-level VaultInfo.CryptoEndpoint and ManagementEndpoint still mint https://<label>-*.kms.<region>.oraclecloud.com (providers/oci/vault/vault.go:466-467), while the wire layer re-renders them against the emulator. The typed Go API and the wire disagree, and the provider fields aren't used by the handler. Either drop them and expose only EndpointLabel, or compute them in one place.
  3. Wire shape gaps against the SDK models: Vault.wrappingkeyId is mandatory:"true" in keymanagement/vault.go but never sent (server/oci/vault/types.go:29-41). SecretVersion.contentType is also missing; live, every version shows contentType=.
  4. No etag header on Get or Update for vaults, keys and secrets, and if-match is ignored. This is already tracked in #1550.
  5. Once the portable default vault is scheduled for deletion, every portable CreateSecret fails with FailedPrecondition from then on. defaultVaultLocked (providers/oci/vault/portable.go:208) checks only that the vault exists, not that it's ACTIVE. Mint a new default when the stored one is no longer ACTIVE.

Terraform and serve

Terraform was not run. The oracle/oci provider has no plain-HTTP per-service endpoint override, and routing it through a local intercepting proxy wasn't an option here. Also, oci_kms_key would fail on High 1 regardless: its management_endpoint is the unresolvable *.kms.localhost host. As the fallback, I drove the real oci-go-sdk v65.106.1 clients (KmsVaultClient, KmsManagementClient, KmsCryptoClient, VaultsClient, SecretsClient) against cloudemu serve --providers oci --oci-port 47425 --persist:

  • Vault: create (ACTIVE, opc-work-request-id set) → GET ×2 byte-identical (also identical via localhost vs 127.0.0.1) → UpdateVault displayName → ScheduleVaultDeletion (PENDING_DELETION). OCI has no hard delete, so the follow-up GET returns the vault as PENDING_DELETION, as real OCI does. GET of an unknown OCID returns 404 NotAuthorizedOrNotFound.
  • Key: create through the management endpoint failed on DNS (High 1). With a loopback dialer it succeeded: ENABLED, vault taken from the host label, then GET ×2 byte-identical → UpdateKey → CreateKeyVersion (2 versions) → ScheduleKeyDeletion → CancelKeyDeletion back to ENABLED.
  • Secret: create → GET ×2 byte-identical → UpdateSecret with a PENDING version → stages [CURRENT] / [PENDING LATEST] → promote v2 → bundles CURRENT=v2 "p2", PREVIOUS=v1 "p1", LATEST=v2 → version schedule/cancel deletion → bad base64 returns 400 InvalidParameter → ScheduleSecretDeletion (PENDING_DELETION, 30-day default) → bundle still readable (Medium 4) → same-name recreate allowed (Medium 3).
  • Pagination: limit=1 returns opc-next-page. opc-request-id is present on every response.
  • Persistence: I stopped serve with SIGINT and restarted it on the same state file. The vault, key and secret came back unchanged. A new vault minted after the restart got a fresh OCID (...0033, no collision). CreateKey through the old endpoint label still resolved to the original vault, and the bundle content survived. Secret content is in the 0600 state file as base64, which persistence needs. Nothing secret appears in the serve log.
  • Docker: the Dockerfile and serve wiring are unchanged (OCI stays opt-in on 4571), so I didn't rebuild the image. serve and the in-process server behave the same.

What I verified

  • Clean build of the PR head merged with development. go vet on providers/oci, server/oci and persist is clean.
  • go test -race passes for providers/oci/..., server/oci/... and persist. Coverage: provider vault 96.9%, wire vault 94.9%.
  • go mod tidy and gofmt are clean, and go generate . leaves no diff. golangci-lint reports 0 issues on the vault packages and on providers/oci and server/oci.
  • CI is all green.
  • A concurrency probe ran 8 goroutines of concurrent secret create/update/bundle read/version list, vault and key tag updates, key rotation, schedule/cancel and Snapshot through the assembled server. It was clean under -race with -count=3.
  • Architecture: the portable services/secrets driver is implemented, with the OCI-only surface in a consumer-side Extras, following the vcn pattern. services/kms/driver is AWS-shaped, and GCP KMS and Azure Key Vault don't implement it either, so not using it is consistent. Wiring in providers/oci/oci.go and server/oci/oci.go plus the compartment checker is in place. Snapshottable is compile-asserted. OCIDs come from idgen. RequireCompartmentID is on every list. Errors use ocirest. No shared portable driver changed, so the other clouds are unaffected.

Verdict

Request changes: High 1 and Mediums 2 to 4. Medium 5 can become a linked follow-up ticket if you'd rather keep this PR to the control plane.

Comment thread server/oci/vault/vault.go Outdated
const (
endpointManagement = "management"
endpointCrypto = "crypto"
endpointDomain = "kms.localhost"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*.kms.localhost doesn't resolve on macOS. With the real oci-go-sdk KmsManagementClient built on this endpoint, CreateKey fails with lookup 0000000000008-management.kms.localhost: no such host, and ping can't resolve it either. Only curl works, because curl handles .localhost itself. Slim Linux client images without systemd-resolved or nss-myhostname fail the same way. Terraform's oci_kms_key follows this endpoint too. Please make the advertised host configurable on serve (like --k8s-host), default it to something that resolves without extra setup, and keep the ?vaultId= and single-vault fallbacks for clients on the plain address. The SDK drops any path on the client host, so a path-based label can't replace this.

Comment thread docs/services.md Outdated
endpoint stays on the emulator, and `/keys` reads the vault from the request's
host label. A client on the plain emulator address can pass `?vaultId=`
instead, and with a single active vault in the compartment it may omit both.
`*.localhost` resolves through the system resolver on macOS and most Linux

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't accurate on macOS. The system resolver doesn't resolve *.localhost there (ping fails, and so do Go and Python clients). Please correct it and say what a user has to do instead, e.g. add /etc/hosts entries or use the plain address while only one vault exists.

return nil, err
}

v.LifecycleState = StatePendingDeletion

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the SDK doc for ScheduleVaultDeletion, this "sets the lifecycle state of the vault and all keys in it that are not already scheduled for deletion to PENDING_DELETION", and returns 409 if a key is already scheduled to be deleted after the vault. Right now the vault's keys stay ENABLED (verified live). Please cascade under the same lock, and have CancelVaultDeletion restore the keys it moved.

Comment thread providers/oci/vault/secret.go Outdated
// until its deletion is canceled.
func (m *Mock) secretByNameLocked(vaultID, name string) (*secretData, bool) {
for _, s := range m.secrets.SortedValues() {
if s.VaultID == vaultID && s.Name == name && s.LifecycleState == StateActive {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real OCI keeps the name reserved while a secret is PENDING_DELETION. Recreating it returns 400 InvalidParameter ... name already exists (oracle/terraform-provider-oci#1765). The AWS Secrets Manager mock here also refuses (providers/aws/secretsmanager/secretsmanager.go:301). Live, this returns 200 with a second OCID. Please count pending secrets when checking the name.

Comment thread providers/oci/vault/version.go Outdated
return nil, err
}

return selectBundle(s, sel)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OCI docs: "When a secret is pending deletion, resources or services that rely on that secret immediately become inaccessible." GetSecretBundle and GetSecretBundleByName (which deliberately falls back to a pending secret at secret.go:422) still return the content after ScheduleSecretDeletion. Please refuse bundle reads while the secret is PENDING_DELETION.

// unemulatedCrypto reports a crypto endpoint operation. CloudEmu stores no key
// material — a master encryption key here is a record, not a cipher — so an
// encrypt or sign call would have to invent a ciphertext.
func unemulatedCrypto(w http.ResponseWriter, r *http.Request, operation string) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AWS KMS, GCP KMS (per-version material, with the version id in the ciphertext) and Azure Key Vault keys all do real crypto in this repo. Here every encrypt/decrypt/generateDataEncryptionKey/sign/verify call is 501, so envelope encryption with OCI keys can't be tested. Please add per-key-version material, persisted via the snapshot and never returned, and serve the crypto endpoint for ENABLED keys. If you'd rather do that separately, open a ticket and link it here.

Comment thread providers/oci/vault/secret.go Outdated
return err
}

if k.VaultID != spec.VaultID {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't check the key's state. A key in PENDING_DELETION is accepted for a new secret (verified live), and rekeyLocked has the same gap. Real OCI requires an ENABLED key.

Comment thread providers/oci/vault/vault.go Outdated
CompartmentID: v.Scope.Compartment,
DisplayName: v.DisplayName,
VaultType: v.VaultType,
CryptoEndpoint: "https://" + label + "-crypto.kms." + region + ".oraclecloud.com",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wire layer re-renders both endpoints against the emulator, so these oraclecloud.com values are only visible through the typed Go API, where they disagree with what the wire returns. Please drop them and keep EndpointLabel, or build the endpoints in one place.

Comment thread providers/oci/vault/portable.go Outdated
// defaultVaultLocked returns the vault and key the portable driver stores its
// secrets in, creating them on first use.
func (m *Mock) defaultVaultLocked() (vaultID, keyID string) {
if m.vaults.Has(m.defaultVaultID) && m.keys.Has(m.defaultKeyID) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the default vault is scheduled for deletion, it still exists, so this returns it and every later portable CreateSecret fails with FailedPrecondition. Please mint a new default when the stored vault isn't ACTIVE.

@arunesh-j

Copy link
Copy Markdown
Collaborator Author

Thanks. Fixed in 1226c97, rebased on current development (with OCI Logging).

High

  1. KMS endpoint resolution — by default a vault's managementEndpoint and cryptoEndpoint are now the plain address the request came in on (e.g. http://127.0.0.1:4571), which always resolves.
    • CreateKey and ListKeys resolve the vault from a label host if there is one, then ?vaultId=, then the only ACTIVE vault in the compartment, and otherwise return 400 naming both options.
    • Label hosts are opt-in through the new --oci-kms-endpoint-domain flag (env CLOUDEMU_OCI_KMS_ENDPOINT_DOMAIN, default empty). It is plumbed like the k8s host flags: serveflags → serverkit → ociserver.Drivers → vault.Handler. With it set, the endpoints are <label>-management.<domain>:<port> / -crypto, and only hosts under that domain are parsed for a label.
    • docs/services.md is corrected. It drops the false macOS claim, covers the default, says to pass ?vaultId= when a compartment has several vaults, and explains that a label domain needs its hosts made resolvable (e.g. /etc/hosts). The flag is added to docs/standalone-server.md.
    • Tests: TestVaultSDKRoundTrip now uses a stock KmsManagementClient on the vault's managementEndpoint with no custom dialer. The new TestVaultSDKLabelDomain covers label mode with two vaults. Wire tests: TestVaultEndpointsDefaultToThePlainAddress, TestLabelHostsAreIgnoredWithoutADomain.

Medium
2. Vault deletion cascade — under the same lock, every key not already pending goes to PENDING_DELETION with the vault's time. A key already due later than the vault makes the call 409. CancelVaultDeletion restores exactly the keys it moved, and that tracking is persisted. Tests: TestVaultDeletionCascadesToItsKeys, TestVaultDeletionRefusesAKeyDueLater, TestVaultDeletionCascadesOverTheWire, plus a cancel-after-restore snapshot test.
3. Pending secret names — a PENDING_DELETION secret keeps its name. Recreating it over the wire is 400 InvalidParameter. Portable DeleteSecret → CreateSecret fails like the AWS mock, and the "recreated while pending" branch is gone. Tests: TestPendingSecretKeepsItsName, TestPortableDeleteKeepsTheNameReserved, TestPendingSecretOverTheWire.
4. Bundles for pending secrets — GetSecretBundle and GetSecretBundleByName refuse a pending secret, and getByName no longer falls back to one (TestBundlesRefuseAPendingSecret).
5. KMS crypto — tracked in #1552, as you offered. The 501 stays and the docs link the ticket.

Low
6. CreateSecret and re-key require an ENABLED key in the vault (TestSecretsRequireAnEnabledKey).
7. The provider's oraclecloud.com endpoint fields are removed. VaultInfo keeps EndpointLabel, and the wire layer is the only place endpoints are built.
8. Each vault has a stable, persisted wrappingkeyId, and secret versions carry contentType: BASE64.
9. Tracked in #1550.
10. The portable default vault is replaced once the stored one is no longer ACTIVE, or its key no longer ENABLED (TestPortableCreateReplacesAPendingDefaultVault).

Local: providers/oci/..., server/oci/..., persist/... (completeness included), internal/coveragegen/..., services/secrets/..., server/serveflags/..., server/serverkit/... and cmd/cloudemu/... pass. -race is clean on the vault packages. golangci-lint --new-from-rev reports 0 issues, and go mod tidy and go generate leave no diff.

@arunesh-j

Copy link
Copy Markdown
Collaborator Author

Rebased onto development now that #421 (OCI Notifications) has merged, and squashed to one commit, since the repo squash-merges anyway and that keeps the conflict resolution in one place. The only conflicts were the shared wiring in providers/oci/oci.go / server/oci/oci.go, where both sides register a service (both are kept), and the generated docs/coverage, which is regenerated. The service code is byte-identical to the commit you reviewed. Build, tests and lint are clean on the rebased tree.

@NitinKumar004 NitinKumar004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One point before approving: with the plain shared management endpoint as the default, a stock KmsManagementClient request no longer says which vault it is for. In my run, CreateKey sent to the endpoint of a vault pending deletion created the key in a different vault. With two ACTIVE vaults in the same compartment, which one gets the key? In OCI each vault has its own management endpoint, so a key always lands in the vault you addressed. Please make sure CreateKey/ListKeys/GetKey through the default endpoint can't silently target the wrong vault (for example, give each vault a distinct path or port on the default address, or reject the request when it's ambiguous), and add an SDK test with two active vaults. Everything else from the last round is fixed and looks good.

Thanks, this round closes everything from my last review. I re-ran it all on 58ad50d, which sits on current development (4cc5c57, with OCI Logging and Notifications). I drove the real oci-go-sdk v65.106.1 against cloudemu serve on this Mac with stock clients and no custom dialer. The squashed commit matches what you described: the vault code changes since 3125e4f are the fixes below, and the rest of the delta is the rebase over #421 and #423.

Earlier findings

# Finding Status
H1 *.kms.localhost endpoint doesn't resolve on macOS Fixed. By default managementEndpoint and cryptoEndpoint are the plain address (http://127.0.0.1:<port>). A stock KmsManagementClient built on that endpoint creates a key in the right vault and lists keys, with no dialer. Label hosts are opt-in via --oci-kms-endpoint-domain. With kms.localhost set, the stock client fails with no such host as the docs now say, and a request carrying the label host routes to the right vault
H1 docs docs/services.md macOS claim Fixed. The text now describes the default, the ?vaultId= and single-vault rules, and the /etc/hosts requirement for label mode. The flag is in docs/standalone-server.md
M2 Vault deletion doesn't cascade to keys Fixed. Live: scheduling v2 for day 20 moved its ENABLED key to PENDING_DELETION at day 20 and left a key already due at day 10 alone. A key due at day 25 makes the call 409 IncorrectState, and nothing changes. CancelVaultDeletion restores only the keys it moved. That also holds after a --persist restart
M3 Pending secret frees its name Fixed. Live: recreating db while it's pending returns 400 InvalidParameter. Cancel brings it back
M4 Bundles served while pending Fixed. Live: GetSecretBundle and GetSecretBundleByName both return 404 NotAuthorizedOrNotFound while the secret is pending, and work again after cancel
M5 KMS crypto 501 Deferred to #1552, as offered. The docs and the handler comment link it
L6 Pending key accepted for a secret Fixed. CreateSecret and the re-key path both return 409 IncorrectState for a pending key
L7 oraclecloud.com endpoints in the provider Fixed. VaultInfo keeps only EndpointLabel, and only the wire layer builds endpoints
L8 wrappingkeyId, contentType missing Fixed. Live: each vault reports a stable wrappingkeyId, which survives a restart, and secret versions report contentType=BASE64
L9 etag / if-match Tracked in #1550
L10 Pending default vault breaks portable CreateSecret Fixed (TestPortableCreateReplacesAPendingDefaultVault)

New findings

No High or Medium.

Low

  1. On the plain address, ListKeys returns the keys of every vault in the compartment.
    providers/oci/vault/key.go:202, reached from server/oci/vault/key.go:150 with an empty vault id.
    CreateKey on the plain address resolves to the only active vault and returns 400 when the choice is ambiguous. ListKeys with no vault skips that step and filters by compartment only. Live, with four vaults in the tenancy, ListKeys through a stock client on a vault's managementEndpoint returned 8 keys from 4 vaults, including PENDING_DELETION keys from vaults already scheduled for deletion. Vaults are never reaped, so destroying and re-applying a stack is enough to trigger this: listing keys for the new vault also returns the old vault's pending keys. Real OCI's ListKeys only returns keys from the vault whose endpoint you call, so an oci_kms_keys data source or an SDK listing gets extra keys here.
    The same fallback also sends a key to the wrong vault. With v2 pending and v1 active, a stock CreateKey on v2's endpoint created the key in v1. Real OCI would refuse it because v2 is pending. All vaults share one endpoint by default, so the emulator can't know which vault the client meant.
    Fix: when no vault is named, have ListKeys go through soleVaultLocked just like CreateKey, so it lists the single active vault or returns the same 400. Also add a line to the docs: on the plain address, /keys always means the compartment's single active vault, whichever vault's endpoint the client took it from.

  2. UpdateSecret accepts keyId, but OCI has no such field.
    server/oci/vault/secret.go:218 passes req.KeyID to rekeyLocked (providers/oci/vault/secret.go:202). OCI's UpdateSecretDetails (API reference, and vault/update_secret_details.go in the SDK) has no keyId. A secret's key is fixed at creation. No SDK, CLI or Terraform client can send this field, so the behavior is reachable only through raw requests, and it shows a capability that real OCI lacks. Fix: reject keyId on update with 400 InvalidParameter, as the handler already does for other inputs it doesn't model, and drop rekeyLocked.

  3. server.New now trips gocyclo (12 > 10).
    server/oci/oci.go:85. It's new against the merge base. CI lint is advisory, so the check stays green. With vault added, the identical compartment-checker closure now appears four times (logging, VCN, notifications and vault, at lines 108, 122, 136 and 151). Move it into one helper, e.g. compartmentChecker(d.Identity) func(string) bool, which returns nil when Identity has no compartments. That fixes the complexity and the duplication together.

OCI docs

  • ScheduleVaultDeletion / CancelVaultDeletion (SDK doc and Deleting a Vault): the cascade, the 409 for a key due later than the vault, and restoring only the keys that were moved now match.
  • Deleting a Secret: a pending secret is now inaccessible.
  • Vault.wrappingkeyId (mandatory in the SDK model) and SecretVersion.contentType are now sent.
  • UpdateSecretDetails has no keyId (Low 2).

Terraform and serve

I didn't run Terraform. The oracle/oci provider has no plain-HTTP per-service endpoint override, and routing it through a local intercepting proxy wasn't available here. Now that managementEndpoint is the plain address by default, oci_kms_key should be able to follow it while there's one active vault in the compartment. As the fallback I used the real oci-go-sdk clients against cloudemu serve --providers oci --oci-port 47525 --persist, all stock and with no dialer override:

  • Vault → stock KmsManagementClient on its managementEndpoint → CreateKey (ENABLED, right vault) → ListKeys. A second vault in the compartment makes CreateKey return 400 with a clear message, as documented.
  • The cascade, refusal and cancel flows, the secret name reservation, the pending-bundle refusal and the pending-key refusal, with results as in the table above.
  • Persistence: I scheduled a vault with an ENABLED key, stopped serve with SIGINT and restarted it on the same state file. The key came back PENDING_DELETION, and CancelVaultDeletion after the restart returned it to ENABLED, so the moved-key tracking survives. A new vault minted after the restart got fresh OCIDs for both the vault and its wrappingkeyId, with no collisions.
  • Label mode: --oci-kms-endpoint-domain kms.localhost advertises <label>-management.kms.localhost:<port>. A stock client gets no such host, as the docs now warn. A request with that Host header lands in the labeled vault, and an unknown label with two vaults returns 400.
  • Docker: the Dockerfile is unchanged. The new flag goes through serveflags → serverkit → ociserver.Drivers like the other serve options, so the image picks it up through the same binary.

What I verified

  • go build ./... and go vet on providers/oci, server/oci, serveflags, serverkit and persist are clean.
  • go test -race passes for providers/oci/vault, server/oci/..., server/serveflags and persist. Coverage: provider vault 97.1%, wire vault 94.9%.
  • gofmt, go mod tidy and go generate . leave no diff.
  • golangci-lint on the touched packages: the only new issue against the merge base is Low 3. The rest are pre-existing.
  • CI: all 23 checks are green.
  • A concurrency probe ran 8 goroutines of vault schedule/cancel, key create/schedule/cancel, OCI and portable secret creates, ListKeys and Snapshot against one mock. It was clean under -race with -count=3. The cascade runs under the vault mutex, so it can't interleave with a key or secret write.
  • Architecture: unchanged from last round. The new flag follows the k8s host flags, and the provider no longer formats endpoints. No shared portable driver changed.

Verdict

Approve. The three Lows can go in this PR or a follow-up. Low 1 is the one I'd fix first, since it returns wrong data silently.

out := make([]KeyInfo, 0, m.keys.Len())

for _, k := range m.keys.SortedValues() {
if !k.Scope.Matches(filter) || (vaultID != "" && k.VaultID != vaultID) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With no vault named, this lists every key in the compartment across all vaults. Live, ListKeys through a stock client on one vault's managementEndpoint returned 8 keys from 4 vaults, including pending keys from vaults already scheduled for deletion. Vaults are never reaped, so destroying and re-applying a stack is enough to hit this. CreateKey resolves the same request through soleVaultLocked, and ListKeys should too, so it lists the single active vault or returns the same 400.

Comment thread server/oci/vault/secret.go Outdated

upd := &vaultprovider.SecretUpdate{
Description: req.Description,
KeyID: req.KeyID,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OCI's UpdateSecretDetails has no keyId (see the API reference and vault/update_secret_details.go in the SDK). A secret's key is fixed at creation, and no SDK, CLI or Terraform client can send this. I'd reject keyId on update with 400 InvalidParameter and drop rekeyLocked.

Comment thread server/oci/oci.go Outdated
vaultHandler := vault.New(d.Vault, d.WorkRequests)
vaultHandler.SetEndpointDomain(d.KMSEndpointDomain)

if comps, ok := d.Identity.(identity.Compartments); ok {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fourth copy of the same compartment-checker closure, and with it New reaches gocyclo 12. A small compartmentChecker(d.Identity) func(string) bool helper, returning nil when Identity has no compartments, would remove the duplication and the lint hit together.

KMS vaults and keys with versions and scheduled deletion, secrets with staged
versions and the bundle data plane, compartment checks, persistence, an
oci-go-sdk round trip, and opt-in per-vault KMS endpoint hosts.
@arunesh-j

Copy link
Copy Markdown
Collaborator Author

Thanks for the approval. All three Lows are in, rebased on current development (with #422 and #1553), still one commit:

  1. ListKeys on the plain address — with no vault named, ListKeys resolves through soleVaultLocked the same way CreateKey does. It lists the compartment's single active vault, and two active vaults return the same 400. Pending keys from vaults already scheduled for deletion no longer appear. docs/services.md now says that on the plain address /keys always means the compartment's single active vault. Test: TestListKeysWithoutAVaultResolvesTheSoleActiveVault.
  2. keyId on UpdateSecret — UpdateSecret with a keyId is 400 InvalidParameter, rekeyLocked and SecretUpdate.KeyID are removed, and the docs are updated. Test: TestUpdateSecretRefusesKeyID.
  3. Duplicated compartment checker — a compartmentChecker(d.Identity) helper returns nil when Identity has no compartments. Every OCI handler (Logging, Object Storage, VCN, Notifications, Vault) now calls SetCompartmentChecker(checker), which removes the four copies and the gocyclo hit.

The rebase conflicts were the shared wiring imports and the serve-flag name list (oci-tenancy from #422 next to oci-kms-endpoint-domain); both are kept.

Local: providers/oci/..., server/oci/..., persist/..., internal/coveragegen/..., services/secrets/..., server/serveflags/... and server/serverkit/... pass. -race is clean on the vault packages. golangci-lint --new-from-rev reports 0 issues, and go generate and go mod tidy leave no diff.

@NitinKumar004 NitinKumar004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick turnaround. All three Lows are fixed. The vault routing point from my last note is only partly closed: two ACTIVE vaults in the same compartment are now refused instead of guessed, but two other cases still put a key in a vault the client never addressed. I re-ran everything on bb011fa with the real oci-go-sdk v65.106.1 against cloudemu serve --providers oci on this Mac. The clients were stock, built on each vault's own managementEndpoint, with no custom dialer.

Earlier findings

# Finding Status
Note Default shared endpoint can't tell vaults apart Partly fixed. Two ACTIVE vaults in one compartment now return 400 for CreateKey and ListKeys. A key sent to a pending vault's endpoint, or to the endpoint of a vault in another compartment, still lands silently in some other vault (Medium 1). No SDK test with two active vaults was added
L1 ListKeys without a vault lists every vault Fixed. Live: after scheduling one vault's deletion, ListKeys on the plain address returned only the active vault's keys, and the old vault's pending keys no longer appear
L2 UpdateSecret accepts keyId Fixed. Live: PUT /secrets/{id} with keyId returns 400 InvalidParameter, and a description update still returns 200. rekeyLocked and SecretUpdate.KeyID are gone
L3 gocyclo on server.New, closure copied four times Fixed on this head (compartmentChecker, no gocyclo hit). It comes back on rebase, see Low 1
M5 / L9 KMS crypto, etag / if-match Still tracked in #1552 and #1550

New findings

Medium

  1. On the default address, a key can still land in a vault the client didn't address.
    providers/oci/vault/vault.go:409 (soleVaultLocked), called from providers/oci/vault/key.go:113 (CreateKey) and :200 (ListKeys).

Every vault advertises the same managementEndpoint (http://127.0.0.1:<port>). The oci-go-sdk keeps only the scheme and host of a client's endpoint and drops any path (common/client.go prepareRequest). So the only thing a stock request carries is the key's compartmentId, and the emulator picks "the only ACTIVE vault in that compartment". I checked three cases live:

  • Two ACTIVE vaults va and vb in compartment c2: CreateKey and ListKeys on either endpoint return 400. That's correct now, though the message says to pass ?vaultId=, and a stock SDK, CLI or Terraform client can't send that. With two vaults in a compartment, those clients can't create a key on the default address at all.
  • va scheduled for deletion, vb ACTIVE, both in c2: CreateKey on va's endpoint returned 200 with vaultId = vb. ListKeys on va's endpoint returned vb's keys. This is the case from my last note, and it hasn't changed. Real OCI refuses the call because va is pending.
  • vc ACTIVE in compartment c3, CreateKey on vc's endpoint with compartmentId c2: the key was created in vb, a different vault. ListKeys on vc's endpoint for c2 listed vb's keys. OCI lets a key sit in a different compartment from its vault (CreateKeyDetails.compartmentId is the key's compartment, and the vault comes from the endpoint). So this is two ACTIVE vaults with a silent wrong target, which is what the note asked to rule out.

Two related effects. When a compartment's only vault is pending, ListKeys on its endpoint returns 400 vaultId is required instead of listing its keys, and CreateKey returns the same 400 instead of 409 IncorrectState. GetKey also ignores the endpoint: va's key comes back through vb's endpoint, where real OCI would return 404.

What breaks for a user: a stack that destroys and re-creates a vault, or that keeps keys in an application compartment separate from a shared security compartment, gets keys in the wrong vault. Later secrets or key operations then refer to a vault the user never created the key in, and nothing errors.

Fix: the endpoint has to identify the vault, and with the SDK dropping the path, only the host or port can carry it. The clean option is a per-vault port on the default address: advertise http://127.0.0.1:<vault-port> and route requests by the listener they arrived on, with the same port kept after a --persist restart. If that's too much for this PR, at least make the fallback refuse every case it can't be sure of. Resolve across all vaults in the tenancy whatever their compartment, count vaults in any lifecycle state, and return 409 IncorrectState when the sole candidate isn't ACTIVE. Fix the 400 message too, so it doesn't point stock clients at a parameter they can't send. Either way, please add the SDK test from the note: two ACTIVE vaults, plus one pending vault and one vault in another compartment, all through stock clients on the default address, asserting that each key lands in and is listed from the addressed vault or the call is refused.

Low

  1. The gocyclo hit comes back when you rebase over #426.
    server/oci/oci.go:131 after merging current development (f488586). #426 (Compute) added its own copy of the compartment-checker closure. On the merged tree New is at gocyclo 11. The merge itself is clean, and go build ./... passes on it. When you rebase, switch Compute to computeHandler.SetCompartmentChecker(checker) like the others.

OCI docs

  • CreateKeyDetails: compartmentId is the key's compartment, and the vault is the one whose management endpoint is called (Medium 1).
  • UpdateSecretDetails: no keyId, and the handler now refuses it.

Terraform and serve

I didn't run Terraform. The oracle/oci provider has no plain-HTTP per-service endpoint override, and a local TLS intercepting proxy wasn't available here. As the fallback I used the real oci-go-sdk against cloudemu serve --providers oci --oci-port 47625, with stock KmsVaultClient, KmsManagementClient and IdentityClient. Each KMS client was built on the vault's own managementEndpoint with no dialer override. That run produced the results in Medium 1 and the table above. The Dockerfile is unchanged, and the new and changed flags go through serveflags → serverkit → ociserver.Drivers as before.

What I verified

  • go build ./... is clean on this head and on this head merged with current development. go vet on server/oci and providers/oci is clean.
  • go test -race passes for providers/oci/vault, server/oci/..., server/serveflags and persist. Coverage: provider vault 97.3%, wire vault 95.0%.
  • gofmt, go mod tidy and go generate . leave no diff.
  • golangci-lint: no new issues on this head (the serveflags goconst hits are pre-existing). Merged with development, there's one gocyclo hit (Low 1).
  • CI: all 23 checks are green.
  • Delta since 58ad50d: the ListKeys fallback, the keyId refusal, the compartmentChecker helper, and the docs lines for both. The rest is the rebase over #422 (Object Storage wiring and its --oci-tenancy flag). SetCompartmentChecker(nil) is safe in every handler, since each one treats a nil checker as "skip".
  • No persisted state changed in this round. Removing rekeyLocked only drops a code path.

Verdict

Changes requested on Medium 1. The two-active-vault case is handled, but a stock client can still put a key in a vault it didn't address, and that's the case my note was about. Everything else looks good.

// soleVaultLocked picks the vault a key request that names none addresses:
// the only active vault in the compartment. This covers a KMS client on the
// emulator's plain address. Callers hold mu.
func (m *Mock) soleVaultLocked(compartmentID string) (string, error) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This picks the only ACTIVE vault in the key's compartment, but every vault advertises the same endpoint and the SDK drops any path, so a stock request never says which vault it's for. Live with stock clients: CreateKey on a pending vault's endpoint created the key in the other, active vault. CreateKey on vault vc's endpoint (compartment c3) with a key compartment of c2 created the key in vb, the active vault in c2. OCI allows a key in a different compartment from its vault. Either give each vault its own port on the default address, or refuse every case that isn't certain: resolve across all vaults in any state and compartment, and return 409 IncorrectState when the sole candidate isn't ACTIVE.

// Real OCI lists the keys of the vault whose endpoint was called. With no
// vault named, resolve it as CreateKey does rather than list every vault.
if vaultID == "" {
id, err := m.soleVaultLocked(compartmentID)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same resolution for ListKeys. On a pending vault's endpoint this lists the other active vault's keys. When a compartment's only vault is pending, it returns 400 vaultId is required, though real OCI would list that vault's keys. The 400 message also points stock SDK, CLI and Terraform clients at ?vaultId=, which they can't send.

Comment thread docs/services.md
always resolves. A key request on that address names its vault with
`?vaultId=`, and with a single active vault in the compartment it may omit it,
so a stock SDK client built on the endpoint works while there is one vault.
Every vault shares that address, so on it `/keys` (create and list alike)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth stating the limits here once the routing is settled. With several vaults, stock clients can't use the default address at all, and a key's compartment has to match the vault's for the fallback to find it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

oci Oracle Cloud Infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OCI Vault: vaults, keys, secrets

2 participants