-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Allow the auth ciphers to be specified with the -c option #1208
Conversation
Thanks for the pull request! It looks like you copied the same default ciphers that are currently used. Is there some guidance on what should be the default (I don't have to deal with PCI-DSS)? I'd rather set the default to something better than force everyone to have to figure it out on their own. |
This is decided by our security officer so I don't know exactly which ciphers were bad but he chose:
If that works for you, I'll make the change. |
It might be interesting or valuable to do a packet capture of the handshake to see what is getting offered by the client and server with that cipher list string. I'm particularly interested in RC4, as OpenSSL Docs - |
What makes you think RC4 is included? Notice that
|
I didn't claim that it was included and I provided my reasoning for how RC4 could be unintentionally allowed, so I don't have anything further to offer, @chewi. |
|
Fair enough, @nbuuck. He created this list in the context of our environment, which has recent versions of OpenSSL everywhere, so perhaps this would be weaker on older systems. Having said that, you don't have to go that far back for TLS v1.2 to be unsupported anyway. I imagine these classifications evolve over time but I think it's fair to say that if RC4 is not in HIGH now then it will never be in HIGH again. |
Clearer head today. How about we go with this? HIGH:!ADH:!EXP:MD5:!RC4:!3DES:!CAMELLIA:@strength No risk of RC4 appearing. |
Should that exclude MD5?
|
This is outside of the scope of this PR - and I do think the PR should be merged - but the project's OpenSSL implementation should be reviewed. The current implementation doesn't initialize Diffie-Hellman parameters so there's no Perfect Forward Secrecy for the TLS communications. This silently excludes any cipher suites listed in a previous comment on this PR that contain the string "DHE" (in which E means Ephemeral, providing PFS) per the wiki page linked. |
@nbuuck feel free to open an issue on that |
Building
And this is the result with the latest cipher list string (including
The new cipher list string is a marked improvement, though it is worth noting the cipher suites available are significantly fewer than what should be possible with such a string when using an OpenSSL context created with |
This also updates the default cipher list. The current list is too weak for PCI-DSS.
I have amended the commit with the new list. Thanks for pointing out the Diffie-Hellman issue. |
The current list is too weak for PCI-DSS.