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 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. Matt -- 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_duration;
+diff.tv_sec = s->d1->timeout_duration_ms / 1000;
+diff.tv_usec = (s->d1->timeout_duration_ms % 1000) * 1000;
+timeradd(>d1->next_timeout, , >d1->next_timeout);
+
BIO_ctrl(SSL_get_rbio(s),
Re: [openssl-dev] DTLS retransmission api
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;
}
...or something like that.
Matt
--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
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(>d1->next_timeout, , >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
Re: [openssl-dev] DTLS retransmission api
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?
>
>
> 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.
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(>d1->next_timeout, , >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
