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

Openssl 3 0 update #96

Merged
merged 11 commits into from
Oct 14, 2022
Merged

Openssl 3 0 update #96

merged 11 commits into from
Oct 14, 2022

Conversation

ColeBollig
Copy link

-Tested Compilation in Fedora 36 docker container with openssl-devel (3.0.5) and saw no compilation warnings for use of deprecated functions
-Made to be compatible with JWT-CPP 0.6.0 for its openssl 3.0 support
-Minimal testing (Due to lack of know how). scitokens-verify works for both RSA and EC methods.

-Please scrutinize everything. I don't want to break scitokens

-Changed internal code to remove use of deprecated functions
 in openssl 3.0
-Updated to be compatable with JWT-CPP 0.6.0
   -Added changes for traits namespace
   -Added error_code for sign & verify
   -Replaced jwt::token_verification_exception() due
    to change from passing string to error_code. I
    find the error_code method to be limited and
    confusing. So, I replaced with Scitoken local
    JWTVerificationException() to use custom error
    messages
Copy link
Contributor

@djw8605 djw8605 left a comment

Choose a reason for hiding this comment

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

Few minor comments. I'll test this as well before merging.

src/scitokens_internal.cpp Outdated Show resolved Hide resolved
src/scitokens_internal.cpp Outdated Show resolved Hide resolved
src/scitokens_internal.cpp Outdated Show resolved Hide resolved
@djw8605
Copy link
Contributor

djw8605 commented Sep 2, 2022

Lots of errors in the build. Could you take a look.

@ColeBollig
Copy link
Author

I believe the build failures are due to the code requiring at minimum JWT-CPP v0.5.0 for the json traits change that breaks the api and isn't getting it. In theory you want v0.6.0 for the openssl 3.0 update within JWT-CPP. I am not sure how to change it so the github build uses the correct JWT-CPP version

@ColeBollig
Copy link
Author

I have figured out how to make the base requirement of jwt-cpp

-Updated vendor to include jwt-cpp v0.6.0 not that this is the minmum
 version to have inorder to have no openssl 3.0 warnings and not break
 the jwt api
-Fixed JWTVerificationException error throw message
 to include 'token verification failed:' like it used
 to have
-PR changes
   -Fixed Openssl 3.0 code to store public ec key correctly thus
    passing Unit tests
   -Set cmake option warnings equal errors to ON thus reverting
    to old behavior of failing to compile when a warning occurs
   -Implemented JWT-CPP patch into JWT v0.6.0 library included
    within vendor to allow for rpm building with older gcc versions
   -Reverted attempted fix for rpm building in .spec file due to
    compatability update added to JWT-CPP. No longer requires
    gcc v5.1 or greater
@ColeBollig
Copy link
Author

Depending on the rpm building test and your approval I believe all changes are here. I do have a couple of things to note.

  1. You should update the Unit test to test for both Openssl 3 and older Openssl versions.
  2. This should resolve open issues #72 and #61
  3. Updated vendor copy of JWT-CPP is v0.6.0 with some added code from a later JWT-CPP patch that fixed rpm build issues. This should probably be replaced when an official version with the change is released.
  4. I kept in the code to turn off warnings becoming errors but made the default to ON. In case another deprecation warning issue occurs again.

Tested in docker Fedora:36 container with Openssl 3. scitoken-verify was successful for both RS256 and ES256 token from demo.scitokens.org, and successfully passed all unit tests.

@djw8605
Copy link
Contributor

djw8605 commented Oct 14, 2022

After a few iterations, I think I'm happy with the pull request. Summary of my changes:

  • Added newer versions of ubuntu with openssl 3.
  • Completely removed the option to turn off warnings as errors
  • Update the gtest submodule because newer compilers threw some errors

@djw8605 djw8605 merged commit 4916492 into scitokens:master Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants