use tls.connect for client #1682

Merged
merged 6 commits into from Oct 25, 2016

Conversation

3 participants
@typingArtist
Collaborator

typingArtist commented Oct 20, 2016

Fixes #1678

[ ] add tests for config

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 20, 2016

Current coverage is 36.05% (diff: 0.00%)

Merging #1682 into master will increase coverage by 0.08%

@@             master      #1682   diff @@
==========================================
  Files            24         24          
  Lines          6076       6054    -22   
  Methods         785        782     -3   
  Messages          0          0          
  Branches       1536       1535     -1   
==========================================
- Hits           2186       2183     -3   
+ Misses         3890       3871    -19   
  Partials          0          0          

Powered by Codecov. Last update 87dec29...8813aee

codecov-io commented Oct 20, 2016

Current coverage is 36.05% (diff: 0.00%)

Merging #1682 into master will increase coverage by 0.08%

@@             master      #1682   diff @@
==========================================
  Files            24         24          
  Lines          6076       6054    -22   
  Methods         785        782     -3   
  Messages          0          0          
  Branches       1536       1535     -1   
==========================================
- Hits           2186       2183     -3   
+ Misses         3890       3871    -19   
  Partials          0          0          

Powered by Codecov. Last update 87dec29...8813aee

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Oct 21, 2016

Member

Tested? (manually?)

Ready to merge?

Member

msimerson commented Oct 21, 2016

Tested? (manually?)

Ready to merge?

@typingArtist

This comment has been minimized.

Show comment
Hide comment
@typingArtist

typingArtist Oct 21, 2016

Collaborator

Tested but had trouble with rejectUnauthorized. It seems as if this feature now correctly barks on a bad certificate what it never did with the old code. However, the new code raises an exception. Quite strange behavior in the old code, but similarly bad in the new one if the server stops through this.

I tried this by addressing an email (of course using swaks 😄) through Haraka to an alternative name of my current (non-Haraka) MX host. The cert is given for ##bbb##, the email is addressed to user@##aaaa##. Haraka happily receives the mail, and it starts outbound. On upgrading to TLS, the connection hangs up, as expected, and the reason is correct, but an exception is thrown, not caught, and Haraka dies.

[PROTOCOL] [844170D2-B862-44C3-AA21-322E0EC7DCA1.1.1] [outbound] S: 250-ENHANCEDSTATUSCODES\r\n
[PROTOCOL] [844170D2-B862-44C3-AA21-322E0EC7DCA1.1.1] [outbound] S: 250 HELP\r\n
[PROTOCOL] [844170D2-B862-44C3-AA21-322E0EC7DCA1.1.1] [outbound] C: STARTTLS
[PROTOCOL] [844170D2-B862-44C3-AA21-322E0EC7DCA1.1.1] [outbound] S: 220 2.0.0 try it\r\n
[DEBUG] [-] [core] client TLS upgrade in progress, awaiting secured.
[PROTOCOL] [844170D2-B862-44C3-AA21-322E0EC7DCA1.1.1] [outbound] C: EHLO ######
[ERROR] [844170D2-B862-44C3-AA21-322E0EC7DCA1.1.1] [outbound] Ongoing connection failed to 192.168.11.2:25 : Error: This socket is closed
[DEBUG] [-] [core] [outbound] release_client: 192.168.##.##:25 to undefined
[DEBUG] [-] [core] [outbound] destroying pool entry for 192.168.##.##:25
[DEBUG] [-] [core] Temp fail for: Tried all MXs
[DEBUG] [844170D2-B862-44C3-AA21-322E0EC7DCA1.1.1] [outbound] running deferred hooks
[INFO] [844170D2-B862-44C3-AA21-322E0EC7DCA1.1.1] [outbound] Temp failing 1477030756478_0_12878_892.##### for 64 seconds: Tried all MXs
[CRIT] [-] [core] Error: Hostname/IP doesn't match certificate's altnames: "Host: ##aaa##. is not in the cert's altnames: DNS:##bbb##"
[CRIT] [-] [core]     at Object.checkServerIdentity (tls.js:199:17)
[CRIT] [-] [core]     at TLSSocket.<anonymous> (_tls_wrap.js:1066:29)
[CRIT] [-] [core]     at emitNone (events.js:91:20)
[CRIT] [-] [core]     at TLSSocket.emit (events.js:185:7)
[CRIT] [-] [core]     at TLSSocket._finishInit (_tls_wrap.js:584:8)
[CRIT] [-] [core]     at TLSWrap.ssl.onhandshakedone (_tls_wrap.js:416:38)
[NOTICE] [-] [core] Shutting down

So in short: Not ready for merge, I need support. How can I avoid this exception to happen and continue with correct NDN processing?

Collaborator

typingArtist commented Oct 21, 2016

Tested but had trouble with rejectUnauthorized. It seems as if this feature now correctly barks on a bad certificate what it never did with the old code. However, the new code raises an exception. Quite strange behavior in the old code, but similarly bad in the new one if the server stops through this.

I tried this by addressing an email (of course using swaks 😄) through Haraka to an alternative name of my current (non-Haraka) MX host. The cert is given for ##bbb##, the email is addressed to user@##aaaa##. Haraka happily receives the mail, and it starts outbound. On upgrading to TLS, the connection hangs up, as expected, and the reason is correct, but an exception is thrown, not caught, and Haraka dies.

[PROTOCOL] [844170D2-B862-44C3-AA21-322E0EC7DCA1.1.1] [outbound] S: 250-ENHANCEDSTATUSCODES\r\n
[PROTOCOL] [844170D2-B862-44C3-AA21-322E0EC7DCA1.1.1] [outbound] S: 250 HELP\r\n
[PROTOCOL] [844170D2-B862-44C3-AA21-322E0EC7DCA1.1.1] [outbound] C: STARTTLS
[PROTOCOL] [844170D2-B862-44C3-AA21-322E0EC7DCA1.1.1] [outbound] S: 220 2.0.0 try it\r\n
[DEBUG] [-] [core] client TLS upgrade in progress, awaiting secured.
[PROTOCOL] [844170D2-B862-44C3-AA21-322E0EC7DCA1.1.1] [outbound] C: EHLO ######
[ERROR] [844170D2-B862-44C3-AA21-322E0EC7DCA1.1.1] [outbound] Ongoing connection failed to 192.168.11.2:25 : Error: This socket is closed
[DEBUG] [-] [core] [outbound] release_client: 192.168.##.##:25 to undefined
[DEBUG] [-] [core] [outbound] destroying pool entry for 192.168.##.##:25
[DEBUG] [-] [core] Temp fail for: Tried all MXs
[DEBUG] [844170D2-B862-44C3-AA21-322E0EC7DCA1.1.1] [outbound] running deferred hooks
[INFO] [844170D2-B862-44C3-AA21-322E0EC7DCA1.1.1] [outbound] Temp failing 1477030756478_0_12878_892.##### for 64 seconds: Tried all MXs
[CRIT] [-] [core] Error: Hostname/IP doesn't match certificate's altnames: "Host: ##aaa##. is not in the cert's altnames: DNS:##bbb##"
[CRIT] [-] [core]     at Object.checkServerIdentity (tls.js:199:17)
[CRIT] [-] [core]     at TLSSocket.<anonymous> (_tls_wrap.js:1066:29)
[CRIT] [-] [core]     at emitNone (events.js:91:20)
[CRIT] [-] [core]     at TLSSocket.emit (events.js:185:7)
[CRIT] [-] [core]     at TLSSocket._finishInit (_tls_wrap.js:584:8)
[CRIT] [-] [core]     at TLSWrap.ssl.onhandshakedone (_tls_wrap.js:416:38)
[NOTICE] [-] [core] Shutting down

So in short: Not ready for merge, I need support. How can I avoid this exception to happen and continue with correct NDN processing?

@typingArtist

This comment has been minimized.

Show comment
Hide comment
@typingArtist

typingArtist Oct 24, 2016

Collaborator

@msimerson, can you check 8b0f410, is this the right solution? Or should we address the issue of double error towards the Node team?

Collaborator

typingArtist commented Oct 24, 2016

@msimerson, can you check 8b0f410, is this the right solution? Or should we address the issue of double error towards the Node team?

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Oct 24, 2016

Member

is this the right solution

I haven't had a chance to look into it fully but that is a common issue, and adding an error handler like that one is usually The Right Thing[TM].

Member

msimerson commented Oct 24, 2016

is this the right solution

I haven't had a chance to look into it fully but that is a common issue, and adding an error handler like that one is usually The Right Thing[TM].

@typingArtist

This comment has been minimized.

Show comment
Hide comment
@typingArtist

typingArtist Oct 24, 2016

Collaborator

Thanks. Do you already have an idea how to test Haraka client/server interaction (properly, gaining coverage at the same time)?

Collaborator

typingArtist commented Oct 24, 2016

Thanks. Do you already have an idea how to test Haraka client/server interaction (properly, gaining coverage at the same time)?

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Oct 24, 2016

Member

Do you already have an idea how to test Haraka client/server interaction

Two ways:

  1. refactor a bit. Move of bunch of code you have into a big if conditional into a function. When your if hits, call the function instead of running a large block of inline code. That function can now be exported, and you can pass in mock arguments to it and test the code to see that it does what you want.
  2. a nested series of callbacks. Basically, mock a config and start up a server. When the server is up and running, it runs a callback which initiates a client. When the client succeeds or fails, it calls it's callback and the tests evaluate the results.

There's a tiny bit of coverage in tests/server.js.

Member

msimerson commented Oct 24, 2016

Do you already have an idea how to test Haraka client/server interaction

Two ways:

  1. refactor a bit. Move of bunch of code you have into a big if conditional into a function. When your if hits, call the function instead of running a large block of inline code. That function can now be exported, and you can pass in mock arguments to it and test the code to see that it does what you want.
  2. a nested series of callbacks. Basically, mock a config and start up a server. When the server is up and running, it runs a callback which initiates a client. When the client succeeds or fails, it calls it's callback and the tests evaluate the results.

There's a tiny bit of coverage in tests/server.js.

@msimerson msimerson merged commit aa6d42e into haraka:master Oct 25, 2016

2 of 3 checks passed

codecov/patch 0.00% of diff hit (target 35.97%)
Details
codecov/project 36.05% (+0.08%) compared to 87dec29
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Oct 25, 2016

Member

Hi @typingArtist , here's an example of what I mean by moving code into functions, for testability. Exporting that function enabled me to add these tests to validate the code is doing what we need.

Member

msimerson commented Oct 25, 2016

Hi @typingArtist , here's an example of what I mean by moving code into functions, for testability. Exporting that function enabled me to add these tests to validate the code is doing what we need.

@typingArtist typingArtist deleted the typingArtist:tlsconnect_client branch Oct 25, 2016

@typingArtist

This comment has been minimized.

Show comment
Hide comment
@typingArtist

typingArtist Oct 26, 2016

Collaborator

Thanks, @msimerson, I’ll try that soon. 👍

Collaborator

typingArtist commented Oct 26, 2016

Thanks, @msimerson, I’ll try that soon. 👍

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