Skip to content

BE: Prevent the masking of externally-provided references in Kafka Connect config#1886

Open
nkachami wants to merge 2 commits into
kafbat:mainfrom
nkachami:feat/config-secret-provider-reference
Open

BE: Prevent the masking of externally-provided references in Kafka Connect config#1886
nkachami wants to merge 2 commits into
kafbat:mainfrom
nkachami:feat/config-secret-provider-reference

Conversation

@nkachami

@nkachami nkachami commented Jun 19, 2026

Copy link
Copy Markdown

Fixes #1887

  • Breaking change? (if so, please describe the impact and migration path for existing application instances)

What changes did you make? (Give an overview)

Made changes to KafkaConfigSanitizer.java and KafkaConfigSanitizerTest.java to review credential based keys' values to confirm they are not a externalized-secret reference.

Is there anything you'd like reviewers to focus on?
Regex Pattern not overstepping into general password domains.

How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)

  • No need to
  • Manually (please, describe, if necessary)
  • Unit checks
  • Integration checks
  • Covered by existing automation

Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. ENVIRONMENT VARIABLES) (EDIT: Not necessary but if requested I can add)
  • My changes generate no new warnings (e.g. Sonar is happy)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged

Check out Contributing and Code of Conduct

A picture of a cute animal (not mandatory but encouraged)

8E2E6E61-22BE-41DA-8D3F-9227D2B20B0C

Summary by CodeRabbit

Summary by CodeRabbit

  • Bug Fixes
    • Fixed sanitization to avoid masking valid external config-provider references (e.g., ${file:...}, ${vault:...}, ${env:...}), while continuing to obfuscate other secret-like values.
  • Tests
    • Added coverage to ensure config-provider references are preserved and adjusted existing assertions for expected sanitization behavior.

Screenshots Proving Functionality:
Config pre submit:

Image

Route to task screen for a refresh after submitting (could be any screen):

Image

Come back to config:

Image

Normal passwords that do not match the exact regex pattern of ${provider:path/to/secret:secret-key} still looks like BAU functionality:

Screenshot 2026-06-24 at 8 48 28 AM

@nkachami nkachami requested a review from a team as a code owner June 19, 2026 18:12
@kapybro kapybro Bot added status/triage/manual Manual triage in progress and removed status/triage/manual Manual triage in progress labels Jun 19, 2026
@kapybro

kapybro Bot commented Jun 19, 2026

Copy link
Copy Markdown

AI Summary

The issue addresses a problem where Kafka Connect config-provider references in connector configurations were being masked, potentially hiding sensitive information. The proposed solution modifies KafkaConfigSanitizer.java to ensure credential-based keys are not mistakenly identified as externalized-secret references. The reviewer should focus on ensuring the regex pattern does not incorrectly flag general password domains. Unit tests have been implemented to verify the fix.

@kapybro kapybro Bot changed the title Don't mask Kafka Connect config-provider references in connector config Prevent masking of Kafka Connect config-provider references in connector config Jun 19, 2026
@kapybro kapybro Bot added area/connect Kafka Connect, its connectors type/enhancement En enhancement/improvement to an already existing feature labels Jun 19, 2026
@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

KafkaConfigSanitizer gains a CONFIG_PROVIDER_REFERENCE regex pattern and an isConfigProviderReference helper to detect ${provider[:path:]key}-style indirections. The sanitization branch for sensitive keys now returns the original value unchanged when it matches this pattern instead of replacing it with SANITIZED_VALUE. Tests are added to cover known (file/vault/env) and unknown reference formats.

Changes

Config-provider reference passthrough in KafkaConfigSanitizer

Layer / File(s) Summary
Pattern, sanitization logic, and helper method
api/src/main/java/io/kafbat/ui/service/KafkaConfigSanitizer.java
Adds CONFIG_PROVIDER_REFERENCE static Pattern, updates the sensitive-key sanitization branch to return the original value when isConfigProviderReference matches, and adds the private helper method with Javadoc.
Tests for config-provider reference handling
api/src/test/java/io/kafbat/ui/service/KafkaConfigSanitizerTest.java
Adds an assertion to the existing credentials test for a non-credential value, and adds doNotObfuscateConfigProviderReferences test verifying file/vault/env references are preserved while plain secrets and unrecognized placeholders are still masked.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

Poem

🐇 A secret that points to a vault far away,
Should hop through the sanitizer without delay.
${file:passwords/key} — don't mask what's a ref!
The rabbit checked patterns and drew a new clef.
Credentials stay guarded, but pointers stay free~

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: preventing masking of externally-provided Kafka Connect config references, which is the core feature implemented in KafkaConfigSanitizer.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi nkachami! 👋

Welcome, and thank you for opening your first PR in the repo!

Please wait for triaging by our maintainers.

Please take a look at our contributing guide.

@nkachami nkachami changed the title Prevent masking of Kafka Connect config-provider references in connector config BE: Prevent masking of Kafka Connect config-provider references in connector config Jun 19, 2026
@nkachami nkachami changed the title BE: Prevent masking of Kafka Connect config-provider references in connector config BE: Prevent the masking of externally-provided references in Kafka Connect config Jun 19, 2026
@yeikel

yeikel commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

@Haarolean Can you please review this and share your feedback if any?

@nkachami works with me and we tested this, and have high confidence that it will work as expected without breaking anything

We are open to make changes as needed baced on feedback and/or share any additional evidence you may need. if any issue comes from this, we'll push a follow up with high priority, just tag us (we'll also watch the repo) 🙏

Thanks as always for your time and support :)

@Haarolean Haarolean added the scope/backend Related to backend changes label Jun 24, 2026
@Haarolean Haarolean added this to the 1.6 milestone Jun 24, 2026
@github-project-automation github-project-automation Bot moved this from Todo to PR Approved in Release 1.6 Jun 24, 2026
@Haarolean Haarolean enabled auto-merge (squash) June 24, 2026 16:32
@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...ava/io/kafbat/ui/service/KafkaConfigSanitizer.java 83.33% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@yeikel

yeikel commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

@Haarolean This failure seems unrelated to the change at hand. Could you please re-try the workflow and/or merge this PR when ready? Thanks as usual!

   ✔ Given KSQL DB long query stream starts with: "STREAM_ONE" stared # src/steps/Ksqldb.steps.ts:144
   ✖ Given KSQL DB long query stoped # src/steps/Ksqldb.steps.ts:161
       locator.click: Element is outside of the viewport
       Call log:
         - waiting for getByText('Abort')
           - locator resolved to <div class="sc-jJBkAM bBMmJX">Abort</div>
         - attempting click action
           - scrolling into view if needed
           - done scrolling

           at PlaywrightWorld.<anonymous> (/home/runner/work/kafka-ui/kafka-ui/e2e-playwright/src/steps/Ksqldb.steps.ts:165:36)

As an aside I was just telling @nkachami that you cannot merge until he adds the cute animal picture :)

@nkachami

Copy link
Copy Markdown
Author

Rolo has been added 🐱

@yeikel

yeikel commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

We need to add a Mandatory check for the animal picture

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

Labels

area/connect Kafka Connect, its connectors scope/backend Related to backend changes type/enhancement En enhancement/improvement to an already existing feature

Projects

Status: PR Approved

Development

Successfully merging this pull request may close these issues.

Prevent the masking of externally-provided references in Kafka Connect config

3 participants