V2 certificate format #1216
V2 certificate format #1216
Conversation
|
|
||
| -- At least 1 ipv4 or ipv6 address must be present if isCA is false | ||
| networks SEQUENCE OF Network, | ||
| unsafeNetworks SEQUENCE OF Network OPTIONAL, |
There was a problem hiding this comment.
Very happy to see this getting renamed from -ips and -subnets which were a bit confusing (many people believed subnets was essentially the CIDR part of -ips.) That being said, calling them both "networks" might also be confusing. I think I'd lean towards simply calling this unsafeRoutes to match the config option.
There was a problem hiding this comment.
I'm not opposed to calling these unsafeRoutes, it doesn't quite feel right though since this field is describing a network that can be used to configure unsafe routes on another host.
cert/cert_v2.go
Outdated
There was a problem hiding this comment.
thoughts on each cert variety going in its own subpackage for namespacing and stuff?
There was a problem hiding this comment.
I started there but then came around to stuffing it all in cert so that we could hide away all the unnecessary bits.
| Nebula DEFINITIONS AUTOMATIC TAGS ::= BEGIN | ||
|
|
||
| Name ::= UTF8String (SIZE (1..253)) | ||
| Time ::= INTEGER (0..18446744073709551615) -- uint64 maximum |
There was a problem hiding this comment.
this is seconds-since-epoch, right?
| @@ -62,6 +62,7 @@ message NebulaHandshakeDetails { | |||
| uint32 ResponderIndex = 3; | |||
| uint64 Cookie = 4; | |||
| uint64 Time = 5; | |||
| uint32 CertVersion = 8; | |||
There was a problem hiding this comment.
thoughts on making this an enum to match cert.go?
There was a problem hiding this comment.
It would be a cyclical import for the cert package or else we'd have to do silly type conversions between the two packages.
I will say using cert.Version* for protocol level stuff gave me the ick.
| default: | ||
| return nil, r, ErrInvalidPEMCertificateBanner | ||
| } | ||
|
|
||
| if err != nil { | ||
| return nil, r, err |
There was a problem hiding this comment.
semantics changed a little bit here -- we used to not return the pem-remainder on err, now we do. It's probably fine? It's more consistent with the default/ErrInvalidPEMCertificateBanner branch.
| notBefore Time, | ||
| notAfter Time, | ||
|
|
||
| issuer OCTET STRING OPTIONAL, |
There was a problem hiding this comment.
how did issuer become optional?
There was a problem hiding this comment.
We are reusing the structure for CAs and they don't have an issuer. I need to put a comment describing the application behavior
…o to the lighthouse request handler
lighthouse.go
Outdated
| func (lhh *LightHouseHandler) handleHostQueryReply(n *NebulaMeta, vpnIp netip.Addr) { | ||
| if !lhh.lh.IsLighthouseIP(vpnIp) { | ||
| func (lhh *LightHouseHandler) handleHostQueryReply(n *NebulaMeta, reqHostinfo *HostInfo) { | ||
| //TODO: this is kind of dumb |
There was a problem hiding this comment.
it is dumb, but it raises some good questions about multi-IP stuff. Are we going to require users to put each IP for a lighthouse in their config file? If we do, does it make sense to group them?
lighthouse:
hosts:
- 10.0.0.1 # the old way
- ["fc00::2", "10.0.0.2"] # a new way
similar ideas apply wrt the static host map
There was a problem hiding this comment.
From a user perspective I would say no. I think it would be better to layer in learned lighthouse vpn addrs but as a stop gap I have included a IsAnyLighthouseIP (which the name is wrong ugh) that checks if any of the addrs are a lighthouse addr.
| return err | ||
| //set route MTU | ||
| for i := range t.vpnNetworks { | ||
| if err = t.setDefaultRoute(t.vpnNetworks[i]); err != nil { |
There was a problem hiding this comment.
this still races and errors sometimes on my proxmox host. Not sure why. On hosts that use systemd-networkd I haven't observed any issues.
WIP based on cert-interface branch