RUMM-2153 Prevent Kronos logic from querying private IPs #830
Conversation
ncreated
force-pushed
the
ncreated/RUMM-2153-prevent-Kronos-from-querying-local-IPs
branch
from
0ae9c40
to
762292f
Compare
| static func mockIPv4(_ bytes: [UInt8]) throws -> KronosInternetAddress { | ||
| precondition(bytes.count == 4, "Expected 4 bytes") | ||
| let numbers = bytes.map { String($0) } | ||
| let ipv4String = numbers.joined(separator: ".") // e.g. '192.168.1.1' | ||
| let address: KronosInternetAddress? = .mockWith(ipv4String: ipv4String) | ||
| return try XCTUnwrap(address, "\(ipv4String) is not a valid IPv4 string") | ||
| } | ||
|
|
||
| static func mockIPv6(_ bytes: [UInt8]) throws -> KronosInternetAddress { | ||
| precondition(bytes.count == 16, "Expected 16 bytes") | ||
| let groups: [String] = (0..<8).map { idx in | ||
| let hexA = String(bytes[idx * 2], radix: 16, uppercase: false) | ||
| let hexB = String(bytes[idx * 2 + 1], radix: 16, uppercase: false) | ||
| return hexA + hexB | ||
| } | ||
| let ipv6String = groups.joined(separator: ":") // e.g. 'ab:ab:ab:ab:ab:ab:ab:ab' | ||
| let address: KronosInternetAddress? = .mockWith(ipv6String: ipv6String) | ||
| return try XCTUnwrap(address, "\(ipv6String) is not a valid IPv6 string") | ||
| } |
There was a problem hiding this comment.
The way we mock KronosInternetAddress values for IPv4 and IPv6 is quite tricky, but couldn't find a way to make it simpler. Alternatively, we could implement filtering as stand-alone function (isPrivate(host:)), but I found computed property (ip.isPrivate) more idiomatic.
There was a problem hiding this comment.
I'm not sure of the fuzzy tests value here 🤔 especially since the generated IPs is valid/expected. We could simply test against edges and some pre-determined values in between, no?
There was a problem hiding this comment.
Yes, we're not checking against invalid input because in practise it is impossible to receive one (IRL the IP is read by CFHostGetAddressing() and bound to arbitrary memory, so invalid input would rather cause a crash in system frameworks than be allowed).
On the practical side, having 100% coverage among all allowed inputs makes it easier to change this classification logic. For that reason I'd rather keep it as it is.
buranmert
changed the title
Tests/DatadogTests/Datadog/Kronos/KronosInternetAddressTests.swift
Outdated
Show resolved
Hide resolved
ncreated
force-pushed
the
ncreated/RUMM-2153-prevent-Kronos-from-querying-local-IPs
branch
from
762292f
to
25d9eae
Compare
|
we can mention our fix in Kronos repo (MobileNativeFoundation/Kronos#94) so that other users can apply a similar fix |
ncreated
deleted the
ncreated/RUMM-2153-prevent-Kronos-from-querying-local-IPs
branch
Changes from DataDog/dd-sdk-ios#830 Also added 'GENERATE_INFOPLIST_FILE = YES;' to Tests target in order for tests to run.
Changes from DataDog/dd-sdk-ios#830 Also added 'GENERATE_INFOPLIST_FILE = YES;' to Tests target in order for tests to run. Signed-off-by: jean <jean@witick.io>
|
We have recently been bitten by this. Is there an easy way to disable NTP altogether? The device clocks are already more than accurate enough for our needs. |
What and why?
🐞 This PR aims at fixing #647. As explained in #647 (comment) we found an evidence (in our internal telemetry) that in some circumstances Kronos (NTP) logic can try to query private IP and hence bring up the "Local Network Permission" alert.
This happens very rarely, and except a single hit in our logs we couldn't manage to reproduce it.
How?
Our telemetry shows a clear attempt on querying
192.168.1.250. To avoid this, in this PR we apply additional filtering inKronosDNSResolverto ensure that private IPs are never used for NTP sync.If all resolved IPs are filtered out, Kronos will fail silently and fallback to previous known time correction (stored in user defaults). In case of fresh start, the SDK falls back to using device time:
Review checklist
Custom CI job configuration (optional)