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

x/crypto/acme/autocert: make hybrid ecc-rsa certificates possible #23698

Closed
phuslu opened this issue Feb 5, 2018 · 8 comments
Closed

x/crypto/acme/autocert: make hybrid ecc-rsa certificates possible #23698

phuslu opened this issue Feb 5, 2018 · 8 comments

Comments

@phuslu
Copy link

phuslu commented Feb 5, 2018

I manage to implement a go https server to support hybird ecc-rsa certificats. [1]
Here're my codes

func (m *MyManager) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, error) {
	if HasECCCiphers(hello) {
		return m.ecc.GetCertificate(hello)
	} else {
		return m.rsa.GetCertificate(hello)
	}
}

func (m *MyManager) HTTPHandler(fallback http.Handler) http.Handler {
	if m.ecc == nil {
		m.ecc = &autocert.Manager{
			Cache:      autocert.DirCache("ecc"),
			Prompt:     autocert.AcceptTOS,
			HostPolicy: m.HostPolicy,
			ForceRSA:   false,
		}
	}
	if m.rsa == nil {
		m.rsa = &autocert.Manager{
			Cache:      autocert.DirCache("rsa"),
			Prompt:     autocert.AcceptTOS,
			HostPolicy: m.HostPolicy,
			ForceRSA:   true,
		}
	}
	return m.ecc.HTTPHandler(m.rsa.HTTPHandler(fallback))
}

It does not work because of autocert http-01 HTTPHandler does not fallback as I expected.
A quick patch is

diff --git a/acme/autocert/autocert.go b/acme/autocert/autocert.go
index e7edb3029..b9bf0cd31 100644
--- a/acme/autocert/autocert.go
+++ b/acme/autocert/autocert.go
@@ -278,7 +278,7 @@ func (m *Manager) HTTPHandler(fallback http.Handler) http.Handler {
 		}
 		data, err := m.httpToken(ctx, r.URL.Path)
 		if err != nil {
-			http.Error(w, err.Error(), http.StatusNotFound)
+			fallback.ServeHTTP(w, r)
 			return
 		}
 		w.Write(data)

Is this requirement/change reasonable? Thought?

  1. https://scotthelme.co.uk/hybrid-rsa-and-ecdsa-certificates-with-nginx/
@gopherbot gopherbot added this to the Unreleased milestone Feb 5, 2018
@phuslu
Copy link
Author

phuslu commented Feb 5, 2018

a bit related to #19265

@ALTree ALTree changed the title x/crypto/acme/autocert: Make hybird ecc-rsa certificates possible x/crypto/acme/autocert: make hybrid ecc-rsa certificates possible Feb 5, 2018
@bradfitz
Copy link
Contributor

bradfitz commented Feb 5, 2018

You kinda jumped to a solution without explaining the problem. I also don't see enough of your code to see how you're using your MyManager or how it's defined.

I suppose the subject is a reasonable problem statement ("make hybrid ecc-rsa certificates possible"), but if this is an important problem to solve (@agl, @x1ddos?), and we can detect it (e.g. your HasECCCiphers), then it seems like we'd just do it automatically without adding new API (like #19265) and without requiring the hoops you seem to be going through.

@agl
Copy link
Contributor

agl commented Feb 5, 2018

ECDSA certificates issued from an RSA CA are a reasonable thing to desire: the ECDSA operation is much faster.

Google's services, for example, often have both and select ECDSA when possible, and RSA as a fallback.

To do this, you would check the ClientHelloInfo and use ECDSA certificates when the client offers ECDSA ciphersuites that are also in the server's config, supports the curve used by the certificate (almost always P-256), supported uncompressed points, and ECDSA signature schemes.

On niggle is that crypto/tls doesn't export its knowledge of ciphersuites, so the map of uint16 cipher suite ID to whether it's ECDSA would have to be maintained externally.

@FiloSottile
Copy link
Contributor

As far as I understand this issue is not about supporting ECDSA+RSA on the crypto/tls side (which is done as @agl suggests and partially hidden in HasECCCiphers), but about running multiple autocert.Manager simultaneously (in order to obtain the two certificates) by chaining their HTTPHandler.

The expectation is that it would work because of the fallback mechanism, but the outer HTTPHandler doesn't fallback if the path is /.well-known/acme-challenge/, even if it doesn't recognize the token, and returns 404 instead.

https://github.com/golang/crypto/blob/13931e22f9e72ea58bb73048bc752b48c6d4d4ac/acme/autocert/autocert.go#L281

I think the full solution is to check for the token first, fallback if not found, and then apply the host policy. Seems reasonable to me.

Title suggestion: "x/crypto/acme/autocert: support running multiple HTTPHandler"

@bradfitz
Copy link
Contributor

bradfitz commented Feb 5, 2018

@FiloSottile, what I was trying to say was that if ECDSA+RSA is something that enough people want, we should just do it automatically, rather than make this bug about easing an obscure workaround for us not supporting their real need.

@FiloSottile
Copy link
Contributor

Oh I see. You're right, we can implement ECDSA+RSA without any API change since GetCertificate takes the ClientHelloInfo. However I think this will count double against the per-domain Let's Encrypt rate-limit, which is probably not what users want by default.

@x1ddos
Copy link

x1ddos commented Feb 7, 2018

It is reasonable to say this is a duplicate of #22066?

@bradfitz
Copy link
Contributor

bradfitz commented Feb 7, 2018

@x1ddos, yes. I'll close this one.

@bradfitz bradfitz closed this as completed Feb 7, 2018
@golang golang locked and limited conversation to collaborators Feb 7, 2019
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

6 participants