From cf1de559027ce688b35ebb1a1779944aa2ec31fb Mon Sep 17 00:00:00 2001 From: Tero Heinonen Date: Wed, 10 Feb 2016 11:20:35 +0200 Subject: [PATCH 1/3] CoAP service refactoring: - mbedtls timer implementation changed to use event timer. - Connection handler to return all error cases --- source/coap_connection_handler.c | 142 +++++++++++++++++-------------- source/coap_security_handler.c | 24 +++--- 2 files changed, 90 insertions(+), 76 deletions(-) diff --git a/source/coap_connection_handler.c b/source/coap_connection_handler.c index 691bc25e63..ea6e770ec9 100644 --- a/source/coap_connection_handler.c +++ b/source/coap_connection_handler.c @@ -10,7 +10,7 @@ #include "nsdynmemLIB.h" #include "socket_api.h" #include "net_interface.h" -#include "eventOS_callback_timer.h" +#include "eventOS_event_timer.h" #define TRACE_GROUP "ThCH" @@ -35,17 +35,19 @@ typedef struct internal_socket_s { static NS_LIST_DEFINE(socket_list, internal_socket_t, link); -static void timer_cb(int8_t timer_id, uint16_t slots); -#define TIMER_FACTOR 20 /* mbedtls timer in ms, our timer in slots (50us), therefore 20 slots per ms */ +static void timer_cb(void* param); + #define TIMER_STATE_CANCELLED -1 /* cancelled */ #define TIMER_STATE_NO_EXPIRY 0 /* none of the delays is expired */ #define TIMER_STATE_INT_EXPIRY 1 /* the intermediate delay only is expired */ #define TIMER_STATE_FIN_EXPIRY 2 /* the final delay is expired */ + typedef struct secure_timer_s { - int8_t id; + uint8_t id; + timeout_t *timer; int8_t state; - uint8_t cycles; - uint8_t cycle_count; + uint32_t fin_ms; + uint32_t int_ms; } secure_timer_t; typedef struct secure_session { @@ -64,6 +66,18 @@ static int receive_from_socket(int8_t socket_id, unsigned char *buf, size_t len) static void start_timer(int8_t timer_id, uint32_t int_ms, uint32_t fin_ms); static int timer_status(int8_t timer_id); +static secure_session_t *secure_session_find_by_timer_id(int8_t timer_id) +{ + secure_session_t *this = NULL; + ns_list_foreach(secure_session_t, cur_ptr, &secure_session_list) { + if (cur_ptr->timer.id == timer_id) { + this = cur_ptr; + break; + } + } + return this; +} + static secure_session_t *secure_session_create(internal_socket_t *parent, uint8_t *address_ptr, uint16_t port) { secure_session_t *this = ns_dyn_mem_alloc(sizeof(secure_session_t)); @@ -72,12 +86,15 @@ static secure_session_t *secure_session_create(internal_socket_t *parent, uint8_ } memset(this, 0, sizeof(secure_session_t)); - this->timer.id = eventOS_callback_timer_register(timer_cb); - if (this->timer.id == -1) { - tr_error("tasklet alloc failed"); - ns_dyn_mem_free(this); - return NULL; + uint8_t timer_id = 1; + + while(secure_session_find_by_timer_id(timer_id)){ + if(timer_id == 0xff){ + return NULL; + } + timer_id++; } + this->timer.id = timer_id; this->sec_handler = coap_security_create(parent->listen_socket, this->timer.id, address_ptr, port, ECJPAKE, &send_to_socket, &receive_from_socket, &start_timer, &timer_status); @@ -101,6 +118,9 @@ static void secure_session_delete(secure_session_t *this) coap_security_destroy(this->sec_handler); this->sec_handler = NULL; } + if(this->timer.timer){ + eventOS_timeout_cancel(this->timer.timer); + } ns_dyn_mem_free(this); this = NULL; } @@ -119,32 +139,6 @@ static void clear_secure_sessions(internal_socket_t *this){ } } -static secure_session_t *secure_session_find_by_timer_id(int8_t timer_id) -{ - secure_session_t *this = NULL; - ns_list_foreach(secure_session_t, cur_ptr, &secure_session_list) { - if (cur_ptr->timer.id == timer_id) { - this = cur_ptr; - break; - } - } - return this; -} - -static secure_session_t *secure_session_find_by_parent(internal_socket_t *parent) -{ - secure_session_t *this = NULL; - ns_list_foreach(secure_session_t, cur_ptr, &secure_session_list) { - if( cur_ptr->sec_handler ){ - if (cur_ptr->parent == parent ) { - this = cur_ptr; - break; - } - } - } - return this; -} - static secure_session_t *secure_session_find(internal_socket_t *parent, uint8_t *address_ptr, uint16_t port) { secure_session_t *this = NULL; @@ -305,29 +299,36 @@ static int receive_from_socket(int8_t socket_id, unsigned char *buf, size_t len) * TODO - might be better to use an event timer in conjunction with * CoAP tasklet */ -static void timer_cb(int8_t timer_id, uint16_t slots) +static void timer_cb(void *param) { - (void)slots; /* No need to look at slots */ - - secure_session_t *sec = secure_session_find_by_timer_id(timer_id); + secure_session_t *sec = param; if( sec ){ - if (++sec->timer.cycle_count == sec->timer.cycles) { - /* We have counted the number of cycles - finish */ - sec->timer.state = TIMER_STATE_FIN_EXPIRY; - /* Stop the timer as we no longer need it */ - (void)eventOS_callback_timer_stop(sec->timer.id); /* We can ignore return; ID is valid */ + if(sec->timer.fin_ms > sec->timer.int_ms){ + /* Intermediate expiry */ + sec->timer.fin_ms -= sec->timer.int_ms; + sec->timer.state = TIMER_STATE_INT_EXPIRY; + int error = coap_security_handler_continue_connecting(sec->sec_handler); + if(MBEDTLS_ERR_SSL_TIMEOUT == error) { + //TODO: How do we handle timeouts? + secure_session_delete(sec); + } + else{ + sec->timer.timer = eventOS_timeout_ms(timer_cb, sec->timer.int_ms, (void*)sec); + } + } + else{ + /* We have counted the number of cycles - finish */ + eventOS_timeout_cancel(sec->timer.timer); + sec->timer.fin_ms = 0; + sec->timer.int_ms = 0; + sec->timer.timer = NULL; + sec->timer.state = TIMER_STATE_FIN_EXPIRY; int error = coap_security_handler_continue_connecting(sec->sec_handler); if(MBEDTLS_ERR_SSL_TIMEOUT == error) { //TODO: How do we handle timeouts? secure_session_delete(sec); } - } else { - /* Intermediate expiry */ - sec->timer.state = TIMER_STATE_INT_EXPIRY; } - //TODO: In case of DTLS and count == 1 || 4 we must call continue connecting of security so - //that mbedtls can handle timeout logic: resending etc... - //Not done, because timer should be refactored to be platform specific! } } @@ -336,15 +337,20 @@ static void start_timer(int8_t timer_id, uint32_t int_ms, uint32_t fin_ms) secure_session_t *sec = secure_session_find_by_timer_id(timer_id); if( sec ){ if ((int_ms > 0) && (fin_ms > 0)) { - /* Note: as it stands, fin_ms is always 4 * int_ms, so cycles is always 4 but this may change */ - sec->timer.cycles = fin_ms/int_ms; - sec->timer.cycle_count = 0; + sec->timer.int_ms = int_ms; + sec->timer.fin_ms = fin_ms; sec->timer.state = TIMER_STATE_NO_EXPIRY; - eventOS_callback_timer_start(sec->timer.id, int_ms * TIMER_FACTOR); + if(sec->timer.timer){ + eventOS_timeout_cancel(sec->timer.timer); + } + sec->timer.timer = eventOS_timeout_ms(timer_cb, int_ms, sec); } else if (fin_ms == 0) { /* fin_ms == 0 means cancel the timer */ sec->timer.state = TIMER_STATE_CANCELLED; - (void)eventOS_callback_timer_stop(sec->timer.id); /* We can ignore return; ID will be valid */ + eventOS_timeout_cancel(sec->timer.timer); + sec->timer.fin_ms = 0; + sec->timer.int_ms = 0; + sec->timer.timer = NULL; } } } @@ -390,17 +396,20 @@ static void secure_recv_sckt_msg(void *cb_res) if( sock && read_data(sckt_data, sock, &src_address) == 0 ){ secure_session_t *session = secure_session_find(sock, src_address.address, src_address.identifier); + + // Create session if( !session ){ memcpy( sock->dest_addr.address, src_address.address, 16 ); sock->dest_addr.identifier = src_address.identifier; sock->dest_addr.type = src_address.type; session = secure_session_create(sock, src_address.address, src_address.identifier); } - if( !session ){ tr_err("secure_recv_sckt_msg session creation failed - OOM"); return; } + + // Start handshake if( !session->sec_handler->_is_started ){ uint8_t *pw = (uint8_t *)ns_dyn_mem_alloc(64); uint8_t pw_len; @@ -414,8 +423,13 @@ static void secure_recv_sckt_msg(void *cb_res) } ns_dyn_mem_free(pw); }else{ + //Continue handshake if( !session->secure_done ){ - if( coap_security_handler_continue_connecting(session->sec_handler) == 0){ + int ret = coap_security_handler_continue_connecting(session->sec_handler); + // Handshake done + if(ret == 0){ + eventOS_timeout_cancel(session->timer.timer); + session->timer.timer = NULL; session->secure_done = true; if( sock->parent->_security_done_cb ){ sock->parent->_security_done_cb(sock->listen_socket, src_address.address, @@ -423,7 +437,12 @@ static void secure_recv_sckt_msg(void *cb_res) session->sec_handler->_keyblk.value); } } - //TODO: error handling + else if (ret < 0){ + // error handling + // TODO: here we also should clear CoAP retransmission buffer and inform that CoAP request sending is failed. + secure_session_delete(session); + } + //Session valid }else{ unsigned char *data = ns_dyn_mem_temporary_alloc(sock->data_len); int len = 0; @@ -431,10 +450,7 @@ static void secure_recv_sckt_msg(void *cb_res) if( len < 0 ){ ns_dyn_mem_free(data); if( len == MBEDTLS_ERR_SSL_PEER_CLOSE_NOTIFY ){ -// if( sock->parent->sec_conn_closed_cb ){ -// sock->parent->sec_conn_closed_cb(sock->listen_socket); secure_session_delete( session ); -// } } }else{ if( sock->parent->_recv_cb ){ diff --git a/source/coap_security_handler.c b/source/coap_security_handler.c index 6a06c47282..f5304c9a29 100644 --- a/source/coap_security_handler.c +++ b/source/coap_security_handler.c @@ -433,7 +433,8 @@ int coap_security_handler_connect_non_blocking(coap_security_t *sec, bool is_ser } int coap_security_handler_continue_connecting(coap_security_t *sec){ - int ret=-1; + int ret = -1; + while( ret != MBEDTLS_ERR_SSL_WANT_READ ){ ret = mbedtls_ssl_handshake_step( &sec->_ssl ); @@ -446,22 +447,20 @@ int coap_security_handler_continue_connecting(coap_security_t *sec){ #endif return 1; } - if(MBEDTLS_ERR_SSL_TIMEOUT == ret || - MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO == ret || - MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE == ret || - MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE_REQUEST == ret || - MBEDTLS_ERR_SSL_BAD_HS_SERVER_KEY_EXCHANGE == ret || - MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO_DONE == ret || - MBEDTLS_ERR_SSL_BAD_HS_CHANGE_CIPHER_SPEC == ret || - MBEDTLS_ERR_SSL_BAD_HS_FINISHED == ret) { - return MBEDTLS_ERR_SSL_TIMEOUT; + else if(ret && (ret != MBEDTLS_ERR_SSL_WANT_READ && ret != MBEDTLS_ERR_SSL_WANT_WRITE)){ + return ret; } - if( sec->_ssl.state == MBEDTLS_SSL_HANDSHAKE_OVER ){ + if( sec->_ssl.state == MBEDTLS_SSL_HANDSHAKE_OVER ){ return 0; } } - return ret; + + if(ret == MBEDTLS_ERR_SSL_WANT_READ || ret == MBEDTLS_ERR_SSL_WANT_WRITE){ + return 1; + } + + return -1; } @@ -500,7 +499,6 @@ int coap_security_handler_read(coap_security_t *sec, unsigned char* buffer, size while( ret == MBEDTLS_ERR_SSL_WANT_READ || ret == MBEDTLS_ERR_SSL_WANT_WRITE ); } - return ret; //bytes read } From 45daf83135b25f7b87294026da3314ed86220f8d Mon Sep 17 00:00:00 2001 From: Tero Heinonen Date: Wed, 10 Feb 2016 13:02:03 +0200 Subject: [PATCH 2/3] unittests fixed --- source/coap_connection_handler.c | 1 + .../unittest/coap_connection_handler/Makefile | 1 + .../test_coap_connection_handler.c | 7 ++++--- .../test_coap_security_handler.c | 6 +++--- test/coap-service/unittest/stub/mbedtls_stub.c | 7 ++++++- test/coap-service/unittest/stub/mbedtls_stub.h | 1 + test/coap-service/unittest/stub/timeout_stub.c | 18 ++++++++++++++++++ 7 files changed, 34 insertions(+), 7 deletions(-) create mode 100644 test/coap-service/unittest/stub/timeout_stub.c diff --git a/source/coap_connection_handler.c b/source/coap_connection_handler.c index ea6e770ec9..0618193e6a 100644 --- a/source/coap_connection_handler.c +++ b/source/coap_connection_handler.c @@ -90,6 +90,7 @@ static secure_session_t *secure_session_create(internal_socket_t *parent, uint8_ while(secure_session_find_by_timer_id(timer_id)){ if(timer_id == 0xff){ + ns_dyn_mem_free(this); return NULL; } timer_id++; diff --git a/test/coap-service/unittest/coap_connection_handler/Makefile b/test/coap-service/unittest/coap_connection_handler/Makefile index d934974d1f..5fcf10453e 100644 --- a/test/coap-service/unittest/coap_connection_handler/Makefile +++ b/test/coap-service/unittest/coap_connection_handler/Makefile @@ -13,6 +13,7 @@ TEST_SRC_FILES = \ ../stub/ns_trace_stub.c \ ../stub/ns_list_stub.c \ ../stub/ns_timer_stub.c \ + ../stub/timeout_stub.c \ ../stub/nsdynmemLIB_stub.c \ ../stub/socket_api_stub.c \ ../stub/coap_security_handler_stub.c \ diff --git a/test/coap-service/unittest/coap_connection_handler/test_coap_connection_handler.c b/test/coap-service/unittest/coap_connection_handler/test_coap_connection_handler.c index 8a90d196fa..2542496c19 100644 --- a/test/coap-service/unittest/coap_connection_handler/test_coap_connection_handler.c +++ b/test/coap-service/unittest/coap_connection_handler/test_coap_connection_handler.c @@ -227,6 +227,7 @@ bool test_coap_connection_handler_virtual_recv() return false; nsdynmemlib_stub.returnCounter = 1; + coap_security_handler_stub.int_value = 0; coap_security_handler_stub.sec_obj->_remote_port = 12; memset(coap_security_handler_stub.sec_obj->_remote_address, 1, 16 ); if( 0 != coap_connection_handler_virtual_recv(handler2,buf, 12, &buf, 1) ) @@ -312,16 +313,16 @@ bool test_timer_callbacks() //Note next tests will affect ns_timer test (cycle & cycle_count if( coap_security_handler_stub.start_timer_cb ){ - coap_security_handler_stub.start_timer_cb(5, 0, 0); + coap_security_handler_stub.start_timer_cb(1, 0, 0); - coap_security_handler_stub.start_timer_cb(5, 1, 2); + coap_security_handler_stub.start_timer_cb(1, 1, 2); } if( coap_security_handler_stub.timer_status_cb ){ if( -1 != coap_security_handler_stub.timer_status_cb(4) ) return false; - if( 0 != coap_security_handler_stub.timer_status_cb(5) ) + if( 0 != coap_security_handler_stub.timer_status_cb(1) ) return false; } diff --git a/test/coap-service/unittest/coap_security_handler/test_coap_security_handler.c b/test/coap-service/unittest/coap_security_handler/test_coap_security_handler.c index 466faf23f3..36d06bb4f6 100644 --- a/test/coap-service/unittest/coap_security_handler/test_coap_security_handler.c +++ b/test/coap-service/unittest/coap_security_handler/test_coap_security_handler.c @@ -203,17 +203,17 @@ bool test_coap_security_handler_continue_connecting() mbedtls_stub.counter = 0; mbedtls_stub.retArray[0] = MBEDTLS_ERR_SSL_BAD_HS_FINISHED; - if( MBEDTLS_ERR_SSL_TIMEOUT != coap_security_handler_continue_connecting(handle) ) + if( MBEDTLS_ERR_SSL_BAD_HS_FINISHED != coap_security_handler_continue_connecting(handle) ) return false; mbedtls_stub.counter = 0; mbedtls_stub.retArray[0] = MBEDTLS_ERR_SSL_WANT_READ; - if( MBEDTLS_ERR_SSL_WANT_READ != coap_security_handler_continue_connecting(handle) ) + if( 1 != coap_security_handler_continue_connecting(handle) ) return false; mbedtls_stub.counter = 0; - mbedtls_stub.retArray[0] = HANDSHAKE_FINISHED_VALUE; + mbedtls_stub.retArray[0] = HANDSHAKE_FINISHED_VALUE_RETURN_ZERO; if( 0 != coap_security_handler_continue_connecting(handle) ) return false; diff --git a/test/coap-service/unittest/stub/mbedtls_stub.c b/test/coap-service/unittest/stub/mbedtls_stub.c index 79e0b6f188..d6d88e6acb 100644 --- a/test/coap-service/unittest/stub/mbedtls_stub.c +++ b/test/coap-service/unittest/stub/mbedtls_stub.c @@ -10,8 +10,13 @@ mbedtls_stub_def mbedtls_stub; int mbedtls_ssl_handshake_step( mbedtls_ssl_context *ssl ) { if( mbedtls_stub.useCounter ){ - if( mbedtls_stub.retArray[mbedtls_stub.counter] == HANDSHAKE_FINISHED_VALUE){ + + if( mbedtls_stub.retArray[mbedtls_stub.counter] == HANDSHAKE_FINISHED_VALUE || + mbedtls_stub.retArray[mbedtls_stub.counter] == HANDSHAKE_FINISHED_VALUE_RETURN_ZERO){ + ssl->state = MBEDTLS_SSL_HANDSHAKE_OVER; + if(mbedtls_stub.retArray[mbedtls_stub.counter] == HANDSHAKE_FINISHED_VALUE_RETURN_ZERO) + return 0; } return mbedtls_stub.retArray[mbedtls_stub.counter++]; } diff --git a/test/coap-service/unittest/stub/mbedtls_stub.h b/test/coap-service/unittest/stub/mbedtls_stub.h index 0c28668131..297f6f7cfb 100644 --- a/test/coap-service/unittest/stub/mbedtls_stub.h +++ b/test/coap-service/unittest/stub/mbedtls_stub.h @@ -32,6 +32,7 @@ #define HANDSHAKE_FINISHED_VALUE 8888 +#define HANDSHAKE_FINISHED_VALUE_RETURN_ZERO 8889 typedef struct { int crt_expected_int; diff --git a/test/coap-service/unittest/stub/timeout_stub.c b/test/coap-service/unittest/stub/timeout_stub.c new file mode 100644 index 0000000000..36bf074305 --- /dev/null +++ b/test/coap-service/unittest/stub/timeout_stub.c @@ -0,0 +1,18 @@ +#include "eventOS_event_timer.h" + +// Timeout structure, already typedefed to timeout_t +struct timeout_entry_t { + uint8_t id; +}; + +static timeout_t timeout_stub; + +timeout_t *eventOS_timeout_ms(void (*callback)(void *), uint32_t ms, void *arg) +{ + return &timeout_stub; +} + +void eventOS_timeout_cancel(timeout_t *t) +{ + +} From 6dd666a4554e9855127a1b17f49bf4fbee1dd9b5 Mon Sep 17 00:00:00 2001 From: Tero Heinonen Date: Thu, 11 Feb 2016 13:23:08 +0200 Subject: [PATCH 3/3] Virtual socket error handling fixed. Entropy fix reverted to make yotta build working. This must be fixed properly later. --- source/coap_connection_handler.c | 9 ++++++++- source/coap_security_handler.c | 4 +++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/source/coap_connection_handler.c b/source/coap_connection_handler.c index 0618193e6a..f8298fbf4b 100644 --- a/source/coap_connection_handler.c +++ b/source/coap_connection_handler.c @@ -529,7 +529,8 @@ int coap_connection_handler_virtual_recv(coap_conn_handler_t *handler, uint8_t a } }else{ if( !session->secure_done ){ - if( coap_security_handler_continue_connecting(session->sec_handler) == 0){ + int ret = coap_security_handler_continue_connecting(session->sec_handler); + if(ret == 0){ session->secure_done = true; if( handler->_security_done_cb ){ handler->_security_done_cb(sock->listen_socket, @@ -538,6 +539,12 @@ int coap_connection_handler_virtual_recv(coap_conn_handler_t *handler, uint8_t a } return 0; } + else if(ret < 0) + { + // error handling + // TODO: here we also should clear CoAP retransmission buffer and inform that CoAP request sending is failed. + secure_session_delete(session); + } //TODO: error handling }else{ unsigned char *data = ns_dyn_mem_temporary_alloc(sock->data_len); diff --git a/source/coap_security_handler.c b/source/coap_security_handler.c index f5304c9a29..3834030a6d 100644 --- a/source/coap_security_handler.c +++ b/source/coap_security_handler.c @@ -60,8 +60,10 @@ static int coap_security_handler_init(coap_security_t *sec){ sec->_is_started = false; + //TODO: Must have at least 1 strong entropy source, otherwise DTLS will fail. + //This is NOT strong even we say it is! if( mbedtls_entropy_add_source( &sec->_entropy, entropy_poll, NULL, - 128, 0 ) < 0 ){ + 128, 1 ) < 0 ){ return -1; }