Re: [openssl-dev] DTLS retransmission api
On 03/06/16 12:19, Matt Caswell wrote: On 03/06/16 10:52, Alfred E. Heggestad wrote: Hi Matt, thanks for the suggested API and code. Please find below a suggested patch that implements this new callback. the patch is based on 1.0.2-dev from GIT: url: git://git.openssl.org/openssl.git branch: origin/OpenSSL_1_0_2-stable I have renamed "timeout_duration" on purpose, since the units have changed from "seconds" to "milliseconds". Hi Alfred Thanks for the submission. In order to ease the review process please read this file for some guidance on how to submit patches: https://github.com/openssl/openssl/blob/master/CONTRIBUTING The preferred way is via github because it makes it much easier for us to comment on the code in detail and provide feedback. I've not looked at your code in detail yet (I'll wait until I see the submission come in via github (or RT if you choose to go that way - see CONTRIBUTING)). I'll make a few high-level points though: - Because this is a new feature you need to create it from the master branch in git not the 1.0.2 branch. 1.0.2 is a stable branch and only receives bug fixes. - We are currently focussing on the 1.1.0 release which is now in feature freeze, so it may be a while before we get to look at it. - All new features must have documentation with them. Take a look at the existing pod files in the doc directory for some examples of our style. thanks, I have created a new PR: https://github.com/openssl/openssl/pull/1160 /alfred -- openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] DTLS retransmission api
On 02/06/16 16:03, Matt Caswell wrote: On 02/06/16 14:33, Alfred E. Heggestad wrote: On 01/06/16 13:58, Matt Caswell wrote: On 01/06/16 11:15, Alfred E. Heggestad wrote: hi, we are using DTLS from OpenSSL to implement DTLS-SRTP in our product (Wire.com) .. The code and implementation works really well and is very robust. We are using OpenSSL version 1.0.2g since our product is deployed globally on mobile data networks, we have quite variable latency and packetloss. The patch below shows my working code, it has an initial retransmit timeout of 400 ms which is incrementing by 10% for every re-trans. obviously this patch cannot make it into the official tree. but I would like to discuss with you guys the option to add some kind of API for: - Setting the initial RTO for DTLS (in milliseconds). - Setting the retransmit policy for DTLS, i.e. should it double or increment by X for every re-trans. I think an API for that would be a great idea. Perhaps a callback could be used so that you can set exactly the policy you want? Thank you, Matt I can work on a patch for this, if you guys can help me to define the API. I think we only need one CTRL api to set the next re-transmit interval. then in the application code that calls this: - DTLSv1_handle_timeout - DTLSv1_get_timeout can also call DTLS_set_retrans_interval(400) I'm not sure I follow you. I was thinking something like: int DTLS_set_timer_cb(SSL *s, int (*cb)(SSL *s, int timer)); Then where in the current code we have: dtls1_double_timeout(s); We might instead do if(s->d1->timer_cb != NULL) s->d1->timeout_duration = timer_cb(s, s->d1->timeout_duration); else dtls1_double_timeout(s); And in dtls1_start_timer() where we have: /* If timer is not set, initialize duration with 1 second */ if (s->d1->next_timeout.tv_sec == 0 && s->d1->next_timeout.tv_usec == 0) { s->d1->timeout_duration = 1; } Instead have: /* If timer is not set, initialize duration with 1 second */ if (s->d1->next_timeout.tv_sec == 0 && s->d1->next_timeout.tv_usec == 0) { if (s->d1->timer_cb != NULL) s->d1->timeout_duration = s->d1_timeout_cb(s, 0); else s->d1->timeout_duration = 1; } Hi Matt, thanks for the suggested API and code. Please find below a suggested patch that implements this new callback. the patch is based on 1.0.2-dev from GIT: url: git://git.openssl.org/openssl.git branch: origin/OpenSSL_1_0_2-stable I have renamed "timeout_duration" on purpose, since the units have changed from "seconds" to "milliseconds". From e6c9fbe470ab1901010e90b727313ebc7875b40f Mon Sep 17 00:00:00 2001 From: "Alfred E. Heggestad" Date: Fri, 3 Jun 2016 11:31:45 +0200 Subject: [PATCH] add support for DTLS callback for timeout value --- ssl/d1_lib.c | 45 + ssl/dtls1.h | 9 +++-- ssl/ssl.h| 4 3 files changed, 48 insertions(+), 10 deletions(-) diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c index ee78921..235635a 100644 --- a/ssl/d1_lib.c +++ b/ssl/d1_lib.c @@ -240,6 +240,8 @@ void dtls1_clear(SSL *s) unsigned int link_mtu; if (s->d1) { +dtls_timer_cb *timer_cb = s->d1->timer_cb; + unprocessed_rcds = s->d1->unprocessed_rcds.q; processed_rcds = s->d1->processed_rcds.q; buffered_messages = s->d1->buffered_messages; @@ -252,6 +254,9 @@ void dtls1_clear(SSL *s) memset(s->d1, 0, sizeof(*(s->d1))); +/* Restore the timer callback from previous state */ +s->d1->timer_cb = timer_cb; + if (s->server) { s->d1->cookie_len = sizeof(s->d1->cookie); } @@ -359,6 +364,8 @@ const SSL_CIPHER *dtls1_get_cipher(unsigned int u) void dtls1_start_timer(SSL *s) { +struct timeval diff; + #ifndef OPENSSL_NO_SCTP /* Disable timer for SCTP */ if (BIO_dgram_is_sctp(SSL_get_wbio(s))) { @@ -367,16 +374,24 @@ void dtls1_start_timer(SSL *s) } #endif -/* If timer is not set, initialize duration with 1 second */ +/* If timer is not set, initialize duration with 1 second or + * a user-specified value if the timer callback is installed. */ if (s->d1->next_timeout.tv_sec == 0 && s->d1->next_timeout.tv_usec == 0) { -s->d1->timeout_duration = 1; + +if (s->d1->timer_cb != NULL) +s->d1->timeout_duration_ms = s->d1->timer_cb(s, 1000); +else +s->d1->timeout_duration_ms = 1000; } /* Set timeout to current time */ get_current_time(&(s->d1->next_timeout)); /* Add duration to current time */ -s->d1->next_timeout.tv_sec += s->d1->timeout_dur
Re: [openssl-dev] DTLS retransmission api
On 01/06/16 13:58, Matt Caswell wrote: On 01/06/16 11:15, Alfred E. Heggestad wrote: hi, we are using DTLS from OpenSSL to implement DTLS-SRTP in our product (Wire.com) .. The code and implementation works really well and is very robust. We are using OpenSSL version 1.0.2g since our product is deployed globally on mobile data networks, we have quite variable latency and packetloss. The patch below shows my working code, it has an initial retransmit timeout of 400 ms which is incrementing by 10% for every re-trans. obviously this patch cannot make it into the official tree. but I would like to discuss with you guys the option to add some kind of API for: - Setting the initial RTO for DTLS (in milliseconds). - Setting the retransmit policy for DTLS, i.e. should it double or increment by X for every re-trans. I think an API for that would be a great idea. Perhaps a callback could be used so that you can set exactly the policy you want? Thank you, Matt I can work on a patch for this, if you guys can help me to define the API. I think we only need one CTRL api to set the next re-transmit interval. then in the application code that calls this: - DTLSv1_handle_timeout - DTLSv1_get_timeout can also call DTLS_set_retrans_interval(400) in addition we have seen the code hit this assert in production: /*OPENSSL_assert(0);*/ /* XDTLS: want to see if we ever get here */ so I would say it should be safe to remove it. Hmthe question is why does it get there? It shouldn't. I can try to reproduce this. We have seen that this assert was executed, when the code was under quite heavy load and lots of traffic. /alfred Matt Best Regards, Alfred E. Heggestad Berlin -- diff -Naur openssl-1.0.2g/ssl/d1_lib.c openssl/ssl/d1_lib.c --- openssl-1.0.2g/ssl/d1_lib.c2016-03-01 14:35:53.0 +0100 +++ openssl/ssl/d1_lib.c2016-06-01 10:45:27.0 +0200 @@ -359,6 +359,8 @@ void dtls1_start_timer(SSL *s) { +struct timeval diff; + #ifndef OPENSSL_NO_SCTP /* Disable timer for SCTP */ if (BIO_dgram_is_sctp(SSL_get_wbio(s))) { @@ -369,14 +371,17 @@ /* If timer is not set, initialize duration with 1 second */ if (s->d1->next_timeout.tv_sec == 0 && s->d1->next_timeout.tv_usec == 0) { -s->d1->timeout_duration = 1; +s->d1->timeout_duration = 0.400; } /* Set timeout to current time */ get_current_time(&(s->d1->next_timeout)); /* Add duration to current time */ -s->d1->next_timeout.tv_sec += s->d1->timeout_duration; +diff.tv_sec = 0; +diff.tv_usec = 100*s->d1->timeout_duration; +timeradd(&s->d1->next_timeout, &diff, &s->d1->next_timeout); + BIO_ctrl(SSL_get_rbio(s), BIO_CTRL_DGRAM_SET_NEXT_TIMEOUT, 0, &(s->d1->next_timeout)); } @@ -441,7 +446,7 @@ void dtls1_double_timeout(SSL *s) { -s->d1->timeout_duration *= 2; +s->d1->timeout_duration *= 1.10; if (s->d1->timeout_duration > 60) s->d1->timeout_duration = 60; dtls1_start_timer(s); diff -Naur openssl-1.0.2g/ssl/d1_pkt.c openssl/ssl/d1_pkt.c --- openssl-1.0.2g/ssl/d1_pkt.c2016-03-01 14:35:53.0 +0100 +++ openssl/ssl/d1_pkt.c2016-03-08 14:39:44.0 +0100 @@ -1502,7 +1502,7 @@ * will happen with non blocking IO */ if (s->s3->wbuf.left != 0) { -OPENSSL_assert(0); /* XDTLS: want to see if we ever get here */ +/*OPENSSL_assert(0);*/ /* XDTLS: want to see if we ever get here */ return (ssl3_write_pending(s, type, buf, len)); } diff -Naur openssl-1.0.2g/ssl/dtls1.h openssl/ssl/dtls1.h --- openssl-1.0.2g/ssl/dtls1.h2016-03-01 14:35:53.0 +0100 +++ openssl/ssl/dtls1.h2016-03-08 14:39:44.0 +0100 @@ -225,8 +225,8 @@ * Indicates when the last handshake msg or heartbeat sent will timeout */ struct timeval next_timeout; -/* Timeout duration */ -unsigned short timeout_duration; +/* Timeout duration in Seconds */ +double timeout_duration; /* * storage for Alert/Handshake protocol data received but not yet * processed by ssl3_read_bytes: -- openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
[openssl-dev] DTLS retransmission api
hi, we are using DTLS from OpenSSL to implement DTLS-SRTP in our product (Wire.com) .. The code and implementation works really well and is very robust. We are using OpenSSL version 1.0.2g since our product is deployed globally on mobile data networks, we have quite variable latency and packetloss. The patch below shows my working code, it has an initial retransmit timeout of 400 ms which is incrementing by 10% for every re-trans. obviously this patch cannot make it into the official tree. but I would like to discuss with you guys the option to add some kind of API for: - Setting the initial RTO for DTLS (in milliseconds). - Setting the retransmit policy for DTLS, i.e. should it double or increment by X for every re-trans. in addition we have seen the code hit this assert in production: /*OPENSSL_assert(0);*/ /* XDTLS: want to see if we ever get here */ so I would say it should be safe to remove it. Best Regards, Alfred E. Heggestad Berlin -- diff -Naur openssl-1.0.2g/ssl/d1_lib.c openssl/ssl/d1_lib.c --- openssl-1.0.2g/ssl/d1_lib.c 2016-03-01 14:35:53.0 +0100 +++ openssl/ssl/d1_lib.c2016-06-01 10:45:27.0 +0200 @@ -359,6 +359,8 @@ void dtls1_start_timer(SSL *s) { +struct timeval diff; + #ifndef OPENSSL_NO_SCTP /* Disable timer for SCTP */ if (BIO_dgram_is_sctp(SSL_get_wbio(s))) { @@ -369,14 +371,17 @@ /* If timer is not set, initialize duration with 1 second */ if (s->d1->next_timeout.tv_sec == 0 && s->d1->next_timeout.tv_usec == 0) { -s->d1->timeout_duration = 1; +s->d1->timeout_duration = 0.400; } /* Set timeout to current time */ get_current_time(&(s->d1->next_timeout)); /* Add duration to current time */ -s->d1->next_timeout.tv_sec += s->d1->timeout_duration; +diff.tv_sec = 0; +diff.tv_usec = 100*s->d1->timeout_duration; +timeradd(&s->d1->next_timeout, &diff, &s->d1->next_timeout); + BIO_ctrl(SSL_get_rbio(s), BIO_CTRL_DGRAM_SET_NEXT_TIMEOUT, 0, &(s->d1->next_timeout)); } @@ -441,7 +446,7 @@ void dtls1_double_timeout(SSL *s) { -s->d1->timeout_duration *= 2; +s->d1->timeout_duration *= 1.10; if (s->d1->timeout_duration > 60) s->d1->timeout_duration = 60; dtls1_start_timer(s); diff -Naur openssl-1.0.2g/ssl/d1_pkt.c openssl/ssl/d1_pkt.c --- openssl-1.0.2g/ssl/d1_pkt.c 2016-03-01 14:35:53.0 +0100 +++ openssl/ssl/d1_pkt.c2016-03-08 14:39:44.0 +0100 @@ -1502,7 +1502,7 @@ * will happen with non blocking IO */ if (s->s3->wbuf.left != 0) { -OPENSSL_assert(0); /* XDTLS: want to see if we ever get here */ +/*OPENSSL_assert(0);*/ /* XDTLS: want to see if we ever get here */ return (ssl3_write_pending(s, type, buf, len)); } diff -Naur openssl-1.0.2g/ssl/dtls1.h openssl/ssl/dtls1.h --- openssl-1.0.2g/ssl/dtls1.h 2016-03-01 14:35:53.0 +0100 +++ openssl/ssl/dtls1.h 2016-03-08 14:39:44.0 +0100 @@ -225,8 +225,8 @@ * Indicates when the last handshake msg or heartbeat sent will timeout */ struct timeval next_timeout; -/* Timeout duration */ -unsigned short timeout_duration; +/* Timeout duration in Seconds */ +double timeout_duration; /* * storage for Alert/Handshake protocol data received but not yet * processed by ssl3_read_bytes: -- openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev