Repository navigation
Conversation
62e6f76 to
284b683
Compare
|
friendly ping |
weiminyu
left a comment
There was a problem hiding this comment.
@weiminyu reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on CydeWeys and gbrodman).
core/src/main/java/google/registry/beam/spec11/SafeBrowsingTransforms.java line 82 at r1 (raw file):
public static int getPrefixInt(byte[] sha256Bytes) { return ByteBuffer.wrap(sha256Bytes).getInt(); }
Is there a way to write a test validating that the prefix-int from a string matches what SafeBrowing would generate?
Code quote:
public static byte[] computeSha256(String expression) {
return Hashing.sha256().hashString(expression, UTF_8).asBytes();
}
/** Converts the first 4 bytes of a SHA-256 hash into an integer. */
public static int getPrefixInt(byte[] sha256Bytes) {
return ByteBuffer.wrap(sha256Bytes).getInt();
}
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on CydeWeys and weiminyu).
core/src/main/java/google/registry/beam/spec11/SafeBrowsingTransforms.java line 82 at r1 (raw file):
Previously, weiminyu (Weimin Yu) wrote…
Is there a way to write a test validating that the prefix-int from a string matches what SafeBrowing would generate?
good call, let's be thorough. I downloaded the prefix-hash response from SafeBrowsing locally with our API key and have a main() method to test a few URLs with. I grabbed a couple known-bad URLs from the most recent run.
static void main(String[] args) throws Exception {
String responseJsonPath =
"/usr/local/google/home/gbrodman/code/nomulus/social_engineering_hashes.json";
int[] prefixes =
processSafeBrowsingFetchResponseJson(Files.readString(Path.of(responseJsonPath), UTF_8));
System.out.printf("Loaded and sorted %,d unique 4-byte prefixes.%n", prefixes.length);
for (String domain : ImmutableList.of("987aa.app", "solyvx.dev", "google.com")) {
byte[] hash = computeSha256(Ascii.toLowerCase(domain) + "/");
int prefixInt = getPrefixInt(hash);
int matchIndex = Arrays.binarySearch(prefixes, prefixInt);
System.out.printf("Domain: %s\n", domain);
System.out.printf("4-byte prefix hex : 0x%08x\n", prefixInt);
System.out.printf(
"Matched in array? : %s (binarySearch index: %d)%n", matchIndex >= 0, matchIndex);
}
}
Output:
Loaded and sorted 2,391,030 unique 4-byte prefixes.
Domain: 987aa.app
4-byte prefix hex : 0x36e0708b
Matched in array? : true (binarySearch index: 1708440)
Domain: solyvx.dev
4-byte prefix hex : 0xd7ddf5fe
Matched in array? : true (binarySearch index: 821040)
Domain: google.com
4-byte prefix hex : 0x88981e62
Matched in array? : false (binarySearch index: -80188)
CydeWeys
left a comment
There was a problem hiding this comment.
Just to be clear here:
"SafeBrowsing provides an API endpoint that returns, for each threat type provided, a base64-encoded string that represents the concatenation of 4-byte prefixes of sha256 hashes of harmful domains."
This is the list of ALL domains of these threat types in the entire SafeBrowsing system, right? Nothing specific to us?
@CydeWeys made 6 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on gbrodman and weiminyu).
core/src/main/java/google/registry/beam/spec11/SafeBrowsingTransforms.java line 71 at r1 (raw file):
/** The threat types evaluated for Spec11 reporting. */ static final ImmutableList<String> THREAT_TYPES =
Does order matter here? Otherwise it can be a Set.
core/src/main/java/google/registry/beam/spec11/SafeBrowsingTransforms.java line 190 at r1 (raw file):
if (rawHashes != null) { int prefixSize = rawHashes.optInt("prefixSize"); checkArgument(prefixSize == 4, "Expected prefixSize of 4, got %s", prefixSize);
I'm not sure this is really a checkArgument. More like a checkState? This is data coming back from the API, right?
core/src/main/java/google/registry/beam/spec11/SafeBrowsingTransforms.java line 215 at r1 (raw file):
} /** Removes duplicates from an already sorted prefix array in-place. */
Any reason to go through all this vs just using a Set of Integers? I think the logic here is sound, but is there actually a need to implement any of this at all?
core/src/main/java/google/registry/beam/spec11/SafeBrowsingTransforms.java line 235 at r1 (raw file):
int statusCode = response.getStatusLine().getStatusCode(); if (statusCode != SC_OK) { if (statusCode == SC_TOO_MANY_REQUESTS) {
I take it the new API doesn't ever return this then? (Or we're not expecting to call it enough to ever hit this?)
core/src/main/java/google/registry/beam/spec11/SafeBrowsingTransforms.java line 347 at r1 (raw file):
byte[] hash = computeSha256(Ascii.toLowerCase(domainNameInfo.domainName()) + "/"); int prefixInt = getPrefixInt(hash); if (Arrays.binarySearch(prefixes, prefixInt) < 0) {
Hrm ... how many elements are we processing here exactly? And we have to do a binary search for each one? It seems like a hashtable (could be provided by Java's Set implementation) might be more performant here?
SafeBrowsing provides an API endpoint that returns, for each threat type
provided, a base64-encoded string that represents the concatenation of
4-byte prefixes of sha256 hashes of harmful domains. That means we can
do the following
- decode the base64 string into a byte[]
- interpret the byte[] as an int[], because each int is four bytes
- for each domain:
- hash the domain and take the hash's first four bytes as an int
- if that int is in the prefix hash array, query SafeBrowsing directly
with that domain name
- if the int is not in the prefix hash array, we know the domain is
not harmful (according to SafeBrowsing at least)
The prefix list provided by SafeBrowsing contains about 2.5 million
entries which corresponds to only about 10 MB of heap memory and only
about 0.1% of the space of all integers. Thus, we can assume there won't
be too many hash collisions.
This will reduce our number of API calls from ~5100 calls (each
containing 490 domains) to 6 API calls + (# actually-harmful domains /
490).
gbrodman
left a comment
There was a problem hiding this comment.
Correct. Not specific to us.
@gbrodman made 6 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on CydeWeys and weiminyu).
core/src/main/java/google/registry/beam/spec11/SafeBrowsingTransforms.java line 71 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Does order matter here? Otherwise it can be a Set.
It doesn't matter. But it shouldn't be a set regardless. This is used for iterating over and converting to a JSONArray object. List is better.
core/src/main/java/google/registry/beam/spec11/SafeBrowsingTransforms.java line 190 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
I'm not sure this is really a checkArgument. More like a checkState? This is data coming back from the API, right?
yeah that seems right
core/src/main/java/google/registry/beam/spec11/SafeBrowsingTransforms.java line 215 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Any reason to go through all this vs just using a Set of Integers? I think the logic here is sound, but is there actually a need to implement any of this at all?
There is a need. 2.5 million integers in array form is only 10-12 MB of heap memory but boxing those and adding the overhead of a giant hash table explodes the memory usage probably 10x or more. This is especially important because we're passing this value out to all the workers.
core/src/main/java/google/registry/beam/spec11/SafeBrowsingTransforms.java line 235 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
I take it the new API doesn't ever return this then? (Or we're not expecting to call it enough to ever hit this?)
The latter. We're reducing the number of our API calls drastically, as only about 0.1% of non-harmful domains will now require inclusion in these API calls.
core/src/main/java/google/registry/beam/spec11/SafeBrowsingTransforms.java line 347 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Hrm ... how many elements are we processing here exactly? And we have to do a binary search for each one? It seems like a hashtable (could be provided by Java's Set implementation) might be more performant here?
This is referenced elsewhere in the PR, but it's roughly 2.5 million prefixes, and we're checking each of our live domains against it. It should be very quick (~20 integer comparisons for each of our active URLs). See the other comment -- a hash table explodes the size too much and we'd still have the overhead of boxing/unboxing/hash-table-querying.
284b683 to
3cf9931
Compare
SafeBrowsing provides an API endpoint that returns, for each threat type provided, a base64-encoded string that represents the concatenation of 4-byte prefixes of sha256 hashes of harmful domains. That means we can do the following
The prefix list provided by SafeBrowsing contains about 2.5 million entries which corresponds to only about 10 MB of heap memory and only about 0.1% of the space of all integers. Thus, we can assume there won't be too many hash collisions.
This will reduce our number of API calls from ~5100 calls (each containing 490 domains) to 6 API calls + (# actually-harmful domains / 490).
This change is