-
Notifications
You must be signed in to change notification settings - Fork 844
Use PP SRC IP as Client IP #12761
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Use PP SRC IP as Client IP #12761
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,7 +81,7 @@ verify_callback(int signature_ok, X509_STORE_CTX *ctx) | |
| Dbg(dbg_ctl_ssl_verify, "verification error:num=%d:%s:depth=%d", err, X509_verify_cert_error_string(err), depth); | ||
| 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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this function is for origin connections. If so, remote_addr means origin's address, and client_addr means ATS's address.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, it may not. But it's confusing because the client is ATS on origin connections.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is a better name? |
||
| if (netvc->options.sni_servername) { | ||
| sni_name = netvc->options.sni_servername.get(); | ||
| } else { | ||
|
|
@@ -110,15 +110,15 @@ verify_callback(int signature_ok, X509_STORE_CTX *ctx) | |
| sni_name = reinterpret_cast<unsigned char *>(netvc->options.sni_servername.get()); | ||
| } else { | ||
| sni_name = reinterpret_cast<unsigned char *>(buff); | ||
| ats_ip_ntop(netvc->get_remote_addr(), buff, INET6_ADDRSTRLEN); | ||
| ats_ip_ntop(netvc->get_client_addr(), buff, INET6_ADDRSTRLEN); | ||
| } | ||
| if (validate_hostname(cert, sni_name, false, &matched_name)) { | ||
| Dbg(dbg_ctl_ssl_verify, "Hostname %s verified OK, matched %s", sni_name, matched_name); | ||
| ats_free(matched_name); | ||
| } else { // Name validation failed | ||
| // Get the server address if we did't already compute it | ||
| if (netvc->options.sni_servername) { | ||
| ats_ip_ntop(netvc->get_remote_addr(), buff, INET6_ADDRSTRLEN); | ||
| ats_ip_ntop(netvc->get_client_addr(), buff, INET6_ADDRSTRLEN); | ||
| } | ||
| // If we got here the verification failed | ||
| Warning("SNI (%s) not in certificate. Action=%s server=%s(%s)", sni_name, enforce_mode ? "Terminate" : "Continue", | ||
|
|
@@ -141,7 +141,7 @@ verify_callback(int signature_ok, X509_STORE_CTX *ctx) | |
| sni_name = reinterpret_cast<unsigned char *>(netvc->options.sni_servername.get()); | ||
| } else { | ||
| sni_name = reinterpret_cast<unsigned char *>(buff); | ||
| ats_ip_ntop(netvc->get_remote_addr(), buff, INET6_ADDRSTRLEN); | ||
| ats_ip_ntop(netvc->get_client_addr(), buff, INET6_ADDRSTRLEN); | ||
| } | ||
| Warning("TS_EVENT_SSL_VERIFY_SERVER plugin failed the origin certificate check for %s. Action=%s SNI=%s", | ||
| netvc->options.ssl_servername.get(), enforce_mode ? "Terminate" : "Continue", sni_name); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -164,7 +164,7 @@ SSLDiagnostic(const SourceLocation &loc, bool debug, SSLNetVConnection *vc, cons | |
| ip_text_buffer ip_buf = {'\0'}; | ||
|
|
||
| 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)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. The name doesn't make sense on origin connections. |
||
| } | ||
|
|
||
| es = reinterpret_cast<unsigned long>(pthread_self()); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_addrdoes. I'm open to better names for this function.