Bug 222208

Summary: PCM: Store and report source unlinkable tokens
Product: WebKit Reporter: Jiewen Tan <jiewen_tan>
Component: WebKit Misc.Assignee: Jiewen Tan <jiewen_tan>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, ews-watchlist, japhet, jiewen_tan, katherine_cheney, webkit-bug-importer, wilander
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 221291    
Attachments:
Description Flags
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
wilander: review+
Patch for landing none

Description Jiewen Tan 2021-02-19 15:27:12 PST
Store and report source unlinkable tokens.
Comment 1 Jiewen Tan 2021-02-19 15:27:27 PST
<rdar://problem/73582032>
Comment 2 Jiewen Tan 2021-02-19 15:33:48 PST
Created attachment 421046 [details]
Patch
Comment 3 John Wilander 2021-02-19 16:11:23 PST
Comment on attachment 421046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421046&action=review

> Source/WebKit/ChangeLog:21
> +        Connects the fraud prevention feature in PCM to generate, store, report the unlinkable tokens. 

I believe it's the secret token that gets reported.
Nit: Token, not tokens.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:149
> +        reportDetails->setInteger("version"_s, 1);

This is not needed. The version is only telling the recipient which version of PCM the browser is running and as soon as we support unlinkable tokens, we are running version 2 regardless of whether there *is* a token for the current report. You'll have to rebase 1-2 test cases for version 2 though.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:229
> +    if (token.tokenBase64URL.isEmpty() || token.signatureBase64URL.isEmpty() || token.keyIDBase64URL.isEmpty())

Could this be an isValid() convenience function on SourceUnlinkableToken?

> Source/WebCore/loader/PrivateClickMeasurement.h:279
>      // MARK: - Fraud Prevention

Is this in source already? Looks like a stray comment.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3036
> +            || statement.bindText(10, sourceUnlinkableToken ? sourceUnlinkableToken->keyIDBase64URL : emptyString()) != SQLITE_OK

Kate should have a look at the DB changes.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:136
> +void PrivateClickMeasurementManager::getTokenPublicKey(PrivateClickMeasurement&& attribution, Function<void(PrivateClickMeasurement&& attribution, const String& publicKeyBase64URL)>&& callback)

This should be a CompletionHandler instead of a Function and also be called completionHandler.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:166
> +        // FIXME(): Retrieve the public key from the content instead of the header, and validate the public key.

Missing bug number. If you don't intend to file one, I'd drop the parenthesis.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:188
> +    // FIXME(): Should check whether the tokenSignatureJSON is valid before continuing.

Missing bug number. If you don't intend to file one, I'd drop the parenthesis.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:200
>              return;

I'm a little confused on the terminology here. Isn't the token that's being signed the "unlinkable token" and not the "secret token?"

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:203
> +        // FIXME(): Retrieve the signature from the content instead of the header.

Ditto on the bug number.
Comment 4 Jiewen Tan 2021-02-19 16:32:25 PST
Comment on attachment 421046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421046&action=review

>> Source/WebKit/ChangeLog:21
>> +        Connects the fraud prevention feature in PCM to generate, store, report the unlinkable tokens. 
> 
> I believe it's the secret token that gets reported.
> Nit: Token, not tokens.

No, the secret token, i.e., blinded secret, is only an intermediate product. We will only store and report the unlinkable token. Will fix tokens -> token.

>> Source/WebCore/loader/PrivateClickMeasurement.cpp:149
>> +        reportDetails->setInteger("version"_s, 1);
> 
> This is not needed. The version is only telling the recipient which version of PCM the browser is running and as soon as we support unlinkable tokens, we are running version 2 regardless of whether there *is* a token for the current report. You'll have to rebase 1-2 test cases for version 2 though.

Will fix it.

>> Source/WebCore/loader/PrivateClickMeasurement.cpp:229
>> +    if (token.tokenBase64URL.isEmpty() || token.signatureBase64URL.isEmpty() || token.keyIDBase64URL.isEmpty())
> 
> Could this be an isValid() convenience function on SourceUnlinkableToken?

It could be. Will fix it.

>> Source/WebCore/loader/PrivateClickMeasurement.h:279
>>      // MARK: - Fraud Prevention
> 
> Is this in source already? Looks like a stray comment.

Yes, it's in the source. This comment is used to mark different portions of the code when people are using the function navigator from Xcode or similar functions from other IDE.

>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3036
>> +            || statement.bindText(10, sourceUnlinkableToken ? sourceUnlinkableToken->keyIDBase64URL : emptyString()) != SQLITE_OK
> 
> Kate should have a look at the DB changes.

Will cc Kate.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:136
>> +void PrivateClickMeasurementManager::getTokenPublicKey(PrivateClickMeasurement&& attribution, Function<void(PrivateClickMeasurement&& attribution, const String& publicKeyBase64URL)>&& callback)
> 
> This should be a CompletionHandler instead of a Function and also be called completionHandler.

CompletionHandler needs to be called at least once. I don't think there is a need to call that if the key is not valid or other early returns.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:166
>> +        // FIXME(): Retrieve the public key from the content instead of the header, and validate the public key.
> 
> Missing bug number. If you don't intend to file one, I'd drop the parenthesis.

This a WIP. Will definitely file one.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:188
>> +    // FIXME(): Should check whether the tokenSignatureJSON is valid before continuing.
> 
> Missing bug number. If you don't intend to file one, I'd drop the parenthesis.

Ditto.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:200
>>              return;
> 
> I'm a little confused on the terminology here. Isn't the token that's being signed the "unlinkable token" and not the "secret token?"

I think the final product is the unlinkable token and the intermediate product is the secret token.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:203
>> +        // FIXME(): Retrieve the signature from the content instead of the header.
> 
> Ditto on the bug number.

Ditto.
Comment 5 Kate Cheney 2021-02-19 16:38:42 PST
Comment on attachment 421046 [details]
Patch

DB changes look good to me.
Comment 6 John Wilander 2021-02-19 16:46:48 PST
(In reply to Jiewen Tan from comment #4)
> Comment on attachment 421046 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421046&action=review
> 
> >> Source/WebKit/ChangeLog:21
> >> +        Connects the fraud prevention feature in PCM to generate, store, report the unlinkable tokens. 
> > 
> > I believe it's the secret token that gets reported.
> > Nit: Token, not tokens.
> 
> No, the secret token, i.e., blinded secret, is only an intermediate product.
> We will only store and report the unlinkable token. Will fix tokens -> token.

We need to settle on the terms used and stick to them. This has been my understanding:
1. Browser generates a secret.
2. Browser blinds the secret.
3. Browser sends blinded secret to server for signing.
4. Time passes and a triggering event happens.
5. Browser creates the report and includes the secret and its signature.

> >> Source/WebCore/loader/PrivateClickMeasurement.cpp:149
> >> +        reportDetails->setInteger("version"_s, 1);
> > 
> > This is not needed. The version is only telling the recipient which version of PCM the browser is running and as soon as we support unlinkable tokens, we are running version 2 regardless of whether there *is* a token for the current report. You'll have to rebase 1-2 test cases for version 2 though.
> 
> Will fix it.
> 
> >> Source/WebCore/loader/PrivateClickMeasurement.cpp:229
> >> +    if (token.tokenBase64URL.isEmpty() || token.signatureBase64URL.isEmpty() || token.keyIDBase64URL.isEmpty())
> > 
> > Could this be an isValid() convenience function on SourceUnlinkableToken?
> 
> It could be. Will fix it.
> 
> >> Source/WebCore/loader/PrivateClickMeasurement.h:279
> >>      // MARK: - Fraud Prevention
> > 
> > Is this in source already? Looks like a stray comment.
> 
> Yes, it's in the source. This comment is used to mark different portions of
> the code when people are using the function navigator from Xcode or similar
> functions from other IDE.
> 
> >> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3036
> >> +            || statement.bindText(10, sourceUnlinkableToken ? sourceUnlinkableToken->keyIDBase64URL : emptyString()) != SQLITE_OK
> > 
> > Kate should have a look at the DB changes.
> 
> Will cc Kate.
> 
> >> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:136
> >> +void PrivateClickMeasurementManager::getTokenPublicKey(PrivateClickMeasurement&& attribution, Function<void(PrivateClickMeasurement&& attribution, const String& publicKeyBase64URL)>&& callback)
> > 
> > This should be a CompletionHandler instead of a Function and also be called completionHandler.
> 
> CompletionHandler needs to be called at least once. I don't think there is a
> need to call that if the key is not valid or other early returns.
> 
> >> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:166
> >> +        // FIXME(): Retrieve the public key from the content instead of the header, and validate the public key.
> > 
> > Missing bug number. If you don't intend to file one, I'd drop the parenthesis.
> 
> This a WIP. Will definitely file one.
> 
> >> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:188
> >> +    // FIXME(): Should check whether the tokenSignatureJSON is valid before continuing.
> > 
> > Missing bug number. If you don't intend to file one, I'd drop the parenthesis.
> 
> Ditto.
> 
> >> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:200
> >>              return;
> > 
> > I'm a little confused on the terminology here. Isn't the token that's being signed the "unlinkable token" and not the "secret token?"
> 
> I think the final product is the unlinkable token and the intermediate
> product is the secret token.
> 
> >> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:203
> >> +        // FIXME(): Retrieve the signature from the content instead of the header.
> > 
> > Ditto on the bug number.
> 
> Ditto.
Comment 7 Jiewen Tan 2021-02-19 19:29:54 PST
Created attachment 421071 [details]
Patch
Comment 8 Jiewen Tan 2021-02-19 19:35:54 PST
Created attachment 421072 [details]
Patch
Comment 9 Jiewen Tan 2021-02-19 23:07:39 PST
Created attachment 421083 [details]
Patch
Comment 10 Jiewen Tan 2021-02-19 23:17:32 PST
Created attachment 421084 [details]
Patch
Comment 11 John Wilander 2021-02-20 09:16:27 PST
Comment on attachment 421084 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421084&action=review

LGTM. See comments.

> Source/WTF/ChangeLog:3
> +        PCM: Turn the fraud prevention on by default

I'm confused. Is this a change log that shouldn't be part of this patch and really only be part of the patch for https://bugs.webkit.org/show_bug.cgi?id=222224? I think so.

> Source/WebCore/ChangeLog:12
> +        Covered by existing tests.

This should say "Existing tests updated."

> Source/WebCore/ChangeLog:16
> +        Adds for mock testing.

I would flesh this out a bit. "Mocks the [...] for testing."

> Source/WebKit/ChangeLog:28
> +        Adds some mock test infrastructures.

Here I would add "... so that we can test [...] without [...]"

> Source/WebCore/loader/PrivateClickMeasurement.cpp:220
> +    reportDetails->setString("source_secret_token"_s, m_sourceSecretToken.valueBase64URL);

We'll have to work through the naming of these things later. I my view, the secret is kept secret until the attribution report is sent. In the old blinded signature terminology, this would be the "blinded secret," which in our new terminology is called "unlinkable token." That's what the server signs. Then in the attribution report the server receives the "unblinded secret" or in our terminology the "secret token." But we can work that out in a later patch.

> Source/WebKit/NetworkProcess/NetworkSession.cpp:361
> +void NetworkSession::setFraudPreventionValuesForTesting(String&& secretToken, String&& unlinkableToken, String&& signature, String&& keyID)

Let's add this comment here:
// FIXME: Switch to non-mocked test data once the right cryptography library is available in open source.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.h:94
> +    struct TestingFraudPreventionValues {

Nice to do it as a struct.
Comment 12 Jiewen Tan 2021-02-20 14:36:21 PST
Comment on attachment 421084 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421084&action=review

Thanks for r+ this patch, John.

>> Source/WTF/ChangeLog:3
>> +        PCM: Turn the fraud prevention on by default
> 
> I'm confused. Is this a change log that shouldn't be part of this patch and really only be part of the patch for https://bugs.webkit.org/show_bug.cgi?id=222224? I think so.

Somehow, those two patches are messed up. Will split them.

>> Source/WebCore/ChangeLog:12
>> +        Covered by existing tests.
> 
> This should say "Existing tests updated."

Fixed.

>> Source/WebCore/ChangeLog:16
>> +        Adds for mock testing.
> 
> I would flesh this out a bit. "Mocks the [...] for testing."

Fixed.

>> Source/WebKit/ChangeLog:28
>> +        Adds some mock test infrastructures.
> 
> Here I would add "... so that we can test [...] without [...]"

Fixed.

>> Source/WebCore/loader/PrivateClickMeasurement.cpp:220
>> +    reportDetails->setString("source_secret_token"_s, m_sourceSecretToken.valueBase64URL);
> 
> We'll have to work through the naming of these things later. I my view, the secret is kept secret until the attribution report is sent. In the old blinded signature terminology, this would be the "blinded secret," which in our new terminology is called "unlinkable token." That's what the server signs. Then in the attribution report the server receives the "unblinded secret" or in our terminology the "secret token." But we can work that out in a later patch.

Sure, as long as those two tokens have different names. I'm fine with either schema.

>> Source/WebKit/NetworkProcess/NetworkSession.cpp:361
>> +void NetworkSession::setFraudPreventionValuesForTesting(String&& secretToken, String&& unlinkableToken, String&& signature, String&& keyID)
> 
> Let's add this comment here:
> // FIXME: Switch to non-mocked test data once the right cryptography library is available in open source.

Fixed.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.h:94
>> +    struct TestingFraudPreventionValues {
> 
> Nice to do it as a struct.

: )
Comment 13 Jiewen Tan 2021-02-20 14:38:04 PST
Created attachment 421115 [details]
Patch for landing
Comment 14 EWS 2021-02-20 15:17:38 PST
Committed r273209: <https://commits.webkit.org/r273209>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421115 [details].