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 SHA256 finger prints for SSH servers #7646

Closed
wants to merge 1 commit into from

Conversation

@Axis-Mats
Copy link

@Axis-Mats Axis-Mats commented Aug 27, 2021

Add support for SHA256 fingerprint in command line curl
and in libcurl.
https://curl.se/docs/todo.html#Support_better_than_MD5_hostkey

N.B. This patch should not be merged until the Curl-patch: #7646 has been released. Target release, for the Curl-project, is 7.80.0. Since this patch is conditional on the 7.80.0 release this patch has to be tested against 7.80.0.

@Axis-Mats
Copy link
Author

@Axis-Mats Axis-Mats commented Aug 27, 2021

Hi,
This commit is intended to serve as discussion material for a good implementation of (https://curl.se/docs/todo.html#Support_better_than_MD5_hostkey) support for SHA256 fingerprints.
Support for SHA256 fingerprint works in libcurl. We have started to implement SHA256 support for command line curl.
Some documentation has been made, but I guess not all of it.
We have not yet implemented any tests for SHA256.

@Axis-Mats
Copy link
Author

@Axis-Mats Axis-Mats commented Aug 30, 2021

I have made a mistake with this commit. It is based on Tag:curl-7_78_0 and not master. Would it be possible for me to move my changes to master-branch and continue this commit or must I make another pull-request?

@bagder
Copy link
Member

@bagder bagder commented Aug 30, 2021

Just rebase it and force-push, no need to create a new one!

@Axis-Mats Axis-Mats force-pushed the sftp_sha256 branch 2 times, most recently from c955ab8 to 918caa7 Sep 1, 2021
@Axis-Mats
Copy link
Author

@Axis-Mats Axis-Mats commented Sep 1, 2021

Hi,
I see that, so far, two 'checks' are failing: LGTM analysis:C/C++ and curl/check.
I have checked the logs and it seems that the define 'LIBSSH2_HOSTKEY_HASH_SHA256' is missing.
I had the same issue when I was developing this patch, on Linux Debian. I solved it by installing version 1.9.0-2 of libssh2-1 and libssh2-1-dev, again for Linux Debian.
Could it be that the build environment for the LGTM analysis:C/C++ and curl/check checks are to old?
Do you have any suggestions on how to solve this issue?
BR

@Axis-Mats
Copy link
Author

@Axis-Mats Axis-Mats commented Sep 1, 2021

Hi,
Could someone assist with the error in 'continous-integration/appveyor/pr (https://ci.appveyor.com/project/curlorg/curl/builds/40603646)?
I don't understand what is going wrong here.
BR

@bagder
Copy link
Member

@bagder bagder commented Sep 2, 2021

I have checked the logs and it seems that the define 'LIBSSH2_HOSTKEY_HASH_SHA256' is missing.

Yes, since that's a recent addition to libssh2 you cannot assume that the user has such a new installation present. You need to make the code handle older versions as well as new enough: #ifdef accordingly..

Could someone assist with the error

It looks like our "normal" flaky Windows CI failures. Not your fault. 😢

@Axis-Mats
Copy link
Author

@Axis-Mats Axis-Mats commented Sep 2, 2021

Would it be good if we #ifdef like this? The new command-line option, STRING_SSH_HOST_PUBLIC_KEY_SHA256 and help texts always exist with the patch. If STRING_SSH_HOST_PUBLIC_KEY_SHA256 is passed and LIBSSH2_HOSTKEY_HASH_SHA256 is not available, we return an error from ssh_check_fingerprint in libssh2.c.

Copy link
Member

@bagder bagder left a comment

Also note that this is too late for inclusion in 7.79.0 but will have a good chance for 7.80.0

docs/libcurl/opts/CURLOPT_SSH_HOST_PUBLIC_KEY_SHA256.3 Outdated Show resolved Hide resolved
docs/libcurl/opts/CURLOPT_SSH_HOST_PUBLIC_KEY_SHA256.3 Outdated Show resolved Hide resolved
lib/setopt.c Show resolved Hide resolved
lib/vssh/libssh2.c Show resolved Hide resolved
lib/vssh/libssh2.c Outdated Show resolved Hide resolved
@bagder bagder changed the title Add SHA256 finger print Support SHA256 finger prints for SSH servers Sep 6, 2021
@Axis-Mats
Copy link
Author

@Axis-Mats Axis-Mats commented Sep 7, 2021

I don't understand what goes wrong with the curl/check test that are failing: https://github.com/curl/curl/pull/7646/checks?check_run_id=3524545729.
There are some tests that are failing: https://curl.zuul.vexxhost.dev/build/ccad3bce106f43e186f7641a77f1bb96
And some documentation that is failing: https://curl.zuul.vexxhost.dev/build/d99f16854ce7441da22f698e4479452a
Could you help us figuring out what is failing?

@bagder
Copy link
Member

@bagder bagder commented Sep 9, 2021

those links have only given me "502 Bad Gateway" for the last day or two! 😭

@Axis-Mats
Copy link
Author

@Axis-Mats Axis-Mats commented Sep 9, 2021

Yepp, it the same for me. Would it possible to restart/rerun the tests and maybe get new links?

@Axis-Mats
Copy link
Author

@Axis-Mats Axis-Mats commented Sep 13, 2021

Hi, The test links seems to be up and running again.
The tests that are failing, https://curl.zuul.vexxhost.dev/build/ccad3bce106f43e186f7641a77f1bb96, seems to be related to some Samba server failing to start "TESTINFO: "failed starting SMB server" 1 time (1451)".

@Axis-Mats
Copy link
Author

@Axis-Mats Axis-Mats commented Sep 16, 2021

Hi,
Could this patch be considered ready enough that no major changes are remaining? In that case we can use this patch as a 'local patch' in our code while waiting for this patch to be integrated/merged to master and released in an official curl release.
BR

@bagder
Copy link
Member

@bagder bagder commented Sep 16, 2021

I believe the distcheck failed CI job is still a valid error. I think because CURLOPT_SSH_HOST_PUBLIC_KEY_SHA256.3 is not added in the Makefile.inc file in the same directory.

@Axis-Mats
Copy link
Author

@Axis-Mats Axis-Mats commented Sep 17, 2021

When I build curl locally I don't see the issue with CURLOPT_SSH_HOST_PUBLIC_KEY_SHA256.3 missing in the Makefile.inc. I build curl and the tests. Is there a way to build the documentation separately or how can I trigger this issue?

@bagder
Copy link
Member

@bagder bagder commented Sep 17, 2021

The 'distcheck' CI job builds a release tarball and then verifies that some key things within the tarball is correct and matches what's in git. Like this:

curl/scripts/zuul/script.sh

Lines 130 to 162 in 4a46177

# find BOM markers and exit if we do
! git grep `printf '\xef\xbb\xbf'`
./configure --without-ssl
make
./maketgz 99.98.97
# verify in-tree build - and install it
tar xf curl-99.98.97.tar.gz
cd curl-99.98.97
./configure --prefix=$HOME/temp --without-ssl
make
make TFLAGS=1 test
make install
# basic check of the installed files
cd ..
bash scripts/installcheck.sh $HOME/temp
rm -rf curl-99.98.97
# verify out-of-tree build
tar xf curl-99.98.97.tar.gz
touch curl-99.98.97/docs/{cmdline-opts,libcurl}/Makefile.inc
mkdir build
cd build
../curl-99.98.97/configure --without-ssl
make
make TFLAGS='-p 1 1139' test
# verify cmake build
cd ..
rm -rf curl-99.98.97
tar xf curl-99.98.97.tar.gz
cd curl-99.98.97
mkdir build
cd build
cmake ..
make

I think you can reproduce this particular failure locally like this:

  1. build curl with configure --prefix=$custominstall
  2. make install
  3. bash scripts/installcheck.sh $custominstall

The installcheck.sh script verifies that the docs and include directories that were installed have the same contents as the docs and include files in your build tree. They will differ if you for example forget to add a man page to the tarball.

@Axis-Mats
Copy link
Author

@Axis-Mats Axis-Mats commented Sep 17, 2021

Will 'make install' install my curl-fork on my system?

@bagder
Copy link
Member

@bagder bagder commented Sep 17, 2021

Yes, which is why you want to use the --prefix flag to configure and point out a safe/harmless path to do the installation to. It will otherwise default to install in /usr/local/, which in most systems won't be writable by "normal users" so make install as a user will fail .

@bagder
Copy link
Member

@bagder bagder commented Sep 17, 2021

In my most common build, I use ./configure --prefix=$HOME/test-curl-install ..

Added support for SHA256 fingerprint in command line curl
and in libcurl.

https://curl.se/docs/todo.html#Support_better_than_MD5_hostkey

Change-Id: Icccd204b3b9a0066ab1760cac20913bd76ac803c
@Axis-Mats
Copy link
Author

@Axis-Mats Axis-Mats commented Sep 17, 2021

The test you suggested worked locally. I could reproduce the error. Now it looks fine:
bash scripts/installcheck.sh $HOME/test-curl-install installcheck: installed libcurl docs and include files look good
Thanks for helping out.

@Axis-Mats
Copy link
Author

@Axis-Mats Axis-Mats commented Sep 17, 2021

Some of the checks are still failing.
https://ci.appveyor.com/project/curlorg/curl/builds/40805640/job/5y4hsx3j0ok1kh4v#L4237:
"09:15:22.156000 Error removing lock file log/serverlogs.lock error: 2 No such file or directory"
We can not reproduce that error. All test succeed when we run the locally.

When it comes to the other errors we don't understand what goes wrong:
https://github.com/curl/curl/pull/7646/checks?check_run_id=3630868575
Not much log-info to go on.
Could you help out?

@Axis-Mats
Copy link
Author

@Axis-Mats Axis-Mats commented Sep 21, 2021

Hi,
Anyone that have had a chance to look at the errors in the tests?
I found a link where you could rerun the curl.curl, windows, tests, but you needed some Azure-login for that.
BR

@bagder
Copy link
Member

@bagder bagder commented Sep 26, 2021

The CI failures are "the usual" flaky ones. Not the fault of your PR.

@bagder bagder closed this in d1e7d91 Sep 26, 2021
@bagder
Copy link
Member

@bagder bagder commented Sep 26, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants