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: connecting to the registry.fedoraproject.org fails in tls #34040

Closed
jcajka opened this issue Sep 3, 2019 · 15 comments
Closed

crypto/tls: connecting to the registry.fedoraproject.org fails in tls #34040

jcajka opened this issue Sep 3, 2019 · 15 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@jcajka
Copy link
Contributor

jcajka commented Sep 3, 2019

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

go1.12(with GODEBUG=tls13=1), go1.13rc2 and master

Does this issue reproduce with the latest release?

yes

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

linux/*

What did you do?

Originally pulling container images with podman built with go1.13beta.
Run Go side reproducer(based on @cverna 's reproducer , thanks!) https://play.golang.org/p/dbRRM9-GdI0 .

I don't believe that this is issue on the side of the registry.fedoraproject.org as other tls1.3 implementations(used by browsers, curl) work just fine, but I don't have yet reduced server side reproducer/trigger. This can be worked-around by reverting to the go1.12 behavior(not enabling tls1.3 by default or GODEBUG=tls13=0).

For the record this got reported and is tracked in Fedora as https://bugzilla.redhat.com/show_bug.cgi?id=1737471

What did you expect to see?

Successful "get".

What did you see instead?

<nil>
Get https://registry.fedoraproject.org/v2/fedora/manifests/latest: local error: tls: unexpected message
@jcajka
Copy link
Contributor Author

jcajka commented Sep 3, 2019

And for the record registry.fedoraproject.org should be actually Go based app(https://github.com/docker/distribution), but run behind proxy/cdn.

@tomato42
Copy link

tomato42 commented Sep 3, 2019

most likely cause is the CertificateRequest message sent by the server when connecting to the SNI host

@katiehockman
Copy link
Contributor

@jcajka can you confirm that issue occurs with the final 1.13 Go release (it looks like you've tested with the release candidates)?

/cc @FiloSottile

@katiehockman katiehockman added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 4, 2019
@FiloSottile FiloSottile self-assigned this Sep 4, 2019
@FiloSottile FiloSottile added this to the Go1.14 milestone Sep 4, 2019
@FiloSottile
Copy link
Contributor

The server is misbehaving here, I'm afraid. In the CertificateRequest message, it's sending a status_request extension with 475 bytes, when it should be empty. You can see Wireshark also flagging this as an issue.

image

crypto/tls is seeing those extra bytes and rejecting the message. See RFC 8446, Section 4.4.2.1.

   A server MAY request that a client present an OCSP response with its
   certificate by sending an empty "status_request" extension in its
   CertificateRequest message.

The extraneous bytes are identical to the contents of the Certificate's status_request extension, so my guess here is that there's a bug in the server's TLS stack where it sets the content of status_request globally for the connection, and ends up sending it also in CertificateRequest where it should be empty (because it's a request for the client's response). CertificateRequest extensions are new in TLS 1.3.

image

The server is not possibly a Go server, because it supports DHE ciphersuites which we don't implement. Can you please find out what TLS stack the server is using, update if a fix is available, and report this upstream otherwise?

We try really hard not to introduce exceptions to tolerate misbehavior, but we'll consider it based on how widespread this implementation bug is.

@marcusmueller
Copy link

So, according to HTTP headers, the actually serving daemon is a Varnish cache. And that would be a rather common thing (but AFAIK it doesn't do TLS at all, but relies on a Proxy to do that).

That seems to be an Apache, or at least it calls itself Apache.

HTTP/2 200 
date: Wed, 04 Sep 2019 17:03:53 GMT
server: Apache
strict-transport-security: max-age=31536000; includeSubDomains; preload
x-frame-options: SAMEORIGIN
x-xss-protection: 1; mode=block
x-content-type-options: nosniff
referrer-policy: same-origin
content-length: 2181
content-type: application/vnd.docker.distribution.manifest.v1+prettyjws
docker-content-digest: sha256:33cd3b523ea3e2e96ee86af501265e919d4cf52389ccf9619a2c153ce8078c54
docker-distribution-api-version: registry/2.0
etag: "sha256:33cd3b523ea3e2e96ee86af501265e919d4cf52389ccf9619a2c153ce8078c54"
vary: Accept
x-varnish: 12541184
age: 0
via: 1.1 varnish (Varnish/6.0)
accept-ranges: bytes
apptime: D=206416
x-fedora-proxyserver: proxy13.fedoraproject.org
x-fedora-requestid: XW-ueWuyXqmV7hQytilB8QAAAAE

@tomato42
Copy link

tomato42 commented Sep 4, 2019

We try really hard not to introduce exceptions to tolerate misbehavior, but we'll consider it based on how widespread this implementation bug is.

well, then the error should be decode_error, not unexpected_message, shouldn't it?

in https://tools.ietf.org/html/rfc8446 it is defined as

   decode_error:  A message could not be decoded because some field was
      out of the specified range or the length of the message was
      incorrect.  This alert is used for errors where the message does
      not conform to the formal protocol syntax.

while the status_request in CertificateRequest is defined like so:

   A server MAY request that a client present an OCSP response with its
   certificate by sending an empty "status_request" extension in its
   CertificateRequest message. 

@FiloSottile
Copy link
Contributor

Yes, our alert selection is pretty bad, it's on my radar to refactor it.

@davidben
Copy link
Contributor

davidben commented Sep 4, 2019

Skimming OpenSSL's source, this looks like a bug in OpenSSL. Adding @mattcaswell to confirm. The status_request extension has flags for both the Certificate and CertificateRequest message:
https://github.com/openssl/openssl/blob/bc5a80910dccbc1e417f96bb7f0a3814d3ad5a4d/ssl/statem/extensions.c#L213

However, the construction callback does not check context and appears to just send it in both.
https://github.com/openssl/openssl/blob/bc5a80910dccbc1e417f96bb7f0a3814d3ad5a4d/ssl/statem/extensions_srvr.c#L1490

Looks like the bug may have been introduced in openssl/openssl@5de683d. The client callbacks was tweaked to ignore the extension in the CertificateRequest, but the server callback wasn't fixed to handle the new context bits.

@jcajka
Copy link
Contributor Author

jcajka commented Sep 4, 2019

@FiloSottile thank you very much for the triage :). I will take it up with out infra folks and others.

@mattcaswell
Copy link

I can confirm this as an OpenSSL bug. I have managed to reproduce this behaviour using OpenSSL's s_server and s_client tools. I have raised and OpenSSL issue to track this. Please see openssl/openssl#9767

@mattcaswell
Copy link

This has now been fixed in OpenSSL (see openssl/openssl@debb64a). That commit will be part of OpenSSL 1.1.1d planned for release on Tuesday.

@FiloSottile
Copy link
Contributor

Fantastic, thank you to all involved!

@HaraldNordgren
Copy link
Member

@FiloSottile Is there any known workaround for the time being? We are using Go 1.12 and trying to send a file over TLS to a server that is likely misbehaving, and only receiving

local error: tls: unexpected message

@HaraldNordgren
Copy link
Member

HaraldNordgren commented Oct 15, 2019

On a previous implementation against the same server we achieved results by disabling dynamic record sizing, like this

tls.Config{
	Certificates:                []tls.Certificate{cert},
	Renegotiation:               tls.RenegotiateOnceAsClient,
	DynamicRecordSizingDisabled: true,
}

but it's not solving the problem now.

On the previous occasion we were doing a POST with request body. Now we want to do a POST with a file as body.

@FiloSottile
Copy link
Contributor

@HaraldNordgren The only workarounds are to turn off TLS 1.3 on either side, or to disable OCSP stapling server-side.

Note that unexpected message can mean a lot of things, it's not necessarily the same issue as here. If you think you are experiencing a different bug please open a new issue with as many details as possible.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

9 participants