[1/2] httpauth: Add quotes around the algorithm string

Message ID 1444729477-25897-1-git-send-email-martin@martin.st
State Deferred
Headers show

Commit Message

Martin Storsjö Oct. 13, 2015, 9:44 a.m.
From: Carl Eugen Hoyos <cehoyos@ag.or.at>

Some overly strict implementations require this, even though it
technically is incorrect according to the spec.

In practice many other implementations seem to be using quotes here,
including cURL.
---
 libavformat/httpauth.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Luca Barbato Oct. 13, 2015, 11:54 a.m. | #1
On 13/10/15 11:44, Martin Storsjö wrote:
> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
> 
> Some overly strict implementations require this, even though it
> technically is incorrect according to the spec.
> 
> In practice many other implementations seem to be using quotes here,
> including cURL.
> ---

Probably won't hurt.
Rémi Denis-Courmont Oct. 13, 2015, 4:21 p.m. | #2
Le 2015-10-13 12:44, Martin Storsjö a écrit :
> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
>
> Some overly strict implementations require this, even though it
> technically is incorrect according to the spec.

That's not overly strict, that's just plain broken. Overly (or not so 
overly) strict implementations require that there be no quotes here.

It might be a good idea to accept invalid extraneous quotes for IOP. 
Sending invalid messages ones on purpose is insane. The problem is known 
and the conclusion from RFC7616 was to verry explicitly FORBID quotes.
Luca Barbato Oct. 13, 2015, 5:47 p.m. | #3
On 13/10/15 18:21, Rémi Denis-Courmont wrote:
> Le 2015-10-13 12:44, Martin Storsjö a écrit :
>> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
>>
>> Some overly strict implementations require this, even though it
>> technically is incorrect according to the spec.
> 
> That's not overly strict, that's just plain broken. Overly (or not so
> overly) strict implementations require that there be no quotes here.
> 
> It might be a good idea to accept invalid extraneous quotes for IOP.
> Sending invalid messages ones on purpose is insane. The problem is known
> and the conclusion from RFC7616 was to verry explicitly FORBID quotes.
> 

You suggest to drop the patch or to update the commit message (I already
amended it with s/overly strict/faulty/ locally.)

lu
Martin Storsjö Oct. 13, 2015, 5:49 p.m. | #4
On Tue, 13 Oct 2015, Rémi Denis-Courmont wrote:

> Le 2015-10-13 12:44, Martin Storsjö a écrit :
>> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
>>
>> Some overly strict implementations require this, even though it
>> technically is incorrect according to the spec.
>
> That's not overly strict, that's just plain broken. Overly (or not so 
> overly) strict implementations require that there be no quotes here.
>
> It might be a good idea to accept invalid extraneous quotes for IOP. 
> Sending invalid messages ones on purpose is insane. The problem is known 
> and the conclusion from RFC7616 was to verry explicitly FORBID quotes.

Oh, I hadn't seen that one - nice.

For reference, the case that required quotes (and even claimed that the 
old RFC said it was required) was here:
https://groups.google.com/d/msg/c-rtmp-server/Rrn27ZNoczw/uAPMRDIYUKcJ

I tried to contact him back then, pointing out how the RFC should be 
interpreted, but didn't get any reply.

I'd rather skip this patch then, and rebase the other one to not depend on 
it.

// Martin
Luca Barbato Oct. 13, 2015, 5:59 p.m. | #5
On 13/10/15 19:49, Martin Storsjö wrote:
> I'd rather skip this patch then, and rebase the other one to not depend
> on it.

I have a rebased patch ready and updated a bit the commit message: the
2617 use quite a generous amount of whitespace in the examples so I
guess everything goes =)

lu
Rémi Denis-Courmont Oct. 13, 2015, 7:30 p.m. | #6
Le 2015-10-13 20:47, Luca Barbato a écrit :
> On 13/10/15 18:21, Rémi Denis-Courmont wrote:
>> Le 2015-10-13 12:44, Martin Storsjö a écrit :
>>> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
>>>
>>> Some overly strict implementations require this, even though it
>>> technically is incorrect according to the spec.
>>
>> That's not overly strict, that's just plain broken. Overly (or not 
>> so
>> overly) strict implementations require that there be no quotes here.
>>
>> It might be a good idea to accept invalid extraneous quotes for IOP.
>> Sending invalid messages ones on purpose is insane. The problem is 
>> known
>> and the conclusion from RFC7616 was to verry explicitly FORBID 
>> quotes.
>>
>
> You suggest to drop the patch or to update the commit message (I 
> already
> amended it with s/overly strict/faulty/ locally.)

I suggest dropping the patch.

Postel's principle is to be strict in what you send, relaxed in what 
you accept. Though there has been recent push for being strict both ways 
due to, notably, security concerns.

There never was a reasonable proposal to be relaxed in what you send, 
for a good reason.
Luca Barbato Oct. 13, 2015, 7:34 p.m. | #7
On 13/10/15 21:30, Rémi Denis-Courmont wrote:
> Le 2015-10-13 20:47, Luca Barbato a écrit :
>> On 13/10/15 18:21, Rémi Denis-Courmont wrote:
>>> Le 2015-10-13 12:44, Martin Storsjö a écrit :
>>>> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
>>>>
>>>> Some overly strict implementations require this, even though it
>>>> technically is incorrect according to the spec.
>>>
>>> That's not overly strict, that's just plain broken. Overly (or not so
>>> overly) strict implementations require that there be no quotes here.
>>>
>>> It might be a good idea to accept invalid extraneous quotes for IOP.
>>> Sending invalid messages ones on purpose is insane. The problem is known
>>> and the conclusion from RFC7616 was to verry explicitly FORBID quotes.
>>>
>>
>> You suggest to drop the patch or to update the commit message (I already
>> amended it with s/overly strict/faulty/ locally.)
> 
> I suggest dropping the patch.
> 
> Postel's principle is to be strict in what you send, relaxed in what you
> accept. Though there has been recent push for being strict both ways due
> to, notably, security concerns.
> 
> There never was a reasonable proposal to be relaxed in what you send,
> for a good reason.
> 

Patch dropped then =)

Patch

diff --git a/libavformat/httpauth.c b/libavformat/httpauth.c
index b96da3e..403b166 100644
--- a/libavformat/httpauth.c
+++ b/libavformat/httpauth.c
@@ -224,8 +224,11 @@  static char *make_digest_auth(HTTPAuthState *state, const char *username,
     av_strlcatf(authstr, len, ",nonce=\"%s\"",     digest->nonce);
     av_strlcatf(authstr, len, ",uri=\"%s\"",       uri);
     av_strlcatf(authstr, len, ",response=\"%s\"",  response);
+
+    // we are violating the RFC and use "" because all others seem to do that too.
     if (digest->algorithm[0])
-        av_strlcatf(authstr, len, ",algorithm=%s",  digest->algorithm);
+        av_strlcatf(authstr, len, ",algorithm=\"%s\"",  digest->algorithm);
+
     if (digest->opaque[0])
         av_strlcatf(authstr, len, ",opaque=\"%s\"", digest->opaque);
     if (digest->qop[0]) {