[RHOAIRFE-496]: feat: add publicPaths to MaaSAuthPolicy for unauthenticated access to docs endpoints #692
Conversation
|
Hi @Gajanan1995. Thanks for your PR. I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
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:
📝 WalkthroughWalkthroughThis PR adds a PublicPaths feature to MaaSAuthPolicy, allowing operators to mark specific API documentation paths (/docs, /openapi.json) as publicly accessible. The implementation defines a PublicPath enum type with two allowed values, adds an optional MaxItems=10 publicPaths field to the API specification and CRD schema, implements deep-copy support, and extends the controller to aggregate publicPaths across all MaaSAuthPolicy objects targeting the same model. When publicPaths are configured, the controller builds an exact-match regex and CEL predicate, then injects an anonymous authentication rule and appends CEL exclusion logic to metadata and authorization evaluators in the generated Kuadrant AuthPolicy. Comprehensive unit tests validate helper function output and end-to-end reconciler behavior. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Security FindingsCWE-203 Observable Discrepancy (Information Disclosure): The public paths regex uses CWE-78 OS Command Injection (Low Risk): The CWE-400 Uncontrolled Resource Consumption (Low Risk): The aggregation loop at lines 517–530 iterates all MaaSAuthPolicy objects for a model and appends to CWE-561 Dead Code (Low Risk): The removed 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 6
🧹 Nitpick comments (2)
maas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go (2)
1236-1267: Test coverage gaps: empty slice and special characters.Missing test cases for
buildPublicPathsRegex:
- Empty
pathsslice (returns empty string? panics?)- Paths with regex metacharacters beyond
.(e.g.,+,*,?,(,),[,],^,$,{,},|,\)The function only escapes
.but these characters could appear in valid URL paths and break the regex.{ name: "empty paths", paths: []string{}, expect: "", }, { name: "path with special regex chars", paths: []string{"/api/v1[test]"}, expect: `.*/api/v1\[test\]$`, // if properly escaped },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@maas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go` around lines 1236 - 1267, Add unit tests to TestBuildPublicPathsRegex to cover the empty paths case and paths containing regex metacharacters; specifically add a case where paths is an empty slice and expect an empty string, and a case such as paths = []string{"/api/v1[test]"} expecting the regex to escape special characters (e.g. `.*/api/v1\[test\]$`). Ensure these new cases are added to the tests slice in TestBuildPublicPathsRegex so buildPublicPathsRegex is validated for empty input and proper escaping of characters beyond `.`.
1269-1295: Missing test for path normalization consistency.
buildPublicPathsCELPredicatenormalizes paths without leading/, but no test verifies this:{ name: "path without leading slash", paths: []string{"docs"}, expect: `!request.path.endsWith("/docs")`, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@maas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go` around lines 1269 - 1295, The TestBuildPublicPathsCELPredicate suite is missing a case that verifies path normalization for inputs without a leading slash; add a subtest to TestBuildPublicPathsCELPredicate that calls buildPublicPathsCELPredicate with paths []string{"docs"} and assert the result equals `!request.path.endsWith("/docs")`, ensuring the function normalizes "docs" to "/docs" in the generated CEL predicate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@deployment/base/maas-controller/crd/bases/maas.opendatahub.io_maasauthpolicies.yaml`:
- Around line 92-101: The publicPaths array items lack a maxLength constraint,
allowing arbitrarily long path strings; update the CRD schema for publicPaths
(in maas.opendatahub.io_maasauthpolicies.yaml) by adding a sensible maxLength to
each string item (e.g., 256 or another project-approved limit) so the items
schema under publicPaths includes "type: string" and "maxLength: <limit>" to
prevent excessively long path entries used when generating AuthPolicy rules.
In `@deployment/base/maas-controller/policies/gateway-default-deny.yaml`:
- Around line 36-37: The gateway default-deny AuthPolicy
(gateway-default-auth.yaml) lacks the same path exclusions as the TRLP: update
the AuthPolicy's rule that uses the when predicate (the predicate string that
currently checks '!request.path.startsWith("/maas-api") &&
!request.path.startsWith("/v1/models")') to also exclude documentation endpoints
by adding '&& !request.path.endsWith("/docs") &&
!request.path.endsWith("/openapi.json")' so the same /docs and /openapi.json
requests bypass the deny-unconfigured-models check; alternatively, if keeping
documentation behind auth is intentional, add a comment explaining that
behavior.
In `@maas-controller/api/maas/v1alpha1/maasauthpolicy_types.go`:
- Around line 33-39: Add a per-item max length kubebuilder marker to the
PublicPaths slice: update the PublicPaths field in maasauthpolicy_types.go (the
PublicPaths []string `json:"publicPaths,omitempty"` declaration) to include a
kubebuilder validation marker enforcing a reasonable maximum path length (e.g.,
add "+kubebuilder:validation:MaxLength=128" above the field) so each string in
the slice is length-validated in addition to the existing MaxItems constraint.
In `@maas-controller/pkg/controller/maas/maasauthpolicy_controller.go`:
- Around line 128-139: The buildPublicPathsCELPredicate function interpolates
raw path strings into CEL expressions, enabling CEL injection via unescaped
quotes or backslashes; update buildPublicPathsCELPredicate to sanitize/escape
CEL special characters (at minimum escape backslashes and double-quotes) before
formatting into the `!request.path.endsWith("...")` fragments so a malicious
path cannot break out of the string literal, and ensure the escaped value is
used when appending to parts and joining them.
- Around line 274-285: The publicPaths slice is aggregated without validation
and can contain regex or CEL-dangerous characters; before calling
deduplicateAndSort/publicPaths usage, validate each item (the publicPaths
elements collected in the loop over allPolicies) using the same approach as
validateCELValue or a new helper (e.g., validatePublicPath) that rejects
metacharacters used in regex and CEL (such as *, +, ?, {, }, [, ], (, ), |, ^,
$, \, `, ", @, #, etc.) or applies proper escaping; if any path is invalid,
return/reconcile with an error similar to the validateCELValue flow so dangerous
inputs are rejected before regex/CEL generation. Ensure you reference/ reuse
validateCELValue or call a new validatePublicPath for each ap.Spec.PublicPaths
item prior to deduplicateAndSort/publicPaths usage.
- Around line 113-126: The buildPublicPathsRegex function only escapes dots and
allows regex metacharacters to be injected; replace the manual escaping with
regexp.QuoteMeta for each input path (use regexp.QuoteMeta(p) to fully escape
all regex metacharacters), ensure a leading "/" is still added if missing, then
build the pattern as currently done (fmt.Sprintf(".*%s$", escaped)) so the
literal path is matched; also add the regexp import if missing and update
references in buildPublicPathsRegex accordingly.
---
Nitpick comments:
In `@maas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go`:
- Around line 1236-1267: Add unit tests to TestBuildPublicPathsRegex to cover
the empty paths case and paths containing regex metacharacters; specifically add
a case where paths is an empty slice and expect an empty string, and a case such
as paths = []string{"/api/v1[test]"} expecting the regex to escape special
characters (e.g. `.*/api/v1\[test\]$`). Ensure these new cases are added to the
tests slice in TestBuildPublicPathsRegex so buildPublicPathsRegex is validated
for empty input and proper escaping of characters beyond `.`.
- Around line 1269-1295: The TestBuildPublicPathsCELPredicate suite is missing a
case that verifies path normalization for inputs without a leading slash; add a
subtest to TestBuildPublicPathsCELPredicate that calls
buildPublicPathsCELPredicate with paths []string{"docs"} and assert the result
equals `!request.path.endsWith("/docs")`, ensuring the function normalizes
"docs" to "/docs" in the generated CEL predicate.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7588c60b-0b37-47b7-9999-f4ae5df14c99
📒 Files selected for processing (6)
deployment/base/maas-controller/crd/bases/maas.opendatahub.io_maasauthpolicies.yamldeployment/base/maas-controller/policies/gateway-default-deny.yamlmaas-controller/api/maas/v1alpha1/maasauthpolicy_types.gomaas-controller/api/maas/v1alpha1/zz_generated.deepcopy.gomaas-controller/pkg/controller/maas/maasauthpolicy_controller.gomaas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
maas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go (2)
1223-1234: Usestrings.Containsinstead of custom implementation.The
containshelper reimplementsstrings.Containswith unnecessary complexity. Go's standard library provides this.-// contains is a helper to check if a string contains a substring (case-sensitive). -func contains(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(substr) == 0 || - func() bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false - }()) -} +import "strings" + +// contains wraps strings.Contains for test readability. +func contains(s, substr string) bool { + return strings.Contains(s, substr) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@maas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go` around lines 1223 - 1234, The custom contains function reimplements standard behavior; replace calls to the contains helper with strings.Contains from the standard library, remove the contains function, and add an import for "strings" if missing; specifically update usages referencing the contains(s, substr) helper and delete the contains function definition in maasauthpolicy_controller_test.go.
1257-1261: Add test case for empty string path to validate rejection.Missing test coverage for the edge case where
publicPathscontains an empty string"". This should be added once the validation fix is implemented to ensure empty paths are rejected.+ { + name: "empty string path rejected", + paths: []string{""}, + expect: "", // or test should verify error is returned + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@maas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go` around lines 1257 - 1261, Add a new test case in maasauthpolicy_controller_test.go mirroring the "empty paths" case but with paths: []string{""} and a descriptive name like "empty string path"; assert that the function under test (the table-driven test uses the variable paths and expect fields) rejects this input (i.e., expect a non-empty error/validation failure) so the test verifies empty string entries in publicPaths are invalid after the validation fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@deployment/base/maas-controller/crd/bases/maas.opendatahub.io_maasauthpolicies.yaml`:
- Around line 98-100: The CRD schema for the maasauthpolicies resource currently
allows empty path strings because the array item schema only has "type: string"
and "maxLength: 256"; add "minLength: 1" under the same "items" block (the
schema that contains "items", "type: string", and "maxLength: 256") so that each
path string must be at least one character, preventing empty "" values from
being admitted.
In `@maas-controller/pkg/controller/maas/helpers.go`:
- Around line 34-38: validatePublicPath currently allows empty strings which
normalize to "/" and grant broad anonymous access; update validatePublicPath to
reject empty or whitespace-only paths (e.g., check path == "" or
strings.TrimSpace(path) == "") and return an error describing that publicPath
must be a non-empty, non-whitespace string so callers cannot pass "" (reference
function validatePublicPath).
In `@maas-controller/pkg/controller/maas/maasauthpolicy_controller.go`:
- Around line 556-568: The authorization rule handling currently overwrites
existing "when" conditions on authRules (using eval["when"] = []any{...}), which
discards previously added predicates; change it to follow the same append
pattern used by the metadata evaluators: read the existing when slice from
eval["when"], append the new publicPaths predicate (built by
buildPublicPathsCELPredicate) to that slice, and assign the appended slice back
to eval["when"] before storing back in authRules (operate on the same eval map
used in the loop so existing conditions are preserved).
---
Nitpick comments:
In `@maas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go`:
- Around line 1223-1234: The custom contains function reimplements standard
behavior; replace calls to the contains helper with strings.Contains from the
standard library, remove the contains function, and add an import for "strings"
if missing; specifically update usages referencing the contains(s, substr)
helper and delete the contains function definition in
maasauthpolicy_controller_test.go.
- Around line 1257-1261: Add a new test case in
maasauthpolicy_controller_test.go mirroring the "empty paths" case but with
paths: []string{""} and a descriptive name like "empty string path"; assert that
the function under test (the table-driven test uses the variable paths and
expect fields) rejects this input (i.e., expect a non-empty error/validation
failure) so the test verifies empty string entries in publicPaths are invalid
after the validation fix.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1d5484bd-9314-44cc-9230-7adee8470d63
📒 Files selected for processing (6)
deployment/base/maas-controller/crd/bases/maas.opendatahub.io_maasauthpolicies.yamldeployment/base/maas-controller/policies/gateway-default-auth.yamlmaas-controller/api/maas/v1alpha1/maasauthpolicy_types.gomaas-controller/pkg/controller/maas/helpers.gomaas-controller/pkg/controller/maas/maasauthpolicy_controller.gomaas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go
✅ Files skipped from review due to trivial changes (1)
- deployment/base/maas-controller/policies/gateway-default-auth.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- maas-controller/api/maas/v1alpha1/maasauthpolicy_types.go
| func validatePublicPath(path string) error { | ||
| if strings.ContainsAny(path, `"'\{}()|*+?^$[]#@`) { | ||
| return fmt.Errorf("publicPath %q contains characters unsafe for regex/CEL expressions", path) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Empty string paths not rejected, enabling broad anonymous access (CWE-285).
validatePublicPath does not reject empty strings. An empty publicPath entry becomes "/" after normalization, matching:
- Regex:
.*/$(anonymous auth for all paths ending with/) - CEL:
!request.path.endsWith("/")
Attack: User with CR create permission sets publicPaths: [""] to gain anonymous access to any path ending with /.
func validatePublicPath(path string) error {
+ if path == "" {
+ return fmt.Errorf("publicPath cannot be empty")
+ }
if strings.ContainsAny(path, `"'\{}()|*+?^$[]#@`) {
return fmt.Errorf("publicPath %q contains characters unsafe for regex/CEL expressions", path)
}
return nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@maas-controller/pkg/controller/maas/helpers.go` around lines 34 - 38,
validatePublicPath currently allows empty strings which normalize to "/" and
grant broad anonymous access; update validatePublicPath to reject empty or
whitespace-only paths (e.g., check path == "" or strings.TrimSpace(path) == "")
and return an error describing that publicPath must be a non-empty,
non-whitespace string so callers cannot pass "" (reference function
validatePublicPath).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
maas-controller/pkg/controller/maas/maasauthpolicy_controller.go (1)
556-567:buildPublicPathsCELPredicatecalled twice for same input.
publicPathsCELis computed at line 414 within the firstif len(publicPaths) > 0block, then recomputed identically at line 558. Hoist the variable to avoid redundant string building.Proposed fix
if len(publicPaths) > 0 { publicPathsRegex := buildPublicPathsRegex(publicPaths) publicPathsCEL := buildPublicPathsCELPredicate(publicPaths) + // Store for later use in authorization rules + _ = publicPathsCEL // will be used below // ... metadata handling ... } - // Add when predicates to authorization rules to skip them for public paths - if len(publicPaths) > 0 { - publicPathsCEL := buildPublicPathsCELPredicate(publicPaths) + // Add when predicates to authorization rules to skip them for public paths + if len(publicPaths) > 0 { + // publicPathsCEL already computed above for name, evaluator := range authRules {Or restructure to compute once and reuse:
+ var publicPathsCEL string if len(publicPaths) > 0 { publicPathsRegex := buildPublicPathsRegex(publicPaths) - publicPathsCEL := buildPublicPathsCELPredicate(publicPaths) + publicPathsCEL = buildPublicPathsCELPredicate(publicPaths) // ... rest of block } // ... authorization rules creation ... - if len(publicPaths) > 0 { - publicPathsCEL := buildPublicPathsCELPredicate(publicPaths) + if publicPathsCEL != "" { for name, evaluator := range authRules {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@maas-controller/pkg/controller/maas/maasauthpolicy_controller.go` around lines 556 - 567, The function buildPublicPathsCELPredicate is called twice with the same publicPaths input, once earlier and again inside the if block where you modify authRules, causing redundant computation. To fix this, move the buildPublicPathsCELPredicate call outside the if blocks so it is computed only once and stored in a variable, then reuse that variable wherever needed in the maasauthpolicy_controller.go code that modifies authRules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@maas-controller/pkg/controller/maas/maasauthpolicy_controller.go`:
- Around line 129-143: buildPublicPathsCELPredicate currently only escapes
backslashes and double-quotes, leaving control characters like \n or \r
unescaped which can produce invalid CEL string literals; update
buildPublicPathsCELPredicate to escape control characters before inserting the
path into the CEL literal (e.g., replace newline, carriage return, tab,
formfeed, backspace with their backslash-escaped sequences and encode any other
non-printable runes (rune < 0x20 or 0x7f) as \uXXXX) so the produced
fmt.Sprintf(`!request.path.endsWith("%s")`, escaped) is always a valid CEL
string; you may also add or call a helper function (e.g., escapeCELString or
extend validatePublicPath) to centralize this escaping logic.
---
Nitpick comments:
In `@maas-controller/pkg/controller/maas/maasauthpolicy_controller.go`:
- Around line 556-567: The function buildPublicPathsCELPredicate is called twice
with the same publicPaths input, once earlier and again inside the if block
where you modify authRules, causing redundant computation. To fix this, move the
buildPublicPathsCELPredicate call outside the if blocks so it is computed only
once and stored in a variable, then reuse that variable wherever needed in the
maasauthpolicy_controller.go code that modifies authRules.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 98e3049c-5916-48e7-bc87-70c6d0002a71
📒 Files selected for processing (4)
deployment/base/maas-controller/crd/bases/maas.opendatahub.io_maasauthpolicies.yamlmaas-controller/api/maas/v1alpha1/maasauthpolicy_types.gomaas-controller/pkg/controller/maas/helpers.gomaas-controller/pkg/controller/maas/maasauthpolicy_controller.go
✅ Files skipped from review due to trivial changes (1)
- maas-controller/pkg/controller/maas/helpers.go
| // buildPublicPathsCELPredicate constructs a CEL predicate that excludes public paths from evaluation. | ||
| // Paths are escaped to prevent CEL injection via quotes or backslashes. | ||
| // Example: ["/docs", "/openapi.json"] -> '!request.path.endsWith("/docs") && !request.path.endsWith("/openapi.json")' | ||
| func buildPublicPathsCELPredicate(paths []string) string { | ||
| var parts []string | ||
| for _, p := range paths { | ||
| if !strings.HasPrefix(p, "/") { | ||
| p = "/" + p | ||
| } | ||
| escaped := strings.ReplaceAll(p, `\`, `\\`) | ||
| escaped = strings.ReplaceAll(escaped, `"`, `\"`) | ||
| parts = append(parts, fmt.Sprintf(`!request.path.endsWith("%s")`, escaped)) | ||
| } | ||
| return strings.Join(parts, " && ") | ||
| } |
There was a problem hiding this comment.
Control characters not escaped in CEL string literals.
validatePublicPath rejects many metacharacters but allows control characters (e.g., \n, \r). If a path contains a literal newline (unlikely via kubectl but possible via API), the generated CEL predicate would have an unescaped newline in the string literal, causing a parse error.
Impact: Fail-closed (AuthPolicy rejected by Kuadrant), not injection. Low likelihood via normal workflows.
Proposed fix: escape or reject control characters
Option 1 - Reject in validation (helpers.go):
func validatePublicPath(path string) error {
if path == "" {
return fmt.Errorf("publicPath cannot be empty")
}
+ for _, r := range path {
+ if r < 0x20 || r == 0x7f {
+ return fmt.Errorf("publicPath %q contains control characters", path)
+ }
+ }
if strings.ContainsAny(path, `"'\{}()|*+?^$[]#@`) {Option 2 - Escape in CEL builder:
for _, p := range paths {
if !strings.HasPrefix(p, "/") {
p = "/" + p
}
escaped := strings.ReplaceAll(p, `\`, `\\`)
escaped = strings.ReplaceAll(escaped, `"`, `\"`)
+ escaped = strings.ReplaceAll(escaped, "\n", `\n`)
+ escaped = strings.ReplaceAll(escaped, "\r", `\r`)
+ escaped = strings.ReplaceAll(escaped, "\t", `\t`)
parts = append(parts, fmt.Sprintf(`!request.path.endsWith("%s")`, escaped))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // buildPublicPathsCELPredicate constructs a CEL predicate that excludes public paths from evaluation. | |
| // Paths are escaped to prevent CEL injection via quotes or backslashes. | |
| // Example: ["/docs", "/openapi.json"] -> '!request.path.endsWith("/docs") && !request.path.endsWith("/openapi.json")' | |
| func buildPublicPathsCELPredicate(paths []string) string { | |
| var parts []string | |
| for _, p := range paths { | |
| if !strings.HasPrefix(p, "/") { | |
| p = "/" + p | |
| } | |
| escaped := strings.ReplaceAll(p, `\`, `\\`) | |
| escaped = strings.ReplaceAll(escaped, `"`, `\"`) | |
| parts = append(parts, fmt.Sprintf(`!request.path.endsWith("%s")`, escaped)) | |
| } | |
| return strings.Join(parts, " && ") | |
| } | |
| // buildPublicPathsCELPredicate constructs a CEL predicate that excludes public paths from evaluation. | |
| // Paths are escaped to prevent CEL injection via quotes or backslashes. | |
| // Example: ["/docs", "/openapi.json"] -> '!request.path.endsWith("/docs") && !request.path.endsWith("/openapi.json")' | |
| func buildPublicPathsCELPredicate(paths []string) string { | |
| var parts []string | |
| for _, p := range paths { | |
| if !strings.HasPrefix(p, "/") { | |
| p = "/" + p | |
| } | |
| escaped := strings.ReplaceAll(p, `\`, `\\`) | |
| escaped = strings.ReplaceAll(escaped, `"`, `\"`) | |
| escaped = strings.ReplaceAll(escaped, "\n", `\n`) | |
| escaped = strings.ReplaceAll(escaped, "\r", `\r`) | |
| escaped = strings.ReplaceAll(escaped, "\t", `\t`) | |
| parts = append(parts, fmt.Sprintf(`!request.path.endsWith("%s")`, escaped)) | |
| } | |
| return strings.Join(parts, " && ") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@maas-controller/pkg/controller/maas/maasauthpolicy_controller.go` around
lines 129 - 143, buildPublicPathsCELPredicate currently only escapes backslashes
and double-quotes, leaving control characters like \n or \r unescaped which can
produce invalid CEL string literals; update buildPublicPathsCELPredicate to
escape control characters before inserting the path into the CEL literal (e.g.,
replace newline, carriage return, tab, formfeed, backspace with their
backslash-escaped sequences and encode any other non-printable runes (rune <
0x20 or 0x7f) as \uXXXX) so the produced
fmt.Sprintf(`!request.path.endsWith("%s")`, escaped) is always a valid CEL
string; you may also add or call a helper function (e.g., escapeCELString or
extend validatePublicPath) to centralize this escaping logic.
|
I like the idea but just late in the game for 3.4 marking as a fast follow |
|
@jland-redhat Thanks for the review and for marking this as a fast follow! Happy to rebase once the target branch for 3.5 is ready, or I can rebase against |
liangwen12year
left a comment
liangwen12year
left a comment
There was a problem hiding this comment.
Nice work on this feature — the per-model AuthPolicy anonymous rule is well-scoped and the CodeRabbit iterations look solid. One concern on the gateway-level changes:
Gateway default-deny/TRLP hardcodes /docs and /openapi.json exclusions globally
The changes to gateway-default-deny.yaml and gateway-default-auth.yaml unconditionally exclude /docs and /openapi.json from rate limiting and auth for all models on the gateway, regardless of whether any model has publicPaths configured.
# gateway-default-deny.yaml
- predicate: '!request.path.startsWith("/maas-api") && !request.path.startsWith("/v1/models") && !request.path.endsWith("/docs") && !request.path.endsWith("/openapi.json")'# gateway-default-auth.yaml
when:
- predicate: '!request.path.endsWith("/docs") && !request.path.endsWith("/openapi.json")'This means a model that does not set publicPaths still has its /docs and /openapi.json bypass the gateway-level deny and rate limiting. The per-route AuthPolicy (generated by the reconciler) would still block unauthenticated access for models without publicPaths, but the defense-in-depth layer from the TRLP is gone for those paths on all models.
Should these gateway-level exclusions be removed so the feature is purely opt-in per model via the publicPaths field? The per-route anonymous auth rule already handles the bypass for models that explicitly configure it.
Minor items:
buildPublicPathsCELPredicateis called twice with the samepublicPathsinput (~line 414 and ~558) — could hoist the variable to compute once.- The test
contains()helper reimplementsstrings.Contains— could just use the stdlib.
Gajanan1995
force-pushed
the
feat/public-paths-maasauthpolicy
branch
from
5ead48f to
5c46e8b
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Gajanan1995 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/retest |
1 similar comment
|
/retest |
|
@Gajanan1995: The following test has Succeeded: OCI Artifact Browser URLInspecting Test Artifacts ManuallyTo inspect your test artifacts manually, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/opendatahub/odh-ci-artifacts:maas-group-test-7q22q |
| // buildPublicPathsRegex constructs a regex pattern that matches any of the given path suffixes. | ||
| // Each path is fully escaped with regexp.QuoteMeta and anchored with .* prefix and $ suffix. | ||
| // Example: ["/docs", "/openapi.json"] -> ".*/docs$|.*/openapi\.json$" | ||
| func buildPublicPathsRegex(paths []string) string { |
There was a problem hiding this comment.
endsWith("/docs") and .*\/docs$ match any path whose last segment is /docs, not just the model's documentation root.
A request to /llm/my-model/v1/completions/docs would match and bypass auth. Consider switching to an exact-path match against the model's route prefix + suffix.
There was a problem hiding this comment.
@EgorLu Good catch you're right that endsWith ("/docs") would also match crafted paths like /v1/completions/docs. I'll switch to exact-path matching using the model's route prefix so the match is scoped to {routePrefix}{suffix} rather than a loose suffix. Will update the PR.
There was a problem hiding this comment.
Changed from endsWith("/docs") / .*\/docs$ (matches any path ending in /docs) to exact-path matching using the model's route prefix: request.path != "/default/llm/docs" / ^/default/llm/docs$
A crafted path like /v1/completions/docs will no longer bypass auth
| var publicPaths []string | ||
| for _, ap := range allPolicies { | ||
| for _, p := range ap.Spec.PublicPaths { | ||
| if err := validatePublicPath(p); err != nil { |
There was a problem hiding this comment.
This opens a possibility for human error of the admin.
For example, an admin could mistakenly set publicPaths: ["/v1/completions"] and disable authentication for a model's inference.
I see two possibilities here. One is restricting publicPaths to a curated allowlist of safe suffixes, such as /docs. Or, on the contrary, having a denylist of known inference path segments, such as /completions, /chat, etc.
There was a problem hiding this comment.
@EgorLu Valid concern. I think an allowlist of known-safe documentation paths is the cleaner approach as it's simpler, less fragile than maintaining a denylist, and there's a small well-known set /docs, /openapi.json. A denylist would need to keep up with new inference paths added over time.
Would a hardcoded allowlist like ["/docs", "/openapi.json"] with CRD-level enum validation work, or do you think we should keep it as free-form strings but validate against the allowlist in the controller?
There was a problem hiding this comment.
I think that your idea with a CRD-level enum is great for this usecase. We won't often need to update that list, so a rare update would justify a CRD update on demand :)
There was a problem hiding this comment.
Changed PublicPaths from free-form []string to a []PublicPath enum type restricted to /docs and /openapi.json at the CRD level
… docs endpoints
Adds a publicPaths field to MaaSAuthPolicySpec that allows specifying
well-known documentation endpoints (e.g. "/docs", "/openapi.json") that
should be accessible without authentication. Values are restricted to a
CRD-level enum allowlist of safe, read-only paths.
The controller generates an anonymous authentication rule in the Kuadrant
AuthPolicy using exact-path matching against the model's route prefix
(/{namespace}/{name}{suffix}), preventing crafted paths from bypassing
auth. Inference and other endpoints remain fully authenticated.
The feature is purely opt-in per model - only models whose MaaSAuthPolicy
includes publicPaths get the anonymous access rule. Gateway-level default
policies are unchanged.
Changes:
- Add PublicPath enum type restricted to /docs and /openapi.json
- Add PublicPaths field to MaaSAuthPolicySpec CRD (maxItems=10)
- Update deepcopy and regenerate CRD manifests
- Add anonymous-public-paths auth rule generation in reconciler
- Use exact-path matching with model route prefix in regex and CEL
- Add CEL predicates to skip metadata/authorization for public paths
- Add comprehensive unit tests for all new functionality
Co-authored-by: Cursor <cursoragent@cursor.com>
Gajanan1995
force-pushed
the
feat/public-paths-maasauthpolicy
branch
from
5c46e8b to
05f97ac
Compare
Gajanan1995
changed the title
Gajanan1995
changed the title
6 participants
Summary
Adds a
publicPathsfield toMaaSAuthPolicySpecthat allows administrators to specify URL path suffixes (e.g.,/docs,/openapi.json) that should be accessible without authentication. This enables users to browse model Swagger/OpenAPI documentation directly in a browser without needing a token, while keeping inference and all other endpoints fully authenticated.Motivation: vLLM and other serving runtimes expose a
/docs(Swagger UI) and/openapi.jsonendpoint, but the current MaaS authentication setup blocks unauthenticated access to all paths. There is no way to selectively expose documentation endpoints without bypassing auth entirely.Changes
maasauthpolicy_types.go): AddPublicPaths []stringfield toMaaSAuthPolicySpecwith+optional, max 10 items validationzz_generated.deepcopy.go): Updated to handle the newPublicPathsslicemaas.opendatahub.io_maasauthpolicies.yaml): Regenerated viacontroller-genmaasauthpolicy_controller.go):buildPublicPathsRegex()— generates arequest.url_pathregex pattern for AuthorinobuildPublicPathsCELPredicate()— generates CELwhenpredicates to skip auth/metadata for public pathsanonymous-public-pathsauthentication rule into the generated AuthPolicy, scoped to matching pathswhenpredicates toapiKeyValidation,subscription-infometadata evaluators andauth-valid,subscription-valid,require-group-membershipauthorization rules to skip them for public pathsgateway-default-deny.yaml): Updated predicate to exclude/docsand/openapi.jsonfrom the zero-rate-limit deny rulemaasauthpolicy_controller_test.go): Added comprehensive tests for helper functions and reconciler behavior with and withoutpublicPathsUsage
What changes, what stays the same
/docsand/openapi.json→ accessible without auth (anonymous)/v1/completions,/v1/chat/completions, etc. → still require full authentication (API key or token)publicPaths→ no behavior change at all (backward compatible)Test plan
buildPublicPathsRegexandbuildPublicPathsCELPredicatehelpersanonymous-public-pathsauth rule whenpublicPathsis setwhenpredicates to metadata/authorization evaluatorspublicPathsis emptymake -C maas-controller test— all tests passmake -C maas-controller verify-codegen— generated files in sync./scripts/ci/validate-manifests.sh— all kustomize manifests validatepublicPaths, verify/docsreturns 200 without auth and inference endpoints still require authSummary by CodeRabbit
Release Notes