Re: [External] Re: Supporting Proxy Protocol in Tomcat
On 28/07/2023 19:21, Amit Pande wrote:
Thank you all for the valuable discussion on this topic.
Is it okay to say that we're agreeing to adding proxy protocol support in
Tomcat?
I think that is a little too strong. At this point there is a proposed
approach and no one is objecting but until there is an actual patch to
discuss...
Keep in mind that any committer can veto a change.
My sense is that it should be possible to implement this feature while
addressing any concerns that may be raised but it is not guaranteed.
Mark
Thanks,
Amit
-Original Message-
From: Christopher Schultz
Sent: Thursday, July 27, 2023 4:13 PM
To: users@tomcat.apache.org
Subject: Re: [External] Re: Supporting Proxy Protocol in Tomcat
All,
On 7/27/23 12:39, Mark Thomas wrote:
On 27/07/2023 16:27, Jonathan S. Fisher wrote:
On the topic of security, may we consider a trustedProxies setting?
Seems reasonable.
We should probably look at what httpd did for all of this.
-chris
This
would be an analog to the internalProxies setting on RemoteIpValve.
It would need to be able to function with APR/NIO listening in a Unix
Domain Socket.
I'm not sure if this is super useful, but the goal would be an added
layer of security to prevent Proxy Protocol header injection.
On Thu, Jul 27, 2023 at 3:47 AM Mark Thomas wrote:
On 26/07/2023 21:53, Christopher Schultz wrote:
Mark,
On 7/26/23 13:58, Mark Thomas wrote:
I'm not a huge fan of this feature in general. I prefer supporting
features backed by specifications rather than vendor specific hacks.
I think the PROXY protocol is fairly standard, even if it's not
backed by an RFC. It's published by haproxy, but supported by
nginx,
(obviously) haproxy, AWS, httpd[1], and a whole bunch of others (
https://ww/
w.haproxy.com%2Fblog%2Fuse-the-proxy-protocol-to-preserve-a-clients-
ip-address&data=05%7C01%7CAmit.Pande%40veritas.com%7C51dbcc5eeac14fa
b5aa708db8ee67aae%7Cfc8e13c0422c4c55b3eaca318e6cac32%7C0%7C0%7C63826
0892775883704%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=RWHWpILa0
rLRM0xPgFAeXdk0y1l2ob%2BNcQHZP55fQDg%3D&reserved=0
).
ACK. That reduces my concerns somewhat.
Well, the reality is that people want to use this in the real world
and this is essentially the only way to do it, barring coming up
with a whole new protocol for the purpose (I'm looking at /you/ AJP!).
Indeed.
So why not use /the/ protocol that (a) exists and (b) is supported
by every single product that currently supports this type of thing?
My support for any patch is going to depend on the specifics of
the patch.
In addition to the comments in the BZ
- exposing the data as a request attribute is inconsistent with
other
mechanisms that solve the same problem (e.g. see
RemoteIpFilter)
+1
The whole point of PROXY is to kind of mix-together the
capabilities of both the RemoteIPFilter/Valve (which uses HTTP
headers for
source-information) and the top-level idea of a Connector
(something that binds to a socket and pushes bytes around).
The confusing thing here is that those two jobs are performed at
relatively different levels in Tomcat at the moment, as I
understand things.
Yes and no. RemoteIP[Filter|Valve] insert/modify the data at a
higher level because that is where they sit but the data originates
from the SocketWrapper.
If some kind of UberConnector could be built which essentially does
something like the following, it would be ideal:
public void accept(Socket s) {
ProxyHeader proxyHeader = readProxyHeader(s);
Connector realConnector = getRealConnector();
realConnector.setRemoteIP(proxyHeader.getRemoteIP());
realConnector.setRemotePort(proxyHeader.getRemotePort());
realConnector.takeItAway(s);
}
I'm sure there are other pieces of information that would be good
to pass-through, but the identity of the remote client is the most
interesting one.
Yes, that is the general idea. Just a couple of minor tweaks to use
the SocketWrapper rather than the Connector and to do it in a
slightly different place. The Acceptor is too early as we want to do
as little as possible on the Acceptor thread.
- needs to be implemented for all Connectors
I hope not. The connectors should be able to just have a thin layer
in front of them "sipping" the header off the beginning of the connection.
I am *way* out of my depth here when it comes to Tomcat internals
and so I don't want to appear to be telling you (Mark) "how it
works/should work", but conceptually it "seems easy". That may not
translate into "easy implementation" or it may mean "tons of
refactoring that we wouldn't need if we didn't care that much."
My point was that the provided patch only implements this for NIO.
It needs to implement it for NIO2 as well. APR/Native looks to be a
lot more difficult to implement and I'd be happy not implementing it
for APR/Native.
- I'd expect it to look more like the SNI processing
SNI p
Re: Pinned threads for HTTP2 using Virtual Threads on Tomcat 10.1.7
On 28/07/2023 13:50, Rémy Maucherat wrote:
On Thu, Jul 27, 2023 at 5:04 PM Mark Thomas wrote:
I've refactored things to the point where the unit tests run without
generating any warnings for pinning. I suspect further issues will be
identified over time and we can address those as they are found.
Ok, so the changes for Loom are: sync -> equivalent lock. That's ok
but it's a bit riskier (syncs are hard to avoid releasing).
True, but each lock I added should follow the
lock.lock();
try {
// Do stuff
} finally {
lock.unlock();
}
pattern. This is also one of things that SpotBugs checks for so we
should catch if any new locks get added incorrectly or if any of these
get changed.
Mark
Mark
On 25/07/2023 10:21, Mark Thomas wrote:
Never mind. Pretty much as soon as I hit send I managed to trigger the
issue.
Mark
On 25/07/2023 10:19, Mark Thomas wrote:
Daniel,
How did you trigger the pinning? I'm running some basic tests with
-Djdk.tracePinnedThreads=short and I'm not seeing any pinned threads
reported.
Mark
On 07/07/2023 13:45, Daniel Andres Pelaez Lopez wrote:
Mark,
Thanks for letting me know. I will wait for the August release to test.
Regards.
El jue, 6 jul 2023 a las 15:13, Mark Thomas ()
escribió:
6 Jul 2023 20:09:01 Daniel Andres Pelaez Lopez :
I am aware Tomcat community did a great effort to move Tomat to
Virtual Threads friendly, but I am not sure why HTTP2 was not part of
that effort?
The plan was always to see where the bottlenecks were as folks start to
experiment with Loom support and fix issues as they arose. It helps
focus
effort on where it is really needed.
These fixes look fairly simple. We should be able to get them done for
the August releases (the July releases have already been tagged). We
can
make -dev builds available earlier or you can build Tomcat from
source to
test the changes if you are interested.
As it happens I've spent most of today looking at ThreadLocal vs
SynchronizedStack vs new Object() in various places in the Tomcat code
base without reaching a clear conclusion. Which kind of proves the
point
that if we guess where bottlenecks might be we'll probably be wrong.
Mark
-
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org
-
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org
-
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org
-
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org
-
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org
-
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org
RE: [External] Re: Supporting Proxy Protocol in Tomcat
Thank you all for the valuable discussion on this topic.
Is it okay to say that we're agreeing to adding proxy protocol support in
Tomcat?
Thanks,
Amit
-Original Message-
From: Christopher Schultz
Sent: Thursday, July 27, 2023 4:13 PM
To: users@tomcat.apache.org
Subject: Re: [External] Re: Supporting Proxy Protocol in Tomcat
All,
On 7/27/23 12:39, Mark Thomas wrote:
> On 27/07/2023 16:27, Jonathan S. Fisher wrote:
>> On the topic of security, may we consider a trustedProxies setting?
>
> Seems reasonable.
We should probably look at what httpd did for all of this.
-chris
>> This
>> would be an analog to the internalProxies setting on RemoteIpValve.
>> It would need to be able to function with APR/NIO listening in a Unix
>> Domain Socket.
>>
>> I'm not sure if this is super useful, but the goal would be an added
>> layer of security to prevent Proxy Protocol header injection.
>>
>> On Thu, Jul 27, 2023 at 3:47 AM Mark Thomas wrote:
>>
>>> On 26/07/2023 21:53, Christopher Schultz wrote:
Mark,
On 7/26/23 13:58, Mark Thomas wrote:
> I'm not a huge fan of this feature in general. I prefer supporting
> features backed by specifications rather than vendor specific hacks.
I think the PROXY protocol is fairly standard, even if it's not
backed by an RFC. It's published by haproxy, but supported by
nginx,
(obviously) haproxy, AWS, httpd[1], and a whole bunch of others (
>>> https://ww/
>>> w.haproxy.com%2Fblog%2Fuse-the-proxy-protocol-to-preserve-a-clients-
>>> ip-address&data=05%7C01%7CAmit.Pande%40veritas.com%7C51dbcc5eeac14fa
>>> b5aa708db8ee67aae%7Cfc8e13c0422c4c55b3eaca318e6cac32%7C0%7C0%7C63826
>>> 0892775883704%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
>>> luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=RWHWpILa0
>>> rLRM0xPgFAeXdk0y1l2ob%2BNcQHZP55fQDg%3D&reserved=0
>>> ).
>>>
>>> ACK. That reduces my concerns somewhat.
>>>
Well, the reality is that people want to use this in the real world
and this is essentially the only way to do it, barring coming up
with a whole new protocol for the purpose (I'm looking at /you/ AJP!).
>>>
>>> Indeed.
>>>
So why not use /the/ protocol that (a) exists and (b) is supported
by every single product that currently supports this type of thing?
> My support for any patch is going to depend on the specifics of
> the patch.
>
> In addition to the comments in the BZ
> - exposing the data as a request attribute is inconsistent with
> other
> mechanisms that solve the same problem (e.g. see
> RemoteIpFilter)
+1
The whole point of PROXY is to kind of mix-together the
capabilities of both the RemoteIPFilter/Valve (which uses HTTP
headers for
source-information) and the top-level idea of a Connector
(something that binds to a socket and pushes bytes around).
The confusing thing here is that those two jobs are performed at
relatively different levels in Tomcat at the moment, as I
understand things.
>>>
>>> Yes and no. RemoteIP[Filter|Valve] insert/modify the data at a
>>> higher level because that is where they sit but the data originates
>>> from the SocketWrapper.
>>>
If some kind of UberConnector could be built which essentially does
something like the following, it would be ideal:
public void accept(Socket s) {
ProxyHeader proxyHeader = readProxyHeader(s);
Connector realConnector = getRealConnector();
realConnector.setRemoteIP(proxyHeader.getRemoteIP());
realConnector.setRemotePort(proxyHeader.getRemotePort());
realConnector.takeItAway(s);
}
I'm sure there are other pieces of information that would be good
to pass-through, but the identity of the remote client is the most
interesting one.
>>>
>>> Yes, that is the general idea. Just a couple of minor tweaks to use
>>> the SocketWrapper rather than the Connector and to do it in a
>>> slightly different place. The Acceptor is too early as we want to do
>>> as little as possible on the Acceptor thread.
>>>
> - needs to be implemented for all Connectors
I hope not. The connectors should be able to just have a thin layer
in front of them "sipping" the header off the beginning of the connection.
I am *way* out of my depth here when it comes to Tomcat internals
and so I don't want to appear to be telling you (Mark) "how it
works/should work", but conceptually it "seems easy". That may not
translate into "easy implementation" or it may mean "tons of
refactoring that we wouldn't need if we didn't care that much."
>>>
>>> My point was that the provided patch only implements this for NIO.
>>> It needs to implement it for NIO2 as well. APR/Native looks to be a
>>> lot more difficult to implement and I'd be happy not implementing it
>>> for APR/Native.
>>>
> - I'd expect it to look m
Re: Pinned threads for HTTP2 using Virtual Threads on Tomcat 10.1.7
On Thu, Jul 27, 2023 at 5:04 PM Mark Thomas wrote: > > I've refactored things to the point where the unit tests run without > generating any warnings for pinning. I suspect further issues will be > identified over time and we can address those as they are found. Ok, so the changes for Loom are: sync -> equivalent lock. That's ok but it's a bit riskier (syncs are hard to avoid releasing). > Mark > > > On 25/07/2023 10:21, Mark Thomas wrote: > > Never mind. Pretty much as soon as I hit send I managed to trigger the > > issue. > > > > Mark > > > > > > On 25/07/2023 10:19, Mark Thomas wrote: > >> Daniel, > >> > >> How did you trigger the pinning? I'm running some basic tests with > >> -Djdk.tracePinnedThreads=short and I'm not seeing any pinned threads > >> reported. > >> > >> Mark > >> > >> > >> On 07/07/2023 13:45, Daniel Andres Pelaez Lopez wrote: > >>> Mark, > >>> > >>> Thanks for letting me know. I will wait for the August release to test. > >>> > >>> Regards. > >>> > >>> > >>> El jue, 6 jul 2023 a las 15:13, Mark Thomas () > >>> escribió: > > > 6 Jul 2023 20:09:01 Daniel Andres Pelaez Lopez : > > > I am aware Tomcat community did a great effort to move Tomat to > > Virtual Threads friendly, but I am not sure why HTTP2 was not part of > > that effort? > > The plan was always to see where the bottlenecks were as folks start to > experiment with Loom support and fix issues as they arose. It helps > focus > effort on where it is really needed. > > These fixes look fairly simple. We should be able to get them done for > the August releases (the July releases have already been tagged). We > can > make -dev builds available earlier or you can build Tomcat from > source to > test the changes if you are interested. > > As it happens I've spent most of today looking at ThreadLocal vs > SynchronizedStack vs new Object() in various places in the Tomcat code > base without reaching a clear conclusion. Which kind of proves the > point > that if we guess where bottlenecks might be we'll probably be wrong. > > Mark > > - > To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org > For additional commands, e-mail: users-h...@tomcat.apache.org > > >>> > >>> > >> > >> - > >> To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org > >> For additional commands, e-mail: users-h...@tomcat.apache.org > >> > > > > - > > To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org > > For additional commands, e-mail: users-h...@tomcat.apache.org > > > > - > To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org > For additional commands, e-mail: users-h...@tomcat.apache.org > - To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org For additional commands, e-mail: users-h...@tomcat.apache.org
