fix(client): wait for public reachability before registering #4
Conversation
doing this in `Present` was too late, it was called in the middle of the ACME dance, and we want to avoid the entire ACME management and flow if we are not publicly reachable. this change delays management only if there is no certificate cached locally, and impacts only the very first cold start when p2p-forge registration needs to occur. entire ACME/p2p-forge flow can be delayed/disabled on nodes that are not (yet) publicly diallable by only calling `ManageAsync` in `func (m *P2PForgeCertMgr) Start() error` once we have connectivity checks passed. for now, we just listen for network.ReachabilityPublic, but this can be refined further in the future.
lidel
changed the title
client/acme.go
Outdated
| @@ -407,6 +409,25 @@ func (d *dns01P2PForgeSolver) Wait(ctx context.Context, challenge acme.Challenge | |||
| func (d *dns01P2PForgeSolver) Present(ctx context.Context, challenge acme.Challenge) error { | |||
| h := d.hostFn() | |||
| addrs := h.Addrs() | |||
|
|
|||
| if !d.allowPrivateForgeAddresses { | |||
There was a problem hiding this comment.
@aschmahmann doing this in Present is too late, it is called in the middle of the ACME dance, and we want to avoid the entire ACME flow if we are not publicly reachable.
Pushed 5157ed9 which is tackling problem at a higher level: it is delaying calling ManageAsync for respective domain/cert in the top level func (m *P2PForgeCertMgr) Start() error, and the delay occurs only when necessary (no cert cached), and does not occur when you already have a cert.
It seems to do the trick for my Kubo setup (ipv4-only, NAT + port forwarding with uPnP) and also gives us framework for plugging up more nuanced checks if needed (right now it just waits for network.ReachabilityPublic).
Thoughts? Ok to remove this check from Present? (feels too late / redundant)
moved logic to reusable funcs and adjusted logger for better ux
There was a problem hiding this comment.
Refactored slightly so connectivity check is in separate func withHostConnectivity that we can improve later.
I'd like to ship this in Kubo 0.32.0-rc2 as-is, we can refine in follow-up releases. It already handles ipv4 NAT without uPnP correctly:
2024-11-06T01:12:34.218+0100 INFO autotls.start client/acme.go:359 no cert found for "*.k51qzi5uqu5dmbef23gjfbtpzbgbm0upn8quu4335d0edxzvmqx9zre6z2nwt5.libp2p.direct"
2024-11-06T01:12:34.218+0100 INFO autotls.start client/acme.go:384 waiting until libp2p reports event network.ReachabilityPublic
Swarm listening on 127.0.0.1:4081 (TCP+UDP)
[..]
Daemon is ready
2024-11-06T01:12:51.348+0100 INFO autotls.start client/acme.go:394 libp2p reachability status changed to Private
2024-11-06T01:12:51.348+0100 INFO autotls.start client/acme.go:399 certificate will not be requested while libp2p reachability status is Private
Filled #7 to tackle IPv6 issue upstream.
This PR attempts to not send out any challenges at all until the libp2p host has signaled that the node is public. The current level of filtering for public IP addresses does not totally work in that if you have a public IPv6 address but it's inaccessible due to a firewall then we will still reach out too early and hit a failure. Some of the current problems with this are:
AFAICT this does not work due to a mismatch between the ObsAddrManager and EvtLocalReachabilityChanged as emitted by autonat:
This seems like a bug we'd ideally fix within go-libp2p, but maybe I'm missing something
cc @sukunrt @MarcoPolo @lidel → please discuss in #7