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

Certificate Verification #1794

Merged
merged 2 commits into from Dec 12, 2019
Merged

Certificate Verification #1794

merged 2 commits into from Dec 12, 2019

Conversation

@yafshar
Copy link
Contributor

yafshar commented Dec 1, 2019

if SSL report an error ("certificate verify failed") during the
handshake and thus refuses further communication with that server,
you can specify your own CA cert path by setting the environment
variable CURL_CA_BUNDLE to the path of your choice.

Summary

enhancement.

Licensing

By submitting this pull request, I agree, that my contribution will be included in LAMMPS and redistributed under either the GNU General Public License version 2 (GPL v2) or the GNU Lesser General Public License version 2.1 (LGPL v2.1).

Post Submission Checklist

Please check the fields below as they are completed after the pull request has been submitted. Delete lines that don't apply

  • The feature or features in this pull request is complete
  • Licensing information is complete
  • Corresponding author information is complete
  • The source code follows the LAMMPS formatting guidelines
  • Suitable new documentation files and/or updates to the existing docs are included
  • The added/updated documentation is integrated and tested with the documentation build system
  • The feature has been verified to work with the conventional build system
  • The feature has been verified to work with the CMake based build system
  • A package specific README file has been included or updated
  • One or more example input decks are included

Further Information, Files, and Links

Put any additional information here, attach relevant text or image files, and URLs to external sites (e.g. DOIs or webpages)

@yafshar

This comment has been minimized.

Copy link
Contributor Author

yafshar commented Dec 1, 2019

@ellio167 I added an extra certificate verification, following the curl protocol docs.

@ellio167

This comment has been minimized.

Copy link
Collaborator

ellio167 commented Dec 1, 2019

@yafshar Thanks for working on this. I think it would make more sense to just check for the CURL_CA_BUNDLE env variable in kim_query.cpp directly. Involving the cmake build process seems unnecessary.

@yafshar yafshar force-pushed the yafshar:master branch from a9e2a71 to 52203af Dec 1, 2019
@yafshar

This comment has been minimized.

Copy link
Contributor Author

yafshar commented Dec 1, 2019

@ellio167 I agree with your suggestion and removed the CMake changes. But I used the c++ std::getenv to set the environment variable. Do you agree with this or would rather to use the macro version?

@ellio167

This comment has been minimized.

Copy link
Collaborator

ellio167 commented Dec 1, 2019

This looks good to me.

@yafshar yafshar marked this pull request as ready for review Dec 2, 2019
@rbberger

This comment has been minimized.

Copy link
Member

rbberger commented Dec 6, 2019

@ellio167 @yafshar I'm missing some sort of documentation about LMP_NO_SSL_CHECK and CURL_CA_BUNDLE. It also doesn't seem to be present in the CMake file.

Copy link
Member

rbberger left a comment

Document CURL_CA_BUNDLE

@ellio167

This comment has been minimized.

Copy link
Collaborator

ellio167 commented Dec 6, 2019

@rbberger Agree that we should add doc for CURL_CA_BUNDLE. However, LMP_NO_SSL_CHECK (and for that matter LMP_DEBUG_CURL) were added by @akohlmey back in 741a7fe

So, I'm not sure what should be done with those... Do you suggest that both of these be added as Package-Specific Configuration Options?

@rbberger

This comment has been minimized.

Copy link
Member

rbberger commented Dec 6, 2019

@ellio167 Sorry, I didn't know this was from @akohlmey . Ok, we'll look into documenting LMP_NO_SSL_CHECK and LMP_DEBUG_CURL. Since those are compile-time options they should be added to the build docs and the CMake build (https://lammps.sandia.gov/doc/Build_extras.html#kim).

However, this CURL_CA_BUNDLE environment variable seems to be a runtime option, so I suppose this should go into a documentation page in the Manual where the usage of KIM models is explained.

@yafshar

This comment has been minimized.

Copy link
Contributor Author

yafshar commented Dec 6, 2019

@ellio167 @rbberger @akohlmey I do not know if there is a specific reason @akohlmey put LMP_NO_SSL_CHECK and LMP_DEBUG_CURL like that. But I can update those and add the documentation as well.

@yafshar

This comment has been minimized.

Copy link
Contributor Author

yafshar commented Dec 12, 2019

@rbberger I did all the changes and added the document to the Build_extras.txt file. Should I also add it to the rst file as well?

@rbberger

This comment has been minimized.

Copy link
Member

rbberger commented Dec 12, 2019

@yafshar Yes, please recreate the RST file(s) (make -C doc rst). FYI, we're migrating away from the txt format. Future changes should be done in the RST file only. Once the txt file is no longer up-to-date, we remove it completely to avoid accidentally overriding changes.

if SSL report an error ("certificate verify failed") during the
handshake and thus refuses further communication with that server,
you can specify your own CA cert path by setting the environment
variable CURL_CA_BUNDLE to the path of your choice.
@yafshar yafshar force-pushed the yafshar:master branch from 52203af to b0d0037 Dec 12, 2019
@yafshar

This comment has been minimized.

Copy link
Contributor Author

yafshar commented Dec 12, 2019

@rbberger @ellio167 I added the changes and updated the CMAKE, txt and rst files as well.

@ellio167

This comment has been minimized.

Copy link
Collaborator

ellio167 commented Dec 12, 2019

Looks good to me. Thanks @yafshar

@rbberger

This comment has been minimized.

Copy link
Member

rbberger commented Dec 12, 2019

@yafshar one last change. Please mark the CMake options as advanced, so they are hidden by default.
I would push myself but you seem to have restricted either your repo or this PR (check allow push by maintainers)

diff --git a/cmake/Modules/Packages/KIM.cmake b/cmake/Modules/Packages/KIM.cmake
index eda8ffe82..a75e24809 100644
--- a/cmake/Modules/Packages/KIM.cmake
+++ b/cmake/Modules/Packages/KIM.cmake
@@ -6,10 +6,12 @@ if(PKG_KIM)
     list(APPEND LAMMPS_LINK_LIBS ${CURL_LIBRARIES})
     add_definitions(-DLMP_KIM_CURL)
     set(LMP_DEBUG_CURL OFF CACHE STRING "Set libcurl verbose mode on/off. If on, it displays a lot of verbose information about its operations.")
+    mark_as_advanced(LMP_DEBUG_CURL)
     if(LMP_DEBUG_CURL)
       add_definitions(-DLMP_DEBUG_CURL)
     endif()
     set(LMP_NO_SSL_CHECK OFF CACHE STRING "Tell libcurl to not verify the peer. If on, the connection succeeds regardless of the names in the certificate. Insecure - Use with caution!")
+    mark_as_advanced(LMP_NO_SSL_CHECK)
     if(LMP_NO_SSL_CHECK)
       add_definitions(-DLMP_NO_SSL_CHECK)
     endif()

@yafshar

This comment has been minimized.

Copy link
Contributor Author

yafshar commented Dec 12, 2019

@rbberger Allow edits from maintainers is on? is there anything else I should do. Please let me know if I should do it myself.

@rbberger

This comment has been minimized.

Copy link
Member

rbberger commented Dec 12, 2019

@yafshar weird. It might be because you forked from someone else, not from lammps/lammps. In any case, something on your repo is blocking me from pushing. You could add me as a contributor to your local repo's settings or just push the small change yourself.

@rbberger

This comment has been minimized.

Copy link
Member

rbberger commented Dec 12, 2019

@yafshar Ok, done. Found my mistake. Once the tests complete I'll merge this. Thanks.

@rbberger rbberger assigned rbberger and unassigned yafshar Dec 12, 2019
@rbberger rbberger merged commit 8030ff2 into lammps:master Dec 12, 2019
6 checks passed
6 checks passed
lammps/pull-requests/cmake/cmake-kokkos-omp-pr head run ended
Details
lammps/pull-requests/cmake/cmake-serial-pr head run ended
Details
lammps/pull-requests/kokkos-omp-pr head run ended
Details
lammps/pull-requests/openmpi-pr head run ended
Details
lammps/pull-requests/serial-pr head run ended
Details
lammps/pull-requests/shlib-pr head run ended
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.