| Message ID | 20190327120307.15292-1-remitamine@gmail.com |
|---|---|
| State | Accepted |
| Commit | bc1749c6e46099ec85110361dbe6f7994a63040d |
| Headers | show |
On Wed, Mar 27, 2019 at 01:03:07PM +0100, Remita Amine wrote: > fixes #7801 > > Signed-off-by: Remita Amine <remitamine@gmail.com> > --- > libavformat/tls_gnutls.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/libavformat/tls_gnutls.c b/libavformat/tls_gnutls.c > index e3c43683be..f32bc2821b 100644 > --- a/libavformat/tls_gnutls.c > +++ b/libavformat/tls_gnutls.c > @@ -182,11 +182,13 @@ static int tls_open(URLContext *h, const char *uri, int flags, AVDictionary **op > gnutls_transport_set_push_function(p->session, gnutls_url_push); > gnutls_transport_set_ptr(p->session, c->tcp); > gnutls_priority_set_direct(p->session, "NORMAL", NULL); > - ret = gnutls_handshake(p->session); > - if (ret) { > - ret = print_tls_error(h, ret); > - goto fail; > - } > + do { > + ret = gnutls_handshake(p->session); > + if (gnutls_error_is_fatal(ret)) { > + ret = print_tls_error(h, ret); > + goto fail; > + } > + } while (ret); is retrying the correct action for every non fatal error ? is there a maximum retry count ? if not this could potentially loop forever [...]
this patch is based on the documentation for gnutls_handshake <https://gnutls.org/manual/html_node/TLS-handshake.html>: > On these non-fatal errors call this function again, until it returns 0 > and the examples from the gnutls manual, such as: https://gnutls.org/manual/html_node/Client-with-Resume-capability-example.html https://gnutls.org/manual/html_node/Simple-client-example-with-X_002e509-certificate-support.html https://gnutls.org/manual/html_node/Datagram-TLS-client-example.html On Thu, Mar 28, 2019 at 12:51 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Mar 27, 2019 at 01:03:07PM +0100, Remita Amine wrote: > > fixes #7801 > > > > Signed-off-by: Remita Amine <remitamine@gmail.com> > > --- > > libavformat/tls_gnutls.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/libavformat/tls_gnutls.c b/libavformat/tls_gnutls.c > > index e3c43683be..f32bc2821b 100644 > > --- a/libavformat/tls_gnutls.c > > +++ b/libavformat/tls_gnutls.c > > @@ -182,11 +182,13 @@ static int tls_open(URLContext *h, const char > *uri, int flags, AVDictionary **op > > gnutls_transport_set_push_function(p->session, gnutls_url_push); > > gnutls_transport_set_ptr(p->session, c->tcp); > > gnutls_priority_set_direct(p->session, "NORMAL", NULL); > > - ret = gnutls_handshake(p->session); > > - if (ret) { > > - ret = print_tls_error(h, ret); > > - goto fail; > > - } > > + do { > > + ret = gnutls_handshake(p->session); > > + if (gnutls_error_is_fatal(ret)) { > > + ret = print_tls_error(h, ret); > > + goto fail; > > + } > > + } while (ret); > > is retrying the correct action for every non fatal error ? > is there a maximum retry count ? if not this could potentially > loop forever > > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Old school: Use the lowest level language in which you can solve the > problem > conveniently. > New school: Use the highest level language in which the latest > supercomputer > can solve the problem without the user falling asleep waiting. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Wed, Mar 27, 2019 at 2:09 PM Remita Amine <remitamine@gmail.com> wrote: > > fixes #7801 > > Signed-off-by: Remita Amine <remitamine@gmail.com> This seems to fix switching the cipher suite, and quickly looking at the gnutls API docs this seems to be the way to do it. Just tested this with the following as I got a report that opening Facebook HTTPS URLs didn't work in a libavformat API client: 1. youtube-dl -g "https://www.facebook.com/downshiftaus/videos/418766325569703/" (you receive a URL) 2. ffprobe 'THAT_URL' Without this patch the handshake fails (as there is a cipher re-negotiation?), and with the patch it works. Additionally, this doesn't seem to enable bad TLS configurations such https://rc4.badssl.com/ to get opened. Which is expected from the gnutls docs, but still I felt like testing. In other words, I think this is LGTM and since there already are reports from people on distros running into this, this should be back-ported to the versions still maintained by us (whatever those are) ? Best regards, Jan
On Sat, Jun 1, 2019 at 8:38 PM Jan Ekström <jeebjp@gmail.com> wrote: > > On Wed, Mar 27, 2019 at 2:09 PM Remita Amine <remitamine@gmail.com> wrote: > > > > fixes #7801 > > > > Signed-off-by: Remita Amine <remitamine@gmail.com> > > This seems to fix switching the cipher suite, and quickly looking at > the gnutls API docs this seems to be the way to do it. > > Just tested this with the following as I got a report that opening > Facebook HTTPS URLs didn't work in a libavformat API client: > 1. youtube-dl -g "https://www.facebook.com/downshiftaus/videos/418766325569703/" > (you receive a URL) > 2. ffprobe 'THAT_URL' > > Without this patch the handshake fails (as there is a cipher > re-negotiation?), and with the patch it works. > > Additionally, this doesn't seem to enable bad TLS configurations such > https://rc4.badssl.com/ to get opened. Which is expected from the > gnutls docs, but still I felt like testing. > > In other words, I think this is LGTM and since there already are > reports from people on distros running into this, this should be > back-ported to the versions still maintained by us (whatever those > are) ? > > Best regards, > Jan Applied to master, as I consider this a pretty important fix and there were no further comments in the span of two weeks. Additionally, after checking wget's and curl's usage of gnutls, this indeed seems to be the modus operandi. The only thing is that they also happen to check how many ms are left towards the I/O timeout, but I am not sure if we can effectively apply it here. Additionally, there were voices on IRC that timing out in the middle of a TLS handshake would be overkill and that fixing the connectivity issues should be prioritized. References of gnutls usage: https://github.com/curl/curl/blob/master/lib/vtls/gtls.c#L332 http://git.savannah.gnu.org/cgit/wget.git/tree/src/gnutls.c#n476 Thanks, Jan
diff --git a/libavformat/tls_gnutls.c b/libavformat/tls_gnutls.c index e3c43683be..f32bc2821b 100644 --- a/libavformat/tls_gnutls.c +++ b/libavformat/tls_gnutls.c @@ -182,11 +182,13 @@ static int tls_open(URLContext *h, const char *uri, int flags, AVDictionary **op gnutls_transport_set_push_function(p->session, gnutls_url_push); gnutls_transport_set_ptr(p->session, c->tcp); gnutls_priority_set_direct(p->session, "NORMAL", NULL); - ret = gnutls_handshake(p->session); - if (ret) { - ret = print_tls_error(h, ret); - goto fail; - } + do { + ret = gnutls_handshake(p->session); + if (gnutls_error_is_fatal(ret)) { + ret = print_tls_error(h, ret); + goto fail; + } + } while (ret); p->need_shutdown = 1; if (c->verify) { unsigned int status, cert_list_size;
fixes #7801 Signed-off-by: Remita Amine <remitamine@gmail.com> --- libavformat/tls_gnutls.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)