| Message ID | 20190327120307.15292-1-remitamine@gmail.com |
|---|---|
| State | Accepted |
| Commit | bc1749c6e46099ec85110361dbe6f7994a63040d |
| Headers |
Return-Path: <ffmpeg-devel-bounces@ffmpeg.org> X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id 0A9634482EB for <patchwork@ffaux-bg.ffmpeg.org>; Wed, 27 Mar 2019 14:09:24 +0200 (EET) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id E11E3689DD1; Wed, 27 Mar 2019 14:09:23 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id AD6BE6807A3 for <ffmpeg-devel@ffmpeg.org>; Wed, 27 Mar 2019 14:09:17 +0200 (EET) Received: by mail-wr1-f65.google.com with SMTP id k17so10315971wrx.10 for <ffmpeg-devel@ffmpeg.org>; Wed, 27 Mar 2019 05:09:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=lllrO1lxO1j6SHuNI4J77LNVeA0ZRkEiSrK53M5oKS4=; b=ULgWuNb7Ph34xBhNqP+2CpCiMq8RyHIK4Gj6Ne5KX41C2GWzSMVSrplWOO9kwNdnWe lPoKDX22ikJUk730VVxf000Suldvq7XFHate7464dmL0obiBzf+mXAYDPuy+Blwj7Sdo SuxTPhCB6bfCM8IwqALw50nMaiUHfU3iDjJ+OB++e84gJKhY9uBCrzwETkQd6GJ1DW1Q cHcr9GChFQj83n/B4J5VNKt0XrXGS1TNftRuhiC0XpCAUtinmEccJoGvbTXtfjnjF+Xu 88R/pXaHWBS60yHUs39tNKILWuskOknXjYXOtggPvxDYQG3FHW6kycKEN5sv70wreKt7 csOg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=lllrO1lxO1j6SHuNI4J77LNVeA0ZRkEiSrK53M5oKS4=; b=EeZsdg8wJELsqBkI2C/fneEw9u8zearfDiI1jLiVT3RL3pa7eXMSC/x24plcAQHG8d dl4GMQtfo9EIcWyV0soLc45Yk4t6NkrgDfT8uMUs9x1QSPTjTsDMCxSuFqqwyhtykjjA GfrfHMTXWx/FuNqaowO4y7CZMwFG3jFIPekKXnAe9qh+njbPU1vodxYBRFx5wpD5fAJU ZVN+CrkQz8Ek4RwwC9XzIgOgxyt+TY/hKhKuC3qWaYStad1jbRyAw6VsDGoDzEFWcduZ LNOaFCCSsuGXtF5PBVRgoPy51O8oCD4aU0L+upw9WqlNCNYdnwaiq18n3DsXlvwKP8st SHPA== X-Gm-Message-State: APjAAAXnGuzjAIMpaA5k/i9vwMcHDxKcjj8YXOGMYhCfGUICa2qk67b6 NjCkImHuSCRLm737q7Rvaxf6hi2n X-Google-Smtp-Source: APXvYqxkkIsSafp+JdHEN/8BGVgetp+XQmQrUehceIVZN+fbMexTSd6DcgT2e84l8MFpASN9flYjMQ== X-Received: by 2002:a5d:6681:: with SMTP id l1mr1527587wru.186.1553688211317; Wed, 27 Mar 2019 05:03:31 -0700 (PDT) Received: from localhost.localdomain ([105.110.116.205]) by smtp.gmail.com with ESMTPSA id n11sm34744757wrt.63.2019.03.27.05.03.27 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 27 Mar 2019 05:03:30 -0700 (PDT) From: Remita Amine <remitamine@gmail.com> To: ffmpeg-devel@ffmpeg.org Date: Wed, 27 Mar 2019 13:03:07 +0100 Message-Id: <20190327120307.15292-1-remitamine@gmail.com> X-Mailer: git-send-email 2.21.0 MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH] lavf/tls_gnutls: retry gnutls_handshake on non fatal errors X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches <ffmpeg-devel.ffmpeg.org> List-Unsubscribe: <http://ffmpeg.org/mailman/options/ffmpeg-devel>, <mailto:ffmpeg-devel-request@ffmpeg.org?subject=unsubscribe> List-Archive: <http://ffmpeg.org/pipermail/ffmpeg-devel/> List-Post: <mailto:ffmpeg-devel@ffmpeg.org> List-Help: <mailto:ffmpeg-devel-request@ffmpeg.org?subject=help> List-Subscribe: <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>, <mailto:ffmpeg-devel-request@ffmpeg.org?subject=subscribe> Reply-To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> Cc: Remita Amine <remitamine@gmail.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" <ffmpeg-devel-bounces@ffmpeg.org> |
Commit Message
Remita Amine
March 27, 2019, 12:03 p.m. UTC
fixes #7801
Signed-off-by: Remita Amine <remitamine@gmail.com>
---
libavformat/tls_gnutls.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
Comments
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;