Fix TLS-ALPN-01 challenge for IP Identifiers (caddyserver#139)

* Fix TLS-ALPN-01 challenge for IP Identifiers

See caddyserver#133

* Add tests for challengeKey function

* Add more tests

* Fix PR comments

* Remove deletion of TLS-ALPN-01 challenge certificate

solvers.go

@@ -32,6 +32,7 @@ import (
  "github.com/libdns/libdns"
  "github.com/mholt/acmez"
  "github.com/mholt/acmez/acme"
+ "github.com/miekg/dns"
 )
 
 // httpSolver solves the HTTP challenge. It must be
@@ -131,10 +132,12 @@ func (s *tlsALPNSolver) Present(ctx context.Context, chal acme.Challenge) error
  if err != nil {
  return err
  }
+
+ key := challengeKey(chal)
  activeChallengesMu.Lock()
- chalData := activeChallenges[chal.Identifier.Value]
+ chalData := activeChallenges[key]
  chalData.data = cert
- activeChallenges[chal.Identifier.Value] = chalData
+ activeChallenges[key] = chalData
  activeChallengesMu.Unlock()
 
  // the rest of this function increments the
@@ -215,10 +218,6 @@ func (*tlsALPNSolver) handleConn(conn net.Conn) {
 // CleanUp removes the challenge certificate from the cache, and if
 // it is the last one to finish, stops the TLS server.
 func (s *tlsALPNSolver) CleanUp(ctx context.Context, chal acme.Challenge) error {
- s.config.certCache.mu.Lock()
- delete(s.config.certCache.cache, tlsALPNCertKeyName(chal.Identifier.Value))
- s.config.certCache.mu.Unlock()
-
  solversMu.Lock()
  defer solversMu.Unlock()
  si := getSolverInfo(s.address)
@@ -236,14 +235,6 @@ func (s *tlsALPNSolver) CleanUp(ctx context.Context, chal acme.Challenge) error
  return nil
 }
 
-// tlsALPNCertKeyName returns the key to use when caching a cert
-// for use with the TLS-ALPN ACME challenge. It is simply to help
-// avoid conflicts (although at time of writing, there shouldn't
-// be, since the cert cache is keyed by hash of certificate chain).
-func tlsALPNCertKeyName(sniName string) string {
- return sniName + ":acme-tls-alpn"
-}
-
 // DNS01Solver is a type that makes libdns providers usable
 // as ACME dns-01 challenge solvers.
 // See https://github.com/libdns/libdns
@@ -478,7 +469,7 @@ func (dhs distributedSolver) Present(ctx context.Context, chal acme.Challenge) e
  return err
  }
 
- err = dhs.storage.Store(dhs.challengeTokensKey(chal.Identifier.Value), infoBytes)
+ err = dhs.storage.Store(dhs.challengeTokensKey(challengeKey(chal)), infoBytes)
  if err != nil {
  return err
  }
@@ -501,7 +492,7 @@ func (dhs distributedSolver) Wait(ctx context.Context, challenge acme.Challenge)
 // CleanUp invokes the underlying solver's CleanUp method
 // and also cleans up any assets saved to storage.
 func (dhs distributedSolver) CleanUp(ctx context.Context, chal acme.Challenge) error {
- err := dhs.storage.Delete(dhs.challengeTokensKey(chal.Identifier.Value))
+ err := dhs.storage.Delete(dhs.challengeTokensKey(challengeKey(chal)))
  if err != nil {
  return err
  }
@@ -648,6 +639,18 @@ type Challenge struct {
  data interface{}
 }
 
+// challengeKey returns the map key for a given challenge; it is the identifier
+// unless it is an IP address using the TLS-ALPN challenge.
+func challengeKey(chal acme.Challenge) string {
+ if chal.Type == acme.ChallengeTypeTLSALPN01 && chal.Identifier.Type == "ip" {
+ reversed, err := dns.ReverseAddr(chal.Identifier.Value)
+ if err == nil {
+ return reversed[:len(reversed)-1] // strip off '.'
+ }
+ }
+ return chal.Identifier.Value
+}
+
 // solverWrapper should be used to wrap all challenge solvers so that
 // we can add the challenge info to memory; this makes challenges globally
 // solvable by a single HTTP or TLS server even if multiple servers with
@@ -656,7 +659,7 @@ type solverWrapper struct{ acmez.Solver }
 
 func (sw solverWrapper) Present(ctx context.Context, chal acme.Challenge) error {
  activeChallengesMu.Lock()
- activeChallenges[chal.Identifier.Value] = Challenge{Challenge: chal}
+ activeChallenges[challengeKey(chal)] = Challenge{Challenge: chal}
  activeChallengesMu.Unlock()
  return sw.Solver.Present(ctx, chal)
 }
@@ -670,7 +673,7 @@ func (sw solverWrapper) Wait(ctx context.Context, chal acme.Challenge) error {
 
 func (sw solverWrapper) CleanUp(ctx context.Context, chal acme.Challenge) error {
  activeChallengesMu.Lock()
- delete(activeChallenges, chal.Identifier.Value)
+ delete(activeChallenges, challengeKey(chal))
  activeChallengesMu.Unlock()
  return sw.Solver.CleanUp(ctx, chal)
 }

solvers_test.go

@@ -0,0 +1,157 @@
+// Copyright 2015 Matthew Holt
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package certmagic
+
+import (
+ "testing"
+
+ "github.com/mholt/acmez/acme"
+)
+
+func Test_challengeKey(t *testing.T) {
+ type args struct {
+ chal acme.Challenge
+ }
+ tests := []struct {
+ name string
+ args args
+ want string
+ }{
+ {
+ name: "ok/dns-dns",
+ args: args{
+ chal: acme.Challenge{
+ Type: acme.ChallengeTypeDNS01,
+ Identifier: acme.Identifier{
+ Type: "dns",
+ Value: "*.example.com",
+ },
+ },
+ },
+ want: "*.example.com",
+ },
+ {
+ name: "ok/http-dns",
+ args: args{
+ chal: acme.Challenge{
+ Type: acme.ChallengeTypeHTTP01,
+ Identifier: acme.Identifier{
+ Type: "dns",
+ Value: "*.example.com",
+ },
+ },
+ },
+ want: "*.example.com",
+ },
+ {
+ name: "ok/tls-dns",
+ args: args{
+ chal: acme.Challenge{
+ Type: acme.ChallengeTypeTLSALPN01,
+ Identifier: acme.Identifier{
+ Type: "dns",
+ Value: "*.example.com",
+ },
+ },
+ },
+ want: "*.example.com",
+ },
+ {
+ name: "ok/http-ipv4",
+ args: args{
+ chal: acme.Challenge{
+ Type: acme.ChallengeTypeHTTP01,
+ Identifier: acme.Identifier{
+ Type: "ip",
+ Value: "127.0.0.1",
+ },
+ },
+ },
+ want: "127.0.0.1",
+ },
+ {
+ name: "ok/http-ipv6",
+ args: args{
+ chal: acme.Challenge{
+ Type: acme.ChallengeTypeHTTP01,
+ Identifier: acme.Identifier{
+ Type: "ip",
+ Value: "2001:db8::1",
+ },
+ },
+ },
+ want: "2001:db8::1",
+ },
+ {
+ name: "ok/tls-ipv4",
+ args: args{
+ chal: acme.Challenge{
+ Type: acme.ChallengeTypeTLSALPN01,
+ Identifier: acme.Identifier{
+ Type: "ip",
+ Value: "127.0.0.1",
+ },
+ },
+ },
+ want: "1.0.0.127.in-addr.arpa",
+ },
+ {
+ name: "ok/tls-ipv6",
+ args: args{
+ chal: acme.Challenge{
+ Type: acme.ChallengeTypeTLSALPN01,
+ Identifier: acme.Identifier{
+ Type: "ip",
+ Value: "2001:db8::1",
+ },
+ },
+ },
+ want: "1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa",
+ },
+ {
+ name: "fail/tls-ipv4",
+ args: args{
+ chal: acme.Challenge{
+ Type: acme.ChallengeTypeTLSALPN01,
+ Identifier: acme.Identifier{
+ Type: "ip",
+ Value: "127.0.0.1000",
+ },
+ },
+ },
+ want: "127.0.0.1000", // reversing this fails; default to identifier value
+ },
+ {
+ name: "fail/tls-ipv6",
+ args: args{
+ chal: acme.Challenge{
+ Type: acme.ChallengeTypeTLSALPN01,
+ Identifier: acme.Identifier{
+ Type: "ip",
+ Value: "2001:db8::10000",
+ },
+ },
+ },
+ want: "2001:db8::10000", // reversing this fails; default to identifier value
+ },
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ if got := challengeKey(tt.args.chal); got != tt.want {
+ t.Errorf("challengeKey() = %v, want %v", got, tt.want)
+ }
+ })
+ }
+}