-
Notifications
You must be signed in to change notification settings - Fork 98
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: Enable TLS 1.3 with mutual auth (AB:2378) #115
Conversation
iso15118/shared/security.py
Outdated
if ENABLE_TLS_1_3: | ||
ssl_context = ssl.create_default_context( | ||
purpose=Purpose.CLIENT_AUTH if server_side else Purpose.SERVER_AUTH, | ||
cafile=CertPath.OEM_ROOT_PEM if server_side else CertPath.V2G_ROOT_PEM, |
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.
shouldnt it be:
cafile=CertPath.V2G_ROOT_PEM if server_side else CertPath.OEM_ROOT_PEM,
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.
It looks like caFile points to the certificate required to verify the peer's chain ...
https://docs.python.org/3/library/ssl.html#ssl.create_default_context
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.
as discussed, the create_default_context already does the same settings as later done in the code; instead we can do the following:
if ENABLE_TLS_1_3:
ssl_context = SSLContext(protocol=ssl.PROTOCOL_TLS)
else:
ssl_context = SSLContext(protocol=ssl.PROTOCOL_TLSv1_2)
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.
Sure, done.
iso15118/shared/security.py
Outdated
ssl_context = SSLContext(protocol=PROTOCOL_TLSv1_2) | ||
|
||
if ENABLE_TLS_1_3: | ||
ssl_context = ssl.SSLContext() |
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.
creating the context without specifying the protocol is deprecated. select ssl.PROTOCOL_TLS instead
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.
Oops...changed now...
Summary of changes: