Bug 222019 – PCM: Generate secret token and corresponding unlinkable token

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jiewen Tan]

Generate secret token and corresponding unlinkable token.

[Jiewen Tan]

<rdar://problem/73581412>

[Jiewen Tan]

<rdar://problem/73581412>

[Jiewen Tan]

Created attachment 420582 [details]
Patch

[Jiewen Tan]

Created attachment 420585 [details]
Patch

[Jiewen Tan]

Created attachment 420592 [details]
Patch

[Jiewen Tan]

Created attachment 420605 [details]
Patch

[Jiewen Tan]

Created attachment 420609 [details]
Patch

[Jiewen Tan]

Created attachment 420615 [details]
Patch

[John Wilander]

Comment on attachment 420615 [details]
Patch

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

r=me with comments.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:150
> +#if HAVE(RSA_BSSA)

These don't have to be behind RSA_BSSA, right? That will only make testing harder since EWS bots will be behind.

> Source/WebCore/loader/PrivateClickMeasurement.h:274
> +#if HAVE(RSA_BSSA)

Again, EphemeralSourceNonce doesn't have to be behind RSA_BSSA. Or am I missing something?

> Source/WebCore/loader/PrivateClickMeasurement.h:319
> +    BlindedToken m_blindedToken;

This should be optional too and it needs to have "source" be part of its name since there will eventually be a token for the attribute-on site too.

> Source/WebCore/loader/cocoa/PrivateClickMeasurementCocoa.mm:58
> +    blindedSecret->setString("click_nonce"_s, m_ephemeralSourceNonce->nonce);

Let's call it source_nonce.

This is how I've set it up in my patch:
    reportDetails->setString("source_engagement_type"_s, "click"_s);
    reportDetails->setString("source_nonce"_s, m_ephemeralSourceNonce->nonce);
    reportDetails->setString("unlinkable_token"_s, "TODO"_s);
    reportDetails->setInteger("version"_s, 2);

> Source/WebCore/loader/cocoa/PrivateClickMeasurementCocoa.mm:59
> +    blindedSecret->setString("blinded_secret"_s, WTF::base64URLEncode([m_blindedToken.waitingToken blindedMessage].bytes, [m_blindedToken.waitingToken blindedMessage].length));

We should try to use unlinkable_token in any developer facing parts so that we are not tied to blinded signatures as the crypto underpinning. Could even be unlinkable_token_to_sign.

> Tools/TestWebKitAPI/Tests/WebCore/PrivateClickMeasurement.cpp:150
> +    EXPECT_STREQ(blindedSecret->getString("click_nonce"_s).utf8().data(), "ABCDEFabcdef0123456789");

source_nonce

> Tools/TestWebKitAPI/Tests/WebCore/PrivateClickMeasurement.cpp:151
> +    EXPECT_FALSE(blindedSecret->getString("blinded_secret"_s).isEmpty());

unlinkable_token or unlinkable_token_to_sign.

[Jiewen Tan]

Comment on attachment 420615 [details]
Patch

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

>> Source/WebCore/loader/PrivateClickMeasurement.cpp:150
>> +#if HAVE(RSA_BSSA)
> 
> These don't have to be behind RSA_BSSA, right? That will only make testing harder since EWS bots will be behind.

I think the testing concern is not that big of deal. You still have coverage from internal bots. It's more important to group the whole feature together. Otherwise, this may end up with part of it dangling on down level OS.

>> Source/WebCore/loader/PrivateClickMeasurement.h:319
>> +    BlindedToken m_blindedToken;
> 
> This should be optional too and it needs to have "source" be part of its name since there will eventually be a token for the attribute-on site too.

It doesn't matter given all the member are pointers. Will change the name.

>> Source/WebCore/loader/cocoa/PrivateClickMeasurementCocoa.mm:58
>> +    blindedSecret->setString("click_nonce"_s, m_ephemeralSourceNonce->nonce);
> 
> Let's call it source_nonce.
> 
> This is how I've set it up in my patch:
>     reportDetails->setString("source_engagement_type"_s, "click"_s);
>     reportDetails->setString("source_nonce"_s, m_ephemeralSourceNonce->nonce);
>     reportDetails->setString("unlinkable_token"_s, "TODO"_s);
>     reportDetails->setInteger("version"_s, 2);

Right, however this is not the actual token. This is the blinded message that the server needs to sign. Will change the nonce name.

>> Source/WebCore/loader/cocoa/PrivateClickMeasurementCocoa.mm:59
>> +    blindedSecret->setString("blinded_secret"_s, WTF::base64URLEncode([m_blindedToken.waitingToken blindedMessage].bytes, [m_blindedToken.waitingToken blindedMessage].length));
> 
> We should try to use unlinkable_token in any developer facing parts so that we are not tied to blinded signatures as the crypto underpinning. Could even be unlinkable_token_to_sign.

Okay, unlinkable_token_to_sign seems good.

>> Tools/TestWebKitAPI/Tests/WebCore/PrivateClickMeasurement.cpp:150
>> +    EXPECT_STREQ(blindedSecret->getString("click_nonce"_s).utf8().data(), "ABCDEFabcdef0123456789");
> 
> source_nonce

Will fix.

>> Tools/TestWebKitAPI/Tests/WebCore/PrivateClickMeasurement.cpp:151
>> +    EXPECT_FALSE(blindedSecret->getString("blinded_secret"_s).isEmpty());
> 
> unlinkable_token or unlinkable_token_to_sign.

Will fix.

[Jiewen Tan]

Created attachment 420748 [details]
Patch

[Jiewen Tan]

Created attachment 420760 [details]
Patch

[Jiewen Tan]

Created attachment 420786 [details]
Patch

[Jiewen Tan]

Created attachment 420942 [details]
Patch

[Jiewen Tan]

Created attachment 420945 [details]
Patch

[Jiewen Tan]

Created attachment 420946 [details]
Patch

[Jiewen Tan]

Created attachment 420947 [details]
Patch

[Jiewen Tan]

Created attachment 420948 [details]
Patch

[Jiewen Tan]

Created attachment 420949 [details]
Patch

[Jiewen Tan]

Created attachment 420952 [details]
Patch

[Jiewen Tan]

Created attachment 420994 [details]
Patch

[Jiewen Tan]

Committed r273167 (234362@main): <https://commits.webkit.org/234362@main>