Prohibit DELETE method in default configuration #2058 #2078

biathlon3 · 2024-03-13T09:13:28Z

My first attempt, I am not sure that it is the best solution, but it seems to work.

EvgeniiMekhanik · 2024-03-14T08:11:45Z

fw/http_limits.c

- }
- __FRANG_FSM_MOVE(Frang_Req_Hdr_UriLen);
- }
+ T_FSM_STATE(Frang_Req_Hdr_Method) {


Invalid tabulation.

Sorry, I have changed my editor's behavior.

EvgeniiMekhanik · 2024-03-14T08:12:09Z

fw/http_limits.c

+
+ if (!f_cfg->http_methods_mask) {
+ /* If we have a default value, we set all bits to 1 except DELETE */
+ f_cfg->http_methods_mask = ~(1 << TFW_HTTP_METH_DELETE);


I think we should also prohibit PUT method here

EvgeniiMekhanik · 2024-03-14T08:19:10Z

You should also fix tests, where DELETE method is used by adding appropriate DELETE method to frang config

krizhanovsky

Definitely need more work and it seems the issue isn't so straightforward as I thought, so we need a good discussion on this.

Also please make pull requests from our repo, not your own - this way we can just switch between branches during review instead of cloning repos of each developers.

Finally, please don't forget to update wiki about allowed methods by default. Probably it also makes sense to describe in the wiki how DELETE behaves with the cache.

fw/http_limits.c

krizhanovsky

LGTM with just a small coding style cleanup

krizhanovsky · 2024-03-21T18:17:21Z

fw/vhost.c

+
+#define TFW_FRANG_HTTP_METHODS_MASK_DEFAULT ((1 << TFW_HTTP_METH_GET) | \
+ (1 << TFW_HTTP_METH_HEAD) | \
+ (1 << TFW_HTTP_METH_POST))
+


Suggested change

#define TFW_FRANG_HTTP_METHODS_MASK_DEFAULT ((1 << TFW_HTTP_METH_GET) | \

(1 << TFW_HTTP_METH_HEAD) | \

(1 << TFW_HTTP_METH_POST))

#define TFW_FRANG_HTTP_METHODS_MASK_DEFAULT ((1 << TFW_HTTP_METH_GET) | \

(1 << TFW_HTTP_METH_HEAD) | \

(1 << TFW_HTTP_METH_POST))

We almost never use 2 empty lines in C/C++ code and we use alignments (tabs plus spaces)

const-t

I would like to suggest another solution. In TfwCfgSpec we can specify default values for each directive.
Example:

{
		.name = "http_methods",
		.deflt = "get post head",
		.handler = tfw_cfgop_frang_http_methods,
		.allow_reconfig = true,
	},

Field deflt it's just value, same as in config, that will be parsed and assigned to directive. Please, change TfwCfgSpec for each context.
Also, I believe, we don't need to remove already made changes, we will treat empty value, as defualt. Or we can remove these changes, and treat empty value of this directive as error - this looks more clear for me. Up to you.

const-t · 2024-03-29T15:49:01Z

fw/vhost.c

@@ -3220,7 +3237,7 @@ static TfwCfgSpec tfw_vhost_internal_specs[] = {
 .handler = tfw_cfg_handle_children,
 .cleanup = tfw_cfgop_frang_cleanup,
 .dest = tfw_vhost_frang_specs,
- .allow_none = true,
+ .allow_none = false,


Why do we set allow_none to false for frang_limits?

If we leave allow_none as true, the default configuration won't be read in spec_finish_handling(TfwCfgSpec specs[]).
It was the reason why I wrote my previous version.

This seems wrong. Are you sure? There are plenty of places in the code with both .allow_none = true and .deflt set to some value.

Yes, guys, I was wrong.

I tested it more careful and found that with .allow_none = true, in this position, it stops using default configuration when vhost is configured in tempesta_fw.cfg.
Looks like I hastened to agree with you. And we need to use .allow_none = false or change behavior of spec_finish_handling(TfwCfgSpec specs[]) or change somthing else.

const-t · 2024-03-29T16:06:06Z

fw/vhost.c

+ return -EINVAL;
+ }
+
+ T_ERR_NL("frang: http_methods should contain at least one "


Looks like dead code and we never reach this place. If we have ce->dflt_value == false it means value was specified in config and now we know that this value is empty. Therefore, we will always return in if (!ce->dflt_value). Is there a case when ce->dflt_value == true but ce->val_n == false?

This situation is possible when we have " http_methods ;" in config

In my understanding when we have " http_methods ;" then we have ce->dflt_value == false and ce->val_n == false in other cases we will have ce->val_n > 0. For example: http_methods ""; or http_methods " "; will have ce->val_n > 0 and will disallowed in TFW_CFG_ENTRY_FOR_EACH_VAL. I can't find another value for http_methods where ce->val_n == 0 except " http_methods ;". Do you know such value for http_methods?

Yes, when we have
.name = "http_methods",
.deflt = "",
in the code

Now we doesn't, and I believe we must not maintain this. If you warring about this, it would be better to write a comment in appropriate place.

const-t · 2024-04-04T07:56:58Z

Please remove todo from http_limits.c, when you will rebase.
TODO: Remove when PR #2078 merged

biathlon3 · 2024-04-10T06:26:48Z

fw/http_limits.c

@@ -1142,14 +1140,13 @@ frang_http_req_process(FrangAcc *ra, TfwConn *conn, TfwFsmData *data,

 /* Ensure that HTTP request method is one of those defined by a user. */
 T_FSM_STATE(Frang_Req_Hdr_Method) {
- if (f_cfg->http_methods_mask) {


I removed checking f_cfg->http_methods_mask because this checking breaks new logic.

configuration tempesta-tech#2058

configuration part. And everything was prohibited except GET, HEAD and POST.

and the request was dropped by frang after that.

krizhanovsky · 2024-05-14T16:15:31Z

Please implement a workaround for #2119 with a TODO comment

Fixed reading default value of http_strict_host_checking. Chenged frang inheritance for locatians in default vhost.

biathlon3 · 2024-05-21T12:09:20Z

Please implement a workaround for #2119 with a TODO comment

I manage to do it in normal way.

const-t · 2024-05-31T10:21:06Z

A want to ask you to check following(and others similar conditions) part in tfw_http_req_process():

if (res.type == TFW_HTTP_RES_REDIR) {
tfw_http_req_redir(req, res.redir.resp_code, &res.redir);
return T_OK;
}

In this case we don't freed split skb as well. To check it just use redirects.

http_chain redirects {
    # Absolute path
    uri == "/company.html" -> 301 = https://tempesta-tech.com/company;

    # Use variables
    uri == "/blog/" -> 308 = https://$host/$request_uri/;

    # Relative path
    uri == "*/services.html" -> 303 = /services;

    # Temporal redirection for the default index page only to a temporal landing page.
    uri == "/" -> 307 = /c++-services;
}

krizhanovsky · 2024-05-31T10:40:46Z

fw/tls.c

@@ -1128,6 +1128,11 @@ tfw_tls_start(void)
 return 0;
 }

+bool tfw_tls_get_allow_any_sni_reconfig(void)


Suggested change

bool tfw_tls_get_allow_any_sni_reconfig(void)

bool

tfw_tls_get_allow_any_sni_reconfig(void)

const-t

In general looks good, but could you add a little bit description to the last commit.

Chenged frang inheritance for locatians in default vhost

Why it's changed, how it's changed. Also looks like you fixed live-reconfig logic for frang, describe it also in the commit message.

const-t · 2024-05-31T10:09:29Z

fw/http.c

 "parsed request has been filtered out",
 HTTP2_ECODE_PROTO);
+ if (skb) {


Seems we can remove this unnecessary condition if (skb) then just assign skb and return r instead of T_DROP.

We should not change return code here, because it is used to determine how Tempesta close connection.
We should also fix this memory leak in all other places in this function.

Code reworked. I tried to make it universal, but if you think it is not the best way, I will rework it for special cases only.

EvgeniiMekhanik

Change fixing memory leak

EvgeniiMekhanik · 2024-06-06T12:12:48Z

etc/tempesta_fw.conf

+# http_uri_len 0;
+# http_body_len 1073741824;
+# http_hdr_len 0;
+# http_header_cnt 0;


50 is default value for this setting. Please also check all other settings. concurrent_tcp_connections - 1000 is default settings

EvgeniiMekhanik · 2024-06-06T13:11:43Z

fw/vhost.c

@@ -2141,7 +2197,7 @@ tfw_cfgop_frang_strict_host_checking(TfwCfgSpec *cs, TfwCfgEntry *ce)
 int r;
 FrangVhostCfg *cfg = tfw_cfgop_frang_get_cfg();

- if (ce->dflt_value && cfg->http_strict_host_checking)
+ if (ce->dflt_value && !cfg->http_strict_host_checking)


We need to discuss this place. If I understand it correctly we have the same problem for http_body_len also and may be some other settings.

frang_limits { http_body_len 0; } frang_limits { }

The second one frang_limits has default http_body_len == 1073741824, should we override previosly set http_body_len or not?

I returned the old behavior, only explicit parameters should be overridden.

EvgeniiMekhanik · 2024-06-06T14:20:22Z

fw/vhost.c

@@ -1903,6 +1911,11 @@ __tfw_cfgop_frang_http_methods(TfwCfgSpec *cs, TfwCfgEntry *ce,
 methods_mask |= (1UL << method_id);
 }

+ if (!ce->val_n && !ce->dflt_value) {


You don't need to check !ce->dflt_value here, because if it is dflt_value vam_n is always 3 (get post head). This check is better to do before TFW_CFG_ENTRY_FOR_EACH_VAL loop

TFW_CFG_CHECK_VAL_N(>, 0, cs, ce);
TFW_CFG_CHECK_NO_ATTRS(cs, ce);

is right way. Tempesta interpret get=hhhsdsd as attribute and silently ignore such invalid configuration

EvgeniiMekhanik · 2024-06-06T14:49:03Z

fw/vhost.c

 .allow_reconfig = true,
 },
 {
 .name = "http_body_len",
 .deflt = "1073741824", /* 1 Gb. */
- .handler = tfw_cfgop_frang_body_len,
+ .handler = tfw_cfgop_frang_glob_set_long,


It is incorrect to use tfw_cfgop_frang_glob_set_long here. If we have global frang limits like below:
frang_limits {
http_body_len 0;
}
frang_limits {
}

We should not override http_body_len 0; in the second config as i understand the idea in all other places.
I thinkn that using tfw_cfgop_frang_glob_set_int for tfw_frang_glob_reconfig.http_hdr_cnt is also incorrect. Same as for concurrent_tcp_connections

I returned the old behavior, only explicit parameters should be overridden.

EvgeniiMekhanik · 2024-06-06T14:58:12Z

fw/vhost.c

 .allow_reconfig = true,
 },
 {
 .name = "http_strict_host_checking",
 .deflt = "true",
- .handler = tfw_cfgop_frang_strict_host_checking,
+ .handler = tfw_cfgop_frang_glob_set_bool,


We can't use tfw_cfgop_frang_glob_set_bool here because default value is true.
frang_limits {
http_strict_host_checking false;
}
frang_limits {
}

We have http_strict_host_checking true, but should have false

Please add tests for all this changes! We need to cover all this cases because I found a lot of mistakes here. you should check several frang_limits on one level and how it is work.

EvgeniiMekhanik · 2024-06-06T16:52:46Z

fw/vhost.c

- if (tfw_frang_cfg_inherit(loc->frang_cfg, vhost->loc_dflt->frang_cfg))
- return NULL;
+ if (strncasecmp(vhost->name.data, TFW_VH_DFT_NAME, vhost->name.len)) {
+ if (tfw_frang_cfg_inherit(loc->frang_cfg,


Please look at this function tfw_frang_cfg_inherit we have BUG in it.
if (!curr->http_ct_vals)
we only set r = -ENOMEM and do not return. Then we memcpy(curr->http_ct_vals, from->http_ct_vals, sz);

Add else to avoid using curr->http_ct_vals after error.

EvgeniiMekhanik · 2024-06-07T07:58:37Z

fw/http_limits.c

- * Do not perform any SNI<=>authority validation if
- * tls_match_any_server_name is set.
- */
- if (tfw_tls_allow_any_sni)


Why do you delete this check? Previously when tls_match_any_server_name is true, we return T_OK. Why this behavior is changed?

When tfw_tls_allow_any_sni is true, it means connection with empty sni or sni not listed in our vhost is "routed" to default vhost. In other words tfw_tls_allow_any_sni is the way to enable default vhost and it's not related to other vhosts. But when we have this check here and we want to check SNI vs authority for some vhosts and don't check for other we can't do this, because tfw_tls_allow_any_sni will override it.

Now I would prefer to introduce new knob only for domain fronting, that must be disabled by default. It will depends on tfw_tls_allow_any_sni also, but in the future we will remove default vhost and tfw_tls_allow_any_sni and will check SNI only when vhost is found by http tables.

EvgeniiMekhanik · 2024-06-07T08:06:56Z

fw/vhost.c

+static int
+tfw_cfgop_frang_glob_rsp_code_block(TfwCfgSpec *cs, TfwCfgEntry *ce)
+{
+ FrangVhostCfg *cfg = &tfw_frang_vhost_reconfig;


Why we can't use tfw_cfgop_frang_rsp_code_block. The only difference that here we directly set FrangVhostCfg *cfg = &tfw_frang_vhost_reconfig; instead of FrangVhostCfg *cfg = tfw_cfgop_frang_get_cfg(); ?

It was attempt to solve strange location inheritance for default vhost. I returned it back now because my code had changed overriding.

EvgeniiMekhanik · 2024-06-07T08:09:00Z

fw/vhost.c

+ kfree(cfg->http_resp_code_block);
+ cfg->http_resp_code_block = NULL;
+ }
+ return __tfw_cfgop_frang_rsp_code_block(cs, ce, cfg);


Can you also check __tfw_cfgop_frang_rsp_code_block. I see that we return -EINVAL after memory allocation without
freeing previously allocated cb = kzalloc(sizeof(FrangHttpRespCodeBlock), GFP_KERNEL); Is there memory leak or we will free it later?

It assigns to conf->http_resp_code_block and will be freed by __tfw_frang_clean() in case of error.

Only GET, HEAD and POST are allowed by default. Global frang initialization happens before reading config file. Global frang was copied before initialization to prevent using old config during live-reconfiguration. Fixed memory leak when skb was split in tfw_http_req_process(). Fixed bug in tfw_frang_cfg_inherit(). tls_match_any_server_name is now permited only globaly. Now tls_match_any_server_name and http_strict_host_checking are used independandly. Changed inheritance for locations of default vhost. Added print frang configuration.

krizhanovsky requested review from krizhanovsky and EvgeniiMekhanik March 13, 2024 14:16

EvgeniiMekhanik reviewed Mar 14, 2024

View reviewed changes

krizhanovsky requested a review from const-t March 16, 2024 19:47

krizhanovsky requested changes Mar 16, 2024

View reviewed changes

fw/http_limits.c Outdated Show resolved Hide resolved

fw/http_limits.c Outdated Show resolved Hide resolved

fw/http_limits.c Show resolved Hide resolved

fw/http_limits.c Show resolved Hide resolved

krizhanovsky approved these changes Mar 21, 2024

View reviewed changes

const-t requested changes Mar 22, 2024

View reviewed changes

const-t reviewed Mar 29, 2024

View reviewed changes

krizhanovsky mentioned this pull request Apr 4, 2024

frang: tests for vhost/location and default config tempesta-tech/tempesta-test#596

Closed

2 tasks

biathlon3 force-pushed the master branch from 668e1b7 to d5f8ad6 Compare April 8, 2024 14:38

biathlon3 commented Apr 10, 2024

View reviewed changes

biathlon3 force-pushed the master branch from adc1698 to 53704b9 Compare April 23, 2024 09:11

RomanBelozerov mentioned this pull request Apr 24, 2024

frang: http2 tests for http_trailer_split_allowed tempesta-tech/tempesta-test#614

Merged

const-t approved these changes May 6, 2024

View reviewed changes

biathlon3 added 8 commits May 6, 2024 17:18

Prohibit DELETE method in default configuration tempesta-tech#2058

68179e6

Correction of first version of Prohibit DELETE method in default

bb339b1

configuration tempesta-tech#2058

Setting http_methods_mask was moved from the checking part to the

9612009

configuration part. And everything was prohibited except GET, HEAD and POST.

The default value of http_methods was specified In TfwCfgSpec.

e57b018

Correcting my mistake

89f37d2

Coming back to previous state

7731167

Changing the config description to match the new behavior.

23baae1

TODO and redundant checking during configuration were removed.

480dedf

biathlon3 force-pushed the master branch from 53704b9 to 480dedf Compare May 6, 2024 13:18

biathlon3 and others added 2 commits May 7, 2024 08:36

Merge branch 'tempesta-tech:master' into master

cf71fec

Fixed memleak when skb was split

c4e77fe

and the request was dropped by frang after that.

biathlon3 requested a review from const-t May 9, 2024 08:35

krizhanovsky mentioned this pull request May 14, 2024

Only the first location uses the default frang configuration. #2119

Open

biathlon3 and others added 2 commits May 21, 2024 14:53

Merge branch 'tempesta-tech:master' into master

e1ace57

Forced reading default frang

b035898

Fixed reading default value of http_strict_host_checking. Chenged frang inheritance for locatians in default vhost.

krizhanovsky reviewed May 31, 2024

View reviewed changes

const-t approved these changes May 31, 2024

View reviewed changes

EvgeniiMekhanik requested changes Jun 6, 2024

View reviewed changes

EvgeniiMekhanik reviewed Jun 6, 2024

View reviewed changes

EvgeniiMekhanik reviewed Jun 7, 2024

View reviewed changes

biathlon3 and others added 2 commits June 10, 2024 13:10

Merge branch 'tempesta-tech:master' into master

709cdbe

	bool tfw_tls_get_allow_any_sni_reconfig(void)
	bool
	tfw_tls_get_allow_any_sni_reconfig(void)

Prohibit DELETE method in default configuration #2058 #2078

Are you sure you want to change the base?

Prohibit DELETE method in default configuration #2058 #2078

Conversation

biathlon3 commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvgeniiMekhanik commented Mar 14, 2024

krizhanovsky left a comment

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

const-t left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

const-t commented Apr 4, 2024

Choose a reason for hiding this comment

krizhanovsky commented May 14, 2024

biathlon3 commented May 21, 2024

const-t commented May 31, 2024

Choose a reason for hiding this comment

const-t left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvgeniiMekhanik Jun 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvgeniiMekhanik left a comment

Choose a reason for hiding this comment

EvgeniiMekhanik Jun 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvgeniiMekhanik Jun 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvgeniiMekhanik Jun 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

const-t left a comment •

edited

const-t left a comment •

edited

EvgeniiMekhanik Jun 6, 2024 •

edited

EvgeniiMekhanik Jun 6, 2024 •

edited

EvgeniiMekhanik Jun 6, 2024 •

edited

EvgeniiMekhanik Jun 6, 2024 •

edited