-
Notifications
You must be signed in to change notification settings - Fork 446
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: require external confirmation of public addresses #2867
base: main
Are you sure you want to change the base?
Conversation
Requires autonat to confirm external IP addresses and domain names before the node will announce them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but left some suggestions
*/ | ||
multiaddr: Multiaddr | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the JSDocs but perhaps this could be called isExternallyRoutable
as confident
seems a bit ambiguous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree confident
is a bit vague, but I'm not sure isExternallyRoutable
is right as things like localhost/loopback addresses will appear here which aren't externally routable.
Maybe something like verified
would be more self-explanatory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, yes I agree verified
is perhaps the best fit here
@@ -41,6 +74,11 @@ export interface AddressManager { | |||
*/ | |||
getAddresses(): Multiaddr[] | |||
|
|||
/** | |||
* Return all addresses we know about with metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Return all addresses we know about with metadata | |
* Return all known addresses that have metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me the suggested change implies that some addresses will not have metadata and will be omitted. This may be better:
* Return all addresses we know about with metadata | |
* Return all known addresses with metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, agreed
return | ||
} | ||
|
||
if (codec === CODEC_DNS4 || codec === CODEC_DNS6) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit difficult to understand with multiple nested loops as well as there's similar logic being repeated between the DNS and IP codec blocks. Perhaps it could be broken up into a few more functions.
function updateConfidenceForIP(
ip: string,
publicAddressMappings: Map<string, Array<{ externalIp: string, externalPort: number, confident: boolean }>>
): boolean {
let wasConfident = false
// Get all mappings as a flat array
const allMappings = Array.from(publicAddressMappings.values()).flat()
// Update confidence for any mapping with matching IP
allMappings
.filter(m => m.externalIp === ip)
.forEach(m => {
wasConfident ||= m.confident // track if any were previously confident
m.confident = true
})
return wasConfident
}
confirmObservedAddr(addr: Multiaddr): void {
addr = stripPeerId(addr, this.components.peerId)
const addrString = addr.toString()
let startingConfidence = false
if (this.observed.has(addrString)) {
startingConfidence = this.observed.get(addrString)?.confident ?? false
this.observed.set(addrString, { confident: true })
} else {
startingConfidence = this.updateAddressConfidence(addr)
}
if (!startingConfidence) {
this._updatePeerStoreAddresses()
}
}
private updateAddressConfidence(addr: Multiaddr): boolean {
const [codec, value] = addr.stringTuples()[0]
if (value == null) {
return false
}
if (codec === CODEC_DNS4 || codec === CODEC_DNS6) {
return this.updateDNSConfidence(value)
}
if (codec === CODEC_IP4 || codec === CODEC_IP6) {
return this.updateConfidenceForIP(value, this.publicAddressMappings)
}
return false
}
private updateDNSConfidence(domain: string): boolean {
for (const [ip, mapping] of Array.from(this.ipDomainMappings)) {
if (mapping.domain === domain) {
mapping.confident = true
return this.updateConfidenceForIP(ip, this.publicAddressMappings)
}
}
return false
}
Requires autonat to confirm external IP addresses and domain names before the node will announce them.
Change checklist