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

crypto/tls: does not support renegotiation #5742

Closed
rogpeppe opened this issue Jun 20, 2013 · 53 comments
Closed

crypto/tls: does not support renegotiation #5742

rogpeppe opened this issue Jun 20, 2013 · 53 comments
Assignees
Milestone

Comments

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Jun 20, 2013

We would like to use net/http to talk to Microsoft Azure services, but
their servers force a TLS renegotiation, which crypto/tls
does not currently support.
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jun 20, 2013

Comment 1:

Owner changed to @agl.

Status changed to Accepted.

@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Jun 20, 2013

Comment 2:

Really don't ever want to support this. TLS renegotiation is a huge mess.
@rogpeppe

This comment has been minimized.

Copy link
Contributor Author

@rogpeppe rogpeppe commented Jun 21, 2013

Comment 3:

> Really don't ever want to support this. TLS renegotiation is a huge mess.
That's a pity, although I do understand. It seems we're ending up using
http://godoc.org/github.com/andelf/go-curl which is a) a pretty messy package and b) a
cgo dependency we could do without.
Have you got a suggestion for a neater way around the issue, by any chance?
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 27, 2013

Comment 4:

Labels changed: added go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 5:

Labels changed: added release-none, removed go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 6:

Labels changed: added repo-main.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 9, 2014

@mikioh

This comment has been minimized.

Copy link
Contributor

@mikioh mikioh commented Jan 10, 2014

Comment 8:

This issue was updated by revision 779ef7b.


      
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 7, 2014

Comment 9 by salmodan:

I am creating a Packer plugin for the Microsoft Azure services but I am getting a "local
error: no renegotiation" error message when I make the client.Do(req)   after setting up
the transport to use my client certificate.  I am using Go version 1.2.1.  Am I
understanding this issue correctly in that 1.2.1 is not be able to to talk to Microsoft
Azure services?  Is my only choice to compile a new dev version of Go?  Are there any
newer binaries available?
@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Apr 7, 2014

Comment 10:

salmodan: there is no published version of the code that does the weird renegotiation
dance that Azure needs. I wrote a patch for some folks who needed it, but it didn't make
Go 1.3. I do think, at this point, that supporting Azure is sufficiently compelling for
1.4 however.

Labels changed: added release-go1.4, removed priority-triage, release-none.

@lukescott

This comment has been minimized.

Copy link

@lukescott lukescott commented Apr 8, 2014

Comment 11:

Is there a reason why this wasn't included in 1.3 when it was reviewed well before the
feature freeze? This occurs with FirstData's payment gateway API. Is there anything
wrong with the patch itself? Is there a better way to address this issue?
@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Apr 9, 2014

Comment 12:

luke@webconnex.com: what patch do you think was reviewed before the freeze? I may have
missed one.
In general, sites that do this are mistaken and the solution is a hack which is why I've
resisted doing it. This form of authentication is also broken by triple-handshake
attacks[1] and the fix for that hasn't gone through the IETF yet.
It needs careful thought in order to minimise the amount of disruption to the code while
supporting these needs.
[1] https://secure-resumption.com/
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jul 25, 2014

Comment 13 by Hope2BelieveIn:

"I wrote a patch for some folks who needed it".
Can you please give a link to your patch?
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jul 25, 2014

Comment 14 by Hope2BelieveIn:

agl@golang.org: "I wrote a patch for some folks who needed it"
Can you please give a link to this patch?
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Sep 15, 2014

Comment 15:

Too late for 1.4.

Labels changed: added release-go1.5, removed release-go1.4.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 20, 2014

Comment 16 by thegroff:

Will it be too late for 1.5 too? #7 patch has been sitting there for a year. Is there a
problem with the patch?
@minux

This comment has been minimized.

Copy link
Member

@minux minux commented Nov 20, 2014

Comment 17:

what #7 mentioned is a commit that is in 1.4.
the reason this issue is not closed by that
commit is that we still need client support,
but the secure solution hasn't been made into
official RFC yet.
To summarize, there is no unreviewed pending
patch for this issue, and before the RFC come
out, we can't do anything about the client
side of the issue. (agl explained why he didn't
want to implement a workaround in #12)
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 9, 2014

https://github.com/MSOpenTech/azure-sdk-for-go includes a forked copy of net/http because they needed to be able to talk to Azure servers. Is it possible to support only the limited behavior of the Azure services without getting into the entire TLS renegotiation mess?

@agl agl self-assigned this Apr 26, 2016
@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Apr 26, 2016

https://go-review.googlesource.com/#/c/22475/ may address this bug, but the primary use case is HTTPS servers which do the Microsoft trick of renegotiating a connection to add client certificates and I don't have any.

Can any interested party here either point me at such a server (with or without a test certificate to authenticate with) or test that patch themselves?

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 26, 2016

CL https://golang.org/cl/22475 mentions this issue.

@ahmetb

This comment has been minimized.

Copy link

@ahmetb ahmetb commented Apr 26, 2016

@agl reaching out to you via email to provide an endpoint and certs.

@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Apr 26, 2016

@ahmetalpbalkan thank you!

@ahmetb

This comment has been minimized.

Copy link

@ahmetb ahmetb commented Apr 26, 2016

@agl thanks for confirming the fix and thanks for the patch.

@hoenirvili

This comment has been minimized.

Copy link

@hoenirvili hoenirvili commented Apr 27, 2016

So this is resolved?

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Apr 28, 2016

There is a CL out that I expect to be in Go 1.7. This bug will be closed when the CL is committed to Git, but you'll have to wait for Go 1.7 to use it (unless you run from the Git master branch, which can be unstable at times and is not recommended for production code).

@ottob

This comment has been minimized.

Copy link

@ottob ottob commented Apr 28, 2016

The patch works fine for me. Huge thanks to @agl!

@gopherbot gopherbot closed this in af125a5 Apr 28, 2016
@hoenirvili

This comment has been minimized.

Copy link

@hoenirvili hoenirvili commented Apr 28, 2016

Thanks for clarifying @rsc, and thanks @agl again for the patch.

@aidylewis

This comment has been minimized.

Copy link

@aidylewis aidylewis commented Jul 8, 2016

Has anyone tested this with the Go 1.7 RC?

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jul 8, 2016

@aidylewis, @ottob above said he had tested this.

@ahmetb

This comment has been minimized.

Copy link

@ahmetb ahmetb commented Aug 9, 2016

Also verified this with go1.7rc6 in azure-sdk-for-go.

@elimisteve

This comment has been minimized.

Copy link
Contributor

@elimisteve elimisteve commented Jan 20, 2017

Just to be explicit to future readers: this feature has been merged into master, and you can use it by setting the Renegotiation field in your *tls.Config to tls.RenegotiateFreelyAsClient:

config := &tls.Config{
    Renegotiation: tls.RenegotiateFreelyAsClient,
}
@Integralist

This comment has been minimized.

Copy link

@Integralist Integralist commented Jan 20, 2017

Thanks @elimisteve :-)

@hoenirvili

This comment has been minimized.

Copy link

@hoenirvili hoenirvili commented Jan 20, 2017

@elimisteve Cheers mate !

@aidylewis

This comment has been minimized.

Copy link

@aidylewis aidylewis commented Jan 20, 2017

@Integralist Does that solve the Vegeta issue or is it a different app?

@Integralist

This comment has been minimized.

Copy link

@Integralist Integralist commented Jan 21, 2017

@aidylewis Different app I think? Can't remember now. It was the team's own CLI app interacting with internal BBC service behind self-signed cert that was the issue iirc

MLauper pushed a commit to MLauper/go-pulp that referenced this issue Feb 7, 2017
@golang golang locked and limited conversation to collaborators Jan 21, 2018
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
This change adds Config.Renegotiation which controls whether a TLS
client will accept renegotiation requests from a server. This is used,
for example, by some web servers that wish to “add” a client certificate
to an HTTPS connection.

This is disabled by default because it significantly complicates the
state machine.

Originally, handshakeMutex was taken before locking either Conn.in or
Conn.out. However, if renegotiation is permitted then a handshake may
be triggered during a Read() call. If Conn.in were unlocked before
taking handshakeMutex then a concurrent Read() call could see an
intermediate state and trigger an error. Thus handshakeMutex is now
locked after Conn.in and the handshake functions assume that Conn.in is
locked for the duration of the handshake.

Additionally, handshakeMutex used to protect Conn.out also. With the
possibility of renegotiation that's no longer viable and so
writeRecordLocked has been split off.

Fixes golang#5742.

Change-Id: I935914db1f185d507ff39bba8274c148d756a1c8
Reviewed-on: https://go-review.googlesource.com/22475
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
This change adds Config.Renegotiation which controls whether a TLS
client will accept renegotiation requests from a server. This is used,
for example, by some web servers that wish to “add” a client certificate
to an HTTPS connection.

This is disabled by default because it significantly complicates the
state machine.

Originally, handshakeMutex was taken before locking either Conn.in or
Conn.out. However, if renegotiation is permitted then a handshake may
be triggered during a Read() call. If Conn.in were unlocked before
taking handshakeMutex then a concurrent Read() call could see an
intermediate state and trigger an error. Thus handshakeMutex is now
locked after Conn.in and the handshake functions assume that Conn.in is
locked for the duration of the handshake.

Additionally, handshakeMutex used to protect Conn.out also. With the
possibility of renegotiation that's no longer viable and so
writeRecordLocked has been split off.

Fixes golang#5742.

Change-Id: I935914db1f185d507ff39bba8274c148d756a1c8
Reviewed-on: https://go-review.googlesource.com/22475
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.