Bug 222208 – PCM: Store and report source unlinkable tokens

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jiewen Tan]

Store and report source unlinkable tokens.

[Jiewen Tan]

<rdar://problem/73582032>

[Jiewen Tan]

Created attachment 421046 [details]
Patch

[John Wilander]

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.

[Jiewen Tan]

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.

[Kate Cheney]

Comment on attachment 421046 [details]
Patch

DB changes look good to me.

[John Wilander]

(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.

[Jiewen Tan]

Created attachment 421071 [details]
Patch

[Jiewen Tan]

Created attachment 421072 [details]
Patch

[Jiewen Tan]

Created attachment 421083 [details]
Patch

[Jiewen Tan]

Created attachment 421084 [details]
Patch

[John Wilander]

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.

[Jiewen Tan]

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.

: )

[Jiewen Tan]

Created attachment 421115 [details]
Patch for landing

[EWS]

Committed r273209: <https://commits.webkit.org/r273209>

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