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

Do not create a top level secureContext #1787

Merged
merged 2 commits into from
Jan 30, 2017

Conversation

gramakri
Copy link
Collaborator

This causes problems because the secureContext ends up being used by
Haraka in server and client mode. Currently, the server mode loads
TLS keys as part of the context in (plugins/tls_socket). The client
mode does not load keys (in smtp_client.js). This discrepancy causes
tls connections to fail when there is an incoming mail immediately
followed by an outgoing mail.

This causes problems because the secureContext ends up being used by
Haraka in server and client mode. Currently, the server mode loads
TLS keys as part of the context in (plugins/tls_socket). The client
mode does not load keys (in smtp_client.js). This discrepancy causes
tls connections to fail when there is an incoming mail immediately
followed by an outgoing mail.
@codecov-io
Copy link

codecov-io commented Jan 28, 2017

Current coverage is 36.11% (diff: 0.00%)

Merging #1787 into master will increase coverage by <.01%

@@             master      #1787   diff @@
==========================================
  Files            22         22          
  Lines          5795       5792     -3   
  Methods         750        750          
  Messages          0          0          
  Branches       1454       1453     -1   
==========================================
- Hits           2093       2092     -1   
+ Misses         3702       3700     -2   
  Partials          0          0          

Powered by Codecov. Last update d5e98ee...f0d8d21

@gramakri
Copy link
Collaborator Author

This was the root cause for #1768. To reproduce this manually:

  1. send mail from haraka to say server S1. the SMTPClient creates a TLS context without "key".
  2. send mail to haraka from server S1. the tls connection will fail. the server re-uses the above context (it requires "key").

@msimerson
Copy link
Member

I think this is fine, but I'd like to test this before merging. I'll try to get that done tomorrow.


secureContext = tls.createSecureContext(options);
var secureContext = tls.createSecureContext(options);
return secureContext;
Copy link
Member

Choose a reason for hiding this comment

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

Might as well remove the variable declaration entirely and just return tls.createSecureContext(options);

@msimerson msimerson self-requested a review January 29, 2017 08:12
Copy link
Member

@msimerson msimerson left a comment

Choose a reason for hiding this comment

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

Tested. Works for me too. Other than the one minor optimization I suggested, I'd be happy to merge this.

@msimerson
Copy link
Member

I've made the change in the PR. Do you agree @gramakri?

@gramakri
Copy link
Collaborator Author

@msimerson yes, OK with me, thanks!

@msimerson msimerson merged commit dd0b292 into haraka:master Jan 30, 2017
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.

None yet

3 participants