Skip to content
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

Enhance CURLOPT_FTP_SKIP_PASV_IP #9758

Closed
wants to merge 1 commit into from
Closed

Enhance CURLOPT_FTP_SKIP_PASV_IP #9758

wants to merge 1 commit into from

Conversation

tenzap
Copy link

@tenzap tenzap commented Oct 18, 2022

This is a refresh of #1470
I took the original authors PR, squashed it, and tried to refresh it towards the latest version of curl

URL_FTP_SKIP_PASV_IP_ALWAYS instructs libcurl to ignore the IP address the server suggests in its 227-response to libcurl's PASV command when libcurl connects the data connection. Instead libcurl will re-use the same IP address it already uses for the control connection. But it will use the port number from the 227-response.

The allowed values are:

  • CURL_FTP_SKIP_PASV_IP_NEVER Always use the IP returned by the server.

  • CURL_FTP_SKIP_PASV_IP_ALWAYS Always ignore the IP returned by the server.

  • CURL_FTP_SKIP_PASV_IP_IF_NOT_ROUTABLE Only ignore the returned IP if it is within the private address space (see RFC1918), which is a common server configuration error.

Closes: #1455

@tenzap
Copy link
Author

tenzap commented Oct 18, 2022

@bagder maybe ?

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a test case for this as well, verifying that it works

@@ -1198,7 +1198,7 @@ ParameterError getparameter(const char *flag, /* f or -long-flag */
config->ignorecl = toggle;
break;
case 'q': /* --ftp-skip-pasv-ip */
config->ftp_skip_ip = toggle;
config->ftp_pasvp_ip_rule = CURL_FTP_SKIP_PASV_IP_ALWAYS;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this breaks behavior for users who do --no-ftp-skip-pasv-ip to switch off the option

@@ -177,7 +177,8 @@ struct OperationConfig {
bool doh_verifystatus;
bool create_dirs;
bool ftp_create_dirs;
bool ftp_skip_ip;
long ftp_pasvp_ip_rule; /* how to handle the IP address the FTP server
passes on to us */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should keep treating this as a boolean cmdline option

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

original author permits 3 values, so I don't think bool would fit here.

docs/libcurl/symbols-in-versions Outdated Show resolved Hide resolved
docs/libcurl/opts/CURLOPT_FTP_SKIP_PASV_IP.3 Outdated Show resolved Hide resolved
Always ignore the IP returned by the server.
.IP CURL_FTP_SKIP_PASV_IP_IF_NOT_ROUTABLE
Only ignore the returned IP if it is within the private address space (see RFC1918),
which is a common server configuration error.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should also mention from which version this option works.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated, but I guess @Zenju has better knowledge of what he wanted to do with this
PR

include/curl/curl.h Outdated Show resolved Hide resolved
@bagder bagder added FTP feature-window A merge of this requires an open feature window libcurl API labels Oct 20, 2022
@tenzap
Copy link
Author

tenzap commented Oct 20, 2022

@Zenju now that I squashed and refreshed your commits, would you please act on the comments?

URL_FTP_SKIP_PASV_IP_ALWAYS instructs libcurl to ignore the IP address the
server suggests in its 227-response to libcurl's PASV command when libcurl
connects the data connection. Instead libcurl will re-use the same IP address
it already uses for the control connection. But it will use the port number
from the 227-response.

The allowed values are:

- CURL_FTP_SKIP_PASV_IP_NEVER
    Always use the IP returned by the server.

- CURL_FTP_SKIP_PASV_IP_ALWAYS
    Always ignore the IP returned by the server.

- CURL_FTP_SKIP_PASV_IP_IF_NOT_ROUTABLE
    Only ignore the returned IP if it is within the private address space
    (see RFC1918), which is a common server configuration error.

Closes: #1455
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this breaks existing behavior

@bagder
Copy link
Member

bagder commented Nov 28, 2022

This can't be merged and no response in weeks. I would also like a stronger motivation why we want/need this feature. Letting the server decide IP address is a security risk.

@bagder bagder closed this Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-window A merge of this requires an open feature window FTP libcurl API needs-info
Development

Successfully merging this pull request may close these issues.

CURLOPT_FTP_SKIP_PASV_IP unusable
3 participants