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

proposal: crypto/tls: support TLS 1.3 post-handshake authentication #40521

Open
skyfmmf opened this issue Jul 31, 2020 · 13 comments
Open

proposal: crypto/tls: support TLS 1.3 post-handshake authentication #40521

skyfmmf opened this issue Jul 31, 2020 · 13 comments
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues Proposal-Hold
Milestone

Comments

@skyfmmf
Copy link

skyfmmf commented Jul 31, 2020

What version of Go are you using (go version)?

$ go version 
go version go1.14.5 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/felix/Library/Caches/go-build"
GOENV="/Users/felix/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/felix/workspace/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.14.5/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.14.5/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/felix/workspace/tests/go-tls-pha/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/gr/j517787d56s73w50gnpv08gr0000gn/T/go-build528430427=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I use the Go TLS client to implement a client for an endpoint that requires authentication with client certificates. The endpoint decides if client authentication is needed based on the path in the HTTP request. Therefore, the server can only decide if client authentication is needed after the TLS connection has been fully set up.

With TLS 1.3, the server would achieve the authentication with client certificates using post-handshake authentication. However, the Go TLS client does not support post-handshake authentication.

For reproduction of the issue, one can use the Apache httpd (I tested with version 2.4) with the config including the snippet below and a small go program like the one below.

httpd.conf snippet

<VirtualHost *:443>
    LogLevel debug

    SSLEngine on
    SSLCertificateFile /usr/local/apache2/conf/server.crt
    SSLCertificateKeyFile /usr/local/apache2/conf/server.key

    SSLVerifyClient none
    SSLCACertificateFile /usr/local/apache2/conf/ca.crt
    <Location "/test-cert.html">
        SSLVerifyClient require
        SSLVerifyDepth 1
    </Location>
</VirtualHost>

client.go

package main

import (
	"fmt"
	"io/ioutil"
	"log"
	"net/http"
	"os"
	"time"

	"go.etcd.io/etcd/pkg/transport"
)

func main() {
	f, err := os.Create("./tls.keylog")
	if err != nil {
		log.Fatal(err)
	}
	defer f.Close()

	tlsInfo := transport.TLSInfo{
		TrustedCAFile: "./ca.crt",
		CertFile:      "./client.crt",
		KeyFile:       "./client.key",
	}
	tlsTransport, err := transport.NewTransport(tlsInfo, 30*time.Second)
	if err != nil {
		log.Fatal(err)
	}
	tlsTransport.TLSClientConfig.KeyLogWriter = f

	client := &http.Client{
		Transport: tlsTransport,
	}

	r, err := client.Get("https://localhost/test-cert.html")
	if err != nil {
		log.Fatal(err)
	}

	defer r.Body.Close()
	body, err := ioutil.ReadAll(r.Body)
	if err != nil {
		log.Fatal(err)
	}

	fmt.Printf("%s\n", body)
}

What did you expect to see?

I would have expected the TLS client to support TLS 1.3 post-handshake authentication and be able to connect to the endpoint.

What did you see instead?

The server closes the connection to the client as soon as it encounters that a client certificate would be required for a specific route. In the httpd logs this error can be found:

[ssl:debug] [pid 7:tid 139688389797632] ssl_engine_kernel.c(1148): [client 172.17.0.1:35172] AH10129: verify client post handshake
[ssl:error] [pid 7:tid 139688389797632] [client 172.17.0.1:35172] AH10158: cannot perform post-handshake authentication
[ssl:error] [pid 7:tid 139688389797632] SSL Library Error: error:14268117:SSL routines:SSL_verify_client_post_handshake:extension not received
172.17.0.1 - - [31/Jul/2020:19:40:50 +0000] "GET /test-cert.html HTTP/1.1" 403 258

This happens because the TLS client does not indicate support for post-handshake authentication in the ClientHello (which is actually correct, as it is not supported right now).

After looking at the TLS 1.3 client implementation, I think adding post-handshake authentication support for the TLS 1.3 client should be possible and I would be happy to help with this.

@gopherbot
Copy link

gopherbot commented Aug 1, 2020

Change https://golang.org/cl/246337 mentions this issue: crypto/tls: support post-handshake client authentication

@skyfmmf
Copy link
Author

skyfmmf commented Aug 1, 2020

I tried to implement the post-handshake authentication for the TLS 1.3 client. The change above supports at least the server configuration from the issue.

The change is just a draft right now. It does not contain any tests, probably breaks tests, and blindly indicates support for post_handshake_auth. However, it is useful to see which areas are a bit difficult to deal with right now:

  • The code for sending Certificate, CertificateVerify and Finished needs to be called from Conn
  • The transcript of all handshake messages needs to be available from Conn. Right now, it is just available from clientHandshakeStateTLS13

@odeke-em odeke-em changed the title Post handshake authentication with the TLS 1.3 client crypto/TVs: POST-handshake authentication failure with the TLS 1.3 client Aug 2, 2020
@odeke-em odeke-em changed the title crypto/TVs: POST-handshake authentication failure with the TLS 1.3 client crypto/tls: POST-handshake authentication failure with the TLS 1.3 client Aug 2, 2020
@odeke-em
Copy link
Member

odeke-em commented Aug 2, 2020

Thank you for this question @skyfmmf and welcome to the Go project!

Kindly ping some cryptography experts @katiehockman @FiloSottile to take a look.

@odeke-em odeke-em added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 2, 2020
@FiloSottile FiloSottile changed the title crypto/tls: POST-handshake authentication failure with the TLS 1.3 client proposal: crypto/tls: support TLS 1.3 post-handshake authentication Sep 22, 2020
@gopherbot gopherbot added this to the Proposal milestone Sep 22, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Oct 28, 2020
@ianlancetaylor ianlancetaylor added the Proposal-Crypto Proposal related to crypto packages or other security issues label Oct 28, 2020
@rsc
Copy link
Contributor

rsc commented Oct 28, 2020

Thoughts, @FiloSottile ?

@rsc rsc moved this from Incoming to Active in Proposals (old) Oct 28, 2020
@rsc
Copy link
Contributor

rsc commented Nov 11, 2020

Ping, @FiloSottile

1 similar comment
@rsc
Copy link
Contributor

rsc commented Dec 2, 2020

Ping, @FiloSottile

@FiloSottile
Copy link
Contributor

FiloSottile commented Jan 13, 2021

It would be useful to know how commonly required this is. I assume web servers do this when in TLS 1.2 they would have asked for a renegotiation?

The main complexity costs of this would be:

  1. keeping handshake state past the end of the handshake
  2. on the client, invoking GetClientCertificate after the end of that handshake, which might be unexpected by the application
  3. on the server, exposing a whole new API

It's unlikely we'll implement this on the server side unless it's a very common request. On the client, we can consider it, like we did for (sigh) renegotiation. I wonder if (1) would affect every connection or if the server acknowledges interest in post-handshake auth.

@skyfmmf
Copy link
Author

skyfmmf commented Jan 14, 2021

Yes, in the place where we now see a request for PHA, with TLS 1.2 the server asked for a renegotiation. Renegotiation support in the TLS 1.2 client is also opt-in in Go, if I am not wrong? So this opt-in behavior is what you could imagine for PHA in the TLS 1.3 client as well?

For the client side, at least Apache considers it a client bug not to support PHA (https://bz.apache.org/bugzilla/show_bug.cgi?id=62975). As it affects some connections to the commonly used Apache httpd, I think PHA on the client side may be a common requirement. On the server side, I do not know, how commonly required it is.

I do not recall if the server in any way indicates that it supports PHA in the handshake, but I will check it later on. At least the RFC does not sound like the server would acknowledge the indicated client support.

@FiloSottile
Copy link
Contributor

FiloSottile commented Jan 14, 2021

Considering it a client bug seems excessive. It is definitely not a mandatory extension. https://tools.ietf.org/html/rfc8446#section-9.2

Chrome has not implemented it and Firefox gated it behind the off-by-default security.tls.enable_post_handshake_auth setting. https://bugs.chromium.org/p/chromium/issues/detail?id=911653#c4

The main problem with it is that it is a layering violation if it's not tightly integrated with the application protocol (like in HTTP/1.1) or if the application protocol is multiplexed (like HTTP/2). Indeed, there is an RFC forbidding its use with HTTP/2. https://www.rfc-editor.org/rfc/rfc8740.html

I don't think this is something we'd want to implement, since it seems it has a complicated deployment, compatibility, and safety story. I believe there are other efforts to bring post-handshake authentication to HTTP/2, although I have not checked in on them in a while.

@skyfmmf
Copy link
Author

skyfmmf commented Jan 14, 2021

I totally agree that considering it a client bug is a bit too much, given that it is not mandatory to implement.

I was not aware that the browsers mostly do not implement PHA. However, I would argue that web browsers are not generally the best reference when it comes to TLS client certificate support. I see client certificates more in use with machine-to-machine communication over HTTPS.

As PHA is standardized and used by at least one common server implementation, I still see that it could be relevant to implement the client side. To me it appears to be less of a mess and security issue than renegotiation. But I also understand that it is not a universally needed feature that adds complexity to the implementation and may therefore not be desired. Maybe we can wait a bit to see if there are further requests to implement it?

@FiloSottile
Copy link
Contributor

FiloSottile commented Jan 15, 2021

I agree client certificate support is probably more relevant to us than to browsers, but I'm not sure that extends to PHA. Most of the times client certificates are used in mTLS configurations where a certificate is negotiated at handshake time.

It is definitely less of a mess than renegotiation, but I still don't think it carries its own weight, due to the HTTP/2 incompatibility. Since applications already have to disable HTTP/2 if they want these semantics, they might as well also disable TLS 1.3 for now.

Happy to put this proposal on hold to collect further requests and gauge interest.

@FiloSottile FiloSottile added Proposal-Hold and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 15, 2021
@rsc
Copy link
Contributor

rsc commented Jan 20, 2021

Placed on hold.
— rsc for the proposal review group

@rsc rsc moved this from Active to Hold in Proposals (old) Jan 20, 2021
@wneessen
Copy link

wneessen commented May 4, 2021

Ran into the same issue today. I was able to work around this, by forcing TLS 1.2 for now, but it would be great to use TLS 1.3 as well. So I'd gladly add my "+1" to the requester list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues Proposal-Hold
Projects
Status: Hold
Development

No branches or pull requests

7 participants