net/http/http_network_transaction.cc - Issue 1781003003: Implement referred Token Bindings

Unified Diff: net/http/http_network_transaction.cc

Issue 1781003003: Implement referred Token Bindings (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Created 4 years, 9 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

Index: net/http/http_network_transaction.cc

diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc

index 94361dac063db5f2152086d21095825635f0b6bf..a6ab6069315d13031c87b69a554f2bb1df99e7cc 100644

--- a/net/http/http_network_transaction.cc

+++ b/net/http/http_network_transaction.cc

@@ -976,11 +976,16 @@ int HttpNetworkTransaction::DoGetTokenBindingKey() {

if (!IsTokenBindingEnabled())

return OK;

+ scoped_ptr<crypto::ECPrivateKey>* key = &provided_token_binding_key_;

+ std::string host = request_->url.host();

+ if (provided_token_binding_key_) {

+ key = &referred_token_binding_key_;

+ host = request_->token_binding_referrer;

+ }

net_log_.BeginEvent(NetLog::TYPE_HTTP_TRANSACTION_GET_TOKEN_BINDING_KEY);

ChannelIDService* channel_id_service = session_->params().channel_id_service;

- return channel_id_service->GetOrCreateChannelID(

- request_->url.host(), &token_binding_key_, io_callback_,

- &token_binding_request_);

+ return channel_id_service->GetOrCreateChannelID(host, key, io_callback_,

+ &token_binding_request_);

}

int HttpNetworkTransaction::DoGetTokenBindingKeyComplete(int rv) {

@@ -989,6 +994,10 @@ int HttpNetworkTransaction::DoGetTokenBindingKeyComplete(int rv) {

if (!IsTokenBindingEnabled())

return OK;

+ if (!request_->token_binding_referrer.empty() &&

+ !referred_token_binding_key_ && rv == OK) {

+ next_state_ = STATE_GET_TOKEN_BINDING_KEY;

davidben 2016/03/15 22:49:56 Do you think this would be better done as two sepa

nharper 2016/03/16 17:49:22 Two separate states would make the state transitio

davidben 2016/03/24 20:53:51 I think have a weak preference towards the state m

I think have a weak preference towards the state machine clarify because there's already so little code in those states. Also state machines are scary. But if you disagree with me or it turns out to be a hassle because my synchronous analogy doesn't quite align (below), feel free to override the weak preference. My thinking is since the state machine stuff roughly mirrors synchronous code, if you were writing it synchronously, you wouldn't write it as: // Earlier stuff... get_token_binding_key: scoped_ptr<crypto::ECPrivateKey>* key = &provided_token_binding_key_; std::string host = request_->url.host(); if (provided_token_binding_key_) { key = &referred_token_binding_key_; host = request_->token_binding_referrer; } net_log_.BeginEvent(NetLog::TYPE_HTTP_TRANSACTION_GET_TOKEN_BINDING_KEY); ChannelIDService* channel_id_service = session_->params().channel_id_service; int rv = channel_id_service->GetOrCreateChannelID(host, key); net_log_.EndEventWithNetErrorCode( NetLog::TYPE_HTTP_TRANSACTION_GET_TOKEN_BINDING_KEY, rv); if (rv < 0) return rv; if (!request_->token_binding_referrer.empty() && !referred_token_binding_key_ && rv == OK) { goto get_token_binding_key; } // DoInitRequestBody stuff... You'd probably write this as: // Earlier stuff... // Get the main token net_log_.BeginEvent(NetLog::TYPE_HTTP_TRANSACTION_GET_TOKEN_BINDING_KEY); ChannelIDService* channel_id_service = session_->params().channel_id_service; int rv = channel_id_service->GetOrCreateChannelID( request_->url.host(), &provided_token_binding_key_); net_log_.EndEventWithNetErrorCode( NetLog::TYPE_HTTP_TRANSACTION_GET_TOKEN_BINDING_KEY, rv); if (rv < 0) return rv; if (!request_->token_binding_referrer.empty()) { // Get the referred token. net_log_.BeginEvent(NetLog::TYPE_HTTP_TRANSACTION_GET_TOKEN_BINDING_KEY); ChannelIDService* channel_id_service = session_->params().channel_id_service; int rv = channel_id_service->GetOrCreateChannelID( request_->token_binding_referrer, &referred_token_binding_key_); net_log_.EndEventWithNetErrorCode( NetLog::TYPE_HTTP_TRANSACTION_GET_TOKEN_BINDING_KEY, rv); if (rv < 0) return rv; } // DoInitRequestBody stuff... Of course, this analogy breaks down a little because conditionals are funny and such. I could believe duplicating the state wouldn't actually be clearer because of that? I dunno, your call.

nharper 2016/03/25 01:34:29 I've rewritten it to have more states - it makes m

On 2016/03/24 20:53:51, davidben wrote: > On 2016/03/16 17:49:22, nharper wrote: > > On 2016/03/15 22:49:56, davidben wrote: > > > Do you think this would be better done as two separate states? > > > > Two separate states would make the state transitions clearer (it would remove > > the loop), but it would also result in code duplication. If you think the > state > > transition clarity is worth the duplication, then I'm fine changing it. > > I think have a weak preference towards the state machine clarify because there's > already so little code in those states. Also state machines are scary. But if > you disagree with me or it turns out to be a hassle because my synchronous > analogy doesn't quite align (below), feel free to override the weak preference. > > My thinking is since the state machine stuff roughly mirrors synchronous code, > if you were writing it synchronously, you wouldn't write it as: > > // Earlier stuff... > > get_token_binding_key: > > scoped_ptr<crypto::ECPrivateKey>* key = &provided_token_binding_key_; > std::string host = request_->url.host(); > if (provided_token_binding_key_) { > key = &referred_token_binding_key_; > host = request_->token_binding_referrer; > } > > net_log_.BeginEvent(NetLog::TYPE_HTTP_TRANSACTION_GET_TOKEN_BINDING_KEY); > ChannelIDService* channel_id_service = > session_->params().channel_id_service; > int rv = channel_id_service->GetOrCreateChannelID(host, key); > net_log_.EndEventWithNetErrorCode( > NetLog::TYPE_HTTP_TRANSACTION_GET_TOKEN_BINDING_KEY, rv); > if (rv < 0) > return rv; > > if (!request_->token_binding_referrer.empty() && > !referred_token_binding_key_ && rv == OK) { > goto get_token_binding_key; > } > > // DoInitRequestBody stuff... > > You'd probably write this as: > > // Earlier stuff... > > // Get the main token > net_log_.BeginEvent(NetLog::TYPE_HTTP_TRANSACTION_GET_TOKEN_BINDING_KEY); > ChannelIDService* channel_id_service = > session_->params().channel_id_service; > int rv = channel_id_service->GetOrCreateChannelID( > request_->url.host(), &provided_token_binding_key_); > net_log_.EndEventWithNetErrorCode( > NetLog::TYPE_HTTP_TRANSACTION_GET_TOKEN_BINDING_KEY, rv); > if (rv < 0) > return rv; > > if (!request_->token_binding_referrer.empty()) { > // Get the referred token. > net_log_.BeginEvent(NetLog::TYPE_HTTP_TRANSACTION_GET_TOKEN_BINDING_KEY); > ChannelIDService* channel_id_service = > session_->params().channel_id_service; > int rv = channel_id_service->GetOrCreateChannelID( > request_->token_binding_referrer, &referred_token_binding_key_); > net_log_.EndEventWithNetErrorCode( > NetLog::TYPE_HTTP_TRANSACTION_GET_TOKEN_BINDING_KEY, rv); > if (rv < 0) > return rv; > } > > // DoInitRequestBody stuff... > > > Of course, this analogy breaks down a little because conditionals are funny and > such. I could believe duplicating the state wouldn't actually be clearer because > of that? I dunno, your call.

I've rewritten it to have more states - it makes much more sense that way than looping back to avoid code duplication.

+ }

net_log_.EndEventWithNetErrorCode(

NetLog::TYPE_HTTP_TRANSACTION_GET_TOKEN_BINDING_KEY, rv);

return rv;

@@ -1028,7 +1037,7 @@ int HttpNetworkTransaction::BuildRequestHeaders(

}

RecordTokenBindingSupport();

- if (token_binding_key_) {

+ if (provided_token_binding_key_) {

std::string token_binding_header;

int rv = BuildTokenBindingHeader(&token_binding_header);

if (rv != OK)

@@ -1066,17 +1075,32 @@ int HttpNetworkTransaction::BuildRequestHeaders(

int HttpNetworkTransaction::BuildTokenBindingHeader(std::string* out) {

std::vector<uint8_t> signed_ekm;

- int rv = stream_->GetSignedEKMForTokenBinding(token_binding_key_.get(),

- &signed_ekm);

+ int rv = stream_->GetSignedEKMForTokenBinding(

+ provided_token_binding_key_.get(), &signed_ekm);

if (rv != OK)

return rv;

std::string provided_token_binding;

- rv = BuildProvidedTokenBinding(token_binding_key_.get(), signed_ekm,

- &provided_token_binding);

+ rv = BuildTokenBinding(TB_TYPE_PROVIDED, provided_token_binding_key_.get(),

+ signed_ekm, &provided_token_binding);

if (rv != OK)

return rv;

std::vector<base::StringPiece> token_bindings;

token_bindings.push_back(provided_token_binding);

+ std::string referred_token_binding;

+ if (referred_token_binding_key_) {

+ std::vector<uint8_t> referred_signed_ekm;

+ int rv = stream_->GetSignedEKMForTokenBinding(

+ referred_token_binding_key_.get(), &referred_signed_ekm);

+ if (rv != OK)

+ return rv;

+ rv = BuildTokenBinding(TB_TYPE_REFERRED, referred_token_binding_key_.get(),

+ referred_signed_ekm, &referred_token_binding);

+ if (rv != OK)

+ return rv;

+ token_bindings.push_back(referred_token_binding);

+ }

std::string header;

rv = BuildTokenBindingMessageFromTokenBindings(token_bindings, &header);

davidben 2016/03/15 22:49:56 This seems sort of a nuisance of a serialization s

nharper 2016/03/16 17:49:22 Is this comment about how the spec defines the ser

davidben 2016/03/24 20:53:51 Just the spec. Sorry, that was unclear. Not a requ

if (rv != OK)

@@ -1563,7 +1587,8 @@ void HttpNetworkTransaction::ResetStateForAuthRestart() {

remote_endpoint_ = IPEndPoint();

net_error_details_.quic_broken = false;

net_error_details_.quic_connection_error = QUIC_NO_ERROR;

- token_binding_key_.reset();

+ provided_token_binding_key_.reset();

+ referred_token_binding_key_.reset();

}

void HttpNetworkTransaction::CacheNetErrorDetailsAndResetStream() {

« no previous file with comments | « net/http/http_network_transaction.h ('k') | net/http/http_network_transaction_ssl_unittest.cc » ('j') | net/ssl/token_binding.h » ('J')