-
Notifications
You must be signed in to change notification settings - Fork 327
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
Set the TLS security level early and on context #685
Set the TLS security level early and on context #685
Conversation
ac54f3c
to
32bd42a
Compare
32bd42a
to
ccee695
Compare
Unfortunately this does not help with #682. Will need to generate an SHA-1 certificate to investigate. |
dd04c5d
to
8214bcb
Compare
@martinetd In order to support SHA-1 personal certificates (see #682), I have decided to call SSL_CTX_set_min_proto_version() and SSL_CTX_set_cipher_list() on the ctx context instead of calling SSL_set_min_proto_version() and SSL_set_cipher_list() on the ssl connection only. Does this sound OK to you? |
8214bcb
to
f350dbe
Compare
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.
Thanks for the @ mention -- it looks safe to me, man page says "The list of ciphers is inherited by all ssl objects created from ctx." and it is create afterwards so should be ok.
(While I'm on the man page I see this doesn't affect TLSv1.3, which should use SSL_CTX_set_ciphersuites
instead.. but the default value looks sensible enough so I guess we don't need to; anyway unrelated to this change)
Thank you for looking into this. About TLSv1.3, see #687. |
f350dbe
to
fe46c2c
Compare
The existing code does work for cipher negotiation with old (FortiOS 4?) FortiGate appliances, but not for personal certificates (SHA-1 certificates). Two reasons I can see: * SSL_set_cipher_list() was called after messing with certificates. * SSL_set_cipher_list() applies only to the SSL connection, not to certificates, call SSL_CTX_set_cipher_list() on context instead. This change addresses both of the above issues. See SSL(7) man page: https://www.openssl.org/docs/man1.1.1/man7/ssl.html#DATA-STRUCTURES SSL_CTX (SSL Context) This is the global context structure which is created by a server or client once per program life-time and which holds mainly default values for the SSL structures which are later created for the connections. SSL (SSL Connection) This is the main SSL/TLS structure which is created by a server or client per established connection. This actually is the core structure in the SSL API. At run-time the application usually deals with this structure which has links to mostly all other structures.
fe46c2c
to
9d4ba91
Compare
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.
this makes sense to me (although I have to admit that am not so familiar with the internals of openssl). I have tested this with one of my vpns where I'm using a client certificate to log in. At least this is not broken. I'm not sure how I could test if this solves the problem that it aims to address, but I think in #682 this has already been confirmed.
The existing code does work for cipher negotiation with old (FortiOS 4?) FortiGate appliances, but not for personal certificates (SHA-1 certificates).
Two reasons I can see:
This change addresses both of the above issues.
See SSL(7) man page: