[draft] Treat Azure buckets without a configured identity as public - #2137
[draft] Treat Azure buckets without a configured identity as public#2137Pranav-error wants to merge 1 commit into
Conversation
azureauth.NewTokenCredential never returns nil, so the token != nil guard in chainCredentialWithSecret was always true, the chain was never empty, and the documented nil return was dead. At the caller that made azblob.NewClientWithNoCredential unreachable, so a Bucket with provider azure and no secretRef could not read a public container. docs/spec/v1/buckets.md states that when no chain can be established the bucket is assumed to be publicly reachable, and ships an azure-public example with no secretRef. Add the token credential only when an identity was actually requested: per-object via .spec.serviceAccountName, or controller-wide via AZURE_CLIENT_ID / AZURE_FEDERATED_TOKEN_FILE. Otherwise the chain stays empty and the caller builds an unauthenticated client. The existing chain test asserted the buggy behaviour, expecting a credential for a nil secret. It now covers all three cases: no identity, object-level, and controller-level. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: sai pranav <rajasaipranav0@gmail.com>
|
Closing out the verification caveat from the description — I have now run the suite locally with envtest, so this is no longer untested on my side.
The workflow runs on this PR are still awaiting maintainer approval, so CI has not exercised it yet. |
|
I'm for correcting the docs and removing the section where we mention public access. None of the Bucket providers work without auth. I'm not sure how we ended up with Azure Bucket supporting public access, I probably missed this during review as I would've blocked it. cc @matheuscscp |
Agree 100% |
|
Thanks @stefanprodan and @matheuscscp — that answers the question this was opened to ask, so closing in favour of #2138, which corrects the docs and removes the dead anonymous path instead. For the record on how it got there: the fallback was already unreachable, so nobody was relying on it. |
Draft, opened to give #2136 something concrete to react to rather than to presume the outcome. Please treat the direction as an open question — if you would rather remove the fallback and correct the docs, say so and I will close this and send that instead.
Refs #2136.
Problem
azureauth.NewTokenCredentialnever returns nil, so the guard inchainCredentialWithSecretis always true,credsis never empty, and the documentedreturn nil, nilis dead. At the caller that makesazblob.NewClientWithNoCredentialunreachable, so aBucketwithprovider: azureand nosecretRefcannot read a public container.docs/spec/v1/buckets.mddocuments the opposite:and ships an
azure-publicexample with nosecretRef, and states that publicly accessible storage needs neithersecretRefnorserviceAccountName.Note this is not a regression from #1875, contrary to what I first wrote on the issue and have since corrected there. The previous chain ended in
azidentity.NewManagedIdentityCredential(nil), which also constructs successfully anywhere, so the fallback was already unreachable. This is a long-standing gap between the docs and the code.Change
Add the token credential only when an identity has actually been requested:
.spec.serviceAccountName, signalled by a newazure.WithObjectLevelIdentity()set by the reconciler;AZURE_CLIENT_IDorAZURE_FEDERATED_TOKEN_FILE.Otherwise the chain stays empty,
chainCredentialWithSecretreturns nil as documented, and the caller builds an unauthenticated client.staticcheckno longer reports SA4023 here, andgo vetis clean. I could not runmake testlocally as it needs envtest assets, so the controller suite has not been exercised on my side — CI will be the real check.The trade-off you should weigh
A user relying on system-assigned managed identity with no environment variables set would now fall back to anonymous access instead of attempting IMDS. That is a real behaviour change and the main reason this is a draft. Detecting that case cheaply is not possible at construction time, since
NewManagedIdentityCredentialsucceeds regardless of whether IMDS is reachable.It is also worth noting
gcp.NewClienthas no anonymous path at all — when no secret is present it always uses a token source. If "cloud providers always authenticate" is the intended direction, then the correct change is the opposite of this one: delete the unreachable fallback and drop the public-bucket language from the Azure docs.Tests
Test_chainCredentialWithSecretpreviously asserted the buggy behaviour, expecting a credential for a nil secret. It now covers no identity, object-level identity and controller-level identity.The pre-existing anonymous coverage runs through
withoutCredentials(), which is unexported and only called fromblob_test.go, so it exercises a path the reconciler cannot reach. This change gives the anonymous path production coverage; happy to add a reconciler-level test too if you want it.