zio-http icon indicating copy to clipboard operation
zio-http copied to clipboard

Adding more options to the SSL configuration (#2039)

Open LannyRipple opened this issue 1 year ago • 6 comments

  • Provides ClientSSLConfig FromJavaxNetSsl to allow configuring client SSLContext using a javax.net.ssl.KeyManagerFactory and/or a javax.net.ssl.TrustManagerFactory.
  • Only partially addresses #2039.

LannyRipple avatar Jan 04 '24 17:01 LannyRipple

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jan 04 '24 17:01 CLAassistant

Attempt to address some of Client SSL Configuration using javax.net.ssl. Leaving this as draft for guidance and criticism.

Things I don't like or am not sure of:

  • My naming of variables and types.
  • ClientSSLConfig.FromJavaxNetSsl has odd builder() entry points. Needed to ensure a valid configuration. Any better way?
  • Implemented tests but ClientHttpsSpec (and my extension) ignores all tests.

LannyRipple avatar Jan 04 '24 17:01 LannyRipple

Codecov Report

Attention: 58 lines in your changes are missing coverage. Please review.

Comparison is base (f2ce5cf) 64.35% compared to head (e117964) 63.91%.

Files Patch % Lines
...http/src/main/scala/zio/http/ClientSSLConfig.scala 11.11% 32 Missing :warning:
...ala/zio/http/netty/client/ClientSSLConverter.scala 0.00% 26 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2583      +/-   ##
==========================================
- Coverage   64.35%   63.91%   -0.44%     
==========================================
  Files         140      140              
  Lines        8326     8388      +62     
  Branches     1517     1505      -12     
==========================================
+ Hits         5358     5361       +3     
- Misses       2968     3027      +59     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jan 04 '24 17:01 codecov-commenter

@LannyRipple I changed the secrets from String to Secret so you have to solve some conflicts please 🙂

987Nabil avatar Jan 07 '24 22:01 987Nabil

And the ClientHttpsSpec is broken, we had before a config for a website that is no longer available via https. Therefore we deactivated the tests to fix CI. You can remove the @@ ignore to test it. If you can fix the test too, we would be very grateful 🙏

987Nabil avatar Jan 07 '24 22:01 987Nabil

@987Nabil Thanks for eyeballing and the feedback. Will fix these up.

LannyRipple avatar Jan 08 '24 16:01 LannyRipple

@LannyRipple Looks good to merge! Please re-open when conflicts are fixed and the build is passing. Thank you!

jdegoes avatar Apr 16 '24 21:04 jdegoes

Thanks. Will do when I can find the time for tests (even if just local).

LannyRipple avatar Apr 18 '24 15:04 LannyRipple