Improve documentation, remove dead code plus some minor refactorings
This improves our code-to-comments ratio by a lot.
On the refactorings:
- Simplify the cluster join mode to just a single protobuf message -
a node can either join an existing cluster or bootstrap a new one.
All of the node-level setup like hostname and trust backend is done
using the setup call, since those are identical for both cases.
- We don't need a node name separate from the hostname. Ideally, we would
get rid of IP addresses for etcd as well.
- Google API design guidelines suggest the `List` term (vs. `Get`).
- Add username to comments for consistency. I think the names provide
useful context, but git blame is a thing. What do you think?
- Fixed or silenced some ignored error checks in preparation of using
an errcheck linter. Especially during early boot, many errors are
obviously not recoverable, but logging them can provide useful debugging info.
- Split up the common package into smaller subpackages.
- Remove the audit package (this will be a separate service that probably
uses it own database, rather than etcd).
- Move storage constants to storage package.
- Remove the unused KV type.
I also added a bunch of TODO comments with discussion points.
Added both of you as blocking reviewers - please comment if I
misunderstood any of your code.
Test Plan: Everything compiles and scripts:launch works (for whatever that's worth).
X-Origin-Diff: phab/D235
GitOrigin-RevId: 922fec5076e8d683e1138f26d2cb490de64a9777
diff --git a/core/internal/consensus/BUILD.bazel b/core/internal/consensus/BUILD.bazel
index c1c6989..5fa33ac 100644
--- a/core/internal/consensus/BUILD.bazel
+++ b/core/internal/consensus/BUILD.bazel
@@ -7,7 +7,7 @@
visibility = ["//:__subpackages__"],
deps = [
"//core/api/api:go_default_library",
- "//core/internal/common:go_default_library",
+ "//core/internal/common/service:go_default_library",
"//core/internal/consensus/ca:go_default_library",
"@com_github_pkg_errors//:go_default_library",
"@io_etcd_go_etcd//clientv3:go_default_library",
diff --git a/core/internal/consensus/ca/ca.go b/core/internal/consensus/ca/ca.go
index 925f030..a8cfbd9 100644
--- a/core/internal/consensus/ca/ca.go
+++ b/core/internal/consensus/ca/ca.go
@@ -14,8 +14,16 @@
// See the License for the specific language governing permissions and
// limitations under the License.
+// package ca implements a simple standards-compliant certificate authority.
+// It only supports ed25519 keys, and does not maintain any persistent state.
+//
+// CA and certificates successfully pass https://github.com/zmap/zlint
+// (minus the CA/B rules that a public CA would adhere to, which requires
+// things like OCSP servers, Certificate Policies and ECDSA/RSA-only keys).
package ca
+// TODO(leo): add zlint test
+
import (
"crypto"
"crypto/ed25519"
@@ -48,7 +56,7 @@
// violates Section 4.2.1.2 of RFC 5280 without this. Should eventually be redundant.
//
// Taken from https://github.com/FiloSottile/mkcert/blob/master/cert.go#L295 written by one of Go's
-// crypto engineers
+// crypto engineers (BSD 3-clause).
func calculateSKID(pubKey crypto.PublicKey) ([]byte, error) {
spkiASN1, err := x509.MarshalPKIXPublicKey(pubKey)
if err != nil {
@@ -67,6 +75,7 @@
return skid[:], nil
}
+// New creates a new certificate authority with the given common name.
func New(name string) (*CA, error) {
pubKey, privKey, err := ed25519.GenerateKey(rand.Reader)
if err != nil {
@@ -76,7 +85,7 @@
serialNumberLimit := new(big.Int).Lsh(big.NewInt(1), 127)
serialNumber, err := rand.Int(rand.Reader, serialNumberLimit)
if err != nil {
- return nil, fmt.Errorf("Failed to generate serial number: %w", err)
+ return nil, fmt.Errorf("failed to generate serial number: %w", err)
}
skid, err := calculateSKID(pubKey)
@@ -101,7 +110,7 @@
caCertRaw, err := x509.CreateCertificate(rand.Reader, caCert, caCert, pubKey, privKey)
if err != nil {
- return nil, fmt.Errorf("Failed to create root certificate: %w", err)
+ return nil, fmt.Errorf("failed to create root certificate: %w", err)
}
ca := &CA{
@@ -109,16 +118,17 @@
CACertRaw: caCertRaw,
CACert: caCert,
}
- if ca.reissueCRL() != nil {
+ if ca.ReissueCRL() != nil {
return nil, fmt.Errorf("failed to create initial CRL: %w", err)
}
return ca, nil
}
+// FromCertificates restores CA state.
func FromCertificates(caCert []byte, caKey []byte, crl []byte) (*CA, error) {
if len(caKey) != ed25519.PrivateKeySize {
- return nil, errors.New("Invalid CA private key size")
+ return nil, errors.New("invalid CA private key size")
}
privateKey := ed25519.PrivateKey(caKey)
@@ -135,11 +145,11 @@
CACertRaw: caCert,
CACert: caCertVal,
Revoked: crlVal.TBSCertList.RevokedCertificates,
- CRLRaw: crl,
}, nil
}
-func (ca *CA) IssueCertificate(hostname string) (cert []byte, privkey []byte, err error) {
+// IssueCertificate issues a certificate
+func (ca *CA) IssueCertificate(commonName string) (cert []byte, privkey []byte, err error) {
serialNumberLimit := new(big.Int).Lsh(big.NewInt(1), 127)
serialNumber, err := rand.Int(rand.Reader, serialNumberLimit)
if err != nil {
@@ -159,7 +169,7 @@
etcdCert := &x509.Certificate{
SerialNumber: serialNumber,
Subject: pkix.Name{
- CommonName: hostname,
+ CommonName: commonName,
OrganizationalUnit: []string{"etcd"},
},
IsCA: false,
@@ -167,13 +177,13 @@
NotBefore: time.Now(),
NotAfter: unknownNotAfter,
ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth},
- DNSNames: []string{hostname},
+ DNSNames: []string{commonName},
}
cert, err = x509.CreateCertificate(rand.Reader, etcdCert, ca.CACert, pubKey, ca.PrivateKey)
return
}
-func (ca *CA) reissueCRL() error {
+func (ca *CA) ReissueCRL() error {
compatCert := CompatCertificate(*ca.CACert)
newCRL, err := compatCert.CreateCRL(rand.Reader, ca.PrivateKey, ca.Revoked, time.Now(), unknownNotAfter)
if err != nil {
@@ -193,5 +203,5 @@
SerialNumber: serial,
RevocationTime: time.Now(),
})
- return ca.reissueCRL()
+ return ca.ReissueCRL()
}
diff --git a/core/internal/consensus/consensus.go b/core/internal/consensus/consensus.go
index d87a506..f5ee949 100644
--- a/core/internal/consensus/consensus.go
+++ b/core/internal/consensus/consensus.go
@@ -14,6 +14,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.
+// package consensus manages the embedded etcd cluster.
package consensus
import (
@@ -23,6 +24,7 @@
"encoding/hex"
"encoding/pem"
"fmt"
+ "git.monogon.dev/source/nexantic.git/core/internal/common/service"
"io/ioutil"
"math/rand"
"net/url"
@@ -33,7 +35,6 @@
"time"
"git.monogon.dev/source/nexantic.git/core/generated/api"
- "git.monogon.dev/source/nexantic.git/core/internal/common"
"git.monogon.dev/source/nexantic.git/core/internal/consensus/ca"
"github.com/pkg/errors"
"go.etcd.io/etcd/clientv3"
@@ -61,13 +62,16 @@
type (
Service struct {
- *common.BaseService
+ *service.BaseService
- etcd *embed.Etcd
- kv clientv3.KV
- ready bool
- bootstrapCA *ca.CA
- bootstrapCert []byte
+ etcd *embed.Etcd
+ kv clientv3.KV
+ ready bool
+
+ // bootstrapCA and bootstrapCert cache the etcd cluster CA data during bootstrap.
+ bootstrapCA *ca.CA
+ bootstrapCert []byte
+
watchCRLTicker *time.Ticker
lastCRL []byte
@@ -79,10 +83,9 @@
DataDir string
InitialCluster string
NewCluster bool
-
- ExternalHost string
- ListenHost string
- ListenPort uint16
+ ExternalHost string
+ ListenHost string
+ ListenPort uint16
}
Member struct {
@@ -97,12 +100,14 @@
consensusServer := &Service{
config: &config,
}
- consensusServer.BaseService = common.NewBaseService("consensus", logger, consensusServer)
+ consensusServer.BaseService = service.NewBaseService("consensus", logger, consensusServer)
return consensusServer, nil
}
func (s *Service) OnStart() error {
+ // See: https://godoc.org/github.com/coreos/etcd/embed#Config
+
if s.config == nil {
return errors.New("config for consensus is nil")
}
@@ -121,17 +126,19 @@
}
s.lastCRL = lastCRL
- // Reset LCUrls because we don't want to expose any client
+ // Reset Listen Client URLs because we don't want to expose any client
cfg.LCUrls = nil
+ // Advertise Peer URLs
apURL, err := url.Parse(fmt.Sprintf("https://%s:%d", s.config.ExternalHost, s.config.ListenPort))
if err != nil {
- return errors.Wrap(err, "invalid external_host or listen_port")
+ return fmt.Errorf("invalid external_host or listen_port: %w", err)
}
+ // Listen Peer URLs
lpURL, err := url.Parse(fmt.Sprintf("https://%s:%d", s.config.ListenHost, s.config.ListenPort))
if err != nil {
- return errors.Wrap(err, "invalid listen_host or listen_port")
+ return fmt.Errorf("invalid listen_host or listen_port: %w", err)
}
cfg.APUrls = []url.URL{*apURL}
cfg.LPUrls = []url.URL{*lpURL}
@@ -160,11 +167,11 @@
// Override the logger
//*server.GetLogger() = *s.Logger.With(zap.String("component", "etcd"))
+ // TODO(leo): can we uncomment this?
go func() {
s.Logger.Info("waiting for etcd to become ready")
<-s.etcd.Server.ReadyNotify()
- s.ready = true
s.Logger.Info("etcd is now ready")
}()
@@ -177,7 +184,10 @@
return nil
}
-func (s *Service) SetupCertificates(certs *api.ConsensusCertificates) error {
+// WriteCertificateFiles writes the given node certificate data to local storage
+// such that it can be used by the embedded etcd server.
+// Unfortunately, we cannot pass the certificates directly to etcd.
+func (s *Service) WriteCertificateFiles(certs *api.ConsensusCertificates) error {
if err := ioutil.WriteFile(filepath.Join(s.config.DataDir, CRLPath), certs.Crl, 0600); err != nil {
return err
}
@@ -196,6 +206,7 @@
return nil
}
+// PrecreateCA generates the etcd cluster certificate authority and writes it to local storage.
func (s *Service) PrecreateCA() error {
// Provision an etcd CA
etcdRootCA, err := ca.New("Smalltown etcd Root CA")
@@ -211,7 +222,7 @@
}
// Preserve certificate for later injection
s.bootstrapCert = cert
- if err := s.SetupCertificates(&api.ConsensusCertificates{
+ if err := s.WriteCertificateFiles(&api.ConsensusCertificates{
Ca: etcdRootCA.CACertRaw,
Crl: etcdRootCA.CRLRaw,
Cert: cert,
@@ -227,14 +238,21 @@
caPathEtcd = "/etcd-ca/ca.der"
caKeyPathEtcd = "/etcd-ca/ca-key.der"
crlPathEtcd = "/etcd-ca/crl.der"
+
+ // This prefix stores the individual certs the etcd CA has issued.
certPrefixEtcd = "/etcd-ca/certs"
)
+// InjectCA copies the CA from data cached during PrecreateCA to etcd.
+// Requires a previous call to PrecreateCA.
func (s *Service) InjectCA() error {
+ if s.bootstrapCA == nil || s.bootstrapCert == nil {
+ panic("bootstrapCA or bootstrapCert are nil - missing PrecreateCA call?")
+ }
if _, err := s.kv.Put(context.Background(), caPathEtcd, string(s.bootstrapCA.CACertRaw)); err != nil {
return err
}
- // TODO: Should be wrapped by the master key
+ // TODO(lorenz): Should be wrapped by the master key
if _, err := s.kv.Put(context.Background(), caKeyPathEtcd, string([]byte(*s.bootstrapCA.PrivateKey))); err != nil {
return err
}
@@ -261,12 +279,12 @@
return nil, -1, fmt.Errorf("failed to get key from etcd: %w", err)
}
if len(res.Kvs) != 1 {
- return nil, -1, errors.New("key not available")
+ return nil, -1, errors.New("key not available or multiple keys returned")
}
return res.Kvs[0].Value, res.Kvs[0].ModRevision, nil
}
-func (s *Service) takeCAOnline() (*ca.CA, int64, error) {
+func (s *Service) getCAFromEtcd() (*ca.CA, int64, error) {
// TODO: Technically this could be done in a single request, but it's more logic
caCert, _, err := s.etcdGetSingle(caPathEtcd)
if err != nil {
@@ -289,7 +307,7 @@
}
func (s *Service) IssueCertificate(hostname string) (*api.ConsensusCertificates, error) {
- idCA, _, err := s.takeCAOnline()
+ idCA, _, err := s.getCAFromEtcd()
if err != nil {
return nil, err
}
@@ -303,6 +321,7 @@
}
serial := hex.EncodeToString(certVal.SerialNumber.Bytes())
if _, err := s.kv.Put(context.Background(), path.Join(certPrefixEtcd, serial), string(cert)); err != nil {
+ // We issued a certificate, but failed to persist it. Return an error and forget it ever happened.
return nil, fmt.Errorf("failed to persist certificate: %w", err)
}
return &api.ConsensusCertificates{
@@ -316,7 +335,7 @@
func (s *Service) RevokeCertificate(hostname string) error {
rand.Seed(time.Now().UnixNano())
for {
- idCA, crlRevision, err := s.takeCAOnline()
+ idCA, crlRevision, err := s.getCAFromEtcd()
if err != nil {
return err
}
@@ -338,6 +357,7 @@
}
}
}
+ // TODO(leo): this needs a test
cmp := clientv3.Compare(clientv3.ModRevision(crlPathEtcd), "=", crlRevision)
op := clientv3.OpPut(crlPathEtcd, string(idCA.CRLRaw))
res, err := s.kv.Txn(context.Background()).If(cmp).Then(op).Commit()
@@ -354,7 +374,7 @@
}
func (s *Service) watchCRL() {
- // TODO: Change etcd client to WatchableKV and make this an actual watch
+ // TODO(lorenz): Change etcd client to WatchableKV and make this an actual watch
// This needs changes in more places, so leaving it now
s.watchCRLTicker = time.NewTicker(30 * time.Second)
for range s.watchCRLTicker.C {