Skip to content

feat(acl): allow config|get for auditability - #341

Open
tkarger wants to merge 1 commit into
valkey-io:mainfrom
tkarger:feat/allow-acl-config-get
Open

feat(acl): allow config|get for auditability#341
tkarger wants to merge 1 commit into
valkey-io:mainfrom
tkarger:feat/allow-acl-config-get

Conversation

@tkarger

@tkarger tkarger commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

This PR closes #335

Summary

The _operator user had only the permission to CONFIG SET, not to CONFIG GET. Due to this limitation the user wasn't able to retrieve the current configurations of the cluster and audit the state.

Features / Behaviour Changes

The _operator user can check the applied settings of the valkey instance and audit changes

Implementation

The permisson for config|get was added to the _operator user.

Testing

Tested by e2e tests

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message explains what changed and why
  • Tests are added or updated.
  • Documentation files are updated.
  • I have run pre-commit locally (pre-commit run --all-files or hooks on commit)

Signed-off-by: Tim Karger <tkarger@users.noreply.github.com>

Remove unrequired annotations

Signed-off-by: Tim Karger <49390121+tkarger@users.noreply.github.com>
@greptile-apps

greptile-apps Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR expands the _operator system user's ACL to permit read-only CONFIG GET commands for configuration verification and auditing.

Confidence Score: 5/5

The PR appears safe to merge with no actionable defects identified.

The change grants the existing privileged operator account read access to Valkey configuration while preserving the command-scoped ACL model and existing credential boundary.

T-Rex T-Rex Logs

What T-Rex did

  • Validated the general-contract proof by showing the before ACL for the _operator lacked +config|get, the after ACL includes both +config|set and +config|get, and the focused test passed.
  • Identified that the retained harness is the exact narrow controller-generation test used for both captures.
  • Cataloged the uploaded artifacts (four logs and a Go test harness) as evidence supporting the contract validation.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
internal/controller/users.go Adds the narrowly scoped `+config

Reviews (1): Last reviewed commit: "feat(acl): allow config|get for auditabi..." | Re-trigger Greptile

@jdheyburn

Copy link
Copy Markdown
Collaborator

Thanks for raising - I left a comment on the issue on what underlying bug we're trying to solve.

@melancholictheory

Copy link
Copy Markdown
Contributor

this one's independently correct regardless of where the broader #335 discussion lands: read-only, no new write surface, and it closes the write-without-read audit gap. it's also useful to the operator itself, the live-config apply path can read a key back after CONFIG SET to confirm it took, which the current -config|get blocks. worth landing on its own.

@jdheyburn jdheyburn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you!

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]: ACL misconfiguration: _operator user can CONFIG SET all parameters

3 participants