[security][o2ims] Verify Kubernetes API TLS certificates and read the token without a shell

[thc1006]

This supersedes #1169, which is closed. Both fixes land here as two reviewable commits against one tree.

They were separate PRs, and that was wrong. The main-branch postsubmit publishes nephio/o2ims-operator:latest on every merge:

--destination=nephio/o2ims-operator:${BUILD_ID} --destination=nephio/o2ims-operator:latest

Merging the TLS fix alone would therefore publish an image that verifies certificates but still builds its bearer token through os.popen and still never re-reads it — while the README it ships with says the kubelet rotates that token. One merge, one image, no window in which a published latest carries a defect the project already has a fix for.

Second of a small series on the operators' Kubernetes clients, after #1168. The first commit makes the operator verify the Kubernetes API server's certificate; the second stops building the bearer token through a shell and re-reads it per request.

controllers/utils.py resolves the bearer token by interpolating an environment variable into a shell command:

TOKEN = os.getenv("TOKEN", "/var/run/secrets/kubernetes.io/serviceaccount/token")
TOKEN = os.popen(f"cat {TOKEN}").read()

TOKEN is documented as a path — tests/create-cluster.sh sets it to /tmp/porch-token — but it reaches a shell verbatim, so whatever it contains runs:

$ TOKEN='/absent; touch /tmp/tmpXXXX/pwned'
os.popen : token=''  marker created: True
open()   : FileNotFoundError          marker created: False

Two things are visible in that output. The injected touch executed, and the token came back empty: cat wrote its complaint to stderr and .read() returned "". CWE-78, and a failure mode that hides itself.

What the empty token does

HEADERS_DICT is built from it at import, so the operator starts with Authorization: Bearer and every call to the API server answers 401. The callers turn that into {"status": False, "reason": "unauthorized"}, which is what a reader of the logs sees — nothing points at the token file.

The same structure has a second consequence that has nothing to do with the shell. The header is built once and never rebuilt, while Kubernetes rotates the projected token it mounts into the pod. Once the value read at startup expires, the operator gets 401 on everything until someone restarts it, and reports the same unauthorized on the way down.

Why this is not a two-line patch

Swapping os.popen for open() is the obvious fix, and on its own it forces a choice about the unreadable-file case that cat currently papers over. Both available answers are bad:

• Raise at import, and the operator refuses to start anywhere the file is absent — including pytest, which imports the module before its fixtures run.
• Swallow the error, and today's silent 401s survive the fix.

Reading the token where it is used answers the question instead of choosing: the caller sees the real error, at the point where it can be reported, and rotation stops being a problem because there is no longer a value cached from startup.

def read_token() -> str:
    """Return the token, read fresh so that rotation is picked up."""
    path = os.getenv("TOKEN", IN_CLUSTER_TOKEN_FILE)
    with open(path, encoding="utf-8") as token_file:
        return token_file.read().strip()


def request_headers() -> dict:
    """Return the API server request headers, carrying the current token."""
    return {**BASE_HEADERS, "Authorization": f"Bearer {read_token()}"}

The .strip() is not cosmetic. A token file written with echo ends in a newline, and requests accepts such a header value while http.client rejects it on the wire:

'Bearer tok'    -> sent, server saw 'Bearer tok'
'Bearer tok\n'  -> ValueError: Invalid header value b'Bearer tok\n'

Proof manifests

$ pytest --disable-warnings -q      # combined tree, both commits
135 passed in 0.94s

Run in the image the operator ships from, against git archive HEAD rather than a working directory, on a laptop and inside a simulated pod:

$ podman run --rm -v $PWD:/w -w /w \
    -e KUBERNETES_SERVICE_HOST=10.96.0.1 -e KUBERNETES_SERVICE_PORT=443 \
    python:3.12.10-alpine3.21 sh -c '...mount token and ca.crt...; tox -e py312'
135 passed in 1.36s

That second run is not decoration. An earlier version of this branch passed on a laptop and failed eight tests in a pod, because the fixture that claimed to isolate the environment did not clear KUBERNETES_SERVICE_HOST.

Restoring the original semantics — os.popen back in read_token, and the token captured once at startup instead of per request — fails thirteen of them:

$ pytest --disable-warnings -q      # with main's semantics restored
FAILED tests/test_utils.py::test_token_surroundings_are_stripped[tok\n]
FAILED tests/test_utils.py::test_token_surroundings_are_stripped[  tok\r\n]
FAILED tests/test_utils.py::test_an_empty_or_blank_token_is_refused[]
FAILED tests/test_utils.py::test_an_empty_or_blank_token_is_refused[ ]
FAILED tests/test_utils.py::test_an_empty_or_blank_token_is_refused[\n]
FAILED tests/test_utils.py::test_an_empty_or_blank_token_is_refused[\r\n]
FAILED tests/test_utils.py::test_an_empty_or_blank_token_is_refused[ \t \r\n]
FAILED tests/test_utils.py::test_a_token_with_internal_whitespace_is_refused[tok en]
FAILED tests/test_utils.py::test_a_token_with_internal_whitespace_is_refused[tok\nen]
FAILED tests/test_utils.py::test_a_token_with_internal_whitespace_is_refused[tok\ten]
FAILED tests/test_utils.py::test_an_invalid_token_stops_before_any_request_is_made
13 failed, 122 passed in 1.52s

The sixth, test_token_surroundings_are_stripped[tok], passes under both, which is correct: a token with nothing around it needs no stripping.

The rotation test is the one the old shape made impossible to write. It changes the file between two requests and reads back what actually went out:

    get_capi_cluster(NAME, NAMESPACE)
    token_file.write_text("second")
    get_capi_cluster(NAME, NAMESPACE)

    sent = [call.request.headers["Authorization"] for call in responses.calls]
    assert sent == ["Bearer first", "Bearer second"]

The injection test's payload only ever writes inside pytest's tmp_path, and asserts both that the read fails and that the marker was not created.

flake8 controllers/utils.py reports 18 findings on main and 17 here; all are pre-existing E501s and the patch adds none. As with #1169, .prow.yaml never runs tox and the root make unit only walks directories containing a go.mod, so none of this runs in CI today — the runs above are local against the pinned requirements.txt.

What review changed

Two defects survived the first version, and the second one was hiding the first:

• An empty or blank token file still produced Authorization: Bearer — the exact failure this PR claims to remove, moved from "cat failed" to "the file is there and empty". read_token now refuses an empty result, and refuses a token containing internal whitespace, which http.client would reject on the wire anyway.
• The autouse fixture created an empty token file. Every pre-existing request test therefore ran with Bearer , and none of them asserted the header, so the passing count proved nothing about token handling. The fixture now writes a real token through monkeypatch — which also stops it leaving TOKEN pointing at a deleted path after the module finishes — and one existing request test now pins the header value.

Four more came out of testing the code against what it actually talks to rather than against its own assumptions:

• The mounted CA was pinned to whatever endpoint this was pointed at. tls_verify used the bundle at /var/run/secrets/.../ca.crt whenever it existed, so an operator in a pod with KUBERNETES_BASE_URL set to another API server had the cluster CA applied to a server that CA does not certify, and every request to it would fail. The bundle now follows the address: the cluster serves its API at the advertised host and at https://kubernetes.default.svc, and anything else keeps the roots requests ships with. tls_verify takes the resolved address for this reason. The FOCOM sibling had the same defect and takes the same rule.

• The validated host was not always the host connected to. Comparing validate_api_server_url against what requests resolves, rather than reading either, turned up three disagreements: urlsplit drops a tab inside the authority and keeps a NUL, while requests percent-encodes both.

  'https://good.example\t.evil'   validator saw 'good.example.evil'
                                  requests    saw 'good.example%09.evil'
  

  Control characters and port zero are now refused. Endpoints are also compared as TLS origins rather than as strings, so https://10.96.0.1 and https://10.96.0.1:443 — one endpoint with one certificate, as are the two spellings of an IPv6 address and any casing of a DNS name — no longer take different trust paths. Four such forms were being sent to the public roots; that fails closed on a private-CA cluster rather than weakening anything, but it fails a configuration that is correct. KUBERNETES_BASE_URL is operator configuration rather than attacker input, so this is not an escalation, but a client that carries a bearer token should refuse an address it cannot agree on. The Go validator was measured the same way and already agrees with net/http, which rejects these at parse time.

• Only one of the four API calls had its TLS setting pinned. Deleting verify=TLS_VERIFY from any of the other three left the suite green — not a state a security invariant should be able to reach. Every call site is now exercised and asserted on both verify and Authorization; all eight deletions fail.

• Test collection depended on the pod it ran in. This module resolves the address and the bundle at import, so a fixture cannot undo an ambient setting: by the time one runs, the test module has already imported the package. tests/conftest.py clears it first, because pytest loads conftest before test modules. It matters because the kubelet injects KUBERNETES_SERVICE_HOST even when automountServiceAccountToken is false, so Run the o2ims Python tests in CI #1175's CI pod looks like a cluster with no CA. Running pytest directly under those variables failed collection before, and passes after. tox happens to filter them out, so Run the o2ims Python tests in CI #1175 was already green — incidental, and now not relied on.

• A rollout window across two repositories. My earlier claim that this has no effect on a healthy pod was too strong. Catalog releases set KUBERNETES_BASE_URL=https://kubernetes.default.svc, which Kubernetes does not guarantee a serving certificate for, and that value cannot be removed in this commit because it lives in nephio-project/catalog. The postsubmit publishes this code as :latest the moment it merges, so between here and catalog#146 a pod can run the new image with the old package. In a pod, that exact value now resolves to the advertised address with a warning — the same server, named the way Kubernetes certifies it. Any other endpoint, any path, and anything outside a pod is untouched. The FOCOM sibling needs no such bridge, because its manifests are in this repository and change in the same commit.

  Worth flagging separately for whoever merges: the catalog pins :latest with imagePullPolicy: IfNotPresent, so merging this does not by itself replace the image on a node that already cached one. catalog#146 pinning an immutable tag is what actually rolls the fix out.

Rotation coverage was also too narrow: it only exercised get_capi_cluster. The production path does a GET and then a POST inside create_package_variant, so there is now a test that rotates the file between those two and asserts the second request carries the new token.

Effect on existing deployments

None in a healthy pod: the token file is there and is now read per request instead of once. A deployment that was running on an empty token was already failing every call with 401 and now gets a FileNotFoundError naming the path it could not read, which is the same outage with a usable message.

HEADERS_DICT is replaced by BASE_HEADERS plus request_headers(). Nothing outside controllers/utils.py referenced it. The autouse fixture in tests/test_utils.py that sets TOKEN to a temporary file starts having an effect for the first time; it previously ran after the module had already been imported and read.

Notes for maintainers

• This is now stacked on [security][o2ims] Verify Kubernetes API TLS certificates by default #1169 rather than forked beside it. The two branches previously shared a base and replaced the same block, so each head still contained the defect the other fixed; resolving that by hand later is exactly where HTTPS_VERIFY or os.popen could have come back. The merge is asserted, not assumed: the resolution checks that no os.popen, disable_warnings, bool(os.getenv("HTTPS_VERIFY" or 127.0.0.1:8080 survives, and that all four call sites carry both request_headers() and TLS_VERIFY.
• Requests still have no timeout, and a token failure is flattened into status=False which cluster_creation_request reports as progressing before a 30-minute generic timeout — so the clear error this PR produces can still reach the user as a slow timeout. Raised as [o2ims] API calls have no timeout and configuration failures are reported as progressing #1174 rather than folded in here.
• Release triage is yours. The change is confined to the o2ims operator's API client and its tests.
• SECURITY.md asks that vulnerabilities not be reported through public issues. I have not opened a public issue for it. Say the word and I will route it through sig-security@lists.nephio.org before this goes further.

[nephio-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign johnbelamaric for approval by writing /assign @johnbelamaric in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[nephio-prow]

Hi @thc1006. Thanks for your PR.

I'm waiting for a nephio-project member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.