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
Milestone

Comments

@rogpeppe
Copy link
Contributor

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
Copy link
Contributor

Comment 1:

Owner changed to @agl.

Status changed to Accepted.

@agl
Copy link
Contributor

agl commented Jun 20, 2013

Comment 2:

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

@rogpeppe
Copy link
Contributor Author

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
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 4:

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 5:

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

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 6:

Labels changed: added repo-main.

@gopherbot
Copy link

Comment 7 by otto.bretz:

Fixed by https://code.google.com/p/go/source/detail?r=f1e918132139?

@mikioh
Copy link
Contributor

mikioh commented Jan 10, 2014

Comment 8:

This issue was updated by revision 779ef7b.


      

@gopherbot
Copy link

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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
Copy link

Comment 13 by Hope2BelieveIn:

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

@gopherbot
Copy link

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
Copy link
Contributor

rsc commented Sep 15, 2014

Comment 15:

Too late for 1.4.

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

@gopherbot
Copy link

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

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

@ahmetb
Copy link

ahmetb commented Apr 26, 2016

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

@agl
Copy link
Contributor

agl commented Apr 26, 2016

@ahmetalpbalkan thank you!

@ahmetb
Copy link

ahmetb commented Apr 26, 2016

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

@hoenirvili
Copy link

So this is resolved?

@rsc
Copy link
Contributor

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
Copy link

ottob commented Apr 28, 2016

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

@hoenirvili
Copy link

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

@ghost
Copy link

ghost commented Jul 8, 2016

Has anyone tested this with the Go 1.7 RC?

@bradfitz
Copy link
Contributor

bradfitz commented Jul 8, 2016

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

@ahmetb
Copy link

ahmetb commented Aug 9, 2016

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

@elimisteve
Copy link
Contributor

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
Copy link

Thanks @elimisteve :-)

@hoenirvili
Copy link

@elimisteve Cheers mate !

@ghost
Copy link

ghost commented Jan 20, 2017

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

@Integralist
Copy link

@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>
@rsc rsc unassigned agl Jun 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests