Use PP SRC IP as Client IP #12761
Use PP SRC IP as Client IP #12761
Conversation
|
After sleeping on this, I decided that I don't like that the meaning of remote address in NetVConnection is overloaded for proxy protocol. So, I reverted that and instead introduced |
cmcfarlen
changed the title
…his time the copy is configurable. Expand autest to cover new setup Introduce get_client_addr to NetVConn, implement rchi log field, improve proxy protocol autests.
cmcfarlen
force-pushed
the
restore-pp-ip-replace
branch
from
8cd9a92 to
1391a66
Compare
|
autest failed: proxy_serve_stale_dns_fail |
|
[approve ci autest 1] |
| for (int i = 0; i < IpAllow::Subject::MAX_SUBJECTS; ++i) { | ||
| if (IpAllow::Subject::PEER == IpAllow::subjects[i]) { | ||
| client_ip = ssl_vc->get_remote_addr(); | ||
| client_ip = ssl_vc->get_client_addr(); |
There was a problem hiding this comment.
Hmm, I don't think we should do this. If the setting value were CLIENT then this would make sense, but "PEER" was picked as a term that means the direct peer to distinguish it from the ambiguous term "client".
Also, this is unrelated to the change, I found that this if-else lacks the support for PLUGIN (verified address). I'll add it.
There was a problem hiding this comment.
The point of this PR and the new pp-clnt flag (if configured) is to use the actual/true client address (from proxy protocol) for PEER or Client instead of the literal peer host's address. So, in all places where PEER is used, we need to check if the PP SRC IP should be used instead. This is what get_client_addr does. I'm open to better names for this function.
| const char *sni_name; | ||
| char buff[INET6_ADDRSTRLEN]; | ||
| ats_ip_ntop(netvc->get_remote_addr(), buff, INET6_ADDRSTRLEN); | ||
| ats_ip_ntop(netvc->get_client_addr(), buff, INET6_ADDRSTRLEN); |
There was a problem hiding this comment.
Looks like this function is for origin connections. If so, remote_addr means origin's address, and client_addr means ATS's address.
There was a problem hiding this comment.
Actually, it may not. But it's confusing because the client is ATS on origin connections.
There was a problem hiding this comment.
What is a better name? effective_remote_addr? I agree it's odd if the connection is not the incoming request one, but it looked like this function is generic. The only important function call change in this last commit is for ACLs. That is client side and does require get_client_addr.
|
|
||
| if (vc) { | ||
| ats_ip_ntop(vc->get_remote_addr(), ip_buf, sizeof(ip_buf)); | ||
| ats_ip_ntop(vc->get_client_addr(), ip_buf, sizeof(ip_buf)); |
There was a problem hiding this comment.
Same as above. The name doesn't make sense on origin connections.
2 participants
Restore old feature to copy PP src IP to remote ip of NetVConn, but this time the copy is configurable. Expand autest to cover new setup.Edit: This doesn't restore the overwriting the remote_addr aspect of the old behavior, but does effectively replace the client IP with the proxy protocol address. /endedit
When the proxy protocol feature was added in #3958, the behavior was that if the
server_portwas configured for proxy protocol, and there was valid information available, the PP SRC IP address was alway copied to the NetVConnection's remote IP address. This meant that for proxy protocol ports, the client's IP (as seen by the API and logs) was not the direct peer host, but the peer of the prior HOPs connection (presumably the original requesting client IP). This might be desirable if ATS was used with some load balancer ingress to provide the PP and the ATS config was written without that hop in mind. As far as I can tell, this feature was introduced for ATS 9 and back ported to later versions of 8.Later, #8544 was filed to request that the prior hop's IP address be made available to the log and an
rchibe added to that end. PR #8893 was created and that changed the behavior of the proxy protocol feature to no longer copy the PP SRC IP to the remote ip address. This fixed the chi log to always report next hop, but removed the utility of having the "true" client IP for use in plugins, acls and logging.This PR intends to revert to the mean and add the ability to restore the PP IP usage, but put that behavior behind a new server_port config. This new flag should allow the change without breaking compatability with 10, but also allow restoring the behavior that was in ATS 9.
Draft for now to finish docs and discussion.