-
-
Notifications
You must be signed in to change notification settings - Fork 964
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
feat: add certificate based authentication for smtp client #2351
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2351 +/- ##
==========================================
- Coverage 76.54% 76.50% -0.04%
==========================================
Files 315 318 +3
Lines 17320 17316 -4
==========================================
- Hits 13257 13248 -9
- Misses 3127 3132 +5
Partials 936 936
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)
clientCertPath := deps.CourierConfig(ctx).CourierSMTPClientCertPath() | ||
clientKeyPath := deps.CourierConfig(ctx).CourierSMTPClientKeyPath() | ||
|
||
if clientCertPath != "" && clientKeyPath != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to provide a test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I added some unit tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you for adding tests! This looks perfect now. As a last item, could you please add a section to the docs, explaining how to use this feature? The perfect place for it would be here:
https://github.com/ory/docs/edit/master/docs/kratos/concepts/email-sms.md
Awesome, thank you! 🎉 Your contribution makes Ory better :) |
Hello @alexGNX |
This is an implementation of certificate based authentication for the smtp client, as described in design document #2350
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
works.