feat:acl with roles: admin, reader, writer#4
Open
ArchiMoebius wants to merge 7 commits intoLulzx:mainfrom
Open
feat:acl with roles: admin, reader, writer#4ArchiMoebius wants to merge 7 commits intoLulzx:mainfrom
ArchiMoebius wants to merge 7 commits intoLulzx:mainfrom
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a role-based ACL system: new Changes
Sequence DiagramsequenceDiagram
actor Client
participant Handler as "Request Handler"
participant SigV4 as "SigV4"
participant S3Ctx as "S3Context (ACL Map)"
participant ACLCtx as "ACLCtx"
Client->>Handler: HTTP Request (GET/PUT/POST/...)
Handler->>SigV4: verify(&S3Context, Request, allocator)
SigV4->>S3Ctx: lookup credential by access_key
S3Ctx-->>SigV4: Credential { role, access_key, secret_key }
SigV4->>ACLCtx: create ACLCtx(allow, role)
SigV4-->>Handler: ACLCtx
Handler->>ACLCtx: allowed(http_method)
ACLCtx-->>Handler: permission result
alt granted
Handler->>Client: process request / 200
else denied
Handler->>Client: 403 Forbidden
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.zig (1)
1625-1667:⚠️ Potential issue | 🟠 MajorACL denial returns
405 MethodNotAllowedinstead of403 Forbidden.When an authenticated user's role disallows a method (e.g., a Reader doing PUT), the combined condition
std.mem.eql(u8, req.method, "PUT") and acl_ctx.allowed("PUT")is false. The request falls through allelse ifbranches and hits the finalelseat line 1665, returning 405.HTTP 405 means the server never supports this method on this resource. HTTP 403 means the user lacks permission. S3 clients (aws-cli, boto3) distinguish between these — a 405 may trigger different retry/error-handling logic than a 403.
Separate method matching from ACL checking so that a recognized-but-denied method yields 403.
🐛 Proposed restructure (standalone block shown; apply same pattern to distributed block)
- if (std.mem.eql(u8, req.method, "GET") and acl_ctx.allowed("GET")) { + if (std.mem.eql(u8, req.method, "GET")) { + if (!acl_ctx.allowed("GET")) { + sendError(res, 403, "AccessDenied", "Insufficient permissions"); + return; + } if (bucket.len == 0) { try handleListBuckets(ctx, allocator, res); } else if (key.len == 0) { try handleListObjects(ctx, allocator, req, res, bucket); } else { try handleGetObject(ctx, allocator, req, res, bucket, key); } - } else if (std.mem.eql(u8, req.method, "PUT") and acl_ctx.allowed("PUT")) { + } else if (std.mem.eql(u8, req.method, "PUT")) { + if (!acl_ctx.allowed("PUT")) { + sendError(res, 403, "AccessDenied", "Insufficient permissions"); + return; + } // ... handle PUT🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.zig` around lines 1625 - 1667, The current chained checks (e.g., std.mem.eql(u8, req.method, "PUT") and acl_ctx.allowed("PUT")) cause denied-but-supported methods to fall through to the final sendError(..., 405). Refactor the dispatch so you first match the HTTP method (check std.mem.eql on req.method for "GET"/"PUT"/"DELETE"/"HEAD"/"POST") and inside each method-branch call acl_ctx.allowed(...) — if allowed invoke the existing handlers (handleGetObject/handleListObjects, handlePutObject/handleCreateBucket/handleUploadPart, handleDeleteObject/handleDeleteBucket/handleAbortMultipart, handleHeadObject/handleHeadBucket, handleInitiateMultipart/handleCompleteMultipart/handleDeleteObjects) and if not allowed call sendError(res, 403, "AccessDenied", "...") (or similar 403 response); keep the final sendError(..., 405) only for truly unsupported methods.
🧹 Nitpick comments (4)
main.zig (2)
983-984:access_control_mapis never deinitialized.
S3Context.deinit()is defined (line 1227) but never called. While the server loop runs indefinitely so this is a benign leak on exit, addingdefer ctx.deinit();after construction would be consistent with the existingdefer _ = gpa.deinit();pattern and would matter if the server ever gains a graceful shutdown path.♻️ Add defer
Add after line 1076:
defer ctx.deinit();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.zig` around lines 983 - 984, S3Context (ctx) is constructed but never deinitialized — call its deinit method via a defer to match the existing pattern (e.g., similar to "defer _ = gpa.deinit();"); add a "defer ctx.deinit();" immediately after ctx is created so S3Context.deinit() runs on scope exit and frees access_control_map and related resources.
1722-1727:ACLCtx.roleinitialized toundefined— fragile default.When auth fails early (missing headers, unknown access key, etc.),
roleremainsundefined. Currently safe becauseroute()checks.allowfirst, but any future code path reading.rolewithout that guard would trigger undefined behavior (panic in safe mode, UB in unsafe mode).Initialize to the most restrictive role as a defensive default:
🛡️ Proposed fix
fn verify(ctx: *const S3Context, req: *const Request, allocator: Allocator) ACLCtx { var acl_ctx = ACLCtx{ .allow = false, - .role = undefined, + .role = .Reader, // safe default; irrelevant when allow=false };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.zig` around lines 1722 - 1727, The ACLCtx.role field is currently initialized to undefined in verify(), which is fragile if code later reads role without first checking .allow; change the initialization in the verify() function to set ACLCtx.role to the most restrictive enum/value (e.g., Role.None/Role.Unauthenticated or whatever the codebase uses for "no permissions") instead of undefined so that ACLCtx always has a safe default; ensure this new default is compatible with existing route() logic that checks .allow first and preserves existing behavior when authentication fails.README.md (1)
51-51: Document multi-credential format and role permissions.The quick start shows a single credential, but there's no mention of how to pass multiple credentials (comma-separated) or what roles are available (
admin,reader,writer) and their permissions. Users won't know, for example, that areadercan onlyGET/HEAD/OPTIONS, or that awritercannotGETorDELETE.Consider adding a brief section like:
-Dacl-list="admin:key1:secret1,reader:key2:secret2,writer:key3:secret3"with a table describing each role's allowed HTTP methods.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 51, Update the README to document the -Dacl-list multi-credential format and role permissions: explain that multiple credentials are comma-separated entries in the form role:key:secret and show an example like -Dacl-list="admin:key1:secret1,reader:key2:secret2,writer:key3:secret3"; add a short table or bullet list mapping roles (admin, writer, reader) to allowed HTTP methods (admin: GET/HEAD/OPTIONS/PUT/POST/DELETE, writer: PUT/POST (and other write actions) but not GET/DELETE, reader: GET/HEAD/OPTIONS only) so users know how to construct ACLs and what each role permits.build.zig (1)
9-27: Build-time round-trip is unnecessarily verbose; simplify to validate-and-pass-through.Lines 10–27 parse the ACL string into
Credentialstructs, convert them back to strings, then join them — producing the same string as the input. The only useful effect is build-time validation (which is good). But you can achieve that with just theparseCredentialscall and then pass through the original string.♻️ Simplified build-time validation
const acl_list = b.option([]const u8, "acl-list", "Admin credentials") orelse "admin:minioadmin:minioadmin"; - const all_credentials = try acl.parseCredentials(b.allocator, acl_list); - - var credential_list = try std.ArrayList([]const u8).initCapacity(b.allocator, 20); - // No defer deinit here if we use toOwnedSlice later - - for (all_credentials) |cred| { - const role_name = switch (cred.role) { - .Admin => "admin", - .Reader => "reader", - .Writer => "writer", - }; - - const entry_str = try std.fmt.allocPrint(b.allocator, "{s}:{s}:{s}", .{ role_name, cred.access_key, cred.secret_key }); - try credential_list.append(b.allocator, entry_str); - } - - const cs = try credential_list.toOwnedSlice(b.allocator); - const joined_acl_list = try std.mem.join(b.allocator, ",", cs); + // Validate at build time; errors surface as build failures + _ = try acl.parseCredentials(b.allocator, acl_list); const options = b.addOptions(); - options.addOption([]const u8, "acl_list", joined_acl_list); + options.addOption([]const u8, "acl_list", acl_list);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.zig` around lines 9 - 27, The code currently parses acl_list into all_credentials then rebuilds the same string via credential_list and joined_acl_list; instead, keep the validation by calling acl.parseCredentials(b.allocator, acl_list) to validate, then reuse the original acl_list as the canonical string; remove the temporary ArrayList (credential_list), the loop that builds entry_str, and the std.mem.join step; ensure you still deinit any allocator-owned structures returned by parseCredentials (all_credentials) if required by acl.parseCredentials' contract and then assign joined_acl_list = acl_list (or pass acl_list through) where used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@acl.zig`:
- Around line 29-41: The parser uses std.mem.tokenizeSequence which skips empty
segments and can cause a panic in parseCredential when unwrapping itr.next().?;
replace the tokenizer usage with std.mem.splitScalar to preserve empty segments,
then explicitly check that the resulting three fields (roleStr, access_key,
secret_key) are present and non-empty (validate roleStr with stringToRole and
return error.BadCredentialFormat or a new error on empty access/secret), and
construct Credential only after these validations; update references in
parseCredential and keep stringToRole and Credential unchanged.
- Around line 44-65: The parseCredentials function contains dead/redundant
tokenizer logic: remove the unreachable if (itr.peek() == null) branch and its
call to parseCredential(input), and instead rely only on the iterator loop to
parse tokens; specifically, in parseCredentials use the existing itr while loop
to call parseCredential(record) for each token and append to the credentials
ArrayList, then return credentials.toOwnedSlice(allocator), ensuring itr,
parseCredential, and the credentials ArrayList are the only pieces used for
token parsing.
In `@main.zig`:
- Around line 1683-1720: The inline comment above ACLCtx.allowed is stale—remove
the "Change return type to !bool to allow returning errors" remark—and then
address Writer semantics: if Writer should be able to read, update the
ACLCtx.allowed switch for acl.Role.Writer (in the Writer arm of ACLCtx.allowed)
to also allow .GET; otherwise add a clarifying comment/docstring near ACLCtx or
acl.Role.Writer stating that Writer is intentionally write-only/ingestion-only
and does not permit GET/DELETE.
- Around line 1053-1059: The current loop inserts credentials into
access_control_map using StringHashMap.put which silently overwrites duplicates;
update the loop that iterates access_control_list to detect duplicate
credential.access_key before insertion (use access_control_map.get or
contains-like lookup) and handle duplicates explicitly—either return an
error/validation failure to reject the ACL (preferred) or log and skip the later
entry—so modify the code around access_control_map.put(credential.access_key,
credential) to check for an existing entry and take the chosen action
(referencing access_control_map, access_control_list, credential, and
credential.access_key).
- Around line 1604-1621: The distributed routing block currently combines method
checks and acl_ctx.allowed in single conditions (e.g., std.mem.eql(u8,
req.method, "PUT") and acl_ctx.allowed("PUT")) which lets denied methods fall
through to the standalone 405 path; separate the method match from the ACL check
for each distributed handler (handleDistributedPut, handleDistributedGet,
handleDistributedDelete, handleDistributedHead, and the distributed LIST branch
calling handleDistributedList): first test the method (std.mem.eql on req.method
or bucket.len conditions), then if the method matches call acl_ctx.allowed(...)
and if allowed invoke the corresponding handler and return, otherwise
immediately respond with the same 403-forbidden behavior used in the standalone
routing code (i.e., return forbidden response via the same helper used
elsewhere) so denied distributed requests do not fall through to the 405 path.
---
Outside diff comments:
In `@main.zig`:
- Around line 1625-1667: The current chained checks (e.g., std.mem.eql(u8,
req.method, "PUT") and acl_ctx.allowed("PUT")) cause denied-but-supported
methods to fall through to the final sendError(..., 405). Refactor the dispatch
so you first match the HTTP method (check std.mem.eql on req.method for
"GET"/"PUT"/"DELETE"/"HEAD"/"POST") and inside each method-branch call
acl_ctx.allowed(...) — if allowed invoke the existing handlers
(handleGetObject/handleListObjects,
handlePutObject/handleCreateBucket/handleUploadPart,
handleDeleteObject/handleDeleteBucket/handleAbortMultipart,
handleHeadObject/handleHeadBucket,
handleInitiateMultipart/handleCompleteMultipart/handleDeleteObjects) and if not
allowed call sendError(res, 403, "AccessDenied", "...") (or similar 403
response); keep the final sendError(..., 405) only for truly unsupported
methods.
---
Nitpick comments:
In `@build.zig`:
- Around line 9-27: The code currently parses acl_list into all_credentials then
rebuilds the same string via credential_list and joined_acl_list; instead, keep
the validation by calling acl.parseCredentials(b.allocator, acl_list) to
validate, then reuse the original acl_list as the canonical string; remove the
temporary ArrayList (credential_list), the loop that builds entry_str, and the
std.mem.join step; ensure you still deinit any allocator-owned structures
returned by parseCredentials (all_credentials) if required by
acl.parseCredentials' contract and then assign joined_acl_list = acl_list (or
pass acl_list through) where used.
In `@main.zig`:
- Around line 983-984: S3Context (ctx) is constructed but never deinitialized —
call its deinit method via a defer to match the existing pattern (e.g., similar
to "defer _ = gpa.deinit();"); add a "defer ctx.deinit();" immediately after ctx
is created so S3Context.deinit() runs on scope exit and frees access_control_map
and related resources.
- Around line 1722-1727: The ACLCtx.role field is currently initialized to
undefined in verify(), which is fragile if code later reads role without first
checking .allow; change the initialization in the verify() function to set
ACLCtx.role to the most restrictive enum/value (e.g.,
Role.None/Role.Unauthenticated or whatever the codebase uses for "no
permissions") instead of undefined so that ACLCtx always has a safe default;
ensure this new default is compatible with existing route() logic that checks
.allow first and preserves existing behavior when authentication fails.
In `@README.md`:
- Line 51: Update the README to document the -Dacl-list multi-credential format
and role permissions: explain that multiple credentials are comma-separated
entries in the form role:key:secret and show an example like
-Dacl-list="admin:key1:secret1,reader:key2:secret2,writer:key3:secret3"; add a
short table or bullet list mapping roles (admin, writer, reader) to allowed HTTP
methods (admin: GET/HEAD/OPTIONS/PUT/POST/DELETE, writer: PUT/POST (and other
write actions) but not GET/DELETE, reader: GET/HEAD/OPTIONS only) so users know
how to construct ACLs and what each role permits.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
main.zig (2)
1819-1821: Role is set even when signature verification fails.Line 1820 assigns
credential.?.roleregardless of whether the signature matched (line 1819). Currently safe becauseroute()checksacl_ctx.allowfirst, but setting the role only on successful auth would be more defensive against future refactors.🛡️ Suggested improvement
- acl_ctx.allow = std.mem.eql(u8, calculated_sig, parsed.signature); - acl_ctx.role = credential.?.role; + if (std.mem.eql(u8, calculated_sig, parsed.signature)) { + acl_ctx.allow = true; + acl_ctx.role = credential.?.role; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.zig` around lines 1819 - 1821, The role is being assigned unconditionally; change the logic so acl_ctx.role is set only when the signature check succeeds: after computing acl_ctx.allow = std.mem.eql(u8, calculated_sig, parsed.signature), if acl_ctx.allow is true then assign acl_ctx.role = credential.?.role, otherwise leave acl_ctx.role unset/at its default. Update the block that currently sets acl_ctx.allow and acl_ctx.role together (references: acl_ctx.allow, acl_ctx.role, calculated_sig, parsed.signature, credential.?) so the role assignment is guarded by the equality result.
983-984: Startup will abort without a user-friendly message on malformed ACL input.If
build_options.acl_listis malformed, thetrypropagates the error tomain's return, producing a generic error trace. Consider catching the error and printing a human-readable message (e.g., expected format) before exiting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.zig` around lines 983 - 984, The startup currently uses try on acl.parseCredentials with build_options.acl_list which will propagate a Zig error back to main and produce a generic trace on malformed ACL input; update the code around acl.parseCredentials so you explicitly catch errors (e.g., using switch or if (err != null)) when calling acl.parseCredentials(allocator, build_options.acl_list), print a clear, user-friendly message describing the expected ACL format (including the offending input), free allocator resources via defer allocator.free(access_control_list) only when parse succeeds, and then exit with a non-zero status (or return an appropriate error code) instead of letting the generic trace surface; reference acl.parseCredentials, build_options.acl_list, allocator.free and main when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.zig`:
- Around line 1676-1681: The DELETE branch currently combines the method match
and acl_ctx.allowed("DELETE") in the same condition which makes the inner if
(!acl_ctx.allowed("DELETE")) dead and causes denied DELETE requests to fall
through to a 405; change the DELETE branch to first check method equality
(std.mem.eql(u8, req.method, "DELETE")) and then inside that branch call
acl_ctx.allowed("DELETE") and call sendError(res, 403, "AccessDenied",
"Insufficient permissions") + return when denied, just like the other methods
(follow the GET/PUT/HEAD/POST pattern), removing the redundant inner dead check
so denied DELETE yields 403 via sendError.
---
Duplicate comments:
In `@main.zig`:
- Around line 1733-1772: The ACLCtx.allowed method currently grants
acl.Role.Writer only PUT/POST/HEAD/OPTIONS and denies GET/DELETE, which prevents
writers from reading objects; decide intent and update accordingly: if writers
should be able to read, add "GET" (and optionally "DELETE"/other read verbs) to
the Writer branch in ACLCtx.allowed (the match on acl.Role.Writer inside fn
allowed); if the write-only behavior is intentional, add a concise comment above
the acl.Role.Writer branch explaining that writers are intentionally
write-only/ingestion-only so reviewers know this is deliberate.
- Around line 1053-1059: The loop that inserts credentials into
access_control_map currently lets std.StringHashMap.put overwrite earlier
entries when two credentials share the same access_key; modify the insertion
logic in the block iterating access_control_list so you first check for an
existing entry using access_control_map.get(credential.access_key) (or contains
if available) and, if found, reject the duplicate (return an error, log and
fail, or otherwise handle per project policy) instead of calling
access_control_map.put; keep using the existing symbols access_control_map,
access_control_list, credential, and access_key to locate and update the code.
---
Nitpick comments:
In `@main.zig`:
- Around line 1819-1821: The role is being assigned unconditionally; change the
logic so acl_ctx.role is set only when the signature check succeeds: after
computing acl_ctx.allow = std.mem.eql(u8, calculated_sig, parsed.signature), if
acl_ctx.allow is true then assign acl_ctx.role = credential.?.role, otherwise
leave acl_ctx.role unset/at its default. Update the block that currently sets
acl_ctx.allow and acl_ctx.role together (references: acl_ctx.allow,
acl_ctx.role, calculated_sig, parsed.signature, credential.?) so the role
assignment is guarded by the equality result.
- Around line 983-984: The startup currently uses try on acl.parseCredentials
with build_options.acl_list which will propagate a Zig error back to main and
produce a generic trace on malformed ACL input; update the code around
acl.parseCredentials so you explicitly catch errors (e.g., using switch or if
(err != null)) when calling acl.parseCredentials(allocator,
build_options.acl_list), print a clear, user-friendly message describing the
expected ACL format (including the offending input), free allocator resources
via defer allocator.free(access_control_list) only when parse succeeds, and then
exit with a non-zero status (or return an appropriate error code) instead of
letting the generic trace surface; reference acl.parseCredentials,
build_options.acl_list, allocator.free and main when applying the change.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.zig (1)
1715-1717:⚠️ Potential issue | 🟡 Minor
OPTIONSis never routed — its ACL permission is unreachable dead code.
ReaderandWriterroles grantOPTIONSviaallowed(), but the routing has noOPTIONSbranch; all OPTIONS requests fall through to the405else-arm without ever callingacl_ctx.allowed("OPTIONS"). This also prevents CORS preflight responses.Either add an
OPTIONShandler (even a minimalAllow-header response), or removeOPTIONSfrom the role permission sets to avoid the misleading grant.🔧 Minimal routing fix
+ } else if (std.mem.eql(u8, req.method, "OPTIONS")) { + if (!acl_ctx.allowed("OPTIONS")) { + sendError(res, 403, "AccessDenied", "Insufficient permissions"); + return; + } + res.status = 200; + res.status_text = "OK"; + res.setHeader("Allow", "GET, PUT, DELETE, HEAD, POST, OPTIONS"); } else { sendError(res, 405, "MethodNotAllowed", "Method not allowed"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.zig` around lines 1715 - 1717, The routing currently falls through to sendError(..., 405, ...) for OPTIONS so acl_ctx.allowed("OPTIONS") is never exercised; fix by adding an explicit OPTIONS branch before the final else that checks acl_ctx.allowed("OPTIONS") (same pattern used for other verbs) and returns a minimal preflight/CORS response (e.g., 200 with an Allow/Access-Control-Allow-* headers) or, alternatively, remove "OPTIONS" from the Reader/Writer role permission sets where roles are defined so the grant is not misleading; locate the routing switch/if handling HTTP methods and the role permission definitions (references: acl_ctx.allowed, sendError, Reader/Writer role declarations) and apply one of these two changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.zig`:
- Around line 1819-1821: The role field is being set even when signature
verification fails; after computing acl_ctx.allow (comparing calculated_sig and
parsed.signature) ensure acl_ctx.role is set to credential.?role only when
acl_ctx.allow is true, and otherwise set acl_ctx.role to Role.Unknown (or the
equivalent unknown role enum); update the assignment near acl_ctx.allow and
acl_ctx.role so that role reflects the allow result and not the credential
unconditionally, preserving existing behavior of allowed().
- Around line 1809-1816: The calculateSignature function currently writes
"AWS4"+secret_key into a fixed [256]u8 (k_secret_buf) which overflows for
secret_key > 252 bytes; fix by either (preferred) validating secret_key length
when credentials are parsed/inserted (reject or truncate keys >252 bytes) so
that credential.?.secret_key can never exceed the buffer, or (alternative)
change calculateSignature to allocate the key buffer on the heap (e.g., use
allocator.alloc/ArrayList) and safely build "AWS4"+secret_key there; ensure all
callers of calculateSignature and the credential insertion/parsing path enforce
or handle the new validation/heap allocation to prevent stack overflow.
---
Outside diff comments:
In `@main.zig`:
- Around line 1715-1717: The routing currently falls through to sendError(...,
405, ...) for OPTIONS so acl_ctx.allowed("OPTIONS") is never exercised; fix by
adding an explicit OPTIONS branch before the final else that checks
acl_ctx.allowed("OPTIONS") (same pattern used for other verbs) and returns a
minimal preflight/CORS response (e.g., 200 with an Allow/Access-Control-Allow-*
headers) or, alternatively, remove "OPTIONS" from the Reader/Writer role
permission sets where roles are defined so the grant is not misleading; locate
the routing switch/if handling HTTP methods and the role permission definitions
(references: acl_ctx.allowed, sendError, Reader/Writer role declarations) and
apply one of these two changes.
---
Duplicate comments:
In `@main.zig`:
- Around line 1754-1766: The acl.Role.Writer branch currently maps the string
method (via std.meta.stringToEnum(HTTP_METHOD, method)) and allows .PUT, .POST,
.HEAD, .OPTIONS but not .GET or .DELETE; decide whether Writer should be
read-capable or intentionally write-only and update accordingly: if Writers
should be able to retrieve objects, add .GET to the switch in the
acl.Role.Writer arm (keep .DELETE out if delete must remain restricted); if this
is intended to be write-only, add a clear doc comment above the acl.Role.Writer
arm stating “Writer is write-only/ingestion-only, does not permit GET/DELETE” so
future readers understand the design decision. Ensure changes reference the
existing code that parses method (std.meta.stringToEnum(HTTP_METHOD, method))
and the switch on http_method within the acl.Role.Writer arm.
- Around line 1053-1059: The loop currently calls access_control_map.put which
overwrites earlier entries with the same credential.access_key; change it to
detect duplicates before inserting and fail or handle them explicitly: for each
credential in access_control_list, check access_control_map (e.g., with
access_control_map.get or a contains-like check) for credential.access_key and
if found either return an error/log and skip the insert or merge/resolve the
conflict, otherwise call put; reference access_control_map, access_control_list,
credential.access_key, and std.StringHashMap.put to locate the code to update.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.zig (1)
1659-1726:⚠️ Potential issue | 🟡 Minor
OPTIONSis allowed by the Reader/Writer ACL but the routing always returns 405 for it.
ACLCtx.allowed("OPTIONS")returnstruefor bothReaderandWriter, but theroute()function has noOPTIONSbranch — it falls through to the finalelse(line 1725) and responds with 405 for every authenticated user regardless of role. The granted permission is never exercised, which will silently break CORS preflight flows if clients sendOPTIONSrequests.Either add an
OPTIONShandler (even a permissive CORS response) or removeOPTIONSfrom the Reader/Writerallowedarms to keep the ACL definitions consistent with what the server actually supports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.zig` around lines 1659 - 1726, The routing in route() checks req.method but has no branch for "OPTIONS" so ACLCtx.allowed("OPTIONS") never gets used and preflight requests get 405; fix by adding an else-if branch that matches std.mem.eql(u8, req.method, "OPTIONS") before the final else and either (a) return a minimal permissive CORS response (e.g., set Allow/Access-Control-Allow-* headers and a 204/200) or (b) if you prefer to disallow OPTIONS, remove "OPTIONS" from the Reader/Writer ACL allowed list; refer to the route() method, req.method checks, and ACLCtx.allowed("OPTIONS") when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.zig`:
- Around line 1061-1064: The log message has a grammar mistake: change "a access
key" to "an access key" in the error string that reports when
credential.access_key is too long (the block checking credential.access_key.len
> 252); update the message passed to std.log.err to read "ACL credential '{s}'
has an access key exceeding 252 bytes; skipping" so the variable
credential.access_key remains used for formatting.
- Around line 1056-1058: The log in main.zig prints credential.secret_key
verbatim; remove the secret from logs by changing the std.log.err call that
formats credential.secret_key to a non-sensitive message (e.g., mention the
credential id/type or that the secret exceeds 252 bytes and was skipped) and
optionally include non-sensitive metadata like credential identifier or secret
length, but never the secret value itself; update the code around the check that
uses credential.secret_key and keep the continue behavior intact.
- Around line 1053-1067: The access_control_map created with
std.StringHashMap(acl.Credential).init(allocator) can leak if a subsequent try
fails; add an immediate errdefer access_control_map.deinit() right after
initializing access_control_map so its backing store is freed on error (before
any try access_control_map.put(...) or try address.listen(...)); then, when you
attach the map to ctx and register ctx.deinit() later, cancel or avoid
double-deinit (e.g., use defer to cancel the errdefer or transfer ownership so
only ctx.deinit() calls access_control_map.deinit()).
---
Outside diff comments:
In `@main.zig`:
- Around line 1659-1726: The routing in route() checks req.method but has no
branch for "OPTIONS" so ACLCtx.allowed("OPTIONS") never gets used and preflight
requests get 405; fix by adding an else-if branch that matches std.mem.eql(u8,
req.method, "OPTIONS") before the final else and either (a) return a minimal
permissive CORS response (e.g., set Allow/Access-Control-Allow-* headers and a
204/200) or (b) if you prefer to disallow OPTIONS, remove "OPTIONS" from the
Reader/Writer ACL allowed list; refer to the route() method, req.method checks,
and ACLCtx.allowed("OPTIONS") when making the change.
---
Duplicate comments:
In `@main.zig`:
- Around line 1763-1774: The acl.Role.Writer branch currently maps method via
std.meta.stringToEnum(HTTP_METHOD, method) and permits .PUT, .POST, .HEAD,
.OPTIONS but excludes .GET and .DELETE; decide whether Writer should be
read-capable—if not, add an inline comment above the acl.Role.Writer case
explaining the intentional append-only/ingest-only semantics; otherwise add .GET
=> return true to the switch (and optionally .DELETE if needed) so the Writer
role can perform reads; keep the existing enum conversion via
std.meta.stringToEnum(HTTP_METHOD, method) and update only the switch arms in
the acl.Role.Writer block.
- Around line 1055-1067: The code currently inserts credentials with
access_control_map.put(credential.access_key, credential) which silently
overwrites prior entries; modify the insertion logic in the access_control_list
loop to first check for an existing entry for credential.access_key (e.g., via
access_control_map.get/contains or equivalent) and handle duplicates
explicitly—either log an error and continue (skipping the new duplicate) or
return a parse error—so duplicate access_key values in access_control_list do
not silently replace earlier credentials; update any tests or callers of
access_control_map accordingly.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
main.zig (1)
1765-1793:OPTIONSin the ACL is dead code — the router has noOPTIONShandler.
ReaderandWriterboth listOPTIONSas an allowed method (lines 1778, 1791), but the router's else-branch returns 405 for any unrecognized method, including OPTIONS.acl_ctx.allowed("OPTIONS")is never evaluated. Either add an OPTIONS handler (e.g., for CORS preflight), or removeOPTIONSfrom both role switch arms to avoid misleading ACL policy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.zig` around lines 1765 - 1793, The ACL allows "OPTIONS" but the router never dispatches OPTIONS so ACLCtx.allowed method's OPTIONS branches are dead; update the allowed(self: *const ACLCtx, method: []const u8) implementation to remove .OPTIONS from the acl.Role.Reader and acl.Role.Writer switch arms (references: ACLCtx, allowed, acl.Role.Reader, acl.Role.Writer, HTTP_METHOD) so the ACL matches router behavior, or alternatively implement an OPTIONS route/handler in the router to actually handle preflight requests if CORS is required; choose one approach and ensure HTTP_METHOD handling and router dispatch stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.zig`:
- Line 1083: Update the log message in the std.log.err call that currently reads
"ACL credential '{s}' has an secret key exceeding 252 bytes; skipping" (the
std.log.err invocation using credential.access_key) to use the correct article
by changing "an secret key" to "a secret key" so the message reads "ACL
credential '{s}' has a secret key exceeding 252 bytes; skipping".
- Around line 1070-1095: The problem is that ctx is constructed with
.access_control_map = undefined and defer ctx.deinit() runs before populating
access_control_map, so if access_control_map.put(...) fails the deferred
ctx.deinit() will try to deinit an undefined map; fix by populating the local
access_control_map first (use
std.StringHashMap(acl.Credential).init(allocator)), run your put loop and on
error explicitly deinit the local map and return, then only after successful
population assign ctx.access_control_map = access_control_map and register defer
ctx.deinit(); avoid adding an errdefer for the local map that would double-free
if ctx is later created. Ensure you reference the symbols access_control_map,
ctx, S3Context, deinit(), and access_control_map.put when making the change.
- Around line 1062-1067: The makeDir call for creating the `.index` directory
(std.fs.cwd().makeDir(dot_index_path)) returns the wrong error variant on
failure — replace the incorrect return of error.FailedDataDirDotCasCreation with
the correct `.index` failure variant (e.g., error.FailedDataDirDotIndexCreation
or the existing enum value that specifically represents `.index` creation
failure) so the error clearly identifies a `.index` creation problem instead of
`.cas`.
- Around line 1847-1850: Replace the non-constant-time equality check that
compares calculated_sig to parsed.signature (currently using std.mem.eql) with a
timing-safe comparison to prevent HMAC timing leaks: use
std.crypto.timing_safe.eql (Zig 0.14+) or std.crypto.utils.timingSafeEql for
older Zig, and branch on that result to set acl_ctx.allow and acl_ctx.role
exactly as before; ensure you import/qualify the correct crypto namespace for
your Zig version and keep the same control flow that assigns acl_ctx.allow =
true and acl_ctx.role = credential.?.role when the timing-safe comparison
succeeds.
- Around line 1847-1850: The current equality check uses std.mem.eql which is
vulnerable to timing attacks; replace it with a constant-time comparison using
std.crypto.utils.timingSafeEql for fixed-size buffers and ensure you first
verify the lengths match (compare parsed.signature.len to calculated_sig.len)
before calling timingSafeEql; update the block that sets acl_ctx.allow and
acl_ctx.role (the comparison involving calculated_sig and parsed.signature) to
use the length check + timingSafeEql and only set acl_ctx.allow = true and
acl_ctx.role = credential.?.role when the timingSafeEql result is true.
---
Duplicate comments:
In `@main.zig`:
- Around line 1782-1793: The Writer role's switch on http_method (in the
acl.Role.Writer arm) currently allows .PUT, .POST, .HEAD, .OPTIONS but omits
.GET; update the switch in that arm to either add “.GET => return true” so
writers can read/list what they uploaded, or if the role is intentionally
write-only, add a clear inline comment in the acl.Role.Writer block documenting
that it intentionally excludes GET for ingestion-only behavior; ensure you
modify the switch that uses std.meta.stringToEnum(HTTP_METHOD, method) and the
http_method variable.
- Around line 1080-1093: The loop over access_control_list currently calls
access_control_map.put(credential.access_key, credential) which will silently
overwrite earlier entries with the same credential.access_key; change the logic
to check for an existing key before inserting (e.g., lookup or tryGet on
access_control_map using credential.access_key) and handle duplicates
explicitly: either skip the new credential and std.log.err/warn including
credential.access_key, or return an error, so duplicates are not overwritten
silently; ensure this check occurs before the try access_control_map.put call
and keep the comment about using slice contents for the key.
---
Nitpick comments:
In `@main.zig`:
- Around line 1765-1793: The ACL allows "OPTIONS" but the router never
dispatches OPTIONS so ACLCtx.allowed method's OPTIONS branches are dead; update
the allowed(self: *const ACLCtx, method: []const u8) implementation to remove
.OPTIONS from the acl.Role.Reader and acl.Role.Writer switch arms (references:
ACLCtx, allowed, acl.Role.Reader, acl.Role.Writer, HTTP_METHOD) so the ACL
matches router behavior, or alternatively implement an OPTIONS route/handler in
the router to actually handle preflight requests if CORS is required; choose one
approach and ensure HTTP_METHOD handling and router dispatch stay consistent.
Owner
|
Good direction overall. A few things worth fixing before merge. Double-free risk The inline Writer cannot GET A Writer can PUT but not GET. Most write-access clients need to verify uploads. Worth deciding if this is intentional. Slice lifetime in parseCredentials
build.zig importing acl.zig
data_dir is now compile-time
Duplicated permission checks
Minor: the 252-byte key length limit needs a comment explaining where that number comes from. |
Author
|
Thanks for the feedback. build.zig importing acl.zigThis is intentional - in-lining the Writer cannot GETCorrect - the writer role has HEAD not GET - this is intentional. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR introduces role-based Access Control Lists (ACLs) with support for multiple credentials, replacing the previous single hardcoded access/secret key configuration.
Authentication is now backed by a credential map, and authorization is enforced per HTTP method based on role (Admin, Reader, Writer).
Why?
That’s it.
Summary by CodeRabbit
New Features
Documentation