Re: [PATCH] BUG/MINOR: ssl: fix CO_FL_EARLY_DATA removal with http mode
On Thu, Nov 30, 2017 at 03:32:20PM +0100, Emmanuel Hocdet wrote: > > > Le 30 nov. 2017 à 13:34, Olivier Houchard a écrit : > > > > Hi Emmanuel, > > > > On Thu, Nov 30, 2017 at 12:15:37PM +0100, Emmanuel Hocdet wrote: > >> Hi Olivier, > >> > >>> Le 29 nov. 2017 à 19:57, Olivier Houchard a écrit > >>> : > >>> > >>> On Mon, Nov 27, 2017 at 06:19:41PM +0100, Emmanuel Hocdet wrote: > > Maybe the best is to add a new flag per conn_stream, > > CS_FL_WAITING_FOR_HS or > > something, instead of relying on CO_FL_EARLY_DATA. > > I think I'm going to do something like that. > > I think it's a good idea, two different things to deal with one tag. > > > That still doesn't help with your problem with TCP mode, though. I > > still want > > the CO_FL_EARLY_DATA to be removed after the handshake, so that we don't > > add the "Early-Data: 1" header if it is not needed. But it just occured > > to me that I can easily fix that by adding the header, not only if > > CO_FL_EARLY_DATA is set, but if CO_FL_EARLY_SSL_HS or CO_FL_SSL_WAIT_HS > > is set. > > That way we will both be happy :) > > >>> > >>> So, this if my first cut at it. It basically does as I said. > >>> Only thing you may be unhappy with, I made the sample fetch > >>> ssl_fc_has_early > >>> return 0 if we had early data but the handshake happened, because the main > >>> use case is to know if there are early data and if they're a potential > >>> security risk. > >>> If you can give me a case where we have a need a sample fetch to know > >>> there > >>> were early data, even after the handshake, maybe we can introduce a new > >>> sample fetch, ssl_fc_has_insecure_early, or something ? > >>> > >> > >> In this case, i don’t understand the interest of ssl_fc_has_early. > >> > >> looking at the documentation > >> ssl_fc_has_early : boolean > >> Returns true if early data were sent, and the handshake didn't happen > >> yet. As > >> it has security implications, it is useful to be able to refuse those, or > >> wait until the handshake happened. > >> > >> ssl_fc_* can be used after the front connection at ssl level: handshake > >> will be finished at this time. > >> ssl_fc_has_early should be: returns true if early data were sent and > >> accepted in ssl level. (425 return is http level) > >> > >> What the description makes me think, and understand what you said, is that > >> it could be a « ssl_hs_has_early ». > >> I’m very interesting to have acl on hs negotiation, i don't know how to do > >> that now in haproxy. > >> > > > > No, sls_fc_has_early can be used before the handshake happened. If we > > received early data, the request will be treated before the handshake (and > > potentially the answer from the server will be sent before the handshake is > > done). So ssl_fc_has_early is there to be able to stop any "dangerous" > > request. > > > > My fault, i don’t read the doc of ssl_fc_has_early: as user it confused me on > the meaning > and usage. > What i mean is about the name of the prefix ssl_fc* ssl front connection. > All ssl_fc_* can be used in log-format because is valid after the end of HS. > ssl_fc_has_early is the first _fc_ with a different behavior and is not PoLA : > ssl_fc_has_early can be used in http mode but it can false report in > log-format, and is useless in tcp mode? but it’s ssl_fc_* ... > > Technically, with early HS, we have new informations who can changed after > the HS. > I think such informations deserves a new prefix for sample. ( ssl_fhs_* ?) > For exemple, it could be used for acl in early HS (of course in http mode and > perhaps one day in tcp mode). > Oh ok you're right, I picked up fc without much thought, I wasn't sure what to use, and didn't bother ask, maybe indeed a new prefix would be wise. Regards, Olivier
Re: [PATCH] BUG/MINOR: ssl: fix CO_FL_EARLY_DATA removal with http mode
> Le 30 nov. 2017 à 13:34, Olivier Houchard a écrit : > > Hi Emmanuel, > > On Thu, Nov 30, 2017 at 12:15:37PM +0100, Emmanuel Hocdet wrote: >> Hi Olivier, >> >>> Le 29 nov. 2017 à 19:57, Olivier Houchard a écrit : >>> >>> On Mon, Nov 27, 2017 at 06:19:41PM +0100, Emmanuel Hocdet wrote: > Maybe the best is to add a new flag per conn_stream, CS_FL_WAITING_FOR_HS > or > something, instead of relying on CO_FL_EARLY_DATA. > I think I'm going to do something like that. I think it's a good idea, two different things to deal with one tag. > That still doesn't help with your problem with TCP mode, though. I still > want > the CO_FL_EARLY_DATA to be removed after the handshake, so that we don't > add the "Early-Data: 1" header if it is not needed. But it just occured > to me that I can easily fix that by adding the header, not only if > CO_FL_EARLY_DATA is set, but if CO_FL_EARLY_SSL_HS or CO_FL_SSL_WAIT_HS > is set. > That way we will both be happy :) >>> >>> So, this if my first cut at it. It basically does as I said. >>> Only thing you may be unhappy with, I made the sample fetch ssl_fc_has_early >>> return 0 if we had early data but the handshake happened, because the main >>> use case is to know if there are early data and if they're a potential >>> security risk. >>> If you can give me a case where we have a need a sample fetch to know there >>> were early data, even after the handshake, maybe we can introduce a new >>> sample fetch, ssl_fc_has_insecure_early, or something ? >>> >> >> In this case, i don’t understand the interest of ssl_fc_has_early. >> >> looking at the documentation >> ssl_fc_has_early : boolean >> Returns true if early data were sent, and the handshake didn't happen yet. >> As >> it has security implications, it is useful to be able to refuse those, or >> wait until the handshake happened. >> >> ssl_fc_* can be used after the front connection at ssl level: handshake will >> be finished at this time. >> ssl_fc_has_early should be: returns true if early data were sent and >> accepted in ssl level. (425 return is http level) >> >> What the description makes me think, and understand what you said, is that >> it could be a « ssl_hs_has_early ». >> I’m very interesting to have acl on hs negotiation, i don't know how to do >> that now in haproxy. >> > > No, sls_fc_has_early can be used before the handshake happened. If we > received early data, the request will be treated before the handshake (and > potentially the answer from the server will be sent before the handshake is > done). So ssl_fc_has_early is there to be able to stop any "dangerous" > request. > My fault, i don’t read the doc of ssl_fc_has_early: as user it confused me on the meaning and usage. What i mean is about the name of the prefix ssl_fc* ssl front connection. All ssl_fc_* can be used in log-format because is valid after the end of HS. ssl_fc_has_early is the first _fc_ with a different behavior and is not PoLA : ssl_fc_has_early can be used in http mode but it can false report in log-format, and is useless in tcp mode? but it’s ssl_fc_* ... Technically, with early HS, we have new informations who can changed after the HS. I think such informations deserves a new prefix for sample. ( ssl_fhs_* ?) For exemple, it could be used for acl in early HS (of course in http mode and perhaps one day in tcp mode). ++ Manu
Re: [PATCH] BUG/MINOR: ssl: fix CO_FL_EARLY_DATA removal with http mode
Hi Emmanuel, On Thu, Nov 30, 2017 at 12:15:37PM +0100, Emmanuel Hocdet wrote: > Hi Olivier, > > > Le 29 nov. 2017 à 19:57, Olivier Houchard a écrit : > > > > On Mon, Nov 27, 2017 at 06:19:41PM +0100, Emmanuel Hocdet wrote: > >>> Maybe the best is to add a new flag per conn_stream, CS_FL_WAITING_FOR_HS > >>> or > >>> something, instead of relying on CO_FL_EARLY_DATA. > >>> I think I'm going to do something like that. > >> > >> I think it's a good idea, two different things to deal with one tag. > >> > >>> That still doesn't help with your problem with TCP mode, though. I still > >>> want > >>> the CO_FL_EARLY_DATA to be removed after the handshake, so that we don't > >>> add the "Early-Data: 1" header if it is not needed. But it just occured > >>> to me that I can easily fix that by adding the header, not only if > >>> CO_FL_EARLY_DATA is set, but if CO_FL_EARLY_SSL_HS or CO_FL_SSL_WAIT_HS > >>> is set. > >>> That way we will both be happy :) > >> > > > > So, this if my first cut at it. It basically does as I said. > > Only thing you may be unhappy with, I made the sample fetch ssl_fc_has_early > > return 0 if we had early data but the handshake happened, because the main > > use case is to know if there are early data and if they're a potential > > security risk. > > If you can give me a case where we have a need a sample fetch to know there > > were early data, even after the handshake, maybe we can introduce a new > > sample fetch, ssl_fc_has_insecure_early, or something ? > > > > In this case, i don’t understand the interest of ssl_fc_has_early. > > looking at the documentation > ssl_fc_has_early : boolean > Returns true if early data were sent, and the handshake didn't happen yet. > As > it has security implications, it is useful to be able to refuse those, or > wait until the handshake happened. > > ssl_fc_* can be used after the front connection at ssl level: handshake will > be finished at this time. > ssl_fc_has_early should be: returns true if early data were sent and accepted > in ssl level. (425 return is http level) > > What the description makes me think, and understand what you said, is that it > could be a « ssl_hs_has_early ». > I’m very interesting to have acl on hs negotiation, i don't know how to do > that now in haproxy. > No, sls_fc_has_early can be used before the handshake happened. If we received early data, the request will be treated before the handshake (and potentially the answer from the server will be sent before the handshake is done). So ssl_fc_has_early is there to be able to stop any "dangerous" request. Regards, Olivier
Re: [PATCH] BUG/MINOR: ssl: fix CO_FL_EARLY_DATA removal with http mode
> Le 30 nov. 2017 à 12:15, Emmanuel Hocdet a écrit : > > In this case, i don’t understand the interest of ssl_fc_has_early. > > looking at the documentation > ssl_fc_has_early : boolean > Returns true if early data were sent, and the handshake didn't happen yet. > As > it has security implications, it is useful to be able to refuse those, or > wait until the handshake happened. > > ssl_fc_* can be used after the front connection at ssl level: handshake will > be finished at this time. > ssl_fc_has_early should be: returns true if early data were sent and accepted > in ssl level. (425 return is http level) > > What the description makes me think, and understand what you said, is that it > could be a « ssl_hs_has_early ». > I’m very interesting to have acl on hs negotiation, i don't know how to do > that now in haproxy. > .. in tcp mode. With acl in http mode ssl_hs_has_early could be a good name for that.
Re: [PATCH] BUG/MINOR: ssl: fix CO_FL_EARLY_DATA removal with http mode
Hi Olivier, > Le 29 nov. 2017 à 19:57, Olivier Houchard a écrit : > > On Mon, Nov 27, 2017 at 06:19:41PM +0100, Emmanuel Hocdet wrote: >>> Maybe the best is to add a new flag per conn_stream, CS_FL_WAITING_FOR_HS or >>> something, instead of relying on CO_FL_EARLY_DATA. >>> I think I'm going to do something like that. >> >> I think it's a good idea, two different things to deal with one tag. >> >>> That still doesn't help with your problem with TCP mode, though. I still >>> want >>> the CO_FL_EARLY_DATA to be removed after the handshake, so that we don't >>> add the "Early-Data: 1" header if it is not needed. But it just occured >>> to me that I can easily fix that by adding the header, not only if >>> CO_FL_EARLY_DATA is set, but if CO_FL_EARLY_SSL_HS or CO_FL_SSL_WAIT_HS is >>> set. >>> That way we will both be happy :) >> > > So, this if my first cut at it. It basically does as I said. > Only thing you may be unhappy with, I made the sample fetch ssl_fc_has_early > return 0 if we had early data but the handshake happened, because the main > use case is to know if there are early data and if they're a potential > security risk. > If you can give me a case where we have a need a sample fetch to know there > were early data, even after the handshake, maybe we can introduce a new > sample fetch, ssl_fc_has_insecure_early, or something ? > In this case, i don’t understand the interest of ssl_fc_has_early. looking at the documentation ssl_fc_has_early : boolean Returns true if early data were sent, and the handshake didn't happen yet. As it has security implications, it is useful to be able to refuse those, or wait until the handshake happened. ssl_fc_* can be used after the front connection at ssl level: handshake will be finished at this time. ssl_fc_has_early should be: returns true if early data were sent and accepted in ssl level. (425 return is http level) What the description makes me think, and understand what you said, is that it could be a « ssl_hs_has_early ». I’m very interesting to have acl on hs negotiation, i don't know how to do that now in haproxy. ++ Manu
Re: [PATCH] BUG/MINOR: ssl: fix CO_FL_EARLY_DATA removal with http mode
On Mon, Nov 27, 2017 at 06:19:41PM +0100, Emmanuel Hocdet wrote:
> > Maybe the best is to add a new flag per conn_stream, CS_FL_WAITING_FOR_HS or
> > something, instead of relying on CO_FL_EARLY_DATA.
> > I think I'm going to do something like that.
>
> I think it's a good idea, two different things to deal with one tag.
>
> > That still doesn't help with your problem with TCP mode, though. I still
> > want
> > the CO_FL_EARLY_DATA to be removed after the handshake, so that we don't
> > add the "Early-Data: 1" header if it is not needed. But it just occured
> > to me that I can easily fix that by adding the header, not only if
> > CO_FL_EARLY_DATA is set, but if CO_FL_EARLY_SSL_HS or CO_FL_SSL_WAIT_HS is
> > set.
> > That way we will both be happy :)
>
So, this if my first cut at it. It basically does as I said.
Only thing you may be unhappy with, I made the sample fetch ssl_fc_has_early
return 0 if we had early data but the handshake happened, because the main
use case is to know if there are early data and if they're a potential
security risk.
If you can give me a case where we have a need a sample fetch to know there
were early data, even after the handshake, maybe we can introduce a new
sample fetch, ssl_fc_has_insecure_early, or something ?
Regards,
Olivier
>From bda3b7800677184ea19fb81f75f9a9b44c79efeb Mon Sep 17 00:00:00 2001
From: Olivier Houchard
Date: Mon, 27 Nov 2017 18:41:32 +0100
Subject: [PATCH 1/2] MINOR: early data: Don't rely on CO_FL_EARLY_DATA to wake
up streams.
Instead of looking for CO_FL_EARLY_DATA to know if we have to try to wake
up a stream, because it is waiting for a SSL handshake, instead add a new
conn_stream flag, CS_FL_WAIT_FOR_HS. This way we don't have to rely on
CO_FL_EARLY_DATA, and we will only wake streams that are actually waiting.
---
include/types/connection.h | 1 +
src/mux_h2.c | 24 ++--
src/ssl_sock.c | 5 -
src/stream_interface.c | 4 +++-
4 files changed, 26 insertions(+), 8 deletions(-)
diff --git a/include/types/connection.h b/include/types/connection.h
index a9d04474d..f220c42a9 100644
--- a/include/types/connection.h
+++ b/include/types/connection.h
@@ -69,6 +69,7 @@ enum {
CS_FL_ERROR = 0x0100, /* a fatal error was reported */
CS_FL_EOS = 0x1000, /* End of stream */
+ CS_FL_WAIT_FOR_HS = 0x0001, /* This stream is waiting for
handhskae */
};
/* cs_shutr() modes */
diff --git a/src/mux_h2.c b/src/mux_h2.c
index 4567b8ff0..4db90d4c8 100644
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -54,6 +54,7 @@ static struct pool_head *pool_head_h2s;
/* other flags */
#define H2_CF_GOAWAY_SENT 0x0100 // a GOAWAY frame was successfully
sent
#define H2_CF_GOAWAY_FAILED 0x0200 // a GOAWAY frame failed to be sent
+#define H2_CF_WAIT_FOR_HS 0x0400 // We did check that at least a
stream was waiting for handshake
/* H2 connection state, in h2c->st0 */
@@ -2091,14 +2092,25 @@ static int h2_wake(struct connection *conn)
struct h2c *h2c = conn->mux_ctx;
/*
-* If we received early data, try to wake any stream, just in case
-* at least one of them was waiting for the handshake
+* If we received early data, and the handshake is done, wake
+* any stream that was waiting for it.
*/
- if ((conn->flags & (CO_FL_EARLY_SSL_HS | CO_FL_EARLY_DATA |
CO_FL_HANDSHAKE)) ==
- CO_FL_EARLY_DATA) {
- h2_wake_some_streams(h2c, 0, 0);
- conn->flags &= ~CO_FL_EARLY_DATA;
+ if (!(h2c->flags & H2_CF_WAIT_FOR_HS) &&
+ (conn->flags & (CO_FL_EARLY_SSL_HS | CO_FL_HANDSHAKE |
CO_FL_EARLY_DATA)) == CO_FL_EARLY_DATA) {
+ struct eb32_node *node;
+ struct h2s *h2s;
+
+ h2c->flags |= H2_CF_WAIT_FOR_HS;
+ node = eb32_lookup_ge(&h2c->streams_by_id, 1);
+
+ while (node) {
+ h2s = container_of(node, struct h2s, by_id);
+ if (h2s->cs->flags & CS_FL_WAIT_FOR_HS)
+ h2s->cs->data_cb->wake(h2s->cs);
+ node = eb32_next(node);
+ }
}
+
if (conn->flags & CO_FL_ERROR || conn_xprt_read0_pending(conn) ||
h2c->st0 == H2_CS_ERROR2 || h2c->flags & H2_CF_GOAWAY_FAILED ||
(eb_is_empty(&h2c->streams_by_id) && h2c->last_sid >= 0 &&
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index da1aecbcc..7c5fd6b10 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -8705,11 +8705,14 @@ enum act_return ssl_action_wait_for_hs(struct act_rule
*rule, struct proxy *px,
struct session *sess, struct stream *s,
int flags)
{
struct connection *conn;
+ struct conn_stream *cs;
conn = objt_conn(sess->origin);
+ cs = objt_cs(s->si[0].end);
- if (conn) {
+ if (conn && cs) {
Re: [PATCH] BUG/MINOR: ssl: fix CO_FL_EARLY_DATA removal with http mode
Hi Manu, On Wed, Nov 29, 2017 at 12:40:46PM +0100, Emmanuel Hocdet wrote: > Can you consider the first patch (included here). > As Olivier said, the fix for ssl_fc_has_early need more works. OK now merged and backported, thanks! Willy
Re: [PATCH] BUG/MINOR: ssl: fix CO_FL_EARLY_DATA removal with http mode
Hi Willy, Can you consider the first patch (included here). As Olivier said, the fix for ssl_fc_has_early need more works. Can be backported to 1.8 ++ Manu 0001-BUG-MINOR-ssl-CO_FL_EARLY_DATA-removal-is-managed-by.patch Description: Binary data
Re: [PATCH] BUG/MINOR: ssl: fix CO_FL_EARLY_DATA removal with http mode
> Le 27 nov. 2017 à 17:52, Olivier Houchard a écrit :
>
> Hi Emmanuel,
>
> On Mon, Nov 27, 2017 at 05:17:54PM +0100, Emmanuel Hocdet wrote:
>>
>> Hi,
>>
>> This patch fix CO_FL_EARLY_DATA removal to have correct ssl_fc_has_early
>> reporting. It work for 'mode http'.
>>
>> It does not fix ssl_fc_has_early for 'mode tcp'. In this mode
>> CO_FL_EARLY_DATA
>> should not be removed if early data was accepted.
>> It is possible to check MODE_TCP in mux_pt_recv? Or there is another way of
>> address this?
>>
>
Hi Olivier,
> The first patch seems fine.
> The second breaks wait-for-handshake for me, I guess because recv() is called
In my tests, wake remove the flag before recv (and ssl_fc_has_early call)
> before wake(), and so the flag is removed before that code in
> stream_interface.c :
> /* If we had early data, and the handshake ended, then
>* we can remove the flag, and attempt to wake the task up,
>* in the event there's an analyser waiting for the end of
>* the handshake.
>*/
> if ((conn->flags & (CO_FL_EARLY_DATA | CO_FL_EARLY_SSL_HS)) ==
> CO_FL_EARLY_DATA) {
> task_wakeup(si_task(si), TASK_WOKEN_MSG);
> }
> So the stream task is never woken up.
> Maybe the best is to add a new flag per conn_stream, CS_FL_WAITING_FOR_HS or
> something, instead of relying on CO_FL_EARLY_DATA.
> I think I'm going to do something like that.
I think it's a good idea, two different things to deal with one tag.
> That still doesn't help with your problem with TCP mode, though. I still want
> the CO_FL_EARLY_DATA to be removed after the handshake, so that we don't
> add the "Early-Data: 1" header if it is not needed. But it just occured
> to me that I can easily fix that by adding the header, not only if
> CO_FL_EARLY_DATA is set, but if CO_FL_EARLY_SSL_HS or CO_FL_SSL_WAIT_HS is
> set.
> That way we will both be happy :)
Indeed :)
Thanks!
Manu
Re: [PATCH] BUG/MINOR: ssl: fix CO_FL_EARLY_DATA removal with http mode
Hi Emmanuel,
On Mon, Nov 27, 2017 at 05:17:54PM +0100, Emmanuel Hocdet wrote:
>
> Hi,
>
> This patch fix CO_FL_EARLY_DATA removal to have correct ssl_fc_has_early
> reporting. It work for 'mode http'.
>
> It does not fix ssl_fc_has_early for 'mode tcp'. In this mode CO_FL_EARLY_DATA
> should not be removed if early data was accepted.
> It is possible to check MODE_TCP in mux_pt_recv? Or there is another way of
> address this?
>
The first patch seems fine.
The second breaks wait-for-handshake for me, I guess because recv() is called
before wake(), and so the flag is removed before that code in
stream_interface.c :
/* If we had early data, and the handshake ended, then
* we can remove the flag, and attempt to wake the task up,
* in the event there's an analyser waiting for the end of
* the handshake.
*/
if ((conn->flags & (CO_FL_EARLY_DATA | CO_FL_EARLY_SSL_HS)) ==
CO_FL_EARLY_DATA) {
task_wakeup(si_task(si), TASK_WOKEN_MSG);
}
So the stream task is never woken up.
Maybe the best is to add a new flag per conn_stream, CS_FL_WAITING_FOR_HS or
something, instead of relying on CO_FL_EARLY_DATA.
I think I'm going to do something like that.
That still doesn't help with your problem with TCP mode, though. I still want
the CO_FL_EARLY_DATA to be removed after the handshake, so that we don't
add the "Early-Data: 1" header if it is not needed. But it just occured
to me that I can easily fix that by adding the header, not only if
CO_FL_EARLY_DATA is set, but if CO_FL_EARLY_SSL_HS or CO_FL_SSL_WAIT_HS is set.
That way we will both be happy :)
Regards,
Olivier
[PATCH] BUG/MINOR: ssl: fix CO_FL_EARLY_DATA removal with http mode
Hi, This patch fix CO_FL_EARLY_DATA removal to have correct ssl_fc_has_early reporting. It work for 'mode http'. It does not fix ssl_fc_has_early for 'mode tcp'. In this mode CO_FL_EARLY_DATA should not be removed if early data was accepted. It is possible to check MODE_TCP in mux_pt_recv? Or there is another way of address this? Thanks ++ Manu 0001-BUG-MINOR-ssl-CO_FL_EARLY_DATA-removal-is-managed-by.patch Description: Binary data 0002-BUG-MINOR-ssl-fix-CO_FL_EARLY_DATA-removal-with-http.patch Description: Binary data
