Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Criteo Adapter Changes #5804

Closed
gglas opened this issue Sep 30, 2020 · 18 comments · Fixed by #6158
Closed

Criteo Adapter Changes #5804

gglas opened this issue Sep 30, 2020 · 18 comments · Fixed by #6158
Assignees

Comments

@gglas
Copy link

gglas commented Sep 30, 2020

Type of issue

Enhancement / Review Requested

Description

Criteo's adapter contains legacy third party javascript dependencies that don't perfectly align with the new Prebid best practices and rules -- this is because the technology predated prebid and appropriately couldn't develop around best practices that didn't exist! We were hoping to have someone in the community review Criteo's documentation to make sure the planned development strategies align with where prebid is going.

Other information

Documentation is available here :

https://docs.google.com/document/d/1pTjeUR7v8QxVyZMarYBlTzc6Dq_TW4vPoI_6hbK9jok/edit?usp=sharing

@patmmccann
Copy link
Collaborator

Of particular note, the build time crypto library seems to only be useful if the pubtag is included. One would hope there is a simple way for publishers to build without this library if a publsher does not use the publisher tag.

@patmmccann
Copy link
Collaborator

A couple other notes, the #5644 hash might be able to be used in place of the criteo hash included at build time and the ceh property can now be read from #5767 if available

@allanjun
Copy link
Collaborator

allanjun commented Oct 7, 2020

Thanks for the comments @patmmccann

Of particular note, the build time crypto library seems to only be useful if the pubtag is included. One would hope there is a simple way for publishers to build without this library if a publsher does not use the publisher tag.

This can be one possibility but then we would perhaps have to split our adapter in two. Or perhaps we could offer an explicit build command option to remove these.

The #5644 hash might be able to be used in place of the criteo hash included at build time

Our crypto library uses assymetrical encryption to validate the content is not modified

The ceh property can now be read from #5767 if available

We do have T&C signed with the publisher to ensure that the data collected is in accordance with our privacy requirements. Setting this parameter at bidder level works as an explicit optin, whereas on #5767 the publisher may send data to all bidders without prior agreement.

@bretg
Copy link
Collaborator

bretg commented Dec 1, 2020

Can someone please explain the crypto concern and the options?

@patmmccann
Copy link
Collaborator

patmmccann commented Dec 1, 2020

The concern is that if a publisher disables the calling of the Criteo external code, the adapter is still importing a huge build time dependency into prebid that it doesn't need. I'm not sure of all the options, if build time dependency options are even a thing, if an alternative or more compact function could be used, if this implies one would need two version of the adapter, or if the functionality could move to the criteo external library. I know the LiveIntent team is working on build time options.

@bretg
Copy link
Collaborator

bretg commented Dec 1, 2020

Got this background:

The function tryGetCriteoFastBid verifies the authenticity of the content stored on criteo_fast_bid
Verification is based on a RSA signature;
First line of criteo_fast_bid contains a hash of the code signed with a secret key;
The public key is available on the adapter code;
The function computes the checksum of the code and validates the authenticity based on the public key and the signed hash;
Finally, the content is loaded via eval and the Publishertag is available.

For that we import a library to handle the RSA in javascript. PAtrick sugested using one of existing hash methods, but they don't actually do asymetric signatures like RSA

My take is that this is just more external code, even though it's build-time. As far as I'm concerned, that just requires another disclosure.

Here's a proposed disclosure that will be placed on the criteoBidAdapter page:

This module loads external code both at run-time and at build-time. The build-time code is an open source
RSA security library. The run-time code is not open source but the default version has been inspected by
Prebid.org core reviewers. Note that disabling the external run-time code will still pull in the build-time code.

I'm signing off on this approach -- over to @gglas to figure out how can we get full signoff from the group so Criteo can start building it.

@patmmccann
Copy link
Collaborator

patmmccann commented Dec 1, 2020 via email

@patmmccann
Copy link
Collaborator

patmmccann commented Dec 1, 2020 via email

@patmmccann
Copy link
Collaborator

patmmccann commented Dec 1, 2020 via email

@bretg
Copy link
Collaborator

bretg commented Dec 1, 2020

the version of the code that is available to prebid core reviewers has been changing regularly

Agreed - this draft disclosure is intended to be what we post when the adapter changes are approved and released.

There hasn't been discussion about putting a disclosure in place for the current situation, but that would be a different block of text.

@patmmccann
Copy link
Collaborator

patmmccann commented Dec 1, 2020 via email

@patmmccann
Copy link
Collaborator

Here is an example of a build time option, I think this is a great approach

#6016

@allanjun
Copy link
Collaborator

allanjun commented Dec 5, 2020

hi all, thanks for the comments

@stale
Copy link

stale bot commented Dec 25, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 25, 2020
@gglas gglas linked a pull request Mar 8, 2021 that will close this issue
10 tasks
@patmmccann
Copy link
Collaborator

  • I see other modules using other npm packages, so they should also work on disclosures for consistency

@allanjun now is a very good time to insist on those disclosures as we transition to 5.0. Could you indicate which ones?

@stale stale bot removed the stale label Mar 30, 2021
@gglas
Copy link
Author

gglas commented May 10, 2021

@allanjun @patmmccann can we close this issue out? Seems like it's been handled for the most part

@patmmccann
Copy link
Collaborator

I believe so

@gglas
Copy link
Author

gglas commented Aug 2, 2021

closing out - adapter pushed in 5.0 with updated infrastructure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants