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

exit with ngx.DECLINED when the request is allowed #62

Merged
merged 1 commit into from
Feb 6, 2024
Merged

Conversation

blotus
Copy link
Member

@blotus blotus commented Feb 1, 2024

Closes #61.

The bouncer is running in the access phase of nginx, and an access handler can have 3 different return code:

  • ngx.OK: request is allowed to go through (and bypass any remaining handler in the phase)
  • >= ngx.HTTP_OK: return this code to nginx and skip all remaining phases
  • ngx.DECLINED: tell nginx we are not interested in handling the request (and so, we are not taken into account when resolving satisfy).

ngx.DECLINED is not really documented in the LUA module (best I could find is this and this

We didn't explicitly tell nginx we were not interested in handling the request in the following case:

  • The bouncer is disabled in the configuration
  • The bouncer has no decision for the IP

This caused any configuration using satisfy any to not behave as expected: because we were implicitly allowing the request, any other check (such as IP or credentials) was ignored.

We now explicitly exit the handler with ngx.DECLINED when:

  • The bouncer is not enabled
  • We are in an excluded location
  • We have no decision for the IP

This slightly changes the behavior of the bouncer: if an IP is allowed (through an allow directive or correct HTTP credentials) AND nginx is configured with satisfy any, the request will be allowed to go through even if there's a decision for the IP or if the appsec component decided to block the request (this can be worked around by using satisfy all, but this is not suitable for all situations).

@LaurenceJJones LaurenceJJones self-requested a review February 1, 2024 22:41
Copy link
Contributor

@LaurenceJJones LaurenceJJones left a comment

Choose a reason for hiding this comment

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

LGTM

@blotus blotus merged commit 26e9c0c into main Feb 6, 2024
@blotus blotus deleted the explict-decline branch February 6, 2024 11:05
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.

satisfy any; will always allow
2 participants