Implementation for FLIP 332: Stricter Validation of Node Network Addresses #484
Conversation
contracts/FlowIDTableStaking.cdc
Outdated
| // Check if domain is an IP address (contains only numbers and dots) | ||
| let domainParts = domain.split(separator: ".") | ||
| if domainParts.length == 4 { | ||
| var isIP = true | ||
| for part in domainParts { | ||
| let num = UInt8.fromString(part) | ||
| if num == nil || num! > 255 { | ||
| isIP = false | ||
| break | ||
| } | ||
| } | ||
| if isIP { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| // Check if domain has at least one dot and valid characters | ||
| if !domain.contains(".") { | ||
| return false | ||
| } |
There was a problem hiding this comment.
here better way is to check last segment is not numeric. ( ip has a lot of quirks: 51.5499081 is a valid IP address for example )
There was a problem hiding this comment.
excellent suggestion!
There was a problem hiding this comment.
I have update the changes to be more robust.
contracts/FlowIDTableStaking.cdc
Outdated
| for char in domain.utf8 { | ||
| // Check if character is: | ||
| // - a letter (a-z, A-Z) | ||
| // - a number (0-9) | ||
| // - a dot (.) | ||
| // - a hyphen (-) | ||
| if !((char >= 48 && char <= 57) || // numbers | ||
| (char >= 65 && char <= 90) || // uppercase letters | ||
| (char >= 97 && char <= 122) || // lowercase letters | ||
| char == 46 || // dot | ||
| char == 45) { // hyphen | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
maybe something like this ?
var validChars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789.-".utf8
for char in domain.utf8{
if !validChars.contains(char) {
return false
}
}
| IDTableSigner, | ||
| adminID, | ||
| fmt.Sprintf("%0128d", admin), | ||
| // Invalid Networking Address: IPv4 address instead of domain name |
There was a problem hiding this comment.
Unit tests to check that IPv4, IPv6, missing port and invalid domain are caught by the validation.
…ving length check to inside the network address validation
| FlowIDTableStaking.isValidNodeID(id): "The node ID must have only numbers and lowercase hex characters" | ||
| FlowIDTableStaking.nodes[id] == nil: "The ID cannot already exist in the record" | ||
| role >= UInt8(1) && role <= UInt8(5): "The role must be 1, 2, 3, 4, or 5" | ||
| networkingAddress.length > 0 && networkingAddress.length <= 510: "The networkingAddress must be less than 510 characters" |
There was a problem hiding this comment.
moved this check inside of the isValidateNetworkingAddress function.
vishalchangrani
changed the title
| true) | ||
| }) | ||
|
|
||
| t.Run("Shouldn't be able to create Node struct with an invalid networking address", func(t *testing.T) { |
There was a problem hiding this comment.
separated out the test for networking address in it's own unit test.
contracts/FlowIDTableStaking.cdc
Outdated
| /// 1. Must not be an IP address | ||
| /// 2. Must contain a port number after a colon | ||
| /// 3. Must be a valid domain name format | ||
| access(contract) view fun isValidNetworkingAddress(address: String): Bool { |
There was a problem hiding this comment.
what do you guys think about making this access(all) instead access(contract) so that any client side apps like FlowPort can use it to verify the networking address before submitting the transaction.
The function does not modify state so should be safe to expose.
There was a problem hiding this comment.
yes, we should make this access(all)
There was a problem hiding this comment.
changed it to access(all).
|
@vishalchangrani Are you ready to merge this? After we merge it, I can update flow-go and we can schedule the upgrades |
| let validChars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789.-".utf8 | ||
| for char in domain.utf8 { | ||
| if !validChars.contains(char) { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
This might be quite inefficient: The lookup array of valid chars is re-built on each call of the function, and the loop ends up effectively performing a nested linear scan: first over all characters of the domain, and then again nested over all valid characters.
There was a problem hiding this comment.
The original implementation was way more efficient: 62f22a4 (#484)
There was a problem hiding this comment.
@turbolent Actually I suggested this one for readability mostly, didn't seem to be too inefficient at first sight. ( ~ maybe half efficient ) and I assumed this would be called once ofc.
There was a problem hiding this comment.
Ah, I see now! Nice 👍 Yeah, this is definitely more readable, the original implementation is not very obvious. I don't know how often this will be called. In case it is a heavily called function, we might want to trade some readability / "straight-forwardness" code for performance.
There was a problem hiding this comment.
This validation will only be called when a new node is registered or when the networking address of an existing node is update. Both of those actions don't happen frequently - once or twice every couple of epoch at most. Hence, I would rather have more readability than performance. Open to suggestions tho.
Co-authored-by: Bastian Müller <[email protected]>
Do we generally first make a release of flow-go and then test it on testnet or the other way round? I was planning on testing this on testnet this week. |
vishalchangrani
changed the title
|
@vishalchangrani We try to merge right before we upgrade on testnet and create a release for flow-go before we upgrade on mainnet. |
I plan to upgrade on testnet by EOD tomorrow. Will merge this in before that. |
|
Updated staking contract on testnet with this change and tested it out. Works as expected.
|
FLIP PR: onflow/flips#333
FLIP Issue: onflow/flips#332