Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2489883007:
[net/auth] Discard current handler token generation fails. (Closed)
Description[net/auth] Discard current handler token generation fails.
Errors that occur during token generation should be considered fatal for
that instance of HttpAuthHandler. I.e. the handler should not be used to
process any additional authentication challenges and definitely should
not be used to generate any tokens.
Why? Because, any external state associated with an HttpAuthHandler may
not in a consistent or known state. While we would like to write auth
handlers that revert to a known safe state after a token generation
failure, that is not trivial when dealing with external token sources
with unpredictable behavior. Hence we are going to take the safer option
of tearing down the HttpAuthHandler following such a failure event.
In the case referred to in https://crbug.com/648366, we were leaving the
HttpAuthSspiWin in a state where it was expecting a new token, but the
credential handle it was in posession of could no longer be used to
generate a new token. This resulted in an ERR_INVALID_HANDLE error.
Oops.
R=mmenke@chromium.org
BUG=648366
Committed: https://crrev.com/463ca4262fc277c5e724d02705ba1a61cbc4e849
Cr-Commit-Position: refs/heads/master@{#432359}
Patch Set 1 #Patch Set 2 : . #
Total comments: 24
Patch Set 3 : . #Patch Set 4 : Missed a few comments in the last round. #
Messages
Total messages: 33 (18 generated)
The CQ bit was checked by asanka@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Haven't looked at the mock changes yet. https://codereview.chromium.org/2489883007/diff/20001/net/http/http_auth_cont... File net/http/http_auth_controller.cc (right): https://codereview.chromium.org/2489883007/diff/20001/net/http/http_auth_cont... net/http/http_auth_controller.cc:484: // If the handler does not support any remaining identity sources, then "The handler" seems incorrect here, since we're destroying it. https://codereview.chromium.org/2489883007/diff/20001/net/http/http_auth_cont... net/http/http_auth_controller.cc:507: InvalidateCurrentHandler(INVALIDATE_HANDLER_AND_DISABLE_SCHEME); None of the new tests exercise this, do they? https://codereview.chromium.org/2489883007/diff/20001/net/http/http_auth_cont... net/http/http_auth_controller.cc:516: void HttpAuthController::OnIOComplete(int result) { Mind renaming this while you're here? This isn't a general DoLoop entry point, but rather a specific callback for GenerateAuthToken. https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:11562: int proxy_auth_rv; Mind renaming this while you're here? first_generate_proxy_token_rv or somesuch? "proxy_auth_rv" sounds a bit more general than that. https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:11565: int server_auth_rv; same of this one, though I guess this shouldn't have a "first". https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:11794: TestRound(kGetProxy, kSuccess, OK)}}, So in this case, the proxy challenges us, we fail to generate auth credentials, so decide to send the request again just for the heck of it, and the server randomly decides it didn't really want to see our credentials after all? https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:11806: TestRound(kGetProxyAuth, kSuccess, OK)}}, And in this case, the server sanely decides to challenge us again, we send credentials, and things work? These tests don't actually check that we send credentials when expected, do they? https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:11880: TestRound(kGetAuthWithProxyAuth, kSuccess, OK)}}, What additional coverage do we get from this one? https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:12224: &kServerChallenge), I don't understand what these extra reads/writes do, or why the last test you modified doesn't need them.
https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:12224: &kServerChallenge), On 2016/11/14 18:03:49, mmenke wrote: > I don't understand what these extra reads/writes do, or why the last test you > modified doesn't need them. By "last test you modified", I mean the new test case you added in this CL that looks just like this one, except this line. I guess it also has a kNoSSL instead of a 2..Oh, is that why?
The CQ bit was checked by asanka@chromium.org to run a CQ dry run
https://codereview.chromium.org/2489883007/diff/20001/net/http/http_auth_cont... File net/http/http_auth_controller.cc (right): https://codereview.chromium.org/2489883007/diff/20001/net/http/http_auth_cont... net/http/http_auth_controller.cc:484: // If the handler does not support any remaining identity sources, then On 2016/11/14 at 18:03:49, mmenke wrote: > "The handler" seems incorrect here, since we're destroying it. DOne. https://codereview.chromium.org/2489883007/diff/20001/net/http/http_auth_cont... net/http/http_auth_controller.cc:507: InvalidateCurrentHandler(INVALIDATE_HANDLER_AND_DISABLE_SCHEME); On 2016/11/14 at 18:03:49, mmenke wrote: > None of the new tests exercise this, do they? Not the new tests. But existing tests in the GenerateAuthToken suite do. You can find them by searching the test cases for ERR_UNSUPPORTED_AUTH_SCHEME. https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:11806: TestRound(kGetProxyAuth, kSuccess, OK)}}, On 2016/11/14 at 18:03:49, mmenke wrote: > And in this case, the server sanely decides to challenge us again, we send credentials, and things work? These tests don't actually check that we send credentials when expected, do they? The kGetProxyAuth sets up the expectation that the request contains Proxy-Authorization header with a token. That's the mechanism used by the test setup to make sure the correct auth handler has generated a token. I.e. the first auth handler will always fail, and the second auth handler always succeed. The test case succeeds if the auth stack discards the first handler and uses the second handler. https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:11880: TestRound(kGetAuthWithProxyAuth, kSuccess, OK)}}, On 2016/11/14 at 18:03:49, mmenke wrote: > What additional coverage do we get from this one? Assuming you are comparing to the test at 11917. As far as proxy auth goes, the difference is synchronous vs. asynchronous failures which have different code paths. Also, the new tests are the only ones which tests that proxy auth recovers from a INVALID_AUTH_CREDENTIALS failure. https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:12224: &kServerChallenge), On 2016/11/14 at 18:07:13, mmenke wrote: > On 2016/11/14 18:03:49, mmenke wrote: > > I don't understand what these extra reads/writes do, or why the last test you > > modified doesn't need them. > > By "last test you modified", I mean the new test case you added in this CL that looks just like this one, except this line. I guess it also has a kNoSSL instead of a 2..Oh, is that why? Yeah, there's an extra "CONNECT" request in the middle. Also under the covers, you'll remember that the token generation happens under HttpProxyClientSocket in this test vs. HttpNetworkTransaction in the other.
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/2489883007/diff/20001/net/http/http_auth_cont... File net/http/http_auth_controller.cc (right): https://codereview.chromium.org/2489883007/diff/20001/net/http/http_auth_cont... net/http/http_auth_controller.cc:516: void HttpAuthController::OnIOComplete(int result) { On 2016/11/14 18:03:49, mmenke wrote: > Mind renaming this while you're here? This isn't a general DoLoop entry point, > but rather a specific callback for GenerateAuthToken. Missed this one? https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:11562: int proxy_auth_rv; On 2016/11/14 18:03:49, mmenke wrote: > Mind renaming this while you're here? first_generate_proxy_token_rv or > somesuch? "proxy_auth_rv" sounds a bit more general than that. Missed this one? https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:11565: int server_auth_rv; On 2016/11/14 18:03:49, mmenke wrote: > same of this one, though I guess this shouldn't have a "first". Missed this one? https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:11880: TestRound(kGetAuthWithProxyAuth, kSuccess, OK)}}, On 2016/11/14 18:49:53, asanka wrote: > On 2016/11/14 at 18:03:49, mmenke wrote: > > What additional coverage do we get from this one? > > Assuming you are comparing to the test at 11917. As far as proxy auth goes, the > difference is synchronous vs. asynchronous failures which have different code > paths. > > Also, the new tests are the only ones which tests that proxy auth recovers from > a INVALID_AUTH_CREDENTIALS failure. I was actually comparing to 11795 - we get an error when generating proxy server credentials, just like in that case, and I don't think this does anything new in the non-proxy challenge, does it? We never actually check a likely case where we get ERR_INVALID_AUTH_CREDENTIALS for authing with the server - normally we'd send another request, get another challenge, and then successfully generate credentials, right?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2489883007/diff/20001/net/http/http_auth_cont... File net/http/http_auth_controller.cc (right): https://codereview.chromium.org/2489883007/diff/20001/net/http/http_auth_cont... net/http/http_auth_controller.cc:516: void HttpAuthController::OnIOComplete(int result) { On 2016/11/14 at 19:01:21, mmenke wrote: > On 2016/11/14 18:03:49, mmenke wrote: > > Mind renaming this while you're here? This isn't a general DoLoop entry point, > > but rather a specific callback for GenerateAuthToken. > > Missed this one? Oops. Done. https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:11562: int proxy_auth_rv; On 2016/11/14 at 19:01:21, mmenke wrote: > On 2016/11/14 18:03:49, mmenke wrote: > > Mind renaming this while you're here? first_generate_proxy_token_rv or > > somesuch? "proxy_auth_rv" sounds a bit more general than that. > > Missed this one? Oops. Done. https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:11565: int server_auth_rv; On 2016/11/14 at 19:01:21, mmenke wrote: > On 2016/11/14 18:03:49, mmenke wrote: > > same of this one, though I guess this shouldn't have a "first". > > Missed this one? Done. Oops again, but the new Rietveld UI didn't show this comment at all :-/ I called this one 'first_...' as well since it also determines what the first server auth handler returns when GenerateAuthToken is called. https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:11794: TestRound(kGetProxy, kSuccess, OK)}}, On 2016/11/14 at 18:03:49, mmenke wrote: > So in this case, the proxy challenges us, we fail to generate auth credentials, so decide to send the request again just for the heck of it, and the server randomly decides it didn't really want to see our credentials after all? Yup. It's a bit of a nonsensical case, but it now tests that we don't have a stray HttpAuthHandler that's causing errors in the HttpNetworkTransaction state machine. I.e. the HttpNetworkTransaction can function normally after a recoverable error. https://codereview.chromium.org/2489883007/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:11880: TestRound(kGetAuthWithProxyAuth, kSuccess, OK)}}, On 2016/11/14 at 19:01:21, mmenke wrote: > On 2016/11/14 18:49:53, asanka wrote: > > On 2016/11/14 at 18:03:49, mmenke wrote: > > > What additional coverage do we get from this one? > > > > Assuming you are comparing to the test at 11917. As far as proxy auth goes, the > > difference is synchronous vs. asynchronous failures which have different code > > paths. > > > > Also, the new tests are the only ones which tests that proxy auth recovers from > > a INVALID_AUTH_CREDENTIALS failure. > > I was actually comparing to 11795 - we get an error when generating proxy server credentials, just like in that case, and I don't think this does anything new in the non-proxy challenge, does it? > > We never actually check a likely case where we get ERR_INVALID_AUTH_CREDENTIALS for authing with the server - normally we'd send another request, get another challenge, and then successfully generate credentials, right? Ah right. 11795 has the property that the second proxy auth handler only generates one token, whereas in this test it generates two tokens. No other differences beyond that. We could argue that the 'passive' token behavior (i.e. where an HttpAuthHandler generates a token for all successive network requests in the same transaction without further challenges) is tested elsewhere. We currently don't have any special code that would cause the second auth handler after a recoverable error to behave any different from one that didn't encounter an error. I'm happy to remove this test case, but I'd be inclined to keep it just because it's a subtly different set of circumstances and acts as a proactive guard against accidentally breaking this case.
LGTM (Sorry didn't get to this one earlier today, forgot about it)
On 2016/11/15 at 19:02:51, mmenke wrote: > LGTM (Sorry didn't get to this one earlier today, forgot about it) Thanks!
The CQ bit was checked by asanka@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by asanka@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by mmenke@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by asanka@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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [net/auth] Discard current handler token generation fails. Errors that occur during token generation should be considered fatal for that instance of HttpAuthHandler. I.e. the handler should not be used to process any additional authentication challenges and definitely should not be used to generate any tokens. Why? Because, any external state associated with an HttpAuthHandler may not in a consistent or known state. While we would like to write auth handlers that revert to a known safe state after a token generation failure, that is not trivial when dealing with external token sources with unpredictable behavior. Hence we are going to take the safer option of tearing down the HttpAuthHandler following such a failure event. In the case referred to in https://crbug.com/648366, we were leaving the HttpAuthSspiWin in a state where it was expecting a new token, but the credential handle it was in posession of could no longer be used to generate a new token. This resulted in an ERR_INVALID_HANDLE error. Oops. R=mmenke@chromium.org BUG=648366 ========== to ========== [net/auth] Discard current handler token generation fails. Errors that occur during token generation should be considered fatal for that instance of HttpAuthHandler. I.e. the handler should not be used to process any additional authentication challenges and definitely should not be used to generate any tokens. Why? Because, any external state associated with an HttpAuthHandler may not in a consistent or known state. While we would like to write auth handlers that revert to a known safe state after a token generation failure, that is not trivial when dealing with external token sources with unpredictable behavior. Hence we are going to take the safer option of tearing down the HttpAuthHandler following such a failure event. In the case referred to in https://crbug.com/648366, we were leaving the HttpAuthSspiWin in a state where it was expecting a new token, but the credential handle it was in posession of could no longer be used to generate a new token. This resulted in an ERR_INVALID_HANDLE error. Oops. R=mmenke@chromium.org BUG=648366 Committed: https://crrev.com/463ca4262fc277c5e724d02705ba1a61cbc4e849 Cr-Commit-Position: refs/heads/master@{#432359} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/463ca4262fc277c5e724d02705ba1a61cbc4e849 Cr-Commit-Position: refs/heads/master@{#432359} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
