Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(2075)

Issue 2529743002: Wait for the attestation to be ready (TPM being prepared for attestation) before trying to enroll. (Closed)

Created:
4 years ago by The one and only Dr. Crash
Modified:
4 years ago
CC:
chromium-reviews, oshima+watch_chromium.org, dkrahn+watch_chromium.org, dkalin1, kumarniranjan, joth
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Wait for the attestation to be ready (TPM being prepared for attestation) before trying to enroll. BUG=667613 TEST=unit tests Committed: https://crrev.com/b79df1c3630a0e5450dbc7e30ec6b87748f1a73e Cr-Commit-Position: refs/heads/master@{#438439}

Patch Set 1 #

Patch Set 2 : Added test for when attestation is not prepared. #

Patch Set 3 : Added all tests #

Total comments: 3

Patch Set 4 : Reset the backoff entry once the TPM indicates it's prepared. #

Patch Set 5 : Inverted condition for easier reading. #

Total comments: 3

Patch Set 6 : AttestationFlow supports a timeout. #

Total comments: 1

Patch Set 7 : Queue GetCertificate calls. #

Patch Set 8 : Independent retries for independent GetCertificate calls. #

Patch Set 9 : Call DoneRetrying() when we give up. #

Patch Set 10 : Slightly lower retry delay. #

Total comments: 15

Patch Set 11 : We don't like naked pointers, so welcome RetryId. #

Patch Set 12 : Hide RetryData as an implementation detail. #

Total comments: 9

Patch Set 13 : Simplified. #

Patch Set 14 : Smaller timeouts or tests fail for being too chatty. #

Total comments: 26

Patch Set 15 : Addressed more feedback. #

Patch Set 16 : Addressed more feedback. #

Patch Set 17 : Changed method names and timeouts/delays. Udated doc. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+286 lines, -34 lines) Patch
M chromeos/attestation/attestation_flow.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 5 chunks +48 lines, -1 line 0 comments Download
M chromeos/attestation/attestation_flow.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 7 chunks +65 lines, -15 lines 0 comments Download
M chromeos/attestation/attestation_flow_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 19 chunks +173 lines, -18 lines 0 comments Download

Messages

Total messages: 51 (23 generated)
The one and only Dr. Crash
Darren, please review changes I made to have the attestation flow wait for the TPM ...
4 years ago (2016-12-02 00:21:17 UTC) #3
joth__google
couple drive-by comments on the retry loop. (only because I was guiding Niranjan around similar ...
4 years ago (2016-12-02 00:40:03 UTC) #6
joth__google
couple drive-by comments on the retry loop. (only because I was guiding Niranjan around similar ...
4 years ago (2016-12-02 00:40:04 UTC) #7
apronin1
https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/attestation_flow.h File chromeos/attestation/attestation_flow.h (right): https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/attestation_flow.h#newcode258 chromeos/attestation/attestation_flow.h:258: int16_t initiate_enroll_retries_ = 7; // -1 is unlimited retries. ...
4 years ago (2016-12-02 01:36:24 UTC) #8
The one and only Dr. Crash
On 2016/12/02 00:40:03, joth__google wrote: > couple drive-by comments on the retry loop. (only because ...
4 years ago (2016-12-02 05:48:32 UTC) #9
The one and only Dr. Crash
On 2016/12/02 01:36:24, apronin1 wrote: > https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/attestation_flow.h > File chromeos/attestation/attestation_flow.h (right): > > https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/attestation_flow.h#newcode258 > ...
4 years ago (2016-12-02 05:49:57 UTC) #10
The one and only Dr. Crash
https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/attestation_flow.cc File chromeos/attestation/attestation_flow.cc (right): https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/attestation_flow.cc#newcode142 chromeos/attestation/attestation_flow.cc:142: if (retry_backoff_.ShouldRejectRequest()) { I actually think that the behavior ...
4 years ago (2016-12-02 05:57:50 UTC) #11
The one and only Dr. Crash
On 2016/12/02 05:49:57, The one and only Dr. Crash wrote: > On 2016/12/02 01:36:24, apronin1 ...
4 years ago (2016-12-02 06:02:23 UTC) #12
The one and only Dr. Crash
https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/attestation_flow_unittest.cc File chromeos/attestation/attestation_flow_unittest.cc (right): https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/attestation_flow_unittest.cc#newcode254 chromeos/attestation/attestation_flow_unittest.cc:254: TEST_F(AttestationFlowTest, GetCertificate_Attestation_Never_Prepared) { Something that is interesting here is ...
4 years ago (2016-12-02 06:31:40 UTC) #13
The one and only Dr. Crash
On 2016/12/02 06:31:40, The one and only Dr. Crash wrote: > https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/attestation_flow_unittest.cc > File chromeos/attestation/attestation_flow_unittest.cc ...
4 years ago (2016-12-02 06:43:52 UTC) #14
The one and only Dr. Crash
https://codereview.chromium.org/2529743002/diff/100001/chromeos/attestation/attestation_flow.h File chromeos/attestation/attestation_flow.h (right): https://codereview.chromium.org/2529743002/diff/100001/chromeos/attestation/attestation_flow.h#newcode262 chromeos/attestation/attestation_flow.h:262: std::unique_ptr<base::OneShotTimer> retry_timer_; Note that the use we make of ...
4 years ago (2016-12-02 06:59:34 UTC) #16
joth__google
On 2016/12/02 05:48:32, The one and only Dr. Crash wrote: > On 2016/12/02 00:40:03, joth__google ...
4 years ago (2016-12-02 21:56:53 UTC) #17
The one and only Dr. Crash
> I think using backoff points to us having even less confidence than normal, in ...
4 years ago (2016-12-02 22:42:55 UTC) #18
apronin1
https://codereview.chromium.org/2529743002/diff/180001/chrome/browser/chromeos/attestation/attestation_policy_observer.cc File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right): https://codereview.chromium.org/2529743002/diff/180001/chrome/browser/chromeos/attestation/attestation_policy_observer.cc#newcode165 chrome/browser/chromeos/attestation/attestation_policy_observer.cc:165: std::move(attestation_ca_client), base::TimeDelta::FromMinutes(1))); here and other places where constructor is ...
4 years ago (2016-12-05 19:25:50 UTC) #21
achuithb
https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/attestation_flow.cc File chromeos/attestation/attestation_flow.cc (right): https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/attestation_flow.cc#newcode137 chromeos/attestation/attestation_flow.cc:137: base::Closure initiate_enroll = const. on_enroll_failure too https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/attestation_flow.cc#newcode153 chromeos/attestation/attestation_flow.cc:153: TryInitiateEnroll(new ...
4 years ago (2016-12-05 19:53:59 UTC) #22
apronin1
lgtm
4 years ago (2016-12-06 00:24:28 UTC) #26
Darren Krahn
Please simplify this. Straightforward async polling of CryptohomeClient::TpmAttestationIsPrepared should be enough. See AttestationPolicyObserver::Reschedule. https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/attestation_flow.cc File ...
4 years ago (2016-12-06 17:31:25 UTC) #27
The one and only Dr. Crash
I'll look at AttestationPolicyObserver::Reschedule which I didn't know about. I had a simple new and ...
4 years ago (2016-12-06 17:38:34 UTC) #28
The one and only Dr. Crash
Darren, can you have another look now? Thanks!
4 years ago (2016-12-08 17:29:31 UTC) #37
Darren Krahn
Looks good overall, just a few minor things. https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/attestation_flow.cc File chromeos/attestation/attestation_flow.cc (right): https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/attestation_flow.cc#newcode24 chromeos/attestation/attestation_flow.cc:24: // ...
4 years ago (2016-12-08 18:06:06 UTC) #38
The one and only Dr. Crash
https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/attestation_flow.cc File chromeos/attestation/attestation_flow.cc (right): https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/attestation_flow.cc#newcode24 chromeos/attestation/attestation_flow.cc:24: // A reasonable reay timeout that gives enough time ...
4 years ago (2016-12-08 19:41:10 UTC) #39
Darren Krahn
https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/attestation_flow.cc File chromeos/attestation/attestation_flow.cc (right): https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/attestation_flow.cc#newcode320 chromeos/attestation/attestation_flow.cc:320: if (base::TimeTicks::Now() < end_time) { On 2016/12/08 19:41:10, The ...
4 years ago (2016-12-09 22:27:16 UTC) #40
The one and only Dr. Crash
I'm good with that bad case (clock forward and immediate timeout), so if you're good ...
4 years ago (2016-12-10 01:46:22 UTC) #41
The one and only Dr. Crash
https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/attestation_flow.cc File chromeos/attestation/attestation_flow.cc (right): https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/attestation_flow.cc#newcode26 chromeos/attestation/attestation_flow.cc:26: constexpr uint16_t kReadyTimeoutInSeconds = 180; On 2016/12/08 18:06:05, Darren ...
4 years ago (2016-12-13 22:37:17 UTC) #42
Darren Krahn
lgtm, thanks!
4 years ago (2016-12-14 00:32:02 UTC) #43
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2529743002/320001
4 years ago (2016-12-14 04:31:58 UTC) #46
commit-bot: I haz the power
Committed patchset #17 (id:320001)
4 years ago (2016-12-14 05:16:24 UTC) #49
commit-bot: I haz the power
4 years ago (2016-12-14 05:18:30 UTC) #51
Message was sent while issue was closed.
Patchset 17 (id:??) landed as
https://crrev.com/b79df1c3630a0e5450dbc7e30ec6b87748f1a73e
Cr-Commit-Position: refs/heads/master@{#438439}

Powered by Google App Engine
This is Rietveld 408576698