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

Support SSH Include directive #51

Merged
merged 1 commit into from Dec 7, 2023

Conversation

DVDAndroid
Copy link
Contributor

This PR adds support for parsing of Include directive used in ssh config file during the sites import process

@DVDAndroid
Copy link
Contributor Author

I've created a post in the forum but it looks like it requires admin approval.
I'm not a C++ expert, so please review it

@martinprikryl martinprikryl changed the base branch from master to dev December 5, 2023 11:33
@martinprikryl
Copy link
Member

Can you re-post your change against the dev branch? If I do it myself, you will lose your contribution :)

Also note that on the "dev" branch, there's ConvertPathFromOpenssh function, which already does some of the logic that you are doing in when handling the path in Include directive

@DVDAndroid
Copy link
Contributor Author

For some reason, WinSCP won't build anymore with the dev branch. I've already re-built all dependencies but got no luck.

[ilink32 Error] Error: Unresolved external '_SSL_get_peer_certificate' referenced from J:\WORKSPACES\C++\WINSCP\SOURCE\NEON.LIB|ne_openssl
[ilink32 Error] Error: Unresolved external '_EVP_PKEY_id' referenced from J:\WORKSPACES\C++\WINSCP\SOURCE\NEON.LIB|ne_openssl
[ilink32 Error] Error: Unresolved external '_EVP_PKEY_bits' referenced from J:\WORKSPACES\C++\WINSCP\SOURCE\NEON.LIB|ne_openssl

You can take my changes and rebase them on the dev branch, no problem if there's no attribution.

I also don't know how I should handle ConvertPathFromOpenssh, since it doesn't cover all cases

  • path starting with ~: already present
  • path with spaces and quotes: to be added, easy edits
  • path with spaces and no quotes: how to handle it? returning null? It may break other ConvertPathFromOpenssh usages. Case to be handled since ssh doesn't support this path type

@martinprikryl
Copy link
Member

I guess you didn't rebuilt the neon library. In WinSCP 6.1 (OpenSSL 3.1), the SSL_get_peer_certificate is an actual function. In WinSCP 6.2 (OpenSSL 3.2), the SSL_get_peer_certificate is just a #define to SSL_get1_peer_certificate. The fact that the linker misses the SSL_get_peer_certificate (not SSL_get1_peer_certificate) in neon.lib, indicates that it was not rebuilt against OpenSSL 3.2 headers.

@DVDAndroid
Copy link
Contributor Author

[ilink32 Error] Error: Unresolved external '_SSL_get_peer_certificate' referenced from J:\WORKSPACES\C++\WINSCP\SOURCE\NEON.LIB|ne_openssl
[ilink32 Error] Error: Unresolved external '_EVP_PKEY_id' referenced from J:\WORKSPACES\C++\WINSCP\SOURCE\NEON.LIB|ne_openssl
[ilink32 Error] Error: Unresolved external '_EVP_PKEY_bits' referenced from J:\WORKSPACES\C++\WINSCP\SOURCE\NEON.LIB|ne_openssl

I just tested it again and I noticed that lib files were not copied in source\Win32 folder automatically. I did it manually and got it compiling. Thanks for the tip

I also don't know how I should handle ConvertPathFromOpenssh, since it doesn't cover all cases

* path starting with `~`: already present

* path with spaces and quotes: to be added, easy edits

* path with spaces and no quotes: how to handle it? returning null? It may break other `ConvertPathFromOpenssh` usages. Case to be handled since ssh doesn't support this path type

What about the third question? Should I return null? Should I throw something? Should I skip this case?

@martinprikryl
Copy link
Member

Sorry, I missed your question. I do not think you need to do anything. Parsing of the directives is out of scope of the ConvertPathFromOpenssh. The quotes are already handled by CutOpensshToken. And if the argument contains spaces and there are no quotes, the line be ignored and ConvertPathFromOpenssh won't even be called.

@DVDAndroid
Copy link
Contributor Author

I rebased on dev branch. I had to edit ConvertPathFromOpenssh since it was not supporting lines like Include myfile or even IdentityFile myfile (myfile is in the .ssh folder).
Tell me if I need to split commits

@martinprikryl martinprikryl merged commit 8a5ec49 into winscp:dev Dec 7, 2023
@martinprikryl
Copy link
Member

martinprikryl commented Dec 7, 2023

While OpenSSH indeed resolves Include relative paths relatively to .ssh, it does not look like it does the same with IdentityFile relative paths. They seem to be resolved relatively to the current working directory, as most relative paths.

@martinprikryl
Copy link
Member

@martinprikryl
Copy link
Member

My updates:
937c2a1
Thanks for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants