Skip to content

fix: relax jwtAuth private_key requirement and add CEL validation#406

Open
AlinsRan wants to merge 8 commits intomasterfrom
fix/jwt-auth-private-key-required
Open

fix: relax jwtAuth private_key requirement and add CEL validation#406
AlinsRan wants to merge 8 commits intomasterfrom
fix/jwt-auth-private-key-required

Conversation

@AlinsRan
Copy link
Copy Markdown
Contributor

@AlinsRan AlinsRan commented May 7, 2026

Summary

Fixes #400

ApisixConsumer.spec.authParameter.jwtAuth.value.private_key was incorrectly marked as required in the CRD schema, rejecting symmetric JWT configurations (e.g. HS256 + secret) before they could reach the translator.

Changes

  • api/v2/apisixconsumer_types.go: Add omitempty to PrivateKey JSON tag to remove the unconditional required constraint from the CRD schema
  • CEL validation rule: Asymmetric algorithms (RS*/ES*/PS*/EdDSA) require at least one of public_key or private_key; symmetric algorithms (HS256/HS384/HS512) and unset algorithm have no key requirement
  • config/crd/bases/apisix.apache.org_apisixconsumers.yaml: Regenerated — private_key removed from required, x-kubernetes-validations added

Backward Compatibility

Existing configuration Result
Asymmetric + private_key (previous norm) Still valid
Asymmetric + public_key Still valid
Asymmetric + both keys Still valid
Symmetric + secret, no private_key Now valid (was rejected before)

No previously accepted configurations are rejected by this change.

Note: x-kubernetes-validations (CEL) requires Kubernetes 1.25+.

Summary by CodeRabbit

  • Bug Fixes

    • CRD validation now enforces algorithm-specific JWT rules: symmetric algorithms (HS*) do not require key material; asymmetric algorithms (RS*/ES*/PS*/EdDSA) require at least a public or private key.
    • Private key is now optional in JSON serialization.
  • Tests

    • Added validation tests covering symmetric/asymmetric JWT cases and related failure scenarios.

Previously, private_key was marked as required in the CRD schema, which
rejected symmetric JWT configurations (e.g. HS256 + secret) before they
reached the translator.

Changes:
- Add omitempty to PrivateKey JSON tag so the CRD schema no longer
  requires it unconditionally
- Add CEL validation rule: asymmetric algorithms (RS*/ES*/PS*/EdDSA)
  require at least one of public_key or private_key; symmetric algorithms
  (HS256/HS384/HS512 or unset) have no key requirement
- Regenerate CRD manifest

Fixes #400
Copilot AI review requested due to automatic review settings May 7, 2026 19:43
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 7, 2026

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR enforces algorithm-dependent JWT key material requirements through a kubebuilder XValidation rule: symmetric algorithms (HS256/HS384/HS512) use secret, while asymmetric algorithms (RS*/ES*/PS*/EdDSA) require at least one of public_key or private_key. The PrivateKey field is marked optional in JSON serialization, and a comprehensive CEL test suite validates the rule against both compliant and non-compliant configurations.

Changes

JWT Auth Key Material Validation

Layer / File(s) Summary
JWT Validation Constraint
api/v2/apisixconsumer_types.go
Adds kubebuilder XValidation rule to ApisixConsumerJwtAuthValue enforcing asymmetric algorithms require public_key or private_key while symmetric algorithms use secret.
Field Serialization
api/v2/apisixconsumer_types.go
Marks ApisixConsumerJwtAuthValue.PrivateKey JSON tag as optional with omitempty.
CRD Schema Validation
config/crd/bases/apisix.apache.org_apisixconsumers.yaml
Adds x-kubernetes-validations rule under spec.authParameter.jwtAuth.value mirroring the Go XValidation rule to enforce validation at API server level.
Tests: Schema Loader & Validator
api/v2/apisixconsumer_validation_test.go
Adds consumerSchemaValidator helper and loadApisixConsumerSchema function to load the CRD and build a CEL validator for runtime constraint checking.
Tests: Symmetric Algorithm Cases
api/v2/apisixconsumer_validation_test.go
Adds CEL tests for HS256, HS384, HS512 with secret, and tests omitted algorithm field defaulting to symmetric behavior.
Tests: Asymmetric Algorithm with Key Material
api/v2/apisixconsumer_validation_test.go
Adds CEL tests validating RS256 acceptance when only public_key, only private_key, or both keys are present.
Tests: Asymmetric Algorithm without Key Material
api/v2/apisixconsumer_validation_test.go
Adds CEL negative tests rejecting RS256, ES256, and EdDSA without any key material, and rejecting RS256 with empty public_key.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
E2e Test Quality Review ⚠️ Warning Tests are unit-level schema validation only, not E2E. Custom check explicitly requires E2E tests covering the full business flow with external dependencies/services. Add E2E tests that exercise the full flow: create ApisixConsumer → deploy to K8s → verify translation → test against real APISIX API. Also fix error handling: line 63 ignores celValidator return value.
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: relaxing the private_key requirement and adding CEL validation for JWT authentication.
Linked Issues check ✅ Passed All coding requirements from issue #400 are met: private_key is relaxed via omitempty, CEL validation enforces asymmetric algorithm requirements, and backward compatibility is maintained.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the JWT authentication schema: updating the types, regenerating the CRD manifest, and adding comprehensive validation tests.
Security Check ✅ Passed No security vulnerabilities found. PR correctly relaxes JWT schema validation while adding appropriate CEL rules. No credential logging detected in code.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/jwt-auth-private-key-required

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes CRD schema validation for ApisixConsumer JWT auth so symmetric JWT configurations (e.g., HS256 + secret) are no longer rejected due to an incorrectly-required private_key, and it adds CEL validation to enforce key requirements for asymmetric algorithms.

Changes:

  • Make jwtAuth.value.private_key optional in the Go type (so it’s no longer required in the generated CRD schema).
  • Add a CEL (x-kubernetes-validations) rule to require at least one of public_key/private_key when using asymmetric JWT algorithms.
  • Regenerate the ApisixConsumer CRD YAML to reflect the schema and validation changes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
api/v2/apisixconsumer_types.go Makes private_key optional and introduces an XValidation rule/comment for JWT key requirements.
config/crd/bases/apisix.apache.org_apisixconsumers.yaml Updates generated CRD schema: removes private_key from required and adds CEL validation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread api/v2/apisixconsumer_types.go Outdated
Comment thread api/v2/apisixconsumer_types.go Outdated
Comment thread config/crd/bases/apisix.apache.org_apisixconsumers.yaml Outdated
Tests directly exercise the x-kubernetes-validations CEL rule using
k8s.io/apiextensions-apiserver's cel.NewValidator, without requiring
envtest or a real API server.

Covers:
- symmetric algorithms (HS256/HS384/HS512) with secret, no private_key
- algorithm omitted (defaults to symmetric)
- asymmetric algorithms (RS256/ES256/EdDSA) with public_key only
- asymmetric algorithms with private_key only (backward compat)
- asymmetric algorithms with both keys
- asymmetric algorithms with no keys (rejected)
- asymmetric algorithms with empty public_key string (rejected)
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@api/v2/apisixconsumer_types.go`:
- Around line 133-137: The XValidation rule on the APISIXConsumer type (the
+kubebuilder:validation:XValidation tag in apisixconsumer_types.go) contains
smart quotes and invalid empty-string literals; update the CEL expression so
empty-string checks use ASCII single-quote literals and normal ASCII quotes
(e.g. change (has(self.public_key) && self.public_key != ”) to
(has(self.public_key) && self.public_key != '') and likewise for private_key),
ensuring the overall rule string remains wrapped correctly in ASCII quotes.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 064ecaeb-d39c-4abf-9525-e5709095a3c2

📥 Commits

Reviewing files that changed from the base of the PR and between b02e842 and 06efa8c.

📒 Files selected for processing (2)
  • api/v2/apisixconsumer_types.go
  • config/crd/bases/apisix.apache.org_apisixconsumers.yaml

Comment thread api/v2/apisixconsumer_types.go Outdated
controller-gen misinterprets empty string literals written with double
quotes inside a double-quoted marker value, producing Unicode right
double quotation marks in the generated CRD. Switch to single-quoted
empty strings ('') which are valid CEL and survive the YAML generation
pipeline intact.

Also add CEL unit tests that load the schema directly from the generated
CRD YAML file, ensuring tests always validate against the real schema.
Copilot AI review requested due to automatic review settings May 7, 2026 19:52
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@api/v2/apisixconsumer_jwtauth_cel_test.go`:
- Line 48: The validation Rule string currently allows whitespace-only
asymmetric keys; update the Rule expressions (the lines containing "Rule:   
\"!has(self.algorithm) || self.algorithm in ['HS256','HS384','HS512'] ||
(has(self.public_key) && self.public_key != '') || (has(self.private_key) &&
self.private_key != '')\"") to require non-whitespace content, e.g. replace the
checks with a regex-based non-whitespace test like "(has(self.public_key) &&
self.public_key.matches('.*\\S.*'))" and similarly for private_key (and apply
the same change to the other Rule instance referenced in the comment), and add a
regression test in apisixconsumer_jwtauth_cel_test.go that asserts an asymmetric
algorithm rejects public_key/private_key values consisting only of spaces.
- Around line 112-135: Replace the PEM-like literal strings in the JwtAuth CEL
tests with simple non-empty placeholders to avoid triggering secret scanners:
update the public_key/private_key fields in
TestJwtAuthCEL_AsymmetricRS256WithPrivateKey,
TestJwtAuthCEL_AsymmetricRS256WithBothKeys (and the earlier test that uses
"public_key") to simple non-empty values like "public-key-placeholder" /
"private-key-placeholder" while leaving algorithm and key fields unchanged, and
keep using validateJwtAuthValue to assert validity.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8cb70d1f-92ad-43cd-b475-cda7e1c42ffe

📥 Commits

Reviewing files that changed from the base of the PR and between 06efa8c and b50d0f6.

📒 Files selected for processing (1)
  • api/v2/apisixconsumer_jwtauth_cel_test.go

Comment thread api/v2/apisixconsumer_jwtauth_cel_test.go Outdated
Comment thread api/v2/apisixconsumer_jwtauth_cel_test.go Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

Comment thread api/v2/apisixconsumer_types.go Outdated
Comment thread api/v2/apisixconsumer_types.go Outdated
Comment thread api/v2/apisixconsumer_jwtauth_cel_test.go Outdated
Replace the previous approach of extracting jwtAuth.value subschema with
a consumerSchemaValidator that loads and validates against the complete
ApisixConsumer CRD schema. Test inputs use Go structs (ApisixConsumer)
marshaled to JSON, matching what Kubernetes actually receives.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
api/v2/apisixconsumer_types.go (1)

138-157: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

ApisixConsumerJwtAuthValue is a secret-bearing struct with no redaction methods.

Secret (HMAC shared secret) and PrivateKey (asymmetric private key) are consumer credentials. Without String(), MarshalJSON(), or MarshalLogObject() redaction, any fmt.Sprintf("%+v", v) / zap.Any("jwt", v) / json.Marshal call will leak these secrets to logs or API responses.

🔒 Example redaction skeleton
+
+// String redacts sensitive fields to prevent accidental credential leakage.
+func (v ApisixConsumerJwtAuthValue) String() string {
+	return fmt.Sprintf("ApisixConsumerJwtAuthValue{Key:%s Algorithm:%s}", v.Key, v.Algorithm)
+}
+
+// MarshalJSON redacts Secret and PrivateKey.
+func (v ApisixConsumerJwtAuthValue) MarshalJSON() ([]byte, error) {
+	type Alias ApisixConsumerJwtAuthValue
+	redacted := struct {
+		Alias
+		Secret     string `json:"secret,omitempty"`
+		PrivateKey string `json:"private_key,omitempty"`
+	}{
+		Alias:      (Alias)(v),
+		Secret:     "[REDACTED]",
+		PrivateKey: "[REDACTED]",
+	}
+	return json.Marshal(redacted)
+}

As per coding guidelines, all secret-bearing structs must implement proper redaction in String(), MarshalJSON(), and MarshalLogObject() methods; verify pointer receiver vs value receiver correctness to ensure redaction is called when logging.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/v2/apisixconsumer_types.go` around lines 138 - 157,
ApisixConsumerJwtAuthValue contains secret fields (Secret, PrivateKey) and must
implement redaction: add String(), MarshalJSON(), and MarshalLogObject() with
pointer receivers on ApisixConsumerJwtAuthValue to ensure all logging/formatting
paths call the redactor; in each method replace Secret and PrivateKey (and any
other sensitive fields like Base64Secret if you consider it sensitive) with a
fixed redacted marker (e.g. "<redacted>") while preserving non-sensitive fields
(Key, PublicKey, Algorithm, Exp, LifetimeGracePeriod), and ensure MarshalJSON
returns a JSON object with redacted values and MarshalLogObject writes redacted
fields to the zapcore.ObjectEncoder so zap.Any/encoding/json/ fmt.Sprintf do not
leak secrets.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@api/v2/apisixconsumer_types.go`:
- Around line 133-137: The file apisixconsumer_types.go is failing gofmt; run
gofmt -w (or go fmt ./...) to reformat the file, stage and commit the change so
the CI passes; specifically reformat the comment/Kubebuilder tag line containing
+kubebuilder:validation:XValidation (the long rule string) so spacing/line
breaks match gofmt output and no gofmt lint remains.

---

Outside diff comments:
In `@api/v2/apisixconsumer_types.go`:
- Around line 138-157: ApisixConsumerJwtAuthValue contains secret fields
(Secret, PrivateKey) and must implement redaction: add String(), MarshalJSON(),
and MarshalLogObject() with pointer receivers on ApisixConsumerJwtAuthValue to
ensure all logging/formatting paths call the redactor; in each method replace
Secret and PrivateKey (and any other sensitive fields like Base64Secret if you
consider it sensitive) with a fixed redacted marker (e.g. "<redacted>") while
preserving non-sensitive fields (Key, PublicKey, Algorithm, Exp,
LifetimeGracePeriod), and ensure MarshalJSON returns a JSON object with redacted
values and MarshalLogObject writes redacted fields to the zapcore.ObjectEncoder
so zap.Any/encoding/json/ fmt.Sprintf do not leak secrets.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3c9cc1ef-d98b-436f-a02c-344a5ec100eb

📥 Commits

Reviewing files that changed from the base of the PR and between b50d0f6 and 6073371.

📒 Files selected for processing (3)
  • api/v2/apisixconsumer_jwtauth_cel_test.go
  • api/v2/apisixconsumer_types.go
  • config/crd/bases/apisix.apache.org_apisixconsumers.yaml

Comment thread api/v2/apisixconsumer_types.go Outdated
gofmt 1.19+ reformats consecutive single quotes ('') in doc comments
to Unicode curly double-quotes, corrupting the CEL rule at generation
time. Replace empty-string comparisons with size() > 0 checks which
use no string literals and are immune to gofmt reformatting.
Copilot AI review requested due to automatic review settings May 7, 2026 20:03
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
api/v2/apisixconsumer_validation_test.go (1)

108-310: ⚡ Quick win

Consider table-driven tests and add explicit PS* asymmetric cases.

Coverage is good, but this block is repetitive and currently skips explicit PS* assertions despite the asymmetric rule intent including PS*. A table-driven matrix (success/failure expectations) would reduce duplication and make algorithm coverage complete and easier to maintain.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/v2/apisixconsumer_validation_test.go` around lines 108 - 310, The tests
for JWT algorithm validation are repetitive and miss explicit PS* asymmetric
cases; replace the many individual TestApisixConsumer_JwtAuth_* functions with a
single table-driven test that iterates cases (algorithm, publicKey, privateKey,
expectError) and constructs the apisixv2.ApisixConsumer with JwtAuth.Value,
calling v := loadApisixConsumerSchema(t) and v.Validate(t, ac) for each row;
include symmetric HS256/384/512 and default-no-algorithm success rows, explicit
asymmetric rows for RS256/ES256/EdDSA and PS256/PS384/PS512 with combinations
(public only, private only, both, none), and assert expectError -> require.Error
+ assert.Contains(err.Error(), "asymmetric JWT algorithms") or no error ->
assert.NoError, using the existing helper and validation calls
(loadApisixConsumerSchema, Validate) to keep coverage complete and reduce
duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@api/v2/apisixconsumer_validation_test.go`:
- Around line 187-188: The test fixtures use realistic PEM headers (e.g. values
assigned to PublicKey and private-key fields alongside Algorithm in
apisixconsumer_validation_test.go) which trigger secret scanners; replace those
PEM-like strings with clearly non-sensitive placeholders (e.g.
"TEST_PUBLIC_KEY_PLACEHOLDER" / "TEST_PRIVATE_KEY_PLACEHOLDER" or a short
non-PEM dummy string) at each occurrence (the PublicKey fields, any PrivateKey
variables, and associated Algorithm test entries) so the tests still exercise
logic without containing PEM headers or realistic key material.

---

Nitpick comments:
In `@api/v2/apisixconsumer_validation_test.go`:
- Around line 108-310: The tests for JWT algorithm validation are repetitive and
miss explicit PS* asymmetric cases; replace the many individual
TestApisixConsumer_JwtAuth_* functions with a single table-driven test that
iterates cases (algorithm, publicKey, privateKey, expectError) and constructs
the apisixv2.ApisixConsumer with JwtAuth.Value, calling v :=
loadApisixConsumerSchema(t) and v.Validate(t, ac) for each row; include
symmetric HS256/384/512 and default-no-algorithm success rows, explicit
asymmetric rows for RS256/ES256/EdDSA and PS256/PS384/PS512 with combinations
(public only, private only, both, none), and assert expectError -> require.Error
+ assert.Contains(err.Error(), "asymmetric JWT algorithms") or no error ->
assert.NoError, using the existing helper and validation calls
(loadApisixConsumerSchema, Validate) to keep coverage complete and reduce
duplication.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 13410381-7886-4289-b856-4db50d97f341

📥 Commits

Reviewing files that changed from the base of the PR and between 6073371 and 9e62471.

📒 Files selected for processing (1)
  • api/v2/apisixconsumer_validation_test.go

Comment thread api/v2/apisixconsumer_validation_test.go Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

Comment thread api/v2/apisixconsumer_types.go Outdated
Comment thread api/v2/apisixconsumer_types.go Outdated
Comment thread api/v2/apisixconsumer_types.go
Comment thread config/crd/bases/apisix.apache.org_apisixconsumers.yaml Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
api/v2/apisixconsumer_types.go (1)

138-157: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

ApisixConsumerJwtAuthValue missing redaction for Secret and PrivateKey fields

The struct contains plaintext string fields for Secret (symmetric signing key) and PrivateKey (asymmetric private key). These must be redacted in String() and MarshalLogObject() methods per the coding guidelines. Any path that logs or serializes the struct (including Kubernetes controller-runtime reconciliation errors) will expose raw key material.

Required implementations:

  • func (v *ApisixConsumerJwtAuthValue) String() string – redact sensitive fields
  • func (v *ApisixConsumerJwtAuthValue) MarshalLogObject(enc zapcore.ObjectEncoder) error – redact sensitive fields

(Note: MarshalJSON() should be handled via a logging-specific wrapper type rather than on the API struct itself to avoid breaking Kubernetes serialization.)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/v2/apisixconsumer_types.go` around lines 138 - 157,
ApisixConsumerJwtAuthValue currently exposes sensitive Secret and PrivateKey
fields; implement func (v *ApisixConsumerJwtAuthValue) String() string and func
(v *ApisixConsumerJwtAuthValue) MarshalLogObject(enc zapcore.ObjectEncoder)
error to redact those fields (e.g. replace Secret and PrivateKey with a constant
like "<redacted>") while still emitting non-sensitive fields (Key, PublicKey,
Algorithm, Exp, Base64Secret, LifetimeGracePeriod); ensure MarshalLogObject uses
the provided zapcore.ObjectEncoder to add fields and returns nil on success, and
do not modify MarshalJSON/serialization on the API struct.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@api/v2/apisixconsumer_types.go`:
- Around line 138-157: ApisixConsumerJwtAuthValue currently exposes sensitive
Secret and PrivateKey fields; implement func (v *ApisixConsumerJwtAuthValue)
String() string and func (v *ApisixConsumerJwtAuthValue) MarshalLogObject(enc
zapcore.ObjectEncoder) error to redact those fields (e.g. replace Secret and
PrivateKey with a constant like "<redacted>") while still emitting non-sensitive
fields (Key, PublicKey, Algorithm, Exp, Base64Secret, LifetimeGracePeriod);
ensure MarshalLogObject uses the provided zapcore.ObjectEncoder to add fields
and returns nil on success, and do not modify MarshalJSON/serialization on the
API struct.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 04a84b25-5ba0-4a24-8541-eace8707a649

📥 Commits

Reviewing files that changed from the base of the PR and between 9e62471 and 97af003.

📒 Files selected for processing (2)
  • api/v2/apisixconsumer_types.go
  • config/crd/bases/apisix.apache.org_apisixconsumers.yaml
✅ Files skipped from review due to trivial changes (1)
  • config/crd/bases/apisix.apache.org_apisixconsumers.yaml

AlinsRan added 2 commits May 8, 2026 04:13
- Reword doc comment: remove misleading 'Exactly one' phrasing; clarify
  that only asymmetric algorithms require at least one key
- Extend CEL rule to treat empty algorithm string (size == 0) as
  symmetric, consistent with APISIX's default of HS256
- Replace PEM-like key fixtures with simple placeholders to avoid
  secret scanner false positives
- Add TestApisixConsumer_JwtAuth_EmptyAlgorithmTreatedAsSymmetric
- Regenerate CRD manifest and reference docs
- Use size(field.trim()) > 0 instead of size(field) > 0 in CEL rule so
  whitespace-only public_key/private_key values are rejected for asymmetric
  algorithms
- Update error message to accurately describe the rule condition (any
  algorithm other than HS256/HS384/HS512 requires a key)
- Add test for whitespace-only public_key regression
- Fix api/adc JwtAuthConsumerConfig.PrivateKey missing omitempty JSON tag
  so empty private_key is omitted from the ADC payload for symmetric configs
Copilot AI review requested due to automatic review settings May 7, 2026 20:22
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comment on lines +298 to +311
ac := &apisixv2.ApisixConsumer{
Spec: apisixv2.ApisixConsumerSpec{
AuthParameter: apisixv2.ApisixConsumerAuthParameter{
JwtAuth: &apisixv2.ApisixConsumerJwtAuth{
Value: &apisixv2.ApisixConsumerJwtAuthValue{
Key: "my-key",
Algorithm: "RS256",
// PublicKey is empty string — omitempty means it won't appear
// in the serialized JSON, same effect as not set
},
},
},
},
}
Comment on lines +317 to +336
// TestApisixConsumer_JwtAuth_EmptyAlgorithmTreatedAsSymmetric verifies that an
// explicitly empty algorithm string is treated the same as an unset algorithm
// (defaults to HS256) and does not require public_key or private_key.
func TestApisixConsumer_JwtAuth_EmptyAlgorithmTreatedAsSymmetric(t *testing.T) {
v := loadApisixConsumerSchema(t)
ac := &apisixv2.ApisixConsumer{
Spec: apisixv2.ApisixConsumerSpec{
AuthParameter: apisixv2.ApisixConsumerAuthParameter{
JwtAuth: &apisixv2.ApisixConsumerJwtAuth{
Value: &apisixv2.ApisixConsumerJwtAuthValue{
Key: "my-key",
Secret: "my-secret",
// Algorithm is explicitly empty string — should be treated as
// unset and not require asymmetric keys.
},
},
},
},
}
assert.NoError(t, v.Validate(t, ac))
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

conformance test report - apisix-standalone mode

apiVersion: gateway.networking.k8s.io/v1
date: "2026-05-07T20:36:47Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.3.0
implementation:
  contact: null
  organization: APISIX
  project: apisix-ingress-controller
  url: https://github.com/apache/apisix-ingress-controller.git
  version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
    result: success
    statistics:
      Failed: 0
      Passed: 12
      Skipped: 0
  name: GATEWAY-GRPC
  summary: Core tests succeeded.
- core:
    result: partial
    skippedTests:
    - HTTPRouteHTTPSListener
    statistics:
      Failed: 0
      Passed: 32
      Skipped: 1
  extended:
    result: partial
    skippedTests:
    - HTTPRouteRedirectPortAndScheme
    statistics:
      Failed: 0
      Passed: 11
      Skipped: 1
    supportedFeatures:
    - GatewayAddressEmpty
    - GatewayPort8080
    - HTTPRouteBackendProtocolWebSocket
    - HTTPRouteDestinationPortMatching
    - HTTPRouteHostRewrite
    - HTTPRouteMethodMatching
    - HTTPRoutePathRewrite
    - HTTPRoutePortRedirect
    - HTTPRouteQueryParamMatching
    - HTTPRouteRequestMirror
    - HTTPRouteResponseHeaderModification
    - HTTPRouteSchemeRedirect
    unsupportedFeatures:
    - GatewayHTTPListenerIsolation
    - GatewayInfrastructurePropagation
    - GatewayStaticAddresses
    - HTTPRouteBackendProtocolH2C
    - HTTPRouteBackendRequestHeaderModification
    - HTTPRouteBackendTimeout
    - HTTPRouteParentRefPort
    - HTTPRoutePathRedirect
    - HTTPRouteRequestMultipleMirrors
    - HTTPRouteRequestPercentageMirror
    - HTTPRouteRequestTimeout
  name: GATEWAY-HTTP
  summary: Core tests partially succeeded with 1 test skips. Extended tests partially
    succeeded with 1 test skips.
- core:
    result: partial
    skippedTests:
    - TLSRouteSimpleSameNamespace
    statistics:
      Failed: 0
      Passed: 10
      Skipped: 1
  name: GATEWAY-TLS
  summary: Core tests partially succeeded with 1 test skips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

conformance test report - apisix mode

apiVersion: gateway.networking.k8s.io/v1
date: "2026-05-07T20:37:53Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.3.0
implementation:
  contact: null
  organization: APISIX
  project: apisix-ingress-controller
  url: https://github.com/apache/apisix-ingress-controller.git
  version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
    result: partial
    skippedTests:
    - TLSRouteSimpleSameNamespace
    statistics:
      Failed: 0
      Passed: 10
      Skipped: 1
  name: GATEWAY-TLS
  summary: Core tests partially succeeded with 1 test skips.
- core:
    result: success
    statistics:
      Failed: 0
      Passed: 12
      Skipped: 0
  name: GATEWAY-GRPC
  summary: Core tests succeeded.
- core:
    failedTests:
    - HTTPRouteInvalidBackendRefUnknownKind
    result: failure
    skippedTests:
    - HTTPRouteHTTPSListener
    statistics:
      Failed: 1
      Passed: 31
      Skipped: 1
  extended:
    result: partial
    skippedTests:
    - HTTPRouteRedirectPortAndScheme
    statistics:
      Failed: 0
      Passed: 11
      Skipped: 1
    supportedFeatures:
    - GatewayAddressEmpty
    - GatewayPort8080
    - HTTPRouteBackendProtocolWebSocket
    - HTTPRouteDestinationPortMatching
    - HTTPRouteHostRewrite
    - HTTPRouteMethodMatching
    - HTTPRoutePathRewrite
    - HTTPRoutePortRedirect
    - HTTPRouteQueryParamMatching
    - HTTPRouteRequestMirror
    - HTTPRouteResponseHeaderModification
    - HTTPRouteSchemeRedirect
    unsupportedFeatures:
    - GatewayHTTPListenerIsolation
    - GatewayInfrastructurePropagation
    - GatewayStaticAddresses
    - HTTPRouteBackendProtocolH2C
    - HTTPRouteBackendRequestHeaderModification
    - HTTPRouteBackendTimeout
    - HTTPRouteParentRefPort
    - HTTPRoutePathRedirect
    - HTTPRouteRequestMultipleMirrors
    - HTTPRouteRequestPercentageMirror
    - HTTPRouteRequestTimeout
  name: GATEWAY-HTTP
  summary: Core tests failed with 1 test failures. Extended tests partially succeeded
    with 1 test skips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

conformance test report

apiVersion: gateway.networking.k8s.io/v1
date: "2026-05-07T20:58:39Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.3.0
implementation:
  contact: null
  organization: APISIX
  project: apisix-ingress-controller
  url: https://github.com/apache/apisix-ingress-controller.git
  version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
    failedTests:
    - GRPCExactMethodMatching
    - GRPCRouteHeaderMatching
    - GRPCRouteListenerHostnameMatching
    - GatewayModifyListeners
    result: failure
    statistics:
      Failed: 4
      Passed: 8
      Skipped: 0
  name: GATEWAY-GRPC
  summary: Core tests failed with 4 test failures.
- core:
    failedTests:
    - GatewayModifyListeners
    result: failure
    skippedTests:
    - HTTPRouteHTTPSListener
    statistics:
      Failed: 1
      Passed: 31
      Skipped: 1
  extended:
    failedTests:
    - HTTPRouteBackendProtocolWebSocket
    result: failure
    skippedTests:
    - HTTPRouteRedirectPortAndScheme
    statistics:
      Failed: 1
      Passed: 10
      Skipped: 1
    supportedFeatures:
    - GatewayAddressEmpty
    - GatewayPort8080
    - HTTPRouteBackendProtocolWebSocket
    - HTTPRouteDestinationPortMatching
    - HTTPRouteHostRewrite
    - HTTPRouteMethodMatching
    - HTTPRoutePathRewrite
    - HTTPRoutePortRedirect
    - HTTPRouteQueryParamMatching
    - HTTPRouteRequestMirror
    - HTTPRouteResponseHeaderModification
    - HTTPRouteSchemeRedirect
    unsupportedFeatures:
    - GatewayHTTPListenerIsolation
    - GatewayInfrastructurePropagation
    - GatewayStaticAddresses
    - HTTPRouteBackendProtocolH2C
    - HTTPRouteBackendRequestHeaderModification
    - HTTPRouteBackendTimeout
    - HTTPRouteParentRefPort
    - HTTPRoutePathRedirect
    - HTTPRouteRequestMultipleMirrors
    - HTTPRouteRequestPercentageMirror
    - HTTPRouteRequestTimeout
  name: GATEWAY-HTTP
  summary: Core tests failed with 1 test failures. Extended tests failed with 1 test
    failures.
- core:
    failedTests:
    - GatewayModifyListeners
    - TLSRouteSimpleSameNamespace
    result: failure
    statistics:
      Failed: 2
      Passed: 9
      Skipped: 0
  name: GATEWAY-TLS
  summary: Core tests failed with 2 test failures.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: ApisixConsumer jwtAuth requires private_key in schema

2 participants