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 authentication is broken #2521

Closed
gopherbot opened this issue Dec 3, 2011 · 7 comments
Closed

crypto/tls: client authentication is broken #2521

gopherbot opened this issue Dec 3, 2011 · 7 comments

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 3, 2011

by jeff.allen:

TLS client authentication as implemented as of rev 10679
is not good enough for production use.

The server side cannot be instructed to require certs. It cannot be given a list of
roots to give to the client.

The client side does not choose certificates according to the
incoming trust list.

The marchalling/unmarshalling of the certificateRequest message is wrong, meaning that
it only works in the case where the server
elects to send no trust list.

What steps will reproduce the problem?
1. configure a lighttpd to require client certs signed by a certain CA:
$SERVER["socket"] == "0.0.0.0:443" {
    ssl.engine                 = "enable" 
    ssl.verifyclient.activate  = "enable" 
    ssl.verifyclient.enforce   = "enable"
    ssl.ca-file = "ca.pem"  <-- cert of CA that signed the client cert
}

2. use get.go (attached) to try to fetch something, you will get a handshake error

What is the expected output?

Go's client certificate support should work.

What do you see instead?

Handshake error, and when the unmarshaler is fixed, the HTTPS client does not send the
cert due to the simple check for len(trust list)==0.

Which compiler are you using (5g, 6g, 8g, gccgo)?

6g and 8g

Which operating system are you using?

Linux

Which revision are you using?  (hg identify)

10679

Attachments:

  1. get.go (596 bytes)
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 5, 2011

Comment 1:

Owner changed to @bradfitz.

Status changed to Accepted.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 8, 2011

Comment 2:

This bug has too much stuff in it, so let me try to break this down.
1) The server side cannot be instructed to require certs.
I'm not sure the server's the right place for this.  This can be down with a custom
handler wrapping a Handler.
2) It [the server] cannot be given a list of roots to give to the client.
I'm not sure what this means.  I've never used this part of TLS.  Jeff, Adam?
3) The client side does not choose certificates according to the incoming trust list.
Does this logic exist elsewhere in the crypto/tls package?  Is the
http.Transport.TLSClientConfig just not being used somewhere it should be?
4) The marchalling/unmarshalling of the certificateRequest message is wrong, meaning
that it only works in the case where the server elects to send no trust list.
This sounds like a bug.  More details / repro test case?

Status changed to HelpWanted.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 8, 2011

Comment 3:

[+agl, who I thought was on this bug already]
@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Dec 8, 2011

Comment 4:

This guy sent me a partial patch which I promised to look at, but which then scrolled
off the bottom of my email. Will do tomorrow.

Owner changed to @agl.

Status changed to Assigned.

@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Dec 9, 2011

Comment 5 by jeff.allen:

This is the CL, I am currently working on it:
  http://golang.org/cl/5448093/
I've adjusted for agl's proposed API, and am now making test cases for it.
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 9, 2011

Comment 6:

Labels changed: added priority-later.

@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Jan 5, 2012

Comment 7:

This issue was closed by revision c581ec4.

Status changed to Fixed.

@gopherbot gopherbot added fixed labels Jan 5, 2012
@mikioh mikioh changed the title tls client authentication is broken crypto/tls: client authentication is broken Jan 14, 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
Fix incorrect marshal/unmarshal of certificateRequest.
Add support for configuring client-auth on the server side.
Fix the certificate selection in the client side.
Update generate_cert.go to new time package

Fixes golang#2521.

R=krautz, agl, bradfitz
CC=golang-dev, mikkel
https://golang.org/cl/5448093
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
4 participants
You can’t perform that action at this time.