-
Notifications
You must be signed in to change notification settings - Fork 900
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
Add Constellation/STAR encryption for P3A #14399
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few questions in here, but nothing major stands out to me for this
491f29f
to
e590d91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comments have been addressed. Approving
@fmarier feel free to close this one out once you're satisfied with it
c71f93b
to
4a34aaf
Compare
4bbe614
to
241c7c5
Compare
…service in browser process instead, misc feedback fixes
…e randomness server
c9fa07b
to
a477fae
Compare
…ponse/server unavailability, misc feedback updates for p3a config
Will perform smoke tests before merge |
How's this going? It would be good to get this merged so other code can build on it. Since it's disabled by default, it should be fine to land as long as it hasn't broken the default experience. |
i intend on merging this later today |
@DJAndries would you be able to write a test plan for QA? |
updated now that #18340 has landed |
} | ||
|
||
content::BrowserTaskEnvironment task_environment_; | ||
network::TestURLLoaderFactory url_loader_factory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These variables are not named according to convention required by the the style guide [1]. I'm flagging this to raise some awareness, to authors and reviewers, as I've come across a few places today where such cases seemed to have got into master.
I also think we could there are some area for improvement about which members should have been kept private, or protected.
[1] https://google.github.io/styleguide/cppguide.html#Variable_Names
Resolves brave/brave-browser#24338
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
Please test in a version that includes #18340 or #18410.
Access https://star-randsrv.bsg.brave.software/info and note the
currentEpoch
value.Perform this first test without a MITM proxy (Nitro Enclave attestation will not work without proxy).
--enable-features="BraveP3AConstellation" --p3a-star-randomness-host=https://star-randsrv.bsg.bravesoftware.com --p3a-constellation-upload-url=https://collector.bsg.bravesoftware.com/
brave.p3a.approved_cert_fp
should contain a value prefixed withsha256/
,brave.p3a.current_epoch
should contain the value noted above,brave.p3a.current_pk
should contain a long string,brave.p3a.next_epoch_time
should contain a numerical timestamp value,p3a.logs_constellation_prep
should exist with the typical list of metrics/values/etc.p3a.constellation_logs.{epoch number}
should contain a list of metrics with the name as the key, and the encrypted base-64 value as the value. A new log should appear in the list every minute. Verify that the relevant metrics in thep3a.logs_constellation_prep
list indicate being sent.--p3a-fake-star-epoch=
. Add one to the current epoch and append the value to the flag.p3a.constellation_logs.{original epoch number}
.p3a.constellation_logs.{original epoch number}
do not exist (due to expiry).Perform this second test using a MITM proxy, and add the
--p3a-disable-star-attestation
flag./info
request is sent to the randomness server./randomness
requests are sent to the randomness server. Ensure that the number of metrics existing in the Constellation logs (in local state) matches the number of randomness requests sent. Ensure no requests are sent to the upload url./info
request should be sent. Encrypted metrics should be uploaded to the upload URL after a few minutes. Randomness requests should fail since the current fake epoch is not valid.Ensure JSON metrics are sent as usual with or without the Constellation feature enabled.