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: client handshake regression #3339

Closed
gopherbot opened this issue Mar 17, 2012 · 11 comments
Closed

crypto/tls: client handshake regression #3339

gopherbot opened this issue Mar 17, 2012 · 11 comments
Milestone

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 17, 2012

by graham@gkgk.org:

## What steps will reproduce the problem?
1. _, err := tls.Dial("tcp", "irc.freenode.net:6697", nil); 
2. fmt.Println(err); 

## What is the expected output?
nil

## What do you see instead?
remote error: unexpected message

## Which compiler are you using (5g, 6g, 8g, gccgo)?
"go run"

## Which operating system are you using?
Linux x86, Ubuntu

## Which revision are you using?  (hg identify)
c8614af8523a tip 

## Please provide any additional information below.

In "release" version, this worked. In latest weekly and tip it doesn't. The
problem appeared in 11106:d620ce23ebe4, in handshake_client.go

The attached diff gets it working, as does presenting a client cert.

The problem may be that a "recordTypeHandshake" is not getting written if a
client cert is not presented.

Thread in golang-nuts:
https://groups.google.com/forum/?fromgroups#!topic/golang-nuts/04va6nnOp8I

Attachments:

  1. handshake_client.diff (746 bytes)
@dsymonds

This comment has been minimized.

Copy link
Member

@dsymonds dsymonds commented Mar 17, 2012

Comment 1:

By "release", I assume you mean release.r60?

Labels changed: added priority-go1, packagebug, removed priority-triage.

@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Mar 17, 2012

Comment 2 by graham@gkgk.org:

Yes, I mean r60 (9516:c1702f36df03). 
It works up to and including 11105:8be74fb194f3, then does not work from
11106:d620ce23ebe4 up to tip (12748:c8614af8523a).
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Mar 17, 2012

Comment 3 by graham@gkgk.org:

Yes, specifically I mean release.r60.3 (9516:c1702f36df03). 
It works up to and including 11105:8be74fb194f3, then does not work from
11106:d620ce23ebe4 up to tip (12748:c8614af8523a).
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Mar 17, 2012

Comment 4:

Adding Adam.
Graham, if you'd like us to use your patch, have you submitted a CLA?  See the copyright
section here: http://weekly.golang.org/doc/contribute.html#copyright
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Mar 17, 2012

Comment 5 by graham@gkgk.org:

I have submitted a CLA.
As for using the patch, I think it needs someone with TLS knowledge to look at it.
@ality

This comment has been minimized.

Copy link
Member

@ality ality commented Mar 18, 2012

Comment 6:

certToSend may be nil in either of two cases: when the server doesn't
send a certificate request message at all or when we have no matching
certificates to send. The patch is only correct for the second case.
This is a valid bug though since we're not following the RFC here.
Section 7.4.6 of RFC 4346 states that we must send a certificate
message with an empty certificate_list if the server requests a
client certificate but we have none to give.

Status changed to Accepted.

@ality

This comment has been minimized.

Copy link
Member

@ality ality commented Mar 18, 2012

Comment 7:

I have a fix for this but I'm still trying to find an old
version of gnutls-cli so I can generate some new tests. It
no longer seems to print the raw bytes of each message so
Adam's python script no longer works. :(
Anyone know what versions still work?
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Mar 18, 2012

Comment 8 by graham@gkgk.org:

Could you use openssl s_client instead? It prints a full hex dump:
    openssl s_client -tls1 -debug -connect irc.freenode.net:6697
@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Mar 18, 2012

Comment 9:

http://golang.org/cl/5845067
The testing could probably do with an overhaul, probably by generating the traces using
a special net.Conn. But that's not something to do prior to Go 1. I'm happy just to fix
this given that the change is small.

Owner changed to @agl.

@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Mar 19, 2012

Comment 10:

This issue was closed by revision aa1d417.

Status changed to Fixed.

@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Mar 20, 2012

Comment 11 by graham@gkgk.org:

I confirm the fix works for me. Thanks a lot!
@rsc rsc added this to the Go1 milestone Apr 10, 2015
@rsc rsc removed the priority-go1 label Apr 10, 2015
@golang golang locked and limited conversation to collaborators Jun 24, 2016
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
If a CertificateRequest is received we have to reply with a
Certificate message, even if we don't have a certificate to offer.

Fixes golang#3339.

R=golang-dev, r, ality
CC=golang-dev
https://golang.org/cl/5845067
This issue was closed.
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
6 participants
You can’t perform that action at this time.