Skip to content

Fix https:// notifications if TLSvX is mandatory #223

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

Merged
merged 1 commit into from
Jun 21, 2014
Merged

Conversation

dgeo
Copy link

@dgeo dgeo commented Jun 19, 2014

Nowadays servers tend to use TLSv1.[12] and SSLv3 will start to disappear
as only ie6 requires it…
In the post-receive hook SSLv3 is forced and notification fails
if server only accepts TLSv1.[12](eg: server gets 'A+' on https://www.ssllabs.com/ssltest/)

remote: Notifying Redmine about changes to this repository : 'project/repo' ...
remote: Redmine URL : 'https://myforge/githooks/post-receive/redmine/project'
remote: HTTP_ERROR : SSL_connect returned=1 errno=0 state=SSLv3 read server hello A: sslv3 alert handshake failure
remote: Error contacting Redmine about changes to this repo.

This change let ruby's ssl stack negociate protocol with the server (works for me™ with a TLS-only server)

Nowadays servers tend to use TLSv1.[12] and SSLv3 will start to disappear
as only ie6 requires it…
In the post-receive hook SSLv3 is forced and notification fails
if server only accepts TLSv1.[12]
(eg: server gets 'A+' on https://www.ssllabs.com/ssltest/)

```
remote: Notifying Redmine about changes to this repository : 'project/repo' ...
remote: Redmine URL : 'https://myforge/githooks/post-receive/redmine/project'
remote: HTTP_ERROR : SSL_connect returned=1 errno=0 state=SSLv3 read server hello A: sslv3 alert handshake failure
remote: Error contacting Redmine about changes to this repo.
```
@@ -83,7 +83,6 @@ def run_http_query(git_config)

if url.scheme == 'https'
http.use_ssl = true
http.ssl_version = :SSLv3 if http.respond_to?(:ssl_version)
Copy link
Author

Choose a reason for hiding this comment

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

default seems to negociate SSL/TLS protocol

@n-rodriguez n-rodriguez self-assigned this Jun 20, 2014
@n-rodriguez
Copy link
Contributor

Hi! Thanks for your feedback!

I don't know if I can merge this patch now as it conflicts with #53.

@dgeo
Copy link
Author

dgeo commented Jun 20, 2014

I dont think sslv2 is still default...
And locking to sslv3 is not future-proof.
My server doesn't accept v2 nor v3, only tlsv1.x, ant it works without forcing anything...
Should be safe for most (in my opinion).
Openssl's way to deal with that is the opposite: you can just forbid some versions of the protocol.
My 2 cts

n-rodriguez pushed a commit that referenced this pull request Jun 21, 2014
Fix https:// notifications if TLSvX is mandatory
@n-rodriguez n-rodriguez merged commit 2558df5 into redmine-git-hosting:devel Jun 21, 2014
@dgeo
Copy link
Author

dgeo commented Jun 24, 2014

thank you :)

n-rodriguez pushed a commit that referenced this pull request Sep 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants