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

add support for TLS per-connection #334

Merged
merged 3 commits into from
Jul 26, 2022

Conversation

drakkan
Copy link
Collaborator

@drakkan drakkan commented Jul 20, 2022

Alternative approach to support TLS per-connection

@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #334 (4be51eb) into main (333ec9c) will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #334      +/-   ##
==========================================
+ Coverage   85.67%   85.86%   +0.18%     
==========================================
  Files          11       11              
  Lines        1543     1563      +20     
==========================================
+ Hits         1322     1342      +20     
  Misses        152      152              
  Partials       69       69              
Impacted Files Coverage Δ
client_handler.go 86.53% <100.00%> (+0.49%) ⬆️
handle_auth.go 92.50% <100.00%> (+1.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 333ec9c...4be51eb. Read the comment docs.

@drakkan
Copy link
Collaborator Author

drakkan commented Jul 20, 2022

The test coverage misses are the same as #331 and #332 and so unrelated to this patch

Copy link
Owner

@fclairamb fclairamb left a comment

Choose a reason for hiding this comment

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

I stay by what I said here, it's much easier but adds one more field to the ClientHandler.

... which is fine. I'll start a discussion about this topic and ways to reduce the bloating memory footprint.

@drakkan
Copy link
Collaborator Author

drakkan commented Jul 24, 2022

I stay by what I said here, it's much easier but adds one more field to the ClientHandler.

... which is fine. I'll start a discussion about this topic and ways to reduce the bloating memory footprint.

@fclairamb I don't want to force you to accept this PR instead of #312. This is only to demostrate the alternative approach I proposed in #312. I prefer this one for the reasons explained there

@fclairamb
Copy link
Owner

Sorry I forgot to answer. Both options are fine to me, I prefer this code though.

@drakkan
Copy link
Collaborator Author

drakkan commented Jul 26, 2022

Sorry I forgot to answer. Both options are fine to me, I prefer this code though.

Thanks, this code allows me to avoid checking if the property is set for the SFTPGo user every time a transfer connection is opened. I'll be adding this functionality to SFTPGo later today after a final code review

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

Successfully merging this pull request may close these issues.

None yet

2 participants