Skip to content

Commit

Permalink
Do not use main/server contexts for creating/merging configuration
Browse files Browse the repository at this point in the history
  • Loading branch information
@defanator
defanator committed Nov 29, 2018
1 parent 71ede63 commit e00e2cc
Showing 1 changed file with 50 additions and 118 deletions.
168 changes: 50 additions & 118 deletions src/ngx_http_modsecurity_module.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,9 @@
#include <ngx_http.h>

static ngx_int_t ngx_http_modsecurity_init(ngx_conf_t *cf);
static void *ngx_http_modsecurity_create_main_conf(ngx_conf_t *cf);
static void *ngx_http_modsecurity_create_conf(ngx_conf_t *cf);
static char *ngx_http_modsecurity_merge_loc_conf(ngx_conf_t *cf, void *parent, void *child);
static char *ngx_http_modsecurity_merge_srv_conf(ngx_conf_t *cf, void *parent, void *child);
static char *ngx_http_modsecurity_merge_conf(ngx_conf_t *cf, void *parent, void *child);
static void ngx_http_modsecurity_config_cleanup(void *data);
static char *ngx_http_modsecurity_init_main_conf(ngx_conf_t *cf, void *conf);


/*
Expand Down Expand Up @@ -234,31 +231,30 @@ ngx_http_modsecurity_cleanup(void *data)
ngx_inline ngx_http_modsecurity_ctx_t *
ngx_http_modsecurity_create_ctx(ngx_http_request_t *r)
{
ngx_http_modsecurity_ctx_t *ctx;
ngx_http_modsecurity_conf_t *loc_cf = NULL;
ngx_http_modsecurity_conf_t *cf = NULL;
ngx_pool_cleanup_t *cln = NULL;
ngx_str_t s;
ngx_str_t s;
ngx_pool_cleanup_t *cln;
ngx_http_modsecurity_ctx_t *ctx;
ngx_http_modsecurity_conf_t *mcf;

ctx = ngx_pcalloc(r->pool, sizeof(ngx_http_modsecurity_ctx_t));
if (ctx == NULL)
{
dd("failed to allocate memory for the context.");
return NULL;
}
cf = ngx_http_get_module_main_conf(r, ngx_http_modsecurity_module);
loc_cf = ngx_http_get_module_loc_conf(r, ngx_http_modsecurity_module);

dd("creating transaction with the following rules: '%p' -- ms: '%p'", loc_cf->rules_set, cf->modsec);
mcf = ngx_http_get_module_loc_conf(r, ngx_http_modsecurity_module);

dd("creating transaction with the following rules: '%p' -- ms: '%p'", mcf->rules_set, mcf->modsec);

if (loc_cf->transaction_id) {
if (ngx_http_complex_value(r, loc_cf->transaction_id, &s) != NGX_OK) {
if (mcf->transaction_id) {
if (ngx_http_complex_value(r, mcf->transaction_id, &s) != NGX_OK) {
return NGX_CONF_ERROR;
}
ctx->modsec_transaction = msc_new_transaction_with_id(cf->modsec, loc_cf->rules_set, (char *) s.data, r->connection->log);
ctx->modsec_transaction = msc_new_transaction_with_id(mcf->modsec, mcf->rules_set, (char *) s.data, r->connection->log);

} else {
ctx->modsec_transaction = msc_new_transaction(cf->modsec, loc_cf->rules_set, r->connection->log);
ctx->modsec_transaction = msc_new_transaction(mcf->modsec, mcf->rules_set, r->connection->log);
}

dd("transaction created");
Expand Down Expand Up @@ -437,32 +433,32 @@ static ngx_command_t ngx_http_modsecurity_commands[] = {


static ngx_http_module_t ngx_http_modsecurity_ctx = {
NULL, /* preconfiguration */
ngx_http_modsecurity_init, /* postconfiguration */
NULL, /* preconfiguration */
ngx_http_modsecurity_init, /* postconfiguration */

ngx_http_modsecurity_create_main_conf, /* create main configuration */
ngx_http_modsecurity_init_main_conf, /* init main configuration */
NULL, /* create main configuration */
NULL, /* init main configuration */

ngx_http_modsecurity_create_conf, /* create server configuration */
ngx_http_modsecurity_merge_srv_conf, /* merge server configuration */
NULL, /* create server configuration */
NULL, /* merge server configuration */

ngx_http_modsecurity_create_conf, /* create location configuration */
ngx_http_modsecurity_merge_loc_conf /* merge location configuration */
ngx_http_modsecurity_create_conf, /* create location configuration */
ngx_http_modsecurity_merge_conf /* merge location configuration */
};


ngx_module_t ngx_http_modsecurity_module = {
NGX_MODULE_V1,
&ngx_http_modsecurity_ctx, /* module context */
ngx_http_modsecurity_commands, /* module directives */
NGX_HTTP_MODULE, /* module type */
NULL, /* init master */
NULL, /* init module */
NULL, /* init process */
NULL, /* init thread */
NULL, /* exit thread */
NULL, /* exit process */
NULL, /* exit master */
&ngx_http_modsecurity_ctx, /* module context */
ngx_http_modsecurity_commands, /* module directives */
NGX_HTTP_MODULE, /* module type */
NULL, /* init master */
NULL, /* init module */
NULL, /* init process */
NULL, /* init thread */
NULL, /* exit thread */
NULL, /* exit process */
NULL, /* exit master */
NGX_MODULE_V1_PADDING
};

Expand Down Expand Up @@ -545,51 +541,15 @@ ngx_http_modsecurity_init(ngx_conf_t *cf)


static void *
ngx_http_modsecurity_create_main_conf(ngx_conf_t *cf)
ngx_http_modsecurity_create_conf(ngx_conf_t *cf)
{
ngx_http_modsecurity_conf_t *conf;
ngx_pool_cleanup_t *cln;
ngx_http_modsecurity_conf_t *conf;

ngx_log_error(NGX_LOG_NOTICE, cf->log, 0, MODSECURITY_NGINX_WHOAMI);

/* ngx_pcalloc already sets all of this scructure to zeros. */
conf = ngx_http_modsecurity_create_conf(cf);

if (conf == NULL || conf == NGX_CONF_ERROR) {
dd("failed to allocate space for the ModSecurity configuration");
return NGX_CONF_ERROR;
}

dd ("conf crated at: '%p'", conf);

/* Create our ModSecurity instace */
conf->modsec = msc_init();
if (conf->modsec == NULL)
{
dd("failed to create the ModSecurity instance");
return NGX_CONF_ERROR;
}

/* Provide our connector information to LibModSecurity */
msc_set_connector_info(conf->modsec, MODSECURITY_NGINX_WHOAMI);
msc_set_log_cb(conf->modsec, ngx_http_modsecurity_log);

return conf;
}


static char *ngx_http_modsecurity_init_main_conf(ngx_conf_t *cf, void *conf)
{
dd("modsec main conf init. Loaded rules:");

return NGX_CONF_OK;
}


static void *ngx_http_modsecurity_create_conf(ngx_conf_t *cf)
{
ngx_pool_cleanup_t *cln = NULL;
ngx_http_modsecurity_conf_t *conf = (ngx_http_modsecurity_conf_t *)
ngx_pcalloc(cf->pool, sizeof(ngx_http_modsecurity_conf_t));
conf = (ngx_http_modsecurity_conf_t *) ngx_pcalloc(cf->pool,
sizeof(ngx_http_modsecurity_conf_t));

if (conf == NULL)
{
Expand Down Expand Up @@ -619,58 +579,30 @@ static void *ngx_http_modsecurity_create_conf(ngx_conf_t *cf)
dd("failed to create the ModSecurity configuration cleanup");
return NGX_CONF_ERROR;
}

cln->handler = ngx_http_modsecurity_config_cleanup;
cln->data = conf;

return conf;
}

dd ("conf created at: '%p'", conf);

static char *
ngx_http_modsecurity_merge_srv_conf(ngx_conf_t *cf, void *parent, void *child)
{
ngx_http_modsecurity_conf_t *p = parent;
ngx_http_modsecurity_conf_t *c = child;
#if defined(MODSECURITY_DDEBUG) && (MODSECURITY_DDEBUG)
ngx_http_core_srv_conf_t *clcf = ngx_http_conf_get_module_srv_conf(cf, ngx_http_core_module);
#endif
int rules;
const char *error = NULL;

dd("merging srv config [%s] - parent: '%p' child: '%p'",
ngx_str_to_char(clcf->server_name, cf->pool), parent,
child);
dd(" state - parent: '%d' child: '%d'",
(int) p->enable, (int) c->enable);

ngx_conf_merge_value(c->enable, p->enable, 0);
ngx_conf_merge_value(c->sanity_checks_enabled, p->sanity_checks_enabled, 0);
ngx_conf_merge_ptr_value(c->transaction_id, p->transaction_id, NULL);

#if defined(MODSECURITY_DDEBUG) && (MODSECURITY_DDEBUG)
dd("PARENT RULES");
msc_rules_dump(p->rules_set);
dd("CHILD RULES");
msc_rules_dump(c->rules_set);
#endif
/* Create our ModSecurity instance */
conf->modsec = msc_init();
if (conf->modsec == NULL)
{
dd("failed to create the ModSecurity instance");
return NGX_CONF_ERROR;
}

rules = msc_rules_merge(c->rules_set, p->rules_set, &error);
/* Provide our connector information to LibModSecurity */
msc_set_connector_info(conf->modsec, MODSECURITY_NGINX_WHOAMI);
msc_set_log_cb(conf->modsec, ngx_http_modsecurity_log);

if (rules < 0) {
return strdup(error);
}
dd(" state - this: '%d'",
(int) c->enable);
#if defined(MODSECURITY_DDEBUG) && (MODSECURITY_DDEBUG)
dd("NEW CHIELD RULES");
msc_rules_dump(c->rules_set);
#endif
return NGX_CONF_OK;
return conf;
}


static char *
ngx_http_modsecurity_merge_loc_conf(ngx_conf_t *cf, void *parent, void *child)
ngx_http_modsecurity_merge_conf(ngx_conf_t *cf, void *parent, void *child)
{
ngx_http_modsecurity_conf_t *p = parent;
ngx_http_modsecurity_conf_t *c = child;
Expand Down Expand Up @@ -704,7 +636,7 @@ ngx_http_modsecurity_merge_loc_conf(ngx_conf_t *cf, void *parent, void *child)
}

#if defined(MODSECURITY_DDEBUG) && (MODSECURITY_DDEBUG)
dd("NEW CHIELD RULES");
dd("NEW CHILD RULES");
msc_rules_dump(c->rules_set);
#endif
return NGX_CONF_OK;
Expand Down

4 comments on commit e00e2cc

@defanator
Copy link
Owner Author

Choose a reason for hiding this comment

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

In comparison with the previous implementation, the msc_init() is called for every configuration context, but this does not lead to significant increase in memory consumption. Here's the results of nginx running with 100 server blocks like this:

server {
    server_name host1;
    listen 80;
    location / {
        modsecurity on;
        modsecurity_rules_file /etc/nginx/modsec/main.conf;
        proxy_pass http://local;
    }
}

where /etc/nginx/modsec/main.conf is:

include /etc/nginx/modsec/modsecurity.conf

# OWASP CRS v3.1.0 rules
include /etc/nginx/modsec/owasp-crs/crs-setup.conf
include /etc/nginx/modsec/owasp-crs/rules/*.conf

So, previous (old) implementation:

  PID USER      PR  NI    VIRT    RES    SHR S  %CPU %MEM     TIME+ COMMAND                                                                                                                      
15445 nginx     20   0 2068956 1.784g   1432 S   0.0 22.9   0:00.02 nginx                                                                                                                        
15446 nginx     20   0 2068956 1.784g   1432 S   0.0 22.9   0:00.01 nginx

New simplified implementation:

  PID USER      PR  NI    VIRT    RES    SHR S  %CPU %MEM     TIME+ COMMAND                                                                                                                      
28530 nginx     20   0 2071684 1.791g   1684 S   0.0 23.0   0:00.04 nginx                                                                                                                        
28531 nginx     20   0 2071684 1.791g   1684 S   0.0 23.0   0:00.04 nginx

@defanator
Copy link
Owner Author

Choose a reason for hiding this comment

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

nginx startup time is also almost the same for the above configuration, previous implementation:

# time -p nginx -t
nginx: the configuration file /etc/nginx/nginx.conf syntax is ok
nginx: configuration file /etc/nginx/nginx.conf test is successful
real 75.91
user 70.82
sys 5.06

New implementation:

# time -p nginx -t
nginx: the configuration file /etc/nginx/nginx.conf syntax is ok
nginx: configuration file /etc/nginx/nginx.conf test is successful
real 71.22
user 67.77
sys 3.44

@defanator
Copy link
Owner Author

Choose a reason for hiding this comment

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

@zimmerle @victorhora would love to hear you thoughts on what is the ideal pattern of calling msc_init() from a connector, i.e. should a new instance be "global", or we can create as many instances as we'd like to? (for various contexts / configurations, etc)

@defanator
Copy link
Owner Author

Choose a reason for hiding this comment

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

@zimmerle @victorhora anyway, I checked the library's code and decided to return to the scenario when an instance is being created only once (in main/http context, in nginx terminology), see this one for details: owasp-modsecurity@7b8f1ef

Please sign in to comment.