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

IP scrubber #2990

Merged
merged 21 commits into from
Sep 5, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions account/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ func GetAccount(ctx context.Context, cfg *config.Configuration, fetcher stored_r
return nil, errs
}

errs = account.Privacy.IPMasking.Validate(errs)
if len(errs) > 0 {
return nil, errs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reasonable way to add a test that throws this error? Or is it challenging with this Validate function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, added!

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Could simplify the syntax...

if errs := account.Privacy.IPv6Config.Validate(nil); len(errs) > 0 {
	account.Privacy.IPv6Config.AnonKeepBits = iputil.IPv6BitSize
}

if errs := account.Privacy.IPv4Config.Validate(nil); len(errs) > 0 {
	account.Privacy.IPv4Config.AnonKeepBits = iputil.IPv4BitSize
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines show as having test coverage, but there doesn't seem to an assertion on setting their default values. I made a type locally where IPv4Config.AnonKeepBits = iputil.IPv6BitSize which is setting the ipv6 value for the ipv4 default and this mistake wasn't caught by a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is tested in a general GetAccount test.
I added extra flag to assert IP config for account id "invalid_acct_ipv6_ipv4".
I also realized I set both ipv6 and ipv4 to max value, instead of default mask bits. I added 2 new constants:

IPv4DefaultMaskingBitSize = 24
IPv6DefaultMaskingBitSize = 56

For the simplified syntax I added new variables for errors to show explicitly we don't want to add IP config validation errors to errs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also tested this update end to end including test with publisher account id.


// set the value of events.enabled field based on deprecated events_enabled field and ensure backward compatibility
deprecateEventsEnabledField(account)

Expand Down
51 changes: 49 additions & 2 deletions config/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ const (
ChannelWeb ChannelType = "web"
)

const (
IPv4 = 32
IPv6 = 128
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
)
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved

// Account represents a publisher account configuration
type Account struct {
ID string `mapstructure:"id" json:"id"`
Expand All @@ -40,7 +45,7 @@ type Account struct {
Validations Validations `mapstructure:"validations" json:"validations"`
DefaultBidLimit int `mapstructure:"default_bid_limit" json:"default_bid_limit"`
BidAdjustments *openrtb_ext.ExtRequestPrebidBidAdjustments `mapstructure:"bidadjustments" json:"bidadjustments"`
Privacy *AccountPrivacy `mapstructure:"privacy" json:"privacy"`
Privacy AccountPrivacy `mapstructure:"privacy" json:"privacy"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my understanding, why is this converted to remove its pointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to non-pointer because we no have a new IPMasking field inside of the Privacy:

type AccountPrivacy struct {
	AllowActivities *AllowActivities `mapstructure:"allowactivities" json:"allowactivities"`
	IPMasking       IPMasking        `mapstructure:"ip_masking" json:"ip_masking"` 
}

We need to have IPMasking to be always present and to be populated with default values from configs.
PBS host account_defaults configs and/or request.account config can overwrite these values.

This is why config.config.go has these lines:

v.SetDefault("account_defaults.privacy.ip_masking.ipv6.left_mask_bits", 56)
v.SetDefault("account_defaults.privacy.ip_masking.ipv4.left_mask_bits", 24)

}

// CookieSync represents the account-level defaults for the cookie sync endpoint.
Expand Down Expand Up @@ -295,5 +300,47 @@ func (a *AccountChannel) IsSet() bool {
}

type AccountPrivacy struct {
AllowActivities AllowActivities `mapstructure:"allowactivities" json:"allowactivities"`
AllowActivities *AllowActivities `mapstructure:"allowactivities" json:"allowactivities"`
IPMasking IPMasking `mapstructure:"ip_masking" json:"ip_masking"`
}

type IPMasking struct {
IpV6 IPMasks `mapstructure:"ipv6" json:"ipv6"`
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
IpV4 IPMasks `mapstructure:"ipv4" json:"ipv4"`
}

type IPMasks struct {
ActivityLeftMaskBits int `mapstructure:"activity_left_mask_bits" json:"activity_left_mask_bits"`
GdprLeftMaskBitsLowest int `mapstructure:"gdpr_left_mask_bits_lowest" json:"gdpr_left_mask_bits_lowest"`
GdprLeftMaskBitsHighest int `mapstructure:"gdpr_left_mask_bits_highest" json:"gdpr_left_mask_bits_highest"`
}
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Let's use underscores to be consistent with the rest of this object: anon-keep-bits -> anon_keep_bits


func (ipMasking *IPMasking) Validate(errs []error) []error {
if e := ipMasking.IpV6.validate(IPv6, "ipv6"); e != nil {
errs = append(errs, e...)
}
if e := ipMasking.IpV4.validate(IPv4, "ipv4"); e != nil {
errs = append(errs, e...)
}
return errs
}

func (ipMasks *IPMasks) validate(maxBits int, ipVersion string) []error {
var errs []error

if ipMasks.ActivityLeftMaskBits > maxBits || ipMasks.ActivityLeftMaskBits < 0 {
err := fmt.Errorf("activity left mask bits cannot exceed %d in %s address, or be less than 0", maxBits, ipVersion)
errs = append(errs, err)
}

if ipMasks.GdprLeftMaskBitsLowest > maxBits || ipMasks.GdprLeftMaskBitsLowest < 0 {
err := fmt.Errorf("gdpr left mask bits lowest cannot exceed %d in %s address, or be less than 0", maxBits, ipVersion)
errs = append(errs, err)
}

if ipMasks.GdprLeftMaskBitsHighest > maxBits || ipMasks.GdprLeftMaskBitsHighest < 0 {
err := fmt.Errorf("gdpr left mask bits highest cannot exceed %d in %s address, or be less than 0", maxBits, ipVersion)
errs = append(errs, err)
}
return errs
}
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
75 changes: 75 additions & 0 deletions config/account_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -910,3 +910,78 @@ func TestAccountPriceFloorsValidate(t *testing.T) {
})
}
}

func TestIPMaskingValidate(t *testing.T) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Could you remove this blank line?

tests := []struct {
name string
masking *IPMasking
want []error
}{
{
name: "valid configuration",
masking: &IPMasking{
IpV4: IPMasks{
ActivityLeftMaskBits: 1,
GdprLeftMaskBitsLowest: 2,
GdprLeftMaskBitsHighest: 0,
},
IpV6: IPMasks{
ActivityLeftMaskBits: 0,
GdprLeftMaskBitsLowest: 2,
GdprLeftMaskBitsHighest: 3,
},
},
},
{
name: "invalid configuration",
masking: &IPMasking{
IpV4: IPMasks{
ActivityLeftMaskBits: 100,
GdprLeftMaskBitsLowest: -200,
GdprLeftMaskBitsHighest: 300,
},
IpV6: IPMasks{
ActivityLeftMaskBits: -100,
GdprLeftMaskBitsLowest: 200,
GdprLeftMaskBitsHighest: -300,
},
},
want: []error{
errors.New("activity left mask bits cannot exceed 32 in ipv4 address, or be less than 0"),
errors.New("gdpr left mask bits lowest cannot exceed 32 in ipv4 address, or be less than 0"),
errors.New("gdpr left mask bits highest cannot exceed 32 in ipv4 address, or be less than 0"),
errors.New("activity left mask bits cannot exceed 128 in ipv6 address, or be less than 0"),
errors.New("gdpr left mask bits lowest cannot exceed 128 in ipv6 address, or be less than 0"),
errors.New("gdpr left mask bits highest cannot exceed 128 in ipv6 address, or be less than 0"),
},
},
{
name: "mixed valid and invalid configuration",
masking: &IPMasking{
IpV4: IPMasks{
ActivityLeftMaskBits: 10,
GdprLeftMaskBitsLowest: -20,
GdprLeftMaskBitsHighest: 30,
},
IpV6: IPMasks{
ActivityLeftMaskBits: 10,
GdprLeftMaskBitsLowest: 20,
GdprLeftMaskBitsHighest: -30,
},
},
want: []error{
errors.New("gdpr left mask bits lowest cannot exceed 32 in ipv4 address, or be less than 0"),
errors.New("gdpr left mask bits highest cannot exceed 128 in ipv6 address, or be less than 0"),
},
},
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
var errs []error
got := tt.masking.Validate(errs)
assert.ElementsMatch(t, got, tt.want)
})
}
}
8 changes: 7 additions & 1 deletion config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,12 +155,14 @@ func (cfg *Configuration) validate(v *viper.Viper) []error {
cfg.TmaxAdjustments.Enabled = false
}

if cfg.AccountDefaults.Privacy != nil {
if cfg.AccountDefaults.Privacy.AllowActivities != nil {
glog.Warning("account_defaults.Privacy has no effect as the feature is under development.")
}

errs = cfg.Experiment.validate(errs)
errs = cfg.BidderInfos.validate(errs)
errs = cfg.AccountDefaults.Privacy.IPMasking.Validate(errs)

return errs
}

Expand Down Expand Up @@ -1025,6 +1027,10 @@ func SetupViper(v *viper.Viper, filename string, bidderInfos BidderInfos) {
v.SetDefault("account_defaults.price_floors.max_rules", 100)
v.SetDefault("account_defaults.price_floors.max_schema_dims", 3)
v.SetDefault("account_defaults.events_enabled", false)
v.SetDefault("account_defaults.privacy.ip_masking.ipv6.activity_left_mask_bits", 56)
v.SetDefault("account_defaults.privacy.ip_masking.ipv6.gdpr_left_mask_bits_lowest", 112)
v.SetDefault("account_defaults.privacy.ip_masking.ipv6.gdpr_left_mask_bits_highest", 96)
v.SetDefault("account_defaults.privacy.ip_masking.ipv4.gdpr_left_mask_bits_lowest", 24)

v.SetDefault("compression.response.enable_gzip", false)
v.SetDefault("compression.request.enable_gzip", false)
Expand Down
18 changes: 18 additions & 0 deletions config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,11 @@ func TestDefaults(t *testing.T) {
cmpUnsignedInts(t, "tmax_adjustments.bidder_network_latency_buffer_ms", 0, cfg.TmaxAdjustments.BidderNetworkLatencyBuffer)
cmpUnsignedInts(t, "tmax_adjustments.pbs_response_preparation_duration_ms", 0, cfg.TmaxAdjustments.PBSResponsePreparationDuration)

cmpInts(t, "account_defaults.privacy.ip_masking.ipv6.activity_left_mask_bits", 56, cfg.AccountDefaults.Privacy.IPMasking.IpV6.ActivityLeftMaskBits)
cmpInts(t, "account_defaults.privacy.ip_masking.ipv6.gdpr_left_mask_bits_lowest", 112, cfg.AccountDefaults.Privacy.IPMasking.IpV6.GdprLeftMaskBitsLowest)
cmpInts(t, "account_defaults.privacy.ip_masking.ipv6.gdpr_left_mask_bits_highest", 96, cfg.AccountDefaults.Privacy.IPMasking.IpV6.GdprLeftMaskBitsHighest)
cmpInts(t, "account_defaults.privacy.ip_masking.ipv4.gdpr_left_mask_bits_lowest", 24, cfg.AccountDefaults.Privacy.IPMasking.IpV4.GdprLeftMaskBitsLowest)

//Assert purpose VendorExceptionMap hash tables were built correctly
expectedTCF2 := TCF2{
Enabled: true,
Expand Down Expand Up @@ -477,6 +482,14 @@ account_defaults:
use_dynamic_data: true
max_rules: 120
max_schema_dims: 5
privacy:
ip_masking:
ipv6:
activity_left_mask_bits: 50
gdpr_left_mask_bits_lowest: 110
gdpr_left_mask_bits_highest: 90
ipv4:
gdpr_left_mask_bits_lowest: 20
tmax_adjustments:
enabled: true
bidder_response_duration_min_ms: 700
Expand Down Expand Up @@ -583,6 +596,11 @@ func TestFullConfig(t *testing.T) {
cmpBools(t, "account_defaults.events_enabled", *cfg.AccountDefaults.EventsEnabled, true)
cmpNils(t, "account_defaults.events.enabled", cfg.AccountDefaults.Events.Enabled)

cmpInts(t, "account_defaults.privacy.ip_masking.ipv6.activity_left_mask_bits", 50, cfg.AccountDefaults.Privacy.IPMasking.IpV6.ActivityLeftMaskBits)
cmpInts(t, "account_defaults.privacy.ip_masking.ipv6.gdpr_left_mask_bits_lowest", 110, cfg.AccountDefaults.Privacy.IPMasking.IpV6.GdprLeftMaskBitsLowest)
cmpInts(t, "account_defaults.privacy.ip_masking.ipv6.gdpr_left_mask_bits_highest", 90, cfg.AccountDefaults.Privacy.IPMasking.IpV6.GdprLeftMaskBitsHighest)
cmpInts(t, "account_defaults.privacy.ip_masking.ipv4.gdpr_left_mask_bits_lowest", 20, cfg.AccountDefaults.Privacy.IPMasking.IpV4.GdprLeftMaskBitsLowest)

// Assert compression related defaults
cmpBools(t, "enable_gzip", false, cfg.EnableGzip)
cmpBools(t, "compression.request.enable_gzip", true, cfg.Compression.Request.GZIP)
Expand Down
6 changes: 3 additions & 3 deletions endpoints/cookie_sync_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2101,9 +2101,9 @@ func (p *fakePermissions) AuctionActivitiesAllowed(ctx context.Context, bidderCo
}, nil
}

func getDefaultActivityConfig(componentName string, allow bool) *config.AccountPrivacy {
return &config.AccountPrivacy{
AllowActivities: config.AllowActivities{
func getDefaultActivityConfig(componentName string, allow bool) config.AccountPrivacy {
return config.AccountPrivacy{
AllowActivities: &config.AllowActivities{
SyncUser: config.Activity{
Default: ptrutil.ToPtr(true),
Rules: []config.ActivityRule{
Expand Down
2 changes: 1 addition & 1 deletion exchange/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ func (rs *requestSplitter) cleanOpenRTBRequests(ctx context.Context,
applyFPD(auctionReq.FirstPartyData[bidderRequest.BidderName], bidderRequest.BidRequest)
}

privacyEnforcement.Apply(bidderRequest.BidRequest)
privacyEnforcement.Apply(bidderRequest.BidRequest, auctionReq.Account.Privacy.IPMasking)
allowedBidderRequests = append(allowedBidderRequests, bidderRequest)

// GPP downgrade: always downgrade unless we can confirm GPP is supported
Expand Down
20 changes: 10 additions & 10 deletions exchange/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4272,7 +4272,7 @@ func TestCleanOpenRTBRequestsActivitiesFetchBids(t *testing.T) {
testCases := []struct {
name string
req *openrtb2.BidRequest
privacyConfig *config.AccountPrivacy
privacyConfig config.AccountPrivacy
componentName string
allow bool
expectedReqNumber int
Expand Down Expand Up @@ -4382,25 +4382,25 @@ func buildDefaultActivityConfig(componentName string, allow bool) config.Activit
}
}

func getFetchBidsActivityConfig(componentName string, allow bool) *config.AccountPrivacy {
return &config.AccountPrivacy{
AllowActivities: config.AllowActivities{
func getFetchBidsActivityConfig(componentName string, allow bool) config.AccountPrivacy {
return config.AccountPrivacy{
AllowActivities: &config.AllowActivities{
FetchBids: buildDefaultActivityConfig(componentName, allow),
},
}
}

func getTransmitUFPDActivityConfig(componentName string, allow bool) *config.AccountPrivacy {
return &config.AccountPrivacy{
AllowActivities: config.AllowActivities{
func getTransmitUFPDActivityConfig(componentName string, allow bool) config.AccountPrivacy {
return config.AccountPrivacy{
AllowActivities: &config.AllowActivities{
TransmitUserFPD: buildDefaultActivityConfig(componentName, allow),
},
}
}

func getTransmitPreciseGeoActivityConfig(componentName string, allow bool) *config.AccountPrivacy {
return &config.AccountPrivacy{
AllowActivities: config.AllowActivities{
func getTransmitPreciseGeoActivityConfig(componentName string, allow bool) config.AccountPrivacy {
return config.AccountPrivacy{
AllowActivities: &config.AllowActivities{
TransmitPreciseGeo: buildDefaultActivityConfig(componentName, allow),
},
}
Expand Down
9 changes: 6 additions & 3 deletions privacy/enforcement.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package privacy

import "github.com/prebid/openrtb/v19/openrtb2"
import (
"github.com/prebid/openrtb/v19/openrtb2"
"github.com/prebid/prebid-server/config"
)

// Enforcement represents the privacy policies to enforce for an OpenRTB bid request.
type Enforcement struct {
Expand All @@ -27,8 +30,8 @@ func (e Enforcement) AnyActivities() bool {
}

// Apply cleans personally identifiable information from an OpenRTB bid request.
func (e Enforcement) Apply(bidRequest *openrtb2.BidRequest) {
e.apply(bidRequest, NewScrubber())
func (e Enforcement) Apply(bidRequest *openrtb2.BidRequest, ipMasking config.IPMasking) {
e.apply(bidRequest, NewScrubber(ipMasking))
}

func (e Enforcement) apply(bidRequest *openrtb2.BidRequest, scrubber Scrubber) {
Expand Down
4 changes: 2 additions & 2 deletions privacy/enforcer.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ type ActivityControl struct {
plans map[Activity]ActivityPlan
}

func NewActivityControl(privacyConf *config.AccountPrivacy) (ActivityControl, error) {
func NewActivityControl(privacyConf config.AccountPrivacy) (ActivityControl, error) {
ac := ActivityControl{}
var err error

if privacyConf == nil {
if privacyConf.AllowActivities == nil {
return ac, nil
}

Expand Down
Loading