knative-extensions / knative-extensions/func-go

Harden Kafka security env-var parsing

Open
#187 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
7
Forks
12
PR merge metrics
No merged PRs in 30d

Description

Follow-up from https://github.com/knative-extensions/func-go/pull/186#issuecomment-5496815228 (review by @matejvasek).

None of these are reachable through the normal func deployment path — the CLI validates enums and only emits env vars alongside a matching `KAFKA_SECURITY_PROTOCOL`. They only matter for direct callers (hand-written manifests, `docker run`, standalone host runner).

### 1. Silent PLAINTEXT fallback

If `KAFKA_SECURITY_PROTOCOL` is unset but `KAFKA_TLS_*` / `KAFKA_SASL_*` are set, `configureSecurity` returns nil and connects in cleartext with no warning. A log warning or error would prevent silently insecure connections for direct callers.

### 2. Case-sensitive enums

`KAFKA_SECURITY_PROTOCOL` and `KAFKA_SASL_MECHANISM` are matched case-sensitively (`SASL_SSL` works but `sasl_ssl` doesn't). Inconsistent with the `EqualFold` used for `KAFKA_TLS_SKIP_VERIFY`. A `strings.ToUpper` before the switch would align them.

### 3. `KAFKA_TLS_SKIP_VERIFY` parsing

Only the exact token `true` is honored; `1` / `yes` / `" true "` silently leave verification on. Fails safe, but silently ignores operator intent.

### 4. `scramClient.Step` / `Done` nil-deref

`c.conv` is nil until `Begin` succeeds; there's no defensive nil check. Sarama's current flow guards this by aborting on a `Begin` error, so it's latent rather than reachable today. A nil guard would keep it safe if Sarama's flow changes.

Contributor guide

Open the contributing guide

Research direction

Start by locating configureSecurity and the scramClient Step, Done, and Begin methods. Review how the listed KAFKA_* variables are parsed, then add coverage for insecure fallback warnings or errors, case-insensitive values, accepted skip-verify input, and nil safety; done means these direct-caller cases are handled without changing the normal deployment path.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, kafka
Domain
backend, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.