-
Notifications
You must be signed in to change notification settings - Fork 188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
p/a/path: fail on broken pattern early, add tests #250
p/a/path: fail on broken pattern early, add tests #250
Conversation
b3ed530
to
aaf2a62
Compare
e035a1d
to
39e0223
Compare
docs/path.md
Outdated
Paths that are provided by `--ignore-path` won't be rejected. | ||
It is only possible to define one of both. | ||
|
||
## Change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a separate section for changes to previous kube-rbac-versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean in addition to the CHANGELOG.md?
docs/path.md
Outdated
E.g. `--allow-path="/api/v1/*/values"` will cause kube-rbac-proxy fail to start. | ||
E.g. `--ignore-path="/api/v1/*/values"` will cause kube-rbac-proxy fail to start. | ||
|
||
Previously `*` would count as a wildcard in between `/`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't it a single-path-segment wildcard?
pkg/authorization/path/path.go
Outdated
type pathAuthorizer struct { | ||
matchDecision, noMatchDecision authorizer.Decision | ||
|
||
paths sets.String | ||
pathPatterns []string | ||
} | ||
|
||
func newPathAuthorizer(onMatch, onNoMatch authorizer.Decision, inputPaths []string) *pathAuthorizer { | ||
var patterns []string | ||
paths := sets.NewString() // faster than trying to match every pattern every time | ||
for _, p := range inputPaths { | ||
p = strings.TrimPrefix(p, "/") | ||
if len(p) == 0 { | ||
// matches "/" | ||
paths.Insert(p) | ||
continue | ||
} | ||
if strings.ContainsRune(p, '*') { | ||
patterns = append(patterns, p) | ||
} else { | ||
paths.Insert(p) | ||
} | ||
} | ||
|
||
return &pathAuthorizer{ | ||
matchDecision: onMatch, | ||
noMatchDecision: onNoMatch, | ||
paths: paths, | ||
pathPatterns: patterns, | ||
} | ||
delegatedPathAuthorizer authorizer.Authorizer | ||
} | ||
|
||
func (a *pathAuthorizer) Authorize(ctx context.Context, attr authorizer.Attributes) (authorizer.Decision, string, error) { | ||
pth := strings.TrimPrefix(attr.GetPath(), "/") | ||
if a.paths.Has(pth) { | ||
return a.matchDecision, "", nil | ||
} | ||
decision, reason, err := a.delegatedPathAuthorizer.Authorize(ctx, attr) | ||
|
||
for _, pattern := range a.pathPatterns { | ||
if found, err := path.Match(pattern, pth); err != nil { | ||
return authorizer.DecisionNoOpinion, "Error", err | ||
} else if found { | ||
return a.matchDecision, "", nil | ||
} | ||
// There is a match on the path, so we have no opinion and let subsequent | ||
// authorizers in a union decide. | ||
if err == nil && decision == authorizer.DecisionAllow { | ||
return authorizer.DecisionNoOpinion, reason, nil | ||
} | ||
|
||
return a.noMatchDecision, "", nil | ||
return authorizer.DecisionDeny, fmt.Sprintf("NOT(%s)", reason), err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need a new type for the allowedPathsAuthorizer - inline as
// pseudocode
func NewAllowedPathsAuthorizer(allowPaths []string) (authorizer.Authorizer, error) {
pathAuthorizer = NewAuthorizer()
return authorizer.AuthorizerFunc(func() {
// Authorize() logic here
})
}
873b303
to
86110c5
Compare
docs/path.md
Outdated
|
||
Previously `*` would count as a single-path-segment wildcard ([path.Match](https://pkg.go.dev/path#Match)). | ||
Now the wildcard is matching any string, even if it contains `/`. | ||
E.g. `--allow-path="/api/v1/*"` would have rejected `/api/v1/label/values`, not it evaluated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not it evaluated
<- that sentence appears to be missing something
docs/path.md
Outdated
Previously `*` would count as a single-path-segment wildcard ([path.Match](https://pkg.go.dev/path#Match)). | ||
Now the wildcard is matching any string, even if it contains `/`. | ||
E.g. `--allow-path="/api/v1/*"` would have rejected `/api/v1/label/values`, not it evaluated. | ||
E.g. `--ignore-path="/api/v1/*"` would have evaluated `/api/v1/label/values`, now it is passed through. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g. `--ignore-path="/api/v1/*"` would have evaluated `/api/v1/label/values`, now it is passed through. | |
E.g. `--ignore-path="/api/v1/*"` would have matched `/api/v1/label/values`, now it is passed through. |
docs/path.md
Outdated
@@ -0,0 +1,44 @@ | |||
# Allow Path and Ignore Path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you may want to create a separate docs/migration
folder for these files
fd06738
to
b42539e
Compare
What
Why