Skip to content

Commit 73e7946

Browse files
JackDoanJackDoan
committed
try to make certificate addition/removal reloadable in some cases
1 parent 73cfa7b commit 73e7946

File tree

2 files changed

+74
-47
lines changed

2 files changed

+74
-47
lines changed

‎connection_manager.go‎

Lines changed: 30 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -476,8 +476,8 @@ func (cm *connectionManager) swapPrimary(current, primary *HostInfo) {
476476
}
477477

478478
// isInvalidCertificate will check if we should destroy a tunnel if pki.disconnect_invalid is true and
479-
// the certificate is no longer valid. Block listed certificates will skip the pki.disconnect_invalid
480-
// check and return true.
479+
// the certificate is no longer valid, or if we no longer have a certificate of the same version as the remote.
480+
// Blocklisted certificates will skip the pki.disconnect_invalid check and return true.
481481
func (cm *connectionManager) isInvalidCertificate(now time.Time, hostinfo *HostInfo) bool {
482482
remoteCert := hostinfo.GetCert()
483483
if remoteCert == nil {
@@ -488,18 +488,38 @@ func (cm *connectionManager) isInvalidCertificate(now time.Time, hostinfo *HostI
488488
err := caPool.VerifyCachedCertificate(now, remoteCert)
489489
if err == nil {
490490
return false
491-
}
492-
493-
if !cm.intf.disconnectInvalid.Load() && err != cert.ErrBlockListed {
491+
} else if err == cert.ErrBlockListed { //avoiding errors.Is for speed
494492
// Block listed certificates should always be disconnected
495-
return false
493+
hostinfo.logger(cm.l).WithError(err).
494+
WithField("fingerprint", remoteCert.Fingerprint).
495+
Info("Remote certificate is blocked, tearing down the tunnel")
496+
return true
497+
} else if cm.intf.disconnectInvalid.Load() {
498+
hostinfo.logger(cm.l).WithError(err).
499+
WithField("fingerprint", remoteCert.Fingerprint).
500+
Info("Remote certificate is no longer valid, tearing down the tunnel")
501+
return true
496502
}
497503

498-
hostinfo.logger(cm.l).WithError(err).
499-
WithField("fingerprint", remoteCert.Fingerprint).
500-
Info("Remote certificate is no longer valid, tearing down the tunnel")
504+
//check that we still have a cert version in common with this connection. If we do not, disconnect.
505+
remoteVersion := remoteCert.Certificate.Version()
506+
cs := cm.intf.pki.getCertState()
507+
out := false
508+
switch remoteVersion {
509+
case cert.Version1:
510+
out = cs.v1Cert == nil
511+
case cert.Version2:
512+
out = cs.v2Cert == nil
513+
default:
514+
out = true
515+
}
501516

502-
return true
517+
if out {
518+
hostinfo.logger(cm.l).WithField("fingerprint", remoteCert.Fingerprint).
519+
WithField("version", remoteVersion).
520+
Info("We no longer have a certificate in common with remote, tearing down the tunnel")
521+
}
522+
return out
503523
}
504524

505525
func (cm *connectionManager) sendPunch(hostinfo *HostInfo) {

‎pki.go‎

Lines changed: 44 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -100,55 +100,62 @@ func (p *PKI) reloadCerts(c *config.C, initial bool) *util.ContextualError {
100100
currentState := p.cs.Load()
101101
if newState.v1Cert != nil {
102102
if currentState.v1Cert == nil {
103-
return util.NewContextualError("v1 certificate was added, restart required", nil, err)
103+
//adding certs is fine, actually. Networks-in-common confirmed in newCertState().
104+
} else {
105+
// did IP in cert change? if so, don't set
106+
if !slices.Equal(currentState.v1Cert.Networks(), newState.v1Cert.Networks()) {
107+
return util.NewContextualError(
108+
"Networks in new cert was different from old",
109+
m{"new_networks": newState.v1Cert.Networks(), "old_networks": currentState.v1Cert.Networks()},
110+
nil,
111+
)
112+
}
113+
114+
if currentState.v1Cert.Curve() != newState.v1Cert.Curve() {
115+
return util.NewContextualError(
116+
"Curve in new cert was different from old",
117+
m{"new_curve": newState.v1Cert.Curve(), "old_curve": currentState.v1Cert.Curve()},
118+
nil,
119+
)
120+
}
104121
}
105-
106-
// did IP in cert change? if so, don't set
107-
if !slices.Equal(currentState.v1Cert.Networks(), newState.v1Cert.Networks()) {
108-
return util.NewContextualError(
109-
"Networks in new cert was different from old",
110-
m{"new_networks": newState.v1Cert.Networks(), "old_networks": currentState.v1Cert.Networks()},
111-
nil,
112-
)
113-
}
114-
115-
if currentState.v1Cert.Curve() != newState.v1Cert.Curve() {
116-
return util.NewContextualError(
117-
"Curve in new cert was different from old",
118-
m{"new_curve": newState.v1Cert.Curve(), "old_curve": currentState.v1Cert.Curve()},
119-
nil,
120-
)
121-
}
122-
123-
} else if currentState.v1Cert != nil {
124-
//TODO: CERT-V2 we should be able to tear this down
125-
return util.NewContextualError("v1 certificate was removed, restart required", nil, err)
126122
}
127123

128124
if newState.v2Cert != nil {
129125
if currentState.v2Cert == nil {
130-
return util.NewContextualError("v2 certificate was added, restart required", nil, err)
126+
//adding certs is fine, actually
127+
} else {
128+
// did IP in cert change? if so, don't set
129+
if !slices.Equal(currentState.v2Cert.Networks(), newState.v2Cert.Networks()) {
130+
return util.NewContextualError(
131+
"Networks in new cert was different from old",
132+
m{"new_networks": newState.v2Cert.Networks(), "old_networks": currentState.v2Cert.Networks()},
133+
nil,
134+
)
135+
}
136+
137+
if currentState.v2Cert.Curve() != newState.v2Cert.Curve() {
138+
return util.NewContextualError(
139+
"Curve in new cert was different from old",
140+
m{"new_curve": newState.v2Cert.Curve(), "old_curve": currentState.v2Cert.Curve()},
141+
nil,
142+
)
143+
}
131144
}
132145

133-
// did IP in cert change? if so, don't set
134-
if !slices.Equal(currentState.v2Cert.Networks(), newState.v2Cert.Networks()) {
135-
return util.NewContextualError(
136-
"Networks in new cert was different from old",
137-
m{"new_networks": newState.v2Cert.Networks(), "old_networks": currentState.v2Cert.Networks()},
138-
nil,
139-
)
146+
} else if currentState.v2Cert != nil {
147+
//newState.v1Cert is non-nil bc empty certstates aren't permitted
148+
if newState.v1Cert == nil {
149+
return util.NewContextualError("v1 and v2 certs are nil, this should be impossible", nil, err)
140150
}
141-
142-
if currentState.v2Cert.Curve() != newState.v2Cert.Curve() {
151+
//if we're going to v1-only, we need to make sure we didn't orphan any v2-cert vpnaddrs
152+
if !slices.Equal(currentState.v2Cert.Networks(), newState.v1Cert.Networks()) {
143153
return util.NewContextualError(
144-
"Curve in new cert was different from old",
145-
m{"new_curve": newState.v2Cert.Curve(), "old_curve": currentState.v2Cert.Curve()},
154+
"Removing a V2 cert is not permitted unless it has identical networks to the new V1 cert",
155+
m{"new_networks": newState.v1Cert.Networks(), "old_networks": currentState.v2Cert.Networks()},
146156
nil,
147157
)
148158
}
149-
150-
} else if currentState.v2Cert != nil {
151-
return util.NewContextualError("v2 certificate was removed, restart required", nil, err)
152159
}
153160

154161
// Cipher cant be hot swapped so just leave it at what it was before

0 commit comments

Comments
 (0)