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

internal/appsec: add support for http.request.path_params address #1106

Merged
merged 11 commits into from
Jan 19, 2022

Conversation

Hellzy
Copy link
Contributor

@Hellzy Hellzy commented Dec 27, 2021

This set of commits adds support for the http.request.path_params address in AppSec.

  • Add a field to tracer config to relay data that AppSec can't necessarily retrieve through the http.Request framework context only. This is used for Gorilla specifically, to pass URL variables down to the tracer and in turn to httpsec.

  • Pass the path parameters down to the waf and add a case to run matching on the new address

  • Add/Edit existing tests to make sure the path parameters can be caught/retrieved and trigger the waf matches as expected.

  • Support Echo

  • Support Gorilla

Note: no support for net/http (framework doesn't handle path parameters)

@Hellzy Hellzy requested a review from Julio-Guerra December 27, 2021 15:47
@Hellzy Hellzy added the appsec label Dec 27, 2021
@Hellzy Hellzy added this to the 1.36.0 milestone Dec 27, 2021
@Hellzy Hellzy force-pushed the francois.mazeau/appsec-http-path-params branch from f07f24e to 0b84c4e Compare December 27, 2021 16:12
@Hellzy Hellzy changed the title Francois.mazeau/appsec http path params internal/appsec: add support for http.request.path_params address Dec 27, 2021
@Julio-Guerra Julio-Guerra changed the base branch from julio.guerra/appsec-event-span-tags to julio.guerra/appsec-http-status-code-address January 11, 2022 13:21
Copy link
Contributor

@Julio-Guerra Julio-Guerra left a comment

Choose a reason for hiding this comment

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

I think we can simplify it to PathParams map[string]string only for now.
I understand you introduced this AppSecParams for future AppSec options, but since we have only one today, I think it's fine if we simplify it to that value only.

internal/appsec/dyngo/instrumentation/httpsec/http.go Outdated Show resolved Hide resolved
contrib/internal/httputil/trace.go Outdated Show resolved Hide resolved
contrib/gorilla/mux/waf_test.go Outdated Show resolved Hide resolved
contrib/gorilla/mux/mux.go Outdated Show resolved Hide resolved
@Hellzy Hellzy force-pushed the francois.mazeau/appsec-http-path-params branch from 0b84c4e to 05a98ab Compare January 12, 2022 09:54
@Hellzy
Copy link
Contributor Author

Hellzy commented Jan 12, 2022

Rebased on julio.guerra/appsec-http-status-code-address

@Julio-Guerra Julio-Guerra force-pushed the julio.guerra/appsec-http-status-code-address branch 2 times, most recently from 9f7b023 to 91767d3 Compare January 12, 2022 22:35
Base automatically changed from julio.guerra/appsec-http-status-code-address to v1 January 13, 2022 14:11
…dress

Add an "optional" parameter to httpsec.MakeHandlerOperationArgs to keep
track of the name/values map for path params since the data is not
retrievable through http.Request.
Also edit waf_test.go to add a small test to check e2e soundness.
Add an AppSecParam field to router trace config to pass parameters
to AppSec wrap handler that can't be retrieved from request only.
This allows in this case to pass down path parameters.
Also add waf_test.go to check e2e soundness.
@Julio-Guerra Julio-Guerra force-pushed the francois.mazeau/appsec-http-path-params branch from 05a98ab to bc9b9b2 Compare January 13, 2022 17:35
@Julio-Guerra Julio-Guerra marked this pull request as ready for review January 13, 2022 17:38
@Julio-Guerra Julio-Guerra requested a review from a team as a code owner January 13, 2022 17:38
@Julio-Guerra Julio-Guerra requested a review from a team January 13, 2022 17:38
@Julio-Guerra Julio-Guerra requested a review from gbbr January 17, 2022 14:12
Julio-Guerra
Julio-Guerra previously approved these changes Jan 17, 2022
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

👍🏻

@@ -28,6 +28,9 @@ type ServeConfig struct {
Resource string
// QueryParams specifies any query parameters that be appended to the resulting "http.url" tag.
QueryParams bool
// RouteParams specifies framework-specific route parameters (e.g. for route /user/:id coming
// in as /user/123 we'll have {"id": "123"}). This field is optional and monitored by appsec.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// in as /user/123 we'll have {"id": "123"}). This field is optional and monitored by appsec.
// in as /user/123 we'll have {"id": "123"}). This field is optional and is used for monitoring
// by AppSec.

It's also worth mentioning that it's used specifically for that. In the current form we are saying that it's "monitored by appsec" but it's not clear if it has other uses to.

Is there any point in mentioning more details, as in how AppSec monitors these? I'm thinking that maybe some people who may be using TraceAndServe independently for their own handlers, might be able to benefit from AppSec by using these fields with some other framework which is not supported by us. Is it also worth mentioning that AppSec must be enabled for these to be taken into consideration? For example: if someone passes RouteParams but has AppSec disabled, will it be taken into account?

Copy link
Contributor

Choose a reason for hiding this comment

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

I did a simple addition here: 7ff222d

No need to further explain how AppSec monitors them, as it depends on the security rules, that are exposed and detailed in our UI.

Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Looks great as far as contrib is concerned 👍🏻

@Julio-Guerra Julio-Guerra merged commit 898f5d7 into v1 Jan 19, 2022
@Julio-Guerra Julio-Guerra deleted the francois.mazeau/appsec-http-path-params branch January 19, 2022 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants