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

Fix tls_no_tls_switching_issue. #237

Merged
merged 20 commits into from
Jun 7, 2023
Merged

Conversation

shalinnijel2
Copy link
Contributor

@shalinnijel2 shalinnijel2 commented May 10, 2023

  • Fixed a TLS issue where once TLS server is created, a TCP server wouldn't be created when requested in the following session.

@tropxy
Copy link
Contributor

tropxy commented May 20, 2023

Hey @shalinnijel2 check what I did in my PR for the TLS:
https://github.com/SwitchEV/iso15118/pull/233/files#diff-6c804a5c2a8823c6eb1ff47bd33cc864b7b1cb5bd40869bfa1a5bd5e16e4b79cL140

That should be equivalent to what you did, as you can check in the code here: https://github.com/python/cpython/blob/3.11/Lib/ssl.py#L745

we could just use the default context, but the fact is that we modify it later on for the EVCC side, also we shall not include the cafile attribute unless we want the mutual authentication. I also changed the env name, because we are not enabling TLS 1.3 with it...we are just enforcing the client to authenticate as well and that can be done in 1.2. The 1.3 Vs 1.2 will depend on the cipher suits that the client and server support...

In theory we can even use the -2 certificates for TLS 1.3 connection. let me know your thoughts about this

@shalinnijel2 shalinnijel2 force-pushed the fix_tls_no_tls_switch_issue branch from 8ce2b49 to 7fa3c0a Compare May 23, 2023 16:25
@shalinnijel2
Copy link
Contributor Author

shalinnijel2 commented May 24, 2023

Hi André -
Ok - so based on your PR:

  • Set the client/server to support all cipher suites required for -2 and -20
  • Based on what the peer supports, the negotiation during handshake would pick the right cipher suite
  • The env name change makes sense. But there is one thing that is not clear to me yet. As the pki for -20 uses a different curve, how do we handle both -2 and -20 at the same time? Would we need another env for that if it can't be handled automatically?

I have removed the mutual auth change from this PR as it was initially intended to fix the tls/tcp server switching issue seen during the testival.

@tropxy
Copy link
Contributor

tropxy commented May 24, 2023

The env name change makes sense. But there is one thing that is not clear to me yet. As the pki for -20 uses a different curve, how do we handle both -2 and -20 at the same time? Would we need another env for that if it can't be handled automatically?

so, that is a tricky one... we dont know if the connection is a -20 or -2 one until we are in supportedAppProtocol, which is then too late for the TLS handshake; I dont think we can actively control this if we want to support -2 and -20 at the same time....so I think the best here is to support all the groups by default.

i did some tests, and the Server will pick the client first supported named group in the Client Hello, that the Server also supports. Maybe in SupportedAppProtocol, we can later detect which was the agreed curve, and kill the connection if the curve is not a -20 one, but I think this may be complicated. I also tried to use the set_ecdh_curve and that sets one curve that the server supports and if we call it again it overrides the previous call, which means we cant set multiple curves with that function and there is not another one that we can use in python for that purpose.

I think for the best is really to not set any particular curve in the server and let client and server handle it between them. Once we have the TLS from Louis, maybe we can do some more sophisticated settings.

EDIT:
I have also researched about OCSP stapling and python ssl does not support it; an alternative is to use an external library like pyOpenSSL: https://github.com/pyca/pyopenssl, but that requires some significant changes to the code (I added notes about it in my PR). However, I think that lib let's set more settings than python ssl does and we may be able to select the bunch of curves the server supports....

"""
return
if self.tcp_server_handler and self.tcp_server.is_tls_enabled == with_tls:
return True
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to return it as True, it seems just 'return'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, fixed.

@shalinnijel2 shalinnijel2 requested a review from ikaratass June 7, 2023 10:22
@shalinnijel2 shalinnijel2 merged commit 58456aa into master Jun 7, 2023
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.

3 participants