Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2448213007:
Support different Google attestation (Privacy CA) servers. (Closed)
|
Created:
4 years, 1 month ago by The one and only Dr. Crash Modified:
4 years, 1 month ago CC:
chromium-reviews, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, dkrahn+watch_chromium.org, dkalin1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport different Google attestation (Privacy CA) servers.
Add a --attestation-server flag that allows to pick the default or
the test server, and pass the appropriate value to cryptohomed
for operations involving requests sent to the attestation servers.
Also use the appropriate endpoints for the various servers when
sending requests.
BUG=660260
TEST=unit tests
Committed: https://crrev.com/05374163a5a02f6768c72c1fe73c5f26723cb808
Cr-Commit-Position: refs/heads/master@{#428869}
Patch Set 1 #Patch Set 2 : Simplified. #Patch Set 3 : Period. Lint #
Total comments: 1
Patch Set 4 : Up to date w/ master. #
Total comments: 1
Messages
Total messages: 40 (19 generated)
Description was changed from ========== Support different Google attestation (Privacy CA) servers. Add a --attestation-server flag that allows to pick the default or the test server, and pass the appropriate value to cryptohomed for operations involving requests sent to the attestation servers. Also use the appropriate endpoints for the various servers when sending requests. BUG=660260 TEST=unit tests ========== to ========== Support different Google attestation (Privacy CA) servers. Add a --attestation-server flag that allows to pick the default or the test server, and pass the appropriate value to cryptohomed for operations involving requests sent to the attestation servers. Also use the appropriate endpoints for the various servers when sending requests. BUG=660260 TEST=unit tests ==========
drcrash@chromium.org changed reviewers: + pastarmovj@chromium.org
pastarmovj, can you take a look? Thanks!
On 2016/10/28 04:54:13, The one and only Dr. Crash wrote: > pastarmovj, can you take a look? Thanks! By the way, if that is okay to do so (and it probably is, as AttestationCAClient is in the browser/ subtree), I'd consider getting GetPrivacyCAType() outside of the BrowserConnectorChromeOS and simply use this logic inside AttestationCAClient (which will then give the proper type to AttestationFlow for its calls). This will make this CL even smaller.
drcrash@chromium.org changed reviewers: + apronin@chromium.org, dkrahn@google.com - pastarmovj@chromium.org
Now can be reviewed entirely by apronin or dkrahn.
mnissler@chromium.org changed reviewers: + mnissler@chromium.org
https://codereview.chromium.org/2448213007/diff/40001/chromeos/attestation/at... File chromeos/attestation/attestation_constants.h (right): https://codereview.chromium.org/2448213007/diff/40001/chromeos/attestation/at... chromeos/attestation/attestation_constants.h:51: TEST_PCA, // The test version of the Google-operated Privacy CA. IIUC, this type gets passed via the DBus interface. In that case, it's better to add the new enum value at the end, since you'll otherwise might have to deal with incompatible Chrome vs. attestationd/cryptohomed binaries.
The CQ bit was checked by drcrash@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I agree in general, but that value has never been used. So I deliberately put the new value in the middle, because we want to remove the last value soon, but after the new value gets used and I did not want to end up with an enum with values 0 and 2. On Oct 28, 2016 04:34, <mnissler@chromium.org> wrote: > > https://codereview.chromium.org/2448213007/diff/40001/ > chromeos/attestation/attestation_constants.h > File chromeos/attestation/attestation_constants.h (right): > > https://codereview.chromium.org/2448213007/diff/40001/ > chromeos/attestation/attestation_constants.h#newcode51 > chromeos/attestation/attestation_constants.h:51: TEST_PCA, // The > test version of the Google-operated Privacy CA. > IIUC, this type gets passed via the DBus interface. In that case, it's > better to add the new enum value at the end, since you'll otherwise > might have to deal with incompatible Chrome vs. attestationd/cryptohomed > binaries. > > https://codereview.chromium.org/2448213007/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/28 15:10:14, The one and only Dr. Crash wrote: > I agree in general, but that value has never been used. So I deliberately > put the new value in the middle, because we want to remove the last value > soon, but after the new value gets used and I did not want to end up with > an enum with values 0 and 2. If you want to remove it, and it's not used, why not in this CL? > > On Oct 28, 2016 04:34, <mailto:mnissler@chromium.org> wrote: > > > > > https://codereview.chromium.org/2448213007/diff/40001/ > > chromeos/attestation/attestation_constants.h > > File chromeos/attestation/attestation_constants.h (right): > > > > https://codereview.chromium.org/2448213007/diff/40001/ > > chromeos/attestation/attestation_constants.h#newcode51 > > chromeos/attestation/attestation_constants.h:51: TEST_PCA, // The > > test version of the Google-operated Privacy CA. > > IIUC, this type gets passed via the DBus interface. In that case, it's > > better to add the new enum value at the end, since you'll otherwise > > might have to deal with incompatible Chrome vs. attestationd/cryptohomed > > binaries. > > > > https://codereview.chromium.org/2448213007/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Because I do not want to mix concerns here, which also will help if later on we'd decide to bring that back: we could just revert the CL removing things. On Fri, Oct 28, 2016 at 10:27 AM, <dkrahn@google.com> wrote: > On 2016/10/28 15:10:14, The one and only Dr. Crash wrote: > > I agree in general, but that value has never been used. So I deliberately > > put the new value in the middle, because we want to remove the last value > > soon, but after the new value gets used and I did not want to end up with > > an enum with values 0 and 2. > > If you want to remove it, and it's not used, why not in this CL? > > > > > On Oct 28, 2016 04:34, <mailto:mnissler@chromium.org> wrote: > > > > > > > > https://codereview.chromium.org/2448213007/diff/40001/ > > > chromeos/attestation/attestation_constants.h > > > File chromeos/attestation/attestation_constants.h (right): > > > > > > https://codereview.chromium.org/2448213007/diff/40001/ > > > chromeos/attestation/attestation_constants.h#newcode51 > > > chromeos/attestation/attestation_constants.h:51: TEST_PCA, // The > > > test version of the Google-operated Privacy CA. > > > IIUC, this type gets passed via the DBus interface. In that case, it's > > > better to add the new enum value at the end, since you'll otherwise > > > might have to deal with incompatible Chrome vs. > attestationd/cryptohomed > > > binaries. > > > > > > https://codereview.chromium.org/2448213007/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > https://codereview.chromium.org/2448213007/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
The CQ bit was checked by drcrash@chromium.org
The CQ bit was unchecked by drcrash@chromium.org
The CQ bit was checked by drcrash@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from apronin@chromium.org Link to the patchset: https://codereview.chromium.org/2448213007/#ps60001 (title: "Up to date w/ master.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2448213007/diff/60001/chromeos/attestation/at... File chromeos/attestation/attestation_constants.h (right): https://codereview.chromium.org/2448213007/diff/60001/chromeos/attestation/at... chromeos/attestation/attestation_constants.h:51: TEST_PCA, // The test version of the Google-operated Privacy CA. Are you sure this won't break anything? I'd prefer for it to be defined last. Will you sync the various proto files too?
Yes we are pretty sure this won't break anything, there is no piece of code in Chromium that used anything but DEFAULT_PCA. I don't see protos in Chromium that refer to these? On Mon, Oct 31, 2016 at 1:40 PM, <dkrahn@google.com> wrote: > > https://codereview.chromium.org/2448213007/diff/60001/ > chromeos/attestation/attestation_constants.h > File chromeos/attestation/attestation_constants.h (right): > > https://codereview.chromium.org/2448213007/diff/60001/ > chromeos/attestation/attestation_constants.h#newcode51 > chromeos/attestation/attestation_constants.h:51: TEST_PCA, // The > test version of the Google-operated Privacy CA. > Are you sure this won't break anything? I'd prefer for it to be defined > last. Will you sync the various proto files too? > > https://codereview.chromium.org/2448213007/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/10/31 21:09:09, The one and only Dr. Crash wrote: > Yes we are pretty sure this won't break anything, there is no piece of code > in Chromium that used anything but DEFAULT_PCA. > > I don't see protos in Chromium that refer to these? > > On Mon, Oct 31, 2016 at 1:40 PM, <mailto:dkrahn@google.com> wrote: > > > > > https://codereview.chromium.org/2448213007/diff/60001/ > > chromeos/attestation/attestation_constants.h > > File chromeos/attestation/attestation_constants.h (right): > > > > https://codereview.chromium.org/2448213007/diff/60001/ > > chromeos/attestation/attestation_constants.h#newcode51 > > chromeos/attestation/attestation_constants.h:51: TEST_PCA, // The > > test version of the Google-operated Privacy CA. > > Are you sure this won't break anything? I'd prefer for it to be defined > > last. Will you sync the various proto files too? > > > > https://codereview.chromium.org/2448213007/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. My bad -- it's not in the protos I was thinking of -- LGTM!
The CQ bit was checked by drcrash@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
drcrash@chromium.org changed reviewers: + dkrahn@chromium.org - dkrahn@google.com
On 2016/10/31 22:30:22, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) lgtm
The CQ bit was checked by drcrash@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Support different Google attestation (Privacy CA) servers. Add a --attestation-server flag that allows to pick the default or the test server, and pass the appropriate value to cryptohomed for operations involving requests sent to the attestation servers. Also use the appropriate endpoints for the various servers when sending requests. BUG=660260 TEST=unit tests ========== to ========== Support different Google attestation (Privacy CA) servers. Add a --attestation-server flag that allows to pick the default or the test server, and pass the appropriate value to cryptohomed for operations involving requests sent to the attestation servers. Also use the appropriate endpoints for the various servers when sending requests. BUG=660260 TEST=unit tests ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Support different Google attestation (Privacy CA) servers. Add a --attestation-server flag that allows to pick the default or the test server, and pass the appropriate value to cryptohomed for operations involving requests sent to the attestation servers. Also use the appropriate endpoints for the various servers when sending requests. BUG=660260 TEST=unit tests ========== to ========== Support different Google attestation (Privacy CA) servers. Add a --attestation-server flag that allows to pick the default or the test server, and pass the appropriate value to cryptohomed for operations involving requests sent to the attestation servers. Also use the appropriate endpoints for the various servers when sending requests. BUG=660260 TEST=unit tests Committed: https://crrev.com/05374163a5a02f6768c72c1fe73c5f26723cb808 Cr-Commit-Position: refs/heads/master@{#428869} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/05374163a5a02f6768c72c1fe73c5f26723cb808 Cr-Commit-Position: refs/heads/master@{#428869}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2466273002/ by kjellander@chromium.org. The reason for reverting is: Breaks unit_tests: https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
