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.

Sign up for GitHub

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

Jump to bottom

Fix #658: Add possibility to configure error response behaviour. #848

Merged
merged 6 commits into from
Nov 2, 2017
Merged

Fix #658: Add possibility to configure error response behaviour. #848

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7041f40
Fix #658: Add possibility to configure error response behaviour.
aleksostapenko Oct 9, 2017
9056461
Fix code review comments (PR#848).
aleksostapenko Oct 18, 2017
aceb4ad
Implement configurable custom error pages (PR#848).
aleksostapenko Oct 25, 2017
4c5f17a
Documentation update (PR#848).
aleksostapenko Oct 26, 2017
27bfd26
Fix review comments (PR#848).
aleksostapenko Oct 31, 2017
d7052cc
Unit tests for 'tfw_strcpy_desc' function added (PR#848).
aleksostapenko Nov 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 123 additions & 28 deletions tempesta_fw/http.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@

#include "sync_socket.h"

extern unsigned short tfw_block_action_flags;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure that it's a good idea to refer this way to a configuration variable defined elsewhere. Perhaps, it's better to define a set of inline functions that would allow to access the contents of the variable.


#define RESP_BUF_LEN 128
static DEFINE_PER_CPU(char[RESP_BUF_LEN], g_buf);
int ghprio; /* GFSM hook priority. */
Expand Down Expand Up @@ -713,6 +715,34 @@ tfw_http_req_error(TfwSrvConn *srv_conn, TfwHttpReq *req,
__tfw_http_req_error(req, equeue, status, reason);
}

static inline void
tfw_http_error_resp_switch(TfwHttpReq *req, unsigned short status,
const char *reason)
{
switch(status) {
case 403:
tfw_http_send_403(req, reason);
break;
case 404:
tfw_http_send_404(req, reason);
break;
case 500:
tfw_http_send_500(req, reason);
break;
case 502:
tfw_http_send_502(req, reason);
break;
case 504:
tfw_http_send_504(req, reason);
break;
default:
TFW_WARN("Unexpected response error code: [%d]\n",
status);
tfw_http_send_500(req, reason);
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment #658 (comment) isn't addressed. Please load files named like 404.html or 500.html from etc directory and use the content as a response body. Otherwise a user just sees blank pages. Currently we load TLS certificates from the files, so it'd be good to generalize the routines and use the same code for TLS certificates and the error responses. Probably a new variables, $TFW_PATH/etc should be passed to tempesta_fw.ko - the path can be used for TLS certificates as well as for the error codes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected: configurable custom error pages added.


/*
* Forwarding of requests to a back end server is run under a lock
* on the server connection's forwarding queue. It's performed as
Expand All @@ -735,25 +765,8 @@ tfw_http_req_zap_error(struct list_head *equeue)

list_for_each_entry_safe(req, tmp, equeue, fwd_list) {
list_del_init(&req->fwd_list);
switch(req->httperr.status) {
case 404:
tfw_http_send_404(req, req->httperr.reason);
break;
case 500:
tfw_http_send_500(req, req->httperr.reason);
break;
case 502:
tfw_http_send_502(req, req->httperr.reason);
break;
case 504:
tfw_http_send_504(req, req->httperr.reason);
break;
default:
TFW_WARN("Unexpected response error code: [%d]\n",
req->httperr.status);
tfw_http_send_500(req, req->httperr.reason);
break;
}
tfw_http_error_resp_switch(req, req->httperr.status,
req->httperr.reason);
TFW_INC_STAT_BH(clnt.msgs_otherr);
}
}
Expand Down Expand Up @@ -1789,7 +1802,9 @@ __tfw_http_resp_fwd(TfwCliConn *cli_conn, struct list_head *ret_queue)
list_for_each_entry_safe(req, tmp, ret_queue, msg.seq_list) {
BUG_ON(!req->resp);
tfw_http_resp_init_ss_flags((TfwHttpResp *)req->resp, req);
if (tfw_cli_conn_send(cli_conn, (TfwMsg *)req->resp)) {
if (tfw_cli_conn_send(cli_conn, (TfwMsg *)req->resp) ||
(TFW_CONN_TYPE(cli_conn) & Conn_Suspected &&
Copy link
Contributor

Choose a reason for hiding this comment

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

tfw_http_error_resp_and_log() sets Conn_Suspected for the connection as well as TFW_HTTP_SUSPECTED for the requests and only the function sets TFW_HTTP_SUSPECTED, so there is no sense to check both the flags in the condition. The logic is at HTTP layer, so we should minimize interaction with other layers and work at HTTP layer only as much as possible: reducing inter-layer interaction is always a good thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is HTTP layer. I believe it's sufficient to check the flag for the HTTP layer. It's unnecessary to check flags in both layers as they both are set at the same time. Moreover, it breaks the whole idea of layers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@keshonok thank you for review!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected.

req->flags & TFW_HTTP_SUSPECTED)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please place opening curly brace at next line for multi-line conditions, otherwise the complex statement is hard to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected.

ss_close_sync(cli_conn->sk, true);
return;
}
Expand Down Expand Up @@ -2058,6 +2073,69 @@ tfw_http_req_set_context(TfwHttpReq *req)
return !req->vhost;
}

/*
* Function defines logging and response behaviour during
* detection of malformed or malicious messages. Can be
* called from both - client and server connection contexts.
*
* NOTE: @mark must be set only for client connection context
* because in this case we must mark client connection in
* special manner to delay its closing until transmission
* of error response will be finished.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why? If we find in a server connection context that a request is malicious (e.g. the call in tfw_http_resp_gfsm()), then there is no way to mark the request and the client connection as malicious and close the connection? It seems we're just passing many types of the attacks due to the limitation...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected: added the ability to mark a request and connection for closing from the server context.

*/
static inline void
tfw_http_error_resp_and_log(bool reply, bool nolog, TfwHttpReq *req,
unsigned short code, const char *msg, bool mark)
Copy link
Contributor

Choose a reason for hiding this comment

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

Frankly, I think this is way too many arguments. Perhaps, this can be done in a different way? Please see the comments below to the functions that call this one.

The most confusing thing here is that the notion of marking mixes the necessity of setting the appropriate flags for holding the connection and marking the faulty request with the necessity of adding the request to the seq queue. Is that really the same thing? I wouldn't say so. It seems that there may be cases when a request is in seq queue already, yet it must be marked as faulty and the connection must be put on hold as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected.

{
TfwCliConn *cli_conn = (TfwCliConn *)req->conn;
if (reply) {
if (mark) {
BUG_ON(req->flags & TFW_HTTP_SUSPECTED ||
TFW_CONN_TYPE(cli_conn) & Conn_Suspected);
TFW_CONN_TYPE(cli_conn) |= Conn_Suspected;
req->flags |= TFW_HTTP_SUSPECTED;
tfw_connection_unlink_msg(req->conn);
Copy link
Contributor

Choose a reason for hiding this comment

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

...and, yes, we can call tfw_connection_unlink_msg() only in client connection context, while we're sure that conn->req points to exactly this request. Probably the function must be split into 2 functions: for server and client contexts correspondingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected.

spin_lock(&cli_conn->seq_qlock);
list_add_tail(&req->msg.seq_list, &cli_conn->seq_queue);
spin_unlock(&cli_conn->seq_qlock);
}
tfw_http_error_resp_switch(req, code, msg);
}
else {
spin_lock(&cli_conn->seq_qlock);
if (unlikely(!list_empty(&req->msg.seq_list)))
list_del_init(&req->msg.seq_list);
spin_unlock(&cli_conn->seq_qlock);
tfw_http_conn_msg_free((TfwHttpMsg *)req);
}
if (!nolog)
TFW_WARN("Error response: %s, msg=%p conn=%p\n",
msg, req, req->conn);
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to print client's IP address - it's important information for security logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected.

}

/**
* Defines behaviour for malformed messages depending on configuration
* settings: sending response error messages and logging.
*/
static void
tfw_http_drop(TfwHttpReq *req, unsigned short code, const char *msg, bool mark)
{
bool reply = tfw_block_action_flags & TFW_BLOCK_ACTION_ERROR_REPLY;
bool nolog = tfw_block_action_flags & TFW_BLOCK_ACTION_ERROR_NOLOG;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is a common practice to use such boolean variables just to handle flags, but we don't like so long names (tfw_block_action_flags and TFW_BLOCK_ACTION_ERROR_REPLY) and prefer to use shorter names which can be used just in a function call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected.

tfw_http_error_resp_and_log(reply, nolog, req, code, msg, mark);
}

/**
* Do the same as 'tfw_http_drop' function, but for malicious messages.
*/
static void
tfw_http_block(TfwHttpReq *req, unsigned short code, const char *msg, bool mark)
{
bool reply = tfw_block_action_flags & TFW_BLOCK_ACTION_ATTACK_REPLY;
bool nolog = tfw_block_action_flags & TFW_BLOCK_ACTION_ATTACK_NOLOG;
tfw_http_error_resp_and_log(reply, nolog, req, code, msg, mark);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe these functions should be inlined. Also, I believe it would be better if you had separate versions for client and server contexts, and that may help to reduce the number of arguments. Plus there are other consideration that point in that direction, most significant of which is that the request should be put on seq queue first.

Copy link
Contributor

Choose a reason for hiding this comment

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

@keshonok thank you for the notes!

The modern compiler doesn't treat inline C keyword actually. This is why we have attributes noinline and always_inline, so there is no big deal with inline specifiers in the patch. (However, I also don't like as they are made in the patch). Also, technically speaking, 6 arguments are OK - we have exactly 6 registers for arguments in Linux/x86-64 calling convention. Meantime, I agree that at least mark argument isn't a good one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no sense to split expressions like tfw_blk_flags & TFW_BLK_ERR_REPLY to 2 lines. The original lines fit 80 characters in length, but the change makes them hard to read. It's usually has sense to separate function arguments, rather than make different parts of the same agrument look just like different arguments.


/**
* @return zero on success and negative value otherwise.
* TODO enter the function depending on current GFSM state.
Expand Down Expand Up @@ -2112,16 +2190,19 @@ tfw_http_req_process(TfwConn *conn, struct sk_buff *skb, unsigned int off)
BUG();
case TFW_BLOCK:
TFW_DBG2("Block invalid HTTP request\n");
tfw_http_conn_msg_free((TfwHttpMsg *)req);
TFW_INC_STAT_BH(clnt.msgs_parserr);
tfw_http_drop(req, 403, "failed to"
" parse request", true);
return TFW_BLOCK;
case TFW_POSTPONE:
r = tfw_gfsm_move(&conn->state,
TFW_HTTP_FSM_REQ_CHUNK, skb, off);
TFW_DBG3("TFW_HTTP_FSM_REQ_CHUNK return code %d\n", r);
if (r == TFW_BLOCK) {
tfw_http_conn_msg_free((TfwHttpMsg *)req);
TFW_INC_STAT_BH(clnt.msgs_filtout);
tfw_http_block(req, 403, "postponed"
" request has been"
" filtered out", true);
return TFW_BLOCK;
}
/*
Expand All @@ -2145,8 +2226,9 @@ tfw_http_req_process(TfwConn *conn, struct sk_buff *skb, unsigned int off)
TFW_DBG3("TFW_HTTP_FSM_REQ_MSG return code %d\n", r);
/* Don't accept any following requests from the peer. */
if (r == TFW_BLOCK) {
tfw_http_conn_msg_free((TfwHttpMsg *)req);
TFW_INC_STAT_BH(clnt.msgs_filtout);
tfw_http_block(req, 403, "parsed request"
" has been filtered out", true);
return TFW_BLOCK;
}

Expand All @@ -2159,7 +2241,9 @@ tfw_http_req_process(TfwConn *conn, struct sk_buff *skb, unsigned int off)

/* Assign the right Vhost for this request. */
if (tfw_http_req_set_context(req)) {
tfw_http_conn_msg_free((TfwHttpMsg *)req);
TFW_INC_STAT_BH(clnt.msgs_otherr);
tfw_http_drop(req, 500, "cannot find"
"Vhost for request", true);
Copy link
Contributor

@keshonok keshonok Oct 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It's been stated that special drop/block processing is for the cases where a request either malformed or malicious (i.e. filtered out). This particular case is more of an internal error nature (or maybe even a configuration). So why tfw_http_drop()? Do we need to close the connection in this case? Does the client need to know of this error in the browser?

There are still regular tfw_http_send_500() and tfw_http_send_502() that weren't converted to this block/drop style, and that's justifiable and there's no need to block a request or drop a connection in those case.

Copy link
Contributor Author

@aleksostapenko aleksostapenko Oct 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Does the client need to know of this error in the browser?

But in the opposite case - if we don't send error response - the client will see only hang pages.
Maybe it is more suitable for system administrator responsibility - to decide is it worth to send error responses in these cases.

return TFW_BLOCK;
}

Expand Down Expand Up @@ -2216,8 +2300,9 @@ tfw_http_req_process(TfwConn *conn, struct sk_buff *skb, unsigned int off)
*/
TFW_WARN("Not enough memory to create"
" a request sibling\n");
tfw_http_conn_msg_free((TfwHttpMsg *)req);
TFW_INC_STAT_BH(clnt.msgs_otherr);
tfw_http_drop(req, 500, "cannot create"
" sibling request", true);
Copy link
Contributor

@keshonok keshonok Oct 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Again, this seems like an internal error. Why the change to tfw_http_drop() Do we need to close the connection in this case? Does the client need to know of this error in the browser?

UPDATE: Yes, I guess we do, as otherwise we break the pairing of responses to requests. The other question still remains.

return TFW_BLOCK;
}
}
Expand Down Expand Up @@ -2414,7 +2499,7 @@ tfw_http_resp_gfsm(TfwHttpMsg *hmresp, struct sk_buff *skb, unsigned int off)
return TFW_BLOCK;
}

tfw_http_send_502(req, "response dropped: filtered out");
tfw_http_block(req, 502, "response blocked: filtered out", false);
tfw_http_conn_msg_free(hmresp);
TFW_INC_STAT_BH(serv.msgs_filtout);
return r;
Expand Down Expand Up @@ -2510,6 +2595,7 @@ tfw_http_resp_process(TfwConn *conn, struct sk_buff *skb, unsigned int off)
unsigned int skb_len = skb->len;
TfwHttpReq *bad_req;
TfwHttpMsg *hmresp;
bool filtout = false;

BUG_ON(!conn->msg);
BUG_ON(data_off >= skb_len);
Expand Down Expand Up @@ -2570,6 +2656,7 @@ tfw_http_resp_process(TfwConn *conn, struct sk_buff *skb, unsigned int off)
TFW_DBG3("TFW_HTTP_FSM_RESP_CHUNK return code %d\n", r);
if (r == TFW_BLOCK) {
TFW_INC_STAT_BH(serv.msgs_filtout);
filtout = true;
goto bad_msg;
}
/*
Expand Down Expand Up @@ -2658,8 +2745,16 @@ tfw_http_resp_process(TfwConn *conn, struct sk_buff *skb, unsigned int off)
return r;
bad_msg:
bad_req = tfw_http_popreq(hmresp);
if (bad_req)
tfw_http_send_500(bad_req, "response dropped: processing error");
if (bad_req) {
if (filtout)
tfw_http_block(bad_req, 502,
"response blocked:"
" filtered out", false);
else
tfw_http_drop(bad_req, 500,
"response dropped:"
" processing error", false);
}
tfw_http_conn_msg_free(hmresp);
return r;
}
Expand Down
7 changes: 7 additions & 0 deletions tempesta_fw/http.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ typedef struct {
/* URI has form http://authority/path, not just /path */
#define TFW_HTTP_URI_FULL 0x000400
#define TFW_HTTP_NON_IDEMP 0x000800
#define TFW_HTTP_SUSPECTED 0x001000
Copy link
Contributor

Choose a reason for hiding this comment

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

Like for Conn_Suspected (please see the comment below), I believe the name is misleading as the decision has been made already. Perhaps, simply TFW_HTTP_MSG_BAD? This name may mean either of a message being filtered out, or being malicious, or being erroneous.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't agree: TFW_HTTP_MSG_BAD doesn't say what bad is? Is it corrupted or malicious message? I'm OK with suspected.

Copy link
Contributor

@keshonok keshonok Oct 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

What I was trying to say was that in my opinion there's nothing that is suspected at this point. The decision had been made, the message IS now either filtered out, malicious, or erroneous. Due to decision that had been made already the client connection will be closed. Yet, all of these messages are now marked as suspected.

So I didn't understand what they are suspected of?

Suspicion is when we don't know what it is yet, it's either this or that, and there are further actions to come up with more detailed knowledge. But it doesn't work that way in the code. Again, the decision had been made at the time this flag is set, with the HTTP error response code and the error message. There's nothing that is suspected. Just the opposite, everything is pretty clear and definite.

Just a thought. :-)


/* Response flags */
#define TFW_HTTP_VOID_BODY 0x010000 /* Resp has no body */
Expand Down Expand Up @@ -474,6 +475,12 @@ typedef struct {
#define FOR_EACH_HDR_FIELD_FROM(pos, end, msg, soff) \
__FOR_EACH_HDR_FIELD(pos, end, msg, soff, (msg)->h_tbl->off)

/* Bit flags for block action behaviour. */
#define TFW_BLOCK_ACTION_ERROR_REPLY 0x0001
#define TFW_BLOCK_ACTION_ATTACK_REPLY 0x0002
#define TFW_BLOCK_ACTION_ERROR_NOLOG 0x0004
#define TFW_BLOCK_ACTION_ATTACK_NOLOG 0x0008

Copy link
Contributor

Choose a reason for hiding this comment

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

The flags are somewhat misleading: they're essentially single bit flag, so, given that there is no comments, a one can assume that they can ORed together, but that's not true. Probably they should be split to ATTACK, ERROR, REPLY, DROP (this one seems just 0, i.e. default), and NOLOG. Also usually shorter names are appreciated, e.g. TFW_BLK_ACT_ERR_REPLY or something like that - there is just no sense to keep so long prefix TFW_BLOCK_ACTION_ which is just the same for all the flags.

/* Get current timestamp in secs. */
static inline time_t
tfw_current_timestamp(void)
Expand Down
12 changes: 10 additions & 2 deletions tempesta_fw/sock.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,11 @@ ss_tcp_process_skb(struct sock *sk, struct sk_buff *skb, int *processed)
*/
BUG_ON(conn == NULL);

if (SS_CONN_TYPE(sk) & Conn_Suspected) {
__kfree_skb(skb);
continue;
}

r = SS_CALL(connection_recv, conn, skb, offset);

if (r < 0) {
Expand Down Expand Up @@ -858,10 +863,13 @@ ss_tcp_data_ready(struct sock *sk)
TFW_ERR("error data in socket %p\n", sk);
}
else if (!skb_queue_empty(&sk->sk_receive_queue)) {
if (ss_tcp_process_data(sk)) {
if (ss_tcp_process_data(sk) &&
!(SS_CONN_TYPE(sk) & Conn_Suspected)) {
/*
* Drop connection in case of internal errors,
* banned packets, or FIN in the received packet.
* banned packets, or FIN in the received packet,
* and only if we don't wait for sending the error
* response to client (Conn_Suspected bit set).
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering the comment below for the name of the flag (like Conn_OnHold), the comment should not be that specific about the reasons for the action. It's better to have something a little more abstracted, perhaps something like this: and only if it's not on hold until explicitly closed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected.

*
* ss_linkerror() is responsible for calling
* application layer connection closing callback.
Expand Down
Loading