General cleanup and more godocs

5 files changed, 129 insertions, 86 deletions

diff --git a/modules.go b/modules.go
index 0fd898e..5a4e69f 100644
--- a/modules.go
+++ b/modules.go
@@ -11,15 +11,44 @@ import (
 
 // Module represents a Caddy module.
 type Module struct {
-	Name     string
-	New      func() (interface{}, error)
-	OnLoad   func(instances []interface{}, priorState interface{}) (newState interface{}, err error)
+	// Name is the full name of the module. It
+	// must be unique and properly namespaced.
+	Name string
+
+	// New returns a new, empty instance of
+	// the module's type. The host module
+	// which loads this module will likely
+	// invoke methods on the returned value.
+	// It must return a pointer; if not, it
+	// is converted into one.
+	New func() (interface{}, error)
+
+	// OnLoad is invoked after all module
+	// instances ave been loaded. It receives
+	// pointers to each instance of this
+	// module, and any state from a previous
+	// running configuration, which may be
+	// nil.
+	//
+	// If this module is to carry "global"
+	// state between all instances through
+	// reloads, you might find it helpful
+	// to return it.
+	// TODO: Is this really better/safer than a global variable?
+	OnLoad func(instances []interface{}, priorState interface{}) (newState interface{}, err error)
+
+	// OnUnload is called after all module
+	// instances have been stopped, possibly
+	// in favor of a new configuration. It
+	// receives the state given by OnLoad (if
+	// any).
 	OnUnload func(state interface{}) error
 }
 
 func (m Module) String() string { return m.Name }
 
-// RegisterModule registers a module.
+// RegisterModule registers a module. Modules must call
+// this function in the init phase of runtime.
 func RegisterModule(mod Module) error {
 	if mod.Name == "caddy" {
 		return fmt.Errorf("modules cannot be named 'caddy'")
@@ -35,7 +64,7 @@ func RegisterModule(mod Module) error {
 	return nil
 }
 
-// GetModule returns a module by name.
+// GetModule returns a module by its full name.
 func GetModule(name string) (Module, error) {
 	modulesMu.Lock()
 	defer modulesMu.Unlock()
diff --git a/modules/caddytls/acmemanager.go b/modules/caddytls/acmemanager.go
index a7a460a..40e2d24 100644
--- a/modules/caddytls/acmemanager.go
+++ b/modules/caddytls/acmemanager.go
@@ -3,6 +3,7 @@ package caddytls
 import (
 	"encoding/json"
 	"fmt"
+	"time"
 
 	"github.com/go-acme/lego/certcrypto"
 
@@ -18,11 +19,6 @@ func init() {
 	})
 }
 
-// ManagerMaker TODO: WIP...
-type ManagerMaker interface {
-	newManager(interactive bool) (certmagic.Manager, error)
-}
-
 // acmeManagerMaker makes an ACME manager
 // for managinig certificates using ACME.
 type acmeManagerMaker struct {
@@ -40,9 +36,11 @@ type acmeManagerMaker struct {
 	keyType certcrypto.KeyType
 }
 
-func (m *acmeManagerMaker) Provision() error {
-	m.setDefaults()
+func (m *acmeManagerMaker) newManager(interactive bool) (certmagic.Manager, error) {
+	return nil, nil
+}
 
+func (m *acmeManagerMaker) Provision() error {
 	// DNS providers
 	if m.Challenges.DNS != nil {
 		val, err := caddy2.LoadModuleInline("provider", "tls.dns", m.Challenges.DNS)
@@ -67,18 +65,71 @@ func (m *acmeManagerMaker) Provision() error {
 		m.Storage = nil // allow GC to deallocate - TODO: Does this help?
 	}
 
+	m.setDefaults()
+
 	return nil
 }
 
-// setDefaults indiscriminately sets all the default values in m.
+// setDefaults sets necessary values that are
+// currently empty to their default values.
 func (m *acmeManagerMaker) setDefaults() {
-	m.CA = certmagic.LetsEncryptStagingCA // certmagic.Default.CA // TODO: When not testing, switch to production CA
-	m.Email = certmagic.Default.Email
-	m.RenewAhead = caddy2.Duration(certmagic.Default.RenewDurationBefore)
-	m.keyType = certmagic.Default.KeyType
-	m.storage = certmagic.Default.Storage
+	if m.CA == "" {
+		m.CA = certmagic.LetsEncryptStagingCA // certmagic.Default.CA // TODO: When not testing, switch to production CA
+	}
+	if m.Email == "" {
+		m.Email = certmagic.Default.Email
+	}
+	if m.RenewAhead == 0 {
+		m.RenewAhead = caddy2.Duration(certmagic.Default.RenewDurationBefore)
+	}
+	if m.keyType == "" {
+		m.keyType = certmagic.Default.KeyType
+	}
+	if m.storage == nil {
+		m.storage = certmagic.Default.Storage
+	}
 }
 
-func (m *acmeManagerMaker) newManager(interactive bool) (certmagic.Manager, error) {
-	return nil, nil
+// makeCertMagicConfig converts m into a certmagic.Config, because
+// this is a special case where the default manager is the certmagic
+// Config and not a separate manager.
+func (m *acmeManagerMaker) makeCertMagicConfig() certmagic.Config {
+	storage := m.storage
+	if storage == nil {
+		storage = caddy2.GetStorage()
+	}
+
+	var ond *certmagic.OnDemandConfig
+	if m.OnDemand != nil {
+		ond = &certmagic.OnDemandConfig{
+			// TODO: fill this out
+		}
+	}
+
+	return certmagic.Config{
+		CA:                      certmagic.LetsEncryptStagingCA, //ap.CA, // TODO: Restore true value
+		Email:                   m.Email,
+		Agreed:                  true,
+		DisableHTTPChallenge:    m.Challenges.HTTP.Disabled,
+		DisableTLSALPNChallenge: m.Challenges.TLSALPN.Disabled,
+		RenewDurationBefore:     time.Duration(m.RenewAhead),
+		AltHTTPPort:             m.Challenges.HTTP.AlternatePort,
+		AltTLSALPNPort:          m.Challenges.TLSALPN.AlternatePort,
+		DNSProvider:             m.Challenges.dns,
+		KeyType:                 supportedCertKeyTypes[m.KeyType],
+		CertObtainTimeout:       time.Duration(m.ACMETimeout),
+		OnDemand:                ond,
+		MustStaple:              m.MustStaple,
+		Storage:                 storage,
+		// TODO: listenHost
+	}
+}
+
+// supportedCertKeyTypes is all the key types that are supported
+// for certificates that are obtained through ACME.
+var supportedCertKeyTypes = map[string]certcrypto.KeyType{
+	"RSA2048": certcrypto.RSA2048,
+	"RSA4096": certcrypto.RSA4096,
+	"P256":    certcrypto.EC256,
+	"P384":    certcrypto.EC384,
 }
diff --git a/modules/caddytls/matchers.go b/modules/caddytls/matchers.go
index c376f87..b308bd0 100644
--- a/modules/caddytls/matchers.go
+++ b/modules/caddytls/matchers.go
@@ -7,13 +7,22 @@ import (
 )
 
 type (
+	// MatchServerName matches based on SNI.
 	MatchServerName []string
 
 	// TODO: these others should be enterprise-only, probably
-	MatchProtocol   []string // TODO: version or protocol?
+
+	// MatchProtocol matches based on protocol.
+	MatchProtocol []string // TODO: Protocol or version?
+
+	// MatchClientCert matches based on client certificate / client auth?
 	MatchClientCert struct{} // TODO: client certificate options
-	MatchRemote     []string
-	MatchStarlark   string
+
+	// MatchRemote matches based on the remote address of the connection.
+	MatchRemote []string
+
+	// MatchStarlark matches based on a Starlark script.
+	MatchStarlark string
 )
 
 func init() {
@@ -39,6 +48,7 @@ func init() {
 	})
 }
 
+// Match matches hello based on SNI.
 func (m MatchServerName) Match(hello *tls.ClientHelloInfo) bool {
 	for _, name := range m {
 		// TODO: support wildcards (and regex?)
@@ -49,21 +59,25 @@ func (m MatchServerName) Match(hello *tls.ClientHelloInfo) bool {
 	return false
 }
 
+// Match matches hello based on protocol version.
 func (m MatchProtocol) Match(hello *tls.ClientHelloInfo) bool {
 	// TODO: not implemented
 	return false
 }
 
+// Match matches hello based on client certificate.
 func (m MatchClientCert) Match(hello *tls.ClientHelloInfo) bool {
 	// TODO: not implemented
 	return false
 }
 
+// Match matches hello based on remote address.
 func (m MatchRemote) Match(hello *tls.ClientHelloInfo) bool {
 	// TODO: not implemented
 	return false
 }
 
+// Match matches hello based on a Starlark script.
 func (m MatchStarlark) Match(hello *tls.ClientHelloInfo) bool {
 	// TODO: not implemented
 	return false
diff --git a/modules/caddytls/tls.go b/modules/caddytls/tls.go
index 43ad957..fbc850c 100644
--- a/modules/caddytls/tls.go
+++ b/modules/caddytls/tls.go
@@ -5,10 +5,8 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
-	"time"
 
 	"bitbucket.org/lightcodelabs/caddy2"
-	"github.com/go-acme/lego/certcrypto"
 	"github.com/go-acme/lego/challenge"
 	"github.com/klauspost/cpuid"
 	"github.com/mholt/certmagic"
@@ -30,8 +28,7 @@ type TLS struct {
 	certCache          *certmagic.Cache
 }
 
-// TODO: Finish stubbing out this two-phase setup process: prepare, then start...
-
+// Provision sets up the configuration for the TLS app.
 func (t *TLS) Provision() error {
 	// set up the certificate cache
 	// TODO: this makes a new cache every time; better to only make a new
@@ -97,15 +94,6 @@ func (t *TLS) Start(handle caddy2.Handle) error {
 		if err != nil {
 			return fmt.Errorf("automate: managing %v: %v", names, err)
 		}
-		// for _, name := range names {
-		// 	t.Manage([]string{name)
-		// 	ap := t.getAutomationPolicyForName(name)
-		// 	magic := certmagic.New(t.certCache, ap.makeCertMagicConfig())
-		// 	err := magic.Manage([]string{name})
-		// 	if err != nil {
-		// 		return fmt.Errorf("automate: manage %s: %v", name, err)
-		// 	}
-		// }
 	}
 	t.Certificates = nil // allow GC to deallocate - TODO: Does this help?
 
@@ -191,38 +179,11 @@ type AutomationPolicy struct {
 }
 
 func (ap AutomationPolicy) makeCertMagicConfig() certmagic.Config {
+	// default manager (ACME) is a special case because of how CertMagic is designed
+	// TODO: refactor certmagic so that ACME manager is not a special case by extracting
+	// its config fields out of the certmagic.Config struct, or something...
 	if acmeMgmt, ok := ap.management.(*acmeManagerMaker); ok {
-		// default, which is management via ACME
-
-		storage := acmeMgmt.storage
-		if storage == nil {
-			storage = caddy2.GetStorage()
-		}
-
-		var ond *certmagic.OnDemandConfig
-		if acmeMgmt.OnDemand != nil {
-			ond = &certmagic.OnDemandConfig{
-				// TODO: fill this out
-			}
-		}
-
-		return certmagic.Config{
-			CA:                      certmagic.LetsEncryptStagingCA, //ap.CA, // TODO: Restore true value
-			Email:                   acmeMgmt.Email,
-			Agreed:                  true,
-			DisableHTTPChallenge:    acmeMgmt.Challenges.HTTP.Disabled,
-			DisableTLSALPNChallenge: acmeMgmt.Challenges.TLSALPN.Disabled,
-			RenewDurationBefore:     time.Duration(acmeMgmt.RenewAhead),
-			AltHTTPPort:             acmeMgmt.Challenges.HTTP.AlternatePort,
-			AltTLSALPNPort:          acmeMgmt.Challenges.TLSALPN.AlternatePort,
-			DNSProvider:             acmeMgmt.Challenges.dns,
-			KeyType:                 supportedCertKeyTypes[acmeMgmt.KeyType],
-			CertObtainTimeout:       time.Duration(acmeMgmt.ACMETimeout),
-			OnDemand:                ond,
-			MustStaple:              acmeMgmt.MustStaple,
-			Storage:                 storage,
-			// TODO: listenHost
-		}
+		return acmeMgmt.makeCertMagicConfig()
 	}
 
 	return certmagic.Config{
@@ -260,13 +221,9 @@ type OnDemandConfig struct {
 	AskStarlark string `json:"ask_starlark,omitempty"`
 }
 
-// supportedCertKeyTypes is all the key types that are supported
-// for certificates that are obtained through ACME.
-var supportedCertKeyTypes = map[string]certcrypto.KeyType{
-	"RSA2048": certcrypto.RSA2048,
-	"RSA4096": certcrypto.RSA4096,
-	"P256":    certcrypto.EC256,
-	"P384":    certcrypto.EC384,
+// ManagerMaker makes a certificate manager.
+type ManagerMaker interface {
+	newManager(interactive bool) (certmagic.Manager, error)
 }
 
 // supportedCipherSuites is the unordered map of cipher suite
diff --git a/storage.go b/storage.go
index cb93f59..2b77851 100644
--- a/storage.go
+++ b/storage.go
@@ -16,22 +16,14 @@ func init() {
 }
 
 // StorageConverter is a type that can convert itself
-// to a valid, usable certmagic.Storage value. The
-// value might be short-lived.
+// to a valid, usable certmagic.Storage value. (The
+// value might be short-lived.) This interface allows
+// us to adapt any CertMagic storage implementation
+// into a consistent API for Caddy configuration.
 type StorageConverter interface {
 	CertMagicStorage() (certmagic.Storage, error)
 }
 
-// TODO: Wrappers other than file_system should be enterprise-only.
-
-// It may seem trivial to wrap these, but the benefits are:
-// 1. We don't need to change the actual CertMagic storage implementions
-// to a structure that is operable with Caddy's config (including JSON
-// tags), and
-// 2. We don't need to rely on rely on maintainers of third-party
-// certmagic.Storage implementations. We can make any certmagic.Storage
-// work with Caddy this way.
-
 // fileStorage is a certmagic.Storage wrapper for certmagic.FileStorage.
 type fileStorage struct {
 	Root string `json:"root"`