Improve passive mode FQDN rule creation with domain name prioritization and smart port display#782
Conversation
…ently and add method to FilterDataProvider for retrieving the best hostname from flow, prioritizing domain names over IP addresses.
|
Since I don't have a paid Apple developer account I can't test the network extension myself. It will be great if you could test this. |
| //init addr/port with smart port display (hide common ports 80, 443) | ||
| if (([rule.endpointPort isEqualToString:@"80"] || [rule.endpointPort isEqualToString:@"443"]) && | ||
| NO == [rule.endpointAddr isEqualToString:VALUE_ANY] && | ||
| NO == [rule.endpointPort isEqualToString:VALUE_ANY]) { |
There was a problem hiding this comment.
I don't think the NO == [self.endpointPort isEqualToString:VALUE_ANY] check is needed?
The first condition explicitly checks if endpointPort equals "80" OR "443"
If the port is literally the string "80" or "443", it cannot simultaneously be VALUE_ANY (which is "*")
As such, I think this would suffice:
if (([self.endpointPort isEqualToString:@"80"] || [self.endpointPort isEqualToString:@"443"]) &&
NO == [self.endpointAddr isEqualToString:VALUE_ANY]) {
return address;
}
...right?
| remoteEndpoint = (NWHostEndpoint*)flow.remoteEndpoint; | ||
|
|
||
| //priority 1: try flow.URL.host (best for domain names) | ||
| if(nil != flow.URL.host && 0 != flow.URL.host.length) |
There was a problem hiding this comment.
I know I probably do checks like this elsewhere, this could be simplified to just: if(flow.URL.host.length)
| } | ||
|
|
||
| //priority 3: fallback to remoteEndpoint.hostname (may be IP address) | ||
| if(nil != remoteEndpoint.hostname && 0 != remoteEndpoint.hostname.length) |
There was a problem hiding this comment.
same here: if(remoteEndpoint.hostname.length) should suffice
| // Smart port display (hide common ports 80, 443) | ||
| if (([self.endpointPort isEqualToString:@"80"] || [self.endpointPort isEqualToString:@"443"]) && | ||
| NO == [self.endpointAddr isEqualToString:VALUE_ANY] && | ||
| NO == [self.endpointPort isEqualToString:VALUE_ANY]) { |
There was a problem hiding this comment.
See comment above, but this check NO == [self.endpointPort isEqualToString:VALUE_ANY]) is superfluous?
objective-see
left a comment
There was a problem hiding this comment.
Thanks for this PR! It's great - and I love the improvements!
Left a few comments for you to review/address, and then I'll merge it in! Mahalo again
|
Thanks for the PR, went ahead and merged it! I'll work on the tweaks I commented on earlier (as they are super minor) |
|
Hey, sorry for the late response and thank you for the merge. Maybe I can take care of these in an upcoming PR :) I just joined your discord as well. I plan on contributing to this in the future as well. Is there somewhere I can volunteer in your organization or join as a member. For now my motivation would be to get a contributor(least privileges needed to test network extensions, no malicious intentions, promise!) role in your org Apple developer team so that I can locally test network extensions which is necessary for Lulu. Who knows I could find more time in the future. Would love to play my part. I just went through your website and that's what inspired my decision just now. @objective-see |
|
Could this please be made optionally conditional upon an "Always show all port numbers" type of setting? I make a large number of rules that explicitly distinguish between ports 80 and 443 (e.g. I want a process to only be able to communicate over 443 and not 80) so no longer seeing the port numbers is far more cumbersome than convenient for my use case. |
|
@tophertopaz - yes. Will publish new version soon with the ability to toggle this as a setting 
|
|
@haroondilshad no worries - I implemented my suggested fixes 😁 |
|
Very much appreciated! Thank you both for the exceptional work you do! |
Too confusing, as it could be (mis)interpreted as the rule covered *all* traffic to an endpoints when port(s) weren't shown (#782)
|
Decided to revert back to always showing ports, as it didn't clean up the UI too much, and I think it's a bit confusing, as it could be (mis)interpreted as the rule covered all traffic to an endpoints when port(s) weren't shown. And yes, originally made a setting to toggle this, but really trying to keep the app simple, hence decided was easier just to revert to as was. |
Summary
This PR enhances passive mode rule creation to prioritize domain names over IP addresses and implements smart port display for cleaner UI. When passive mode creates rules automatically, they now show meaningful names like
github.cominstead of140.82.112.3:443.Changes Made
🎯 Domain Name Prioritization
getBestHostnameFromFlow:method toFilterDataProviderthat prioritizes hostname sources:flow.URL.host(best for domain names)flow.remoteHostname(macOS 11+ fallback)remoteEndpoint.hostname(IP address fallback)🎨 Smart Port Display
Test Results
✅ 9/9 tests passed covering:
📝 Testing Note
The included tests use standalone test files for quick validation and demonstration. For long-term maintainability, these could be converted to the standard XCTest framework and integrated as test targets in the Xcode project.
Before/After Examples
140.82.112.3:443github.com52.36.184.210:443api.slack.com127.0.0.1:8080localhost:8080Benefits
Technical Details
FilterDataProvider.m/.h,RulesWindowController.mgetBestHostnameFromFlow:using same logic as active modecreateConnectionCellmethodLuLu/Tests/)