-
Notifications
You must be signed in to change notification settings - Fork 12
feat: appsec lib #42
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
base: main
Are you sure you want to change the base?
feat: appsec lib #42
Conversation
|
Adding some context around this PR, initially when we created the AppSec component within CrowdSec we provided a base idea for what the creators of the integration should aim for. However, since we find issues or things we need to change such as the user-agent header we would have to go back and chase the integrations to update their code. (which isn't hard see traefik PR maxlerebourg/crowdsec-bouncer-traefik-plugin#149 pretty much issued a fix within hours of opening it, but if we forgot to inform them then its bad on our part, as humans we tend to forget) The aim of this PR is to take the responsibility of parsing the incoming client request to an appropriate request for AppSec component away from the integrator and we take some responsibility for setting the correct headers etc etc. The way to use this is create an api_key: {{ API_KEY }}
appsec_config:
url: http://localhost:7422/As you can see the appsec configuration is stored within its own directive and is an extension of the bouncer configuration so any integrator can do this var err error
configExpanded := csstring.StrictExpand(string(configMerged), os.LookupEnv)
reader := strings.NewReader(configExpanded)
bouncer := &csbouncer.StreamBouncer{}
err = bouncer.ConfigReader(reader)
if err != nil {
return err
}
appsec := &csbouncer.AppSec{}
err = appsec.ConfigReader(reader)
if err != nil {
return err
}Then simply the integrator can use the You can see examples shown within the example folder which is a simple http server which creates a middleware function |
| crowdsecAppsecUserAgent = "X-Crowdsec-Appsec-User-Agent" | ||
| ) | ||
|
|
||
| type Timeout struct { |
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.
Maybe rename to AppsecTimeout as Timeout might be misunderstood as also applying to the connection to LAPI itself ?
| ) | ||
|
|
||
| type Timeout struct { | ||
| ConnectTimeout *int `yaml:"connect_timeout"` |
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 should probably transform those to time.Duration (and let the user of the library handle the parsing/validation).
With the way things are currently, it's not possible to have a timeout lower than 1s, which is likely way too long for the majority of users.
| InsecureSkipVerify: *w.AppSecConfig.InsecureSkipVerify, | ||
| RootCAs: caCertPool, | ||
| }, | ||
| TLSHandshakeTimeout: time.Duration(*w.AppSecConfig.Timeout.TLSHandshakeTimeout) * time.Second, |
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.
Hardcoded second is way too long (see my comment above in the Timeout struct)
| func (w *AppSec) ParseClientReq(ctx context.Context, clientReq *http.Request, ipOverride string) (*http.Request, error) { | ||
| var req *http.Request | ||
|
|
||
| if clientReq.Body != nil && clientReq.ContentLength > 0 { |
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 need some kind of override for the body: in some situations, we do not want to read the entire body in memory (for example, a file upload of a few GB).
To keep things simple in the library, it could just be a true/false flag, and we let the user set it (main drawback being this means different bouncers are very likely to have different configuration/support).
Cleaner solution would be to provide additional configuration with for example body size, URL prefix, source IP and so on
(this is an issue we already have in the nginx appsec integration)
Opening a PR for thoughts if this should like in this package?