Chromium Code Reviews
|
|
Chromium Code Reviews
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. |
DescriptionWait 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. #
Messages
Total messages: 51 (23 generated)
Description was changed from ========== Wait for the attestation to be ready (TPM being prepared for attestation) before trying to enroll. BUG=667613 TEST=none yet ========== to ========== Wait for the attestation to be ready (TPM being prepared for attestation) before trying to enroll. BUG=667613 TEST=unit tests ==========
drcrash@chromium.org changed reviewers: + dkrahn@chromium.org
Darren, please review changes I made to have the attestation flow wait for the TPM to be prepared for attestation before proceeding.
drcrash@chromium.org changed reviewers: + apronin@chromium.org
joth@google.com changed reviewers: + joth@google.com
couple drive-by comments on the retry loop. (only because I was guiding Niranjan around similar logic and interested in the differences) https://codereview.chromium.org/2529743002/diff/40001/chromeos/attestation/at... File chromeos/attestation/attestation_flow.cc (right): https://codereview.chromium.org/2529743002/diff/40001/chromeos/attestation/at... chromeos/attestation/attestation_flow.cc:35: // between 90%-100% of the calculated time. maybe comment why fuzzing is not needed (this does not control a network retry, so no risk of it becoming a DDoS) btw - given it's not a network retry, is net::Backoff even needed vs just using a fixed time polling loop for the retry? https://codereview.chromium.org/2529743002/diff/40001/chromeos/attestation/at... chromeos/attestation/attestation_flow.cc:47: }; Nice. This could probably be constexpr too for good measure. https://codereview.chromium.org/2529743002/diff/40001/chromeos/attestation/at... chromeos/attestation/attestation_flow.cc:183: if (initiate_enroll_retries_ == -1 || initiate_enroll_retries_ > 0) { maybe handy to comment what the special values here mean
couple drive-by comments on the retry loop. (only because I was guiding Niranjan around similar logic and interested in the differences)
https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/at... File chromeos/attestation/attestation_flow.h (right): https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/at... chromeos/attestation/attestation_flow.h:258: int16_t initiate_enroll_retries_ = 7; // -1 is unlimited retries. don't we want unlimited retries by default? why 7 is a good magic number here? also, it may make sense to let the caller pass this value to constructor
On 2016/12/02 00:40:03, joth__google wrote: > couple drive-by comments on the retry loop. (only because I was guiding Niranjan > around similar logic and interested in the differences) > > https://codereview.chromium.org/2529743002/diff/40001/chromeos/attestation/at... > File chromeos/attestation/attestation_flow.cc (right): > > https://codereview.chromium.org/2529743002/diff/40001/chromeos/attestation/at... > chromeos/attestation/attestation_flow.cc:35: // between 90%-100% of the > calculated time. > maybe comment why fuzzing is not needed (this does not control a network retry, > so no risk of it becoming a DDoS) > > > btw - given it's not a network retry, is net::Backoff even needed vs just using > a fixed time polling loop for the retry? Honestly, I used net::BackoffEntry because you were guiding Niranjan towards that :) I started with a simple obnoxious timer. We can argue it either way: if the TPM isn't ready, we may want to backoff some anyway instead of pinging cryptohomed like crazy. Or if we're fine pinging cryptohomed, then yes I can make this much simpler. > https://codereview.chromium.org/2529743002/diff/40001/chromeos/attestation/at... > chromeos/attestation/attestation_flow.cc:47: }; > Nice. > This could probably be constexpr too for good measure. Sure. > https://codereview.chromium.org/2529743002/diff/40001/chromeos/attestation/at... > chromeos/attestation/attestation_flow.cc:183: if (initiate_enroll_retries_ == -1 > || initiate_enroll_retries_ > 0) { > maybe handy to comment what the special values here mean The -1 special value is documented in the header.
On 2016/12/02 01:36:24, apronin1 wrote: > https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/at... > File chromeos/attestation/attestation_flow.h (right): > > https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/at... > chromeos/attestation/attestation_flow.h:258: int16_t initiate_enroll_retries_ = > 7; // -1 is unlimited retries. > don't we want unlimited retries by default? why 7 is a good magic number here? > also, it may make sense to let the caller pass this value to constructor Unlimited retries means that we will never surface an error if the TPM never advertises being ready. That seems wrong, but I could be convinced since we log messages on each retry.
https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/at... File chromeos/attestation/attestation_flow.cc (right): https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/at... chromeos/attestation/attestation_flow.cc:142: if (retry_backoff_.ShouldRejectRequest()) { I actually think that the behavior I want is to stack the waiting callbacks. There is no reason to go try ensure the first certificate request will succeed by retrying, but then when we back off simply drop the certificate requests on the floor. Also I am not sure that ShouldRejectRequest() will return true after a first failure only (that doesn't seem right), so a second call to GetCertificate() while a call is pending would mean the first call would never have its callback called. So instead I am thinking of managing a stack (vector) of the callbacks given to GetCertificate(), push callbacks onto that stack and then process them in order once we decide what to do. What do you think?
On 2016/12/02 05:49:57, The one and only Dr. Crash wrote: > On 2016/12/02 01:36:24, apronin1 wrote: > > > https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/at... > > File chromeos/attestation/attestation_flow.h (right): > > > > > https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/at... > > chromeos/attestation/attestation_flow.h:258: int16_t initiate_enroll_retries_ > = > > 7; // -1 is unlimited retries. > > don't we want unlimited retries by default? why 7 is a good magic number here? > > also, it may make sense to let the caller pass this value to constructor > > Unlimited retries means that we will never surface an error if the TPM never > advertises being ready. That seems wrong, but I could be convinced since we log > messages on each retry. I am reluctant to add yet another parameter to the constructor at this point given that callers probably don't have a good idea of what to pass especially if I keep the net::BackoffEntry. Maybe if I made this a timeout it would be better, and pass could be base::TimeDelta::Max()?
https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/at... File chromeos/attestation/attestation_flow_unittest.cc (right): https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/at... chromeos/attestation/attestation_flow_unittest.cc:254: TEST_F(AttestationFlowTest, GetCertificate_Attestation_Never_Prepared) { Something that is interesting here is that I see the logs showing that the retries are after 0 ms, then 2, then 4, etc. etc. Here's the beginning of the log during test. [139700:139700:1201/222219.517837:3935294721413:WARNING:attestation_flow.cc(194)] Attestation: not ready for attestation yet. Retrying in 0s ms. [139700:139700:1201/222219.517942:3935294721516:WARNING:attestation_flow.cc(194)] Attestation: not ready for attestation yet. Retrying in 1.9999s ms. [139700:139700:1201/222219.518040:3935294721615:WARNING:attestation_flow.cc(194)] Attestation: not ready for attestation yet. Retrying in 3.99991s ms. It's interesting given that the initial delay is 2000 ms. However, I see that net::BackoffEntry also uses the real clock (since I don't provide a special TickClock to it) and that may be messing it up as it gets called very fast? I haven't tried to follow all the math in the class when it calculates release time. I tried to use a task runner that can fast forward (https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?r..., as shown in https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?l=44...) but none of the expected mock methods were called. If you have any good idea, I'm all for it :)
On 2016/12/02 06:31:40, The one and only Dr. Crash wrote: > https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/at... > File chromeos/attestation/attestation_flow_unittest.cc (right): > > https://codereview.chromium.org/2529743002/diff/80001/chromeos/attestation/at... > chromeos/attestation/attestation_flow_unittest.cc:254: > TEST_F(AttestationFlowTest, GetCertificate_Attestation_Never_Prepared) { > Something that is interesting here is that I see the logs showing that the > retries are after 0 ms, then 2, then 4, etc. etc. Here's the beginning of the > log during test. > > [139700:139700:1201/222219.517837:3935294721413:WARNING:attestation_flow.cc(194)] > Attestation: not ready for attestation yet. Retrying in 0s ms. > [139700:139700:1201/222219.517942:3935294721516:WARNING:attestation_flow.cc(194)] > Attestation: not ready for attestation yet. Retrying in 1.9999s ms. > [139700:139700:1201/222219.518040:3935294721615:WARNING:attestation_flow.cc(194)] > Attestation: not ready for attestation yet. Retrying in 3.99991s ms. > > It's interesting given that the initial delay is 2000 ms. However, I see that > net::BackoffEntry also uses the real clock (since I don't provide a special > TickClock to it) and that may be messing it up as it gets called very fast? I > haven't tried to follow all the math in the class when it calculates release > time. > > I tried to use a task runner that can fast forward > (https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?r..., > as shown in > https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?l=44...) > but none of the expected mock methods were called. If you have any good idea, > I'm all for it :) Of course all I had to do was to write that I couldn't make it work... to then make it work. Sending a new patchset that addresses previous comments save for my idea of using a stack of callbacks, on which I want feedback (the alternative would be to fail any call while we are retrying, but I don't think that's very nice).
drcrash@chromium.org changed reviewers: + achuith@chromium.org
https://codereview.chromium.org/2529743002/diff/100001/chromeos/attestation/a... File chromeos/attestation/attestation_flow.h (right): https://codereview.chromium.org/2529743002/diff/100001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:262: std::unique_ptr<base::OneShotTimer> retry_timer_; Note that the use we make of all of these is very similar (but not exactly the same, since we also have a timeout) to remoting's BackoffTimer (https://cs.chromium.org/chromium/src/remoting/host/backoff_timer.h?dr=C&sq=pa...). There may be an opportunity for refactoring at some point.
On 2016/12/02 05:48:32, The one and only Dr. Crash wrote: > On 2016/12/02 00:40:03, joth__google wrote: > > couple drive-by comments on the retry loop. (only because I was guiding > Niranjan > > around similar logic and interested in the differences) > > > > > https://codereview.chromium.org/2529743002/diff/40001/chromeos/attestation/at... > > File chromeos/attestation/attestation_flow.cc (right): > > Honestly, I used net::BackoffEntry because you were guiding Niranjan towards > that :) I started with a simple obnoxious timer. We can argue it either way: if > the TPM isn't ready, we may want to backoff some anyway instead of pinging > cryptohomed like crazy. Or if we're fine pinging cryptohomed, then yes I can > make this much simpler. Haha. Yeah fair enough, I see what you're saying. Taking a step back, by using polling for an async operation that is local to the device so we already know we're "working around" a lack of async notification API for that task. Jitter certainly isn't needed, whereas using backoff maybe valuable if we're concerned polling would starve out the service from making forward progress on the task we're it polling about [btw something similar happened before on rialto, with USB vs permission broker with a priority inversion bug -- b/21667662). But at end of day, I think using backoff points to us having even less confidence than normal, in the wisdom of using polling as a work around :-)
> I think using backoff points to us having even less confidence than normal, in the wisdom of using polling as a work around :-) Polling will work. And maybe it's better here to ping, say, every 10 sec. or so rather than in an exponential backoff. After all, we know that attestation will be ready "any time soon now." But yes, this logic really should be in cryptohomed/attestationd at some point and one shouldn't have to poll for readiness when they offer an async interface! On Fri, Dec 2, 2016 at 1:56 PM, <joth@google.com> wrote: > On 2016/12/02 05:48:32, The one and only Dr. Crash wrote: > > On 2016/12/02 00:40:03, joth__google wrote: > > > couple drive-by comments on the retry loop. (only because I was guiding > > Niranjan > > > around similar logic and interested in the differences) > > > > > > > > > https://codereview.chromium.org/2529743002/diff/40001/ > chromeos/attestation/attestation_flow.cc > > > File chromeos/attestation/attestation_flow.cc (right): > > > > Honestly, I used net::BackoffEntry because you were guiding Niranjan > towards > > that :) I started with a simple obnoxious timer. We can argue it either > way: > if > > the TPM isn't ready, we may want to backoff some anyway instead of > pinging > > cryptohomed like crazy. Or if we're fine pinging cryptohomed, then yes I > can > > make this much simpler. > > Haha. Yeah fair enough, I see what you're saying. > > Taking a step back, by using polling for an async operation that is local > to the > device so we already know we're "working around" a lack of async > notification > API for that task. Jitter certainly isn't needed, whereas using backoff > maybe > valuable if we're concerned polling would starve out the service from > making > forward progress on the task we're it polling about [btw something similar > happened before on rialto, with USB vs permission broker with a priority > inversion bug -- b/21667662). But at end of day, I think using backoff > points > to us having even less confidence than normal, in the wisdom of using > polling as > a work around :-) > > > https://codereview.chromium.org/2529743002/ > -- 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 checked by achuith@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...
https://codereview.chromium.org/2529743002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right): https://codereview.chromium.org/2529743002/diff/180001/chrome/browser/chromeo... 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 called: Didn't you plan to go with some default value suggested by AttestationFlow itself? I thought it was going to be something like AttestationFlow::DefaultTimeout (and TimeDelta::Max() where you are ok with waiting as long as it takes). FromMinutes(1) sounds to be somewhat random. https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/a... File chromeos/attestation/attestation_flow.cc (right): https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:354: delete retry_data; Not a big fan of deleting using the pointer passed from above. Especially without the ability to set the variable to NULL in the calling layer. And not sure if it's ok with the styleguide. go/cppguide#Ownership_and_Smart_Pointers Can it be somehow converted to using unique_ptr, for example, and moving it around as necessary?
https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/a... File chromeos/attestation/attestation_flow.cc (right): https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:137: base::Closure initiate_enroll = const. on_enroll_failure too https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:153: TryInitiateEnroll(new RetryData(preparedness_timeout_, tick_clock_), YOu should pass ownership with a unique_ptr. Should retry_data be a data member of this class instead? https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:160: base::Closure do_enroll = base::Bind( const here and below https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:337: base::TimeTicks now = retry_data->tick_clock->NowTicks(); const https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:338: base::TimeDelta elapsed = now - retry_data->begin; const https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:345: LOG(WARNING) << "Attestation: Not prepared yet." Should this be a LOG(WARNING)? Maybe VLOG(1)? https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:351: void AttestationFlow::DoneRetrying(RetryData* retry_data, We don's pass naked pointers around like this with implicit ownership https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/a... File chromeos/attestation/attestation_flow.h (right): https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:23: remove newline https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:25: remove newline https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:26: } // namespace base drop comment. https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:103: // short (e.g. less than 10 seconds). It's unusual to reference a param defined in one function in the comments of another function like this. Is there a way you can move this verbiage to a function comment for the ctor? https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:151: // Called when atestation is not prepared yet, to re-initiate enrollment attestation spelling https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/m... File chromeos/attestation/mock_attestation_flow.cc (right): https://codereview.chromium.org/2529743002/diff/180001/chromeos/attestation/m... chromeos/attestation/mock_attestation_flow.cc:51: : AttestationFlow(NULL, nullptr
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
drcrash@chromium.org changed reviewers: - achuith@chromium.org
lgtm
Please simplify this. Straightforward async polling of CryptohomeClient::TpmAttestationIsPrepared should be enough. See AttestationPolicyObserver::Reschedule. https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... File chromeos/attestation/attestation_flow.cc (right): https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:26: base::TimeDelta kDefaultPreparednessTimeout = base::TimeDelta::FromMinutes(3); POD-type globals only please https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:29: constexpr base::TimeDelta kRetryDelay = base::TimeDelta::FromSeconds(9); same here, POD-type globals only https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... File chromeos/attestation/attestation_flow.h (right): https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:14: #include "base/gtest_prod_util.h" included twice https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:90: // preparedness for a reasonable amount of time. nit: It does a lot of other things too. I don't think this one sub-feature needs to be described in the constructor. If anything, the arguments should be documented. https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:97: void SetPreparednessTimeout(base::TimeDelta preparedness_timeout) { nit: use set_timeout() and timeout() style. optional: I think "preparedness" is awkward. I'd prefer "ready". https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:153: void InitiateEnroll(const base::Closure& on_failure, Better naming please. The semantic difference between "InitiateEnroll" and "StartEnroll" are not clear. Maybe WaitForAttestationServiceReady and StartEnroll? https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:157: // If attestation is not ready yet, retry as needed. This is identical to InitiateEnroll above, they do the same thing? https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:167: // Called when atestation is not prepared yet, to re-initiate enrollment sp:atestation->attestation https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:298: void DoneRetrying(RetryId retry_id, const base::Closure& continuation); 6 methods and a data structure to implement retry, this really seems to add complexity to this class. How is this a bigger problem than AttestationPolicyObserver::Reschedule?
I'll look at AttestationPolicyObserver::Reschedule which I didn't know about. I had a simple new and delete, two lines of code in a previous patchset. Was told not to use a naked pointer ever. Hence the map (passing data by value does not work as timers have no copy constructors). I think new and delete in a single class owning the data was much simpler! On Tue, Dec 6, 2016, 09:31 <dkrahn@chromium.org> wrote: > Please simplify this. Straightforward async polling of > CryptohomeClient::TpmAttestationIsPrepared should be enough. See > AttestationPolicyObserver::Reschedule. > > > > https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... > File chromeos/attestation/attestation_flow.cc (right): > > > https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... > chromeos/attestation/attestation_flow.cc:26: base::TimeDelta > kDefaultPreparednessTimeout = base::TimeDelta::FromMinutes(3); > POD-type globals only please > > > https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... > chromeos/attestation/attestation_flow.cc:29: constexpr base::TimeDelta > kRetryDelay = base::TimeDelta::FromSeconds(9); > same here, POD-type globals only > > > https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... > File chromeos/attestation/attestation_flow.h (right): > > > https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... > chromeos/attestation/attestation_flow.h:14: #include > "base/gtest_prod_util.h" > included twice > > > https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... > chromeos/attestation/attestation_flow.h:90: // preparedness for a > reasonable amount of time. > nit: It does a lot of other things too. I don't think this one > sub-feature needs to be described in the constructor. If anything, the > arguments should be documented. > > > https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... > chromeos/attestation/attestation_flow.h:97: void > SetPreparednessTimeout(base::TimeDelta preparedness_timeout) { > nit: use set_timeout() and timeout() style. > > optional: I think "preparedness" is awkward. I'd prefer "ready". > > > https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... > chromeos/attestation/attestation_flow.h:153: void InitiateEnroll(const > base::Closure& on_failure, > Better naming please. The semantic difference between "InitiateEnroll" > and "StartEnroll" are not clear. Maybe WaitForAttestationServiceReady > and StartEnroll? > > > https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... > chromeos/attestation/attestation_flow.h:157: // If attestation is not > ready yet, retry as needed. > This is identical to InitiateEnroll above, they do the same thing? > > > https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... > chromeos/attestation/attestation_flow.h:167: // Called when atestation > > is not prepared yet, to re-initiate enrollment > sp:atestation->attestation > > > https://codereview.chromium.org/2529743002/diff/220001/chromeos/attestation/a... > chromeos/attestation/attestation_flow.h:298: void DoneRetrying(RetryId > retry_id, const base::Closure& continuation); > 6 methods and a data structure to implement retry, this really seems to > add complexity to this class. How is this a bigger problem than > AttestationPolicyObserver::Reschedule? > > https://codereview.chromium.org/2529743002/ > -- 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 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Darren, can you have another look now? Thanks!
Looks good overall, just a few minor things. https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... File chromeos/attestation/attestation_flow.cc (right): https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:24: // A reasonable reay timeout that gives enough time for attestation reay? https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:26: constexpr uint16_t kReadyTimeoutInSeconds = 180; This does make the caller wait too long -- maybe just drop that comment at least. https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:30: constexpr uint16_t kRetryDelayInSeconds = 9; Too long IMO, why not every 300ms? https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:140: void AttestationFlow::InitiateEnroll(const base::Closure& on_failure, Eliminate this method? GetCertificate() can just as easily create a closure for WaitForAttestationAndStartEnroll(). https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:163: LOG(WARNING) << "Start enroll"; Remove this? https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:320: if (base::TimeTicks::Now() < end_time) { Now() can move if the system clock is changed, right? I'd prefer a max on the number of retries so we don't end up with time related bugs. https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... File chromeos/attestation/attestation_flow.h (right): https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:91: void SetReadyTimeout(base::TimeDelta ready_timeout) { set_ready_timeout() https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:95: base::TimeDelta GetReadyTimeout() const { return ready_timeout_; } ready_timeout() https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:98: void SetRetryDelay(base::TimeDelta retry_delay) { set_retry_delay() https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:103: base::TimeDelta GetRetryDelay() { return retry_delay_; } retry_delay() https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:268: // runs |on_giving_up|, otherwise runs |on_retrying| after a delay. on_giving_up and on_retrying are not defined https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... File chromeos/attestation/attestation_flow_unittest.cc (right): https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow_unittest.cc:91: base::RunLoop run_loop_; Moving this to a class member means you can only call Run() or RunUntilIdle() once per test. An intentional restriction?
https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... File chromeos/attestation/attestation_flow.cc (right): https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:24: // A reasonable reay timeout that gives enough time for attestation On 2016/12/08 18:06:06, Darren Krahn wrote: > reay? You know... Ready :) https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:30: constexpr uint16_t kRetryDelayInSeconds = 9; On 2016/12/08 18:06:06, Darren Krahn wrote: > Too long IMO, why not every 300ms? Sure. You know how quickly this should be ready better than me. https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:140: void AttestationFlow::InitiateEnroll(const base::Closure& on_failure, On 2016/12/08 18:06:05, Darren Krahn wrote: > Eliminate this method? GetCertificate() can just as easily create a closure for > WaitForAttestationAndStartEnroll(). Yes. It used to be there because I allocated data but now that I do not it not necessary to avoid allocating data unless we know the attetstion was not ready. https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:163: LOG(WARNING) << "Start enroll"; On 2016/12/08 18:06:05, Darren Krahn wrote: > Remove this? Good catch. Thanks. https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:320: if (base::TimeTicks::Now() < end_time) { On 2016/12/08 18:06:05, Darren Krahn wrote: > Now() can move if the system clock is changed, right? I'd prefer a max on the > number of retries so we don't end up with time related bugs. Yes, but I wouldn't worry in this case... I did wrote it at some point with a retry count (just dividing the timeout by the retry delay) but I'd rather have a reasonably accurate timeout in the normal case than worry about the clock changing (which, worst case, would mean a very long wait, but normally would mean only wait until the attestation is ready, which will happen). Also, a number of retries with a delay of 0 ms (or very very small retry amounts) is a bit undefined... If we use a timeout, we will actually stop when we reach that time. If you feel very strongly about that, let me know... Because of the small retry amounts, I would then either switch the API to #retries (which I really don't like, because it means nothing in terms of UX), or enforce a minimum retry delay of, say, 10 ms. https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... File chromeos/attestation/attestation_flow_unittest.cc (right): https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow_unittest.cc:91: base::RunLoop run_loop_; On 2016/12/08 18:06:06, Darren Krahn wrote: > Moving this to a class member means you can only call Run() or RunUntilIdle() > once per test. An intentional restriction? No. Changed.
https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... File chromeos/attestation/attestation_flow.cc (right): https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:320: if (base::TimeTicks::Now() < end_time) { On 2016/12/08 19:41:10, The one and only Dr. Crash wrote: > On 2016/12/08 18:06:05, Darren Krahn wrote: > > Now() can move if the system clock is changed, right? I'd prefer a max on the > > number of retries so we don't end up with time related bugs. > > Yes, but I wouldn't worry in this case... I did wrote it at some point with a > retry count (just dividing the timeout by the retry delay) but I'd rather have a > reasonably accurate timeout in the normal case than worry about the clock > changing (which, worst case, would mean a very long wait, but normally would > mean only wait until the attestation is ready, which will happen). > > Also, a number of retries with a delay of 0 ms (or very very small retry > amounts) is a bit undefined... If we use a timeout, we will actually stop when > we reach that time. > > If you feel very strongly about that, let me know... Because of the small retry > amounts, I would then either switch the API to #retries (which I really don't > like, because it means nothing in terms of UX), or enforce a minimum retry delay > of, say, 10 ms. I think the worst case is a forward clock change and immediate timeout. But I'll leave it up to you.
I'm good with that bad case (clock forward and immediate timeout), so if you're good too, I still need you to LGTM as the owner. Thanks Darren! On Fri, Dec 9, 2016 at 2:27 PM, <dkrahn@chromium.org> wrote: > > 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 one and only Dr. Crash wrote: > > On 2016/12/08 18:06:05, Darren Krahn wrote: > > > Now() can move if the system clock is changed, right? I'd prefer a > max on the > > > number of retries so we don't end up with time related bugs. > > > > Yes, but I wouldn't worry in this case... I did wrote it at some point > with a > > retry count (just dividing the timeout by the retry delay) but I'd > rather have a > > reasonably accurate timeout in the normal case than worry about the > clock > > changing (which, worst case, would mean a very long wait, but normally > would > > mean only wait until the attestation is ready, which will happen). > > > > Also, a number of retries with a delay of 0 ms (or very very small > retry > > amounts) is a bit undefined... If we use a timeout, we will actually > stop when > > we reach that time. > > > > If you feel very strongly about that, let me know... Because of the > small retry > > amounts, I would then either switch the API to #retries (which I > really don't > > like, because it means nothing in terms of UX), or enforce a minimum > retry delay > > of, say, 10 ms. > > I think the worst case is a forward clock change and immediate timeout. > But I'll leave it up to you. > > https://codereview.chromium.org/2529743002/ > -- 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.
https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... File chromeos/attestation/attestation_flow.cc (right): https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:26: constexpr uint16_t kReadyTimeoutInSeconds = 180; On 2016/12/08 18:06:05, Darren Krahn wrote: > This does make the caller wait too long -- maybe just drop that comment at > least. Changed to 60 s. https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.cc:320: if (base::TimeTicks::Now() < end_time) { On 2016/12/09 22:27:15, Darren Krahn wrote: > On 2016/12/08 19:41:10, The one and only Dr. Crash wrote: > > On 2016/12/08 18:06:05, Darren Krahn wrote: > > > Now() can move if the system clock is changed, right? I'd prefer a max on > the > > > number of retries so we don't end up with time related bugs. > > > > Yes, but I wouldn't worry in this case... I did wrote it at some point with a > > retry count (just dividing the timeout by the retry delay) but I'd rather have > a > > reasonably accurate timeout in the normal case than worry about the clock > > changing (which, worst case, would mean a very long wait, but normally would > > mean only wait until the attestation is ready, which will happen). > > > > Also, a number of retries with a delay of 0 ms (or very very small retry > > amounts) is a bit undefined... If we use a timeout, we will actually stop when > > we reach that time. > > > > If you feel very strongly about that, let me know... Because of the small > retry > > amounts, I would then either switch the API to #retries (which I really don't > > like, because it means nothing in terms of UX), or enforce a minimum retry > delay > > of, say, 10 ms. > > I think the worst case is a forward clock change and immediate timeout. But I'll > leave it up to you. I think that's ok. The UI should have a retry mechanism if it cares to get the certificate. I think maybe in another CL we could change the boolean nature of the response to a status code and then it could know it has timed out. https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... File chromeos/attestation/attestation_flow.h (right): https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:91: void SetReadyTimeout(base::TimeDelta ready_timeout) { On 2016/12/08 18:06:06, Darren Krahn wrote: > set_ready_timeout() Done. https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:95: base::TimeDelta GetReadyTimeout() const { return ready_timeout_; } On 2016/12/08 18:06:06, Darren Krahn wrote: > ready_timeout() Done. https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:98: void SetRetryDelay(base::TimeDelta retry_delay) { On 2016/12/08 18:06:06, Darren Krahn wrote: > set_retry_delay() Done. https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:103: base::TimeDelta GetRetryDelay() { return retry_delay_; } On 2016/12/08 18:06:06, Darren Krahn wrote: > retry_delay() Done. https://codereview.chromium.org/2529743002/diff/260001/chromeos/attestation/a... chromeos/attestation/attestation_flow.h:268: // runs |on_giving_up|, otherwise runs |on_retrying| after a delay. On 2016/12/08 18:06:06, Darren Krahn wrote: > on_giving_up and on_retrying are not defined Yes I went back to original names. Fixing documentation (which was all wrong, including mentioning my former use of a #retries vs end time).
lgtm, thanks!
The CQ bit was checked by drcrash@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from apronin@google.com Link to the patchset: https://codereview.chromium.org/2529743002/#ps320001 (title: "Changed method names and timeouts/delays. Udated doc.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1481689900968330,
"parent_rev": "24d0346fb0bf05fbbefe54f3c3814badf1f7cd16", "commit_rev":
"1ebc3acbf285157bb0bc460a689ebc4c7f2e4398"}
Message was sent while issue was closed.
Description was changed from ========== Wait for the attestation to be ready (TPM being prepared for attestation) before trying to enroll. BUG=667613 TEST=unit tests ========== to ========== Wait for the attestation to be ready (TPM being prepared for attestation) before trying to enroll. BUG=667613 TEST=unit tests Review-Url: https://codereview.chromium.org/2529743002 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Wait for the attestation to be ready (TPM being prepared for attestation) before trying to enroll. BUG=667613 TEST=unit tests Review-Url: https://codereview.chromium.org/2529743002 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/b79df1c3630a0e5450dbc7e30ec6b87748f1a73e Cr-Commit-Position: refs/heads/master@{#438439} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
