Skip to content
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

validation webhook did not catch misspelled filter #1618

Open
szuecs opened this issue Nov 24, 2020 · 5 comments
Open

validation webhook did not catch misspelled filter #1618

szuecs opened this issue Nov 24, 2020 · 5 comments
Assignees
Labels
bugfix Bug fixes and patches

Comments

@szuecs
Copy link
Member

szuecs commented Nov 24, 2020

Describe the bug

RouteGroup created

spec:
  backends:
  - address: https://app.example.org/
    name: app
    type: network
  defaultBackends:
  - backendName: app
  hosts:
  - my-host.example.org
  routes:
  - filters:
    - bearerInjector("app/app-secret") # <- filter does not exist (camelcase here)
    pathSubtree: /

Expected behavior

This should not be an allowed change

Observed behavior

It was submitted without error

@szuecs szuecs added the bugfix Bug fixes and patches label Nov 24, 2020
@aryszka aryszka changed the title validation webhook did not caught miss spelled filter validation webhook did not catch misspelled filter Nov 24, 2020
@AlexanderYastrebov
Copy link
Member

I guess the proper validation should

  1. parse eskip and check there is a single filter (doable)
  2. check filter name (seems doable, needs filter registry)
  3. check args (seems tricky as it would need to actually create filter which might be impossible (e.g. static filter checks directory permissions) )

somewhere along the

if hasEmpty(r.Predicates) {
return errInvalidPredicate
}
if hasEmpty(r.Filters) {
return errInvalidFilter
}
if hasEmpty(r.Methods) {
return errInvalidMethod
}

The same is for predicates. Maybe 1. and 2. could be a good start.

@AlexanderYastrebov
Copy link
Member

Linking the comment #1537 (comment) from @aryszka touching route validation

@AlexanderYastrebov
Copy link
Member

Related issue #1333

@MustafaSaber
Copy link
Member

I see 2 problems with point 2 in this comment #1618 (comment)

  1. We will need to export filter registry somehow from Skipper and will break admission webhook interface and we won't be able to send it to anyway.

    1. https://github.com/zalando/skipper/blob/master/cmd/webhook/main.go#L58 ->
    2. https://github.com/zalando/skipper/blob/master/cmd/webhook/admission/admission.go#L106 ->
    3. https://github.com/zalando/skipper/blob/master/cmd/webhook/admission/admission.go#L72 ->
    4. https://github.com/zalando/skipper/blob/master/cmd/webhook/admission/admission.go#L87 (where validation happens)
      How can we move filterRegistry from i to iv if it's created in skipper?
  2. We don't have predicate registry so checking predicate name only also not valid

@MustafaSaber
Copy link
Member

We need the routing tree so some ideas:

  1. Initiate Skipper/routing inside admission
  2. Initiate Admission inside Skipper
  3. Initiate Skipper inside RouteSRV

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Bug fixes and patches
Projects
None yet
Development

No branches or pull requests

3 participants