Skip to content

Commit

Permalink
Cleanup and minor refactoring of LogDnsClient
Browse files Browse the repository at this point in the history 
Given that a MerkleAuditProof only proves an inclusion path for a tree of a specific size, a tree_size parameter was added to it in  https://crrev.com/89f010d82559c73e7d23cbf067137ceb1df698df to keep track of which tree the proof was for. Because of this, it's no longer necessary to store the tree size separately in LogDnsClient::AuditProofQuery. Instead, it can just use the tree_size member of the MerkleAuditProof.

The QueryAuditProof method still takes the tree_size as a separate parameter, so as to not make the proof parameter a mixed in/out parameter, which could be confusing.

BUG=624894

Review-Url: https://codereview.chromium.org/2485653002
Cr-Commit-Position: refs/heads/master@{#430604}
  • Loading branch information
robpercival authored and Commit bot committed Nov 8, 2016
1 parent e8b43a4 commit 5bfec4f
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 37 deletions.
39 changes: 18 additions & 21 deletions components/certificate_transparency/log_dns_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,18 +107,18 @@ class LogDnsClient::AuditProofQuery {
// to leave ownership of |dns_client| with LogDnsClient.
AuditProofQuery(net::DnsClient* dns_client,
const std::string& domain_for_log,
uint64_t tree_size,
const net::NetLogWithSource& net_log);

// Begins the process of getting an audit |proof| for the CT log entry with a
// leaf hash of |leaf_hash|. If it cannot be obtained synchronously,
// net::ERR_IO_PENDING will be returned and |callback| will be invoked when
// the operation has completed asynchronously.
// Ownership of |proof| remains with the caller, and it must not be deleted
// until the operation is complete.
// Begins the process of getting an audit proof for the CT log entry with a
// leaf hash of |leaf_hash|. The proof will be for a tree of size |tree_size|.
// If it cannot be obtained synchronously, net::ERR_IO_PENDING will be
// returned and |callback| will be invoked when the operation has completed
// asynchronously. Ownership of |proof| remains with the caller, and it must
// not be deleted until the operation is complete.
net::Error Start(std::string leaf_hash,
uint64_t tree_size,
const net::CompletionCallback& callback,
net::ct::MerkleAuditProof* proof);
net::ct::MerkleAuditProof* out_proof);

private:
enum class State {
Expand Down Expand Up @@ -178,9 +178,6 @@ class LogDnsClient::AuditProofQuery {
std::string domain_for_log_;
// The Merkle leaf hash of the CT log entry an audit proof is required for.
std::string leaf_hash_;
// The size of the CT log's tree, from which the proof is requested.
// TODO(robpercival): Remove |tree_size| once |proof_| has a tree_size member.
uint64_t tree_size_;
// The audit proof to populate.
net::ct::MerkleAuditProof* proof_;
// The callback to invoke when the query is complete.
Expand All @@ -201,11 +198,9 @@ class LogDnsClient::AuditProofQuery {
LogDnsClient::AuditProofQuery::AuditProofQuery(
net::DnsClient* dns_client,
const std::string& domain_for_log,
uint64_t tree_size,
const net::NetLogWithSource& net_log)
: next_state_(State::NONE),
domain_for_log_(domain_for_log),
tree_size_(tree_size),
dns_client_(dns_client),
net_log_(net_log),
weak_ptr_factory_(this) {
Expand All @@ -217,11 +212,13 @@ LogDnsClient::AuditProofQuery::AuditProofQuery(
// the method, avoiding the need to make a copy.
net::Error LogDnsClient::AuditProofQuery::Start(
std::string leaf_hash,
uint64_t tree_size,
const net::CompletionCallback& callback,
net::ct::MerkleAuditProof* proof) {
// It should not already be in progress.
DCHECK_EQ(State::NONE, next_state_);
proof_ = proof;
proof_->tree_size = tree_size;
leaf_hash_ = std::move(leaf_hash);
callback_ = callback;
// The first step in the query is to request the leaf index corresponding to
Expand Down Expand Up @@ -312,7 +309,7 @@ net::Error LogDnsClient::AuditProofQuery::RequestLeafIndexComplete(
// b) the wrong leaf hash was provided.
// c) there is a bug server-side.
// The first two are more likely, so return ERR_INVALID_ARGUMENT.
if (proof_->leaf_index >= tree_size_) {
if (proof_->leaf_index >= proof_->tree_size) {
return net::ERR_INVALID_ARGUMENT;
}

Expand All @@ -322,15 +319,15 @@ net::Error LogDnsClient::AuditProofQuery::RequestLeafIndexComplete(

net::Error LogDnsClient::AuditProofQuery::RequestAuditProofNodes() {
// Test pre-conditions (should be guaranteed by DNS response validation).
if (proof_->leaf_index >= tree_size_ ||
proof_->nodes.size() >=
net::ct::CalculateAuditPathLength(proof_->leaf_index, tree_size_)) {
if (proof_->leaf_index >= proof_->tree_size ||
proof_->nodes.size() >= net::ct::CalculateAuditPathLength(
proof_->leaf_index, proof_->tree_size)) {
return net::ERR_UNEXPECTED;
}

std::string qname = base::StringPrintf(
"%zu.%" PRIu64 ".%" PRIu64 ".tree.%s.", proof_->nodes.size(),
proof_->leaf_index, tree_size_, domain_for_log_.c_str());
proof_->leaf_index, proof_->tree_size, domain_for_log_.c_str());

if (!StartDnsTransaction(qname)) {
return net::ERR_NAME_RESOLUTION_FAILED;
Expand All @@ -347,7 +344,7 @@ net::Error LogDnsClient::AuditProofQuery::RequestAuditProofNodesComplete(
}

const uint64_t audit_path_length =
net::ct::CalculateAuditPathLength(proof_->leaf_index, tree_size_);
net::ct::CalculateAuditPathLength(proof_->leaf_index, proof_->tree_size);

// The calculated |audit_path_length| can't ever be greater than 64, so
// deriving the amount of memory to reserve from the untrusted |leaf_index|
Expand Down Expand Up @@ -432,11 +429,11 @@ net::Error LogDnsClient::QueryAuditProof(
}

AuditProofQuery* query = new AuditProofQuery(
dns_client_.get(), domain_for_log.as_string(), tree_size, net_log_);
dns_client_.get(), domain_for_log.as_string(), net_log_);
// Transfers ownership of |query| to |audit_proof_queries_|.
audit_proof_queries_.emplace_back(query);

return query->Start(std::move(leaf_hash),
return query->Start(std::move(leaf_hash), tree_size,
base::Bind(&LogDnsClient::QueryAuditProofComplete,
weak_ptr_factory_.GetWeakPtr(),
base::Unretained(query), callback),
Expand Down
6 changes: 2 additions & 4 deletions components/certificate_transparency/log_dns_client.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class LogDnsClient : public net::NetworkChangeNotifier::DNSObserver {
// The size of the CT log tree, for which the proof is requested, must be
// provided in |tree_size|.
// The leaf index and audit proof obtained from the CT log will be placed in
// |proof|.
// |out_proof|.
// If the proof cannot be obtained synchronously, this method will return
// net::ERR_IO_PENDING and invoke |callback| once the query is complete.
// Returns:
Expand All @@ -78,16 +78,14 @@ class LogDnsClient : public net::NetworkChangeNotifier::DNSObserver {
// continuing asynchronously.
// - net::ERR_TEMPORARILY_THROTTLED if the maximum number of concurrent
// queries are already in progress. Try again later.
// TODO(robpercival): Provide a mechanism to notify the caller when no
// longer throttled.
// - net::ERR_NAME_RESOLUTION_FAILED if DNS queries are not possible.
// Check that the DnsConfig returned by NetworkChangeNotifier is valid.
// - net::ERR_INVALID_ARGUMENT if an argument is invalid, e.g. |leaf_hash| is
// not a SHA-256 hash.
net::Error QueryAuditProof(base::StringPiece domain_for_log,
std::string leaf_hash,
uint64_t tree_size,
net::ct::MerkleAuditProof* proof,
net::ct::MerkleAuditProof* out_proof,
const net::CompletionCallback& callback);

private:
Expand Down
18 changes: 6 additions & 12 deletions components/certificate_transparency/log_dns_client_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,8 +289,7 @@ TEST_P(LogDnsClientTest, QueryAuditProof) {
ASSERT_THAT(QueryAuditProof("ct.test", kLeafHashes[0], 999999, &proof),
IsOk());
EXPECT_THAT(proof.leaf_index, Eq(123456u));
// TODO(robpercival): Enable this once MerkleAuditProof has tree_size.
// EXPECT_THAT(proof.tree_size, Eq(999999));
EXPECT_THAT(proof.tree_size, Eq(999999u));
EXPECT_THAT(proof.nodes, Eq(audit_proof));
}

Expand Down Expand Up @@ -325,8 +324,7 @@ TEST_P(LogDnsClientTest, QueryAuditProofHandlesResponsesWithShortAuditPaths) {
ASSERT_THAT(QueryAuditProof("ct.test", kLeafHashes[0], 999999, &proof),
IsOk());
EXPECT_THAT(proof.leaf_index, Eq(123456u));
// TODO(robpercival): Enable this once MerkleAuditProof has tree_size.
// EXPECT_THAT(proof.tree_size, Eq(999999));
EXPECT_THAT(proof.tree_size, Eq(999999u));
EXPECT_THAT(proof.nodes, Eq(audit_proof));
}

Expand Down Expand Up @@ -570,8 +568,7 @@ TEST_P(LogDnsClientTest, AdoptsLatestDnsConfigMidQuery) {
// The DNS config should be updated during this time.
ASSERT_THAT(callback.WaitForResult(), IsOk());
EXPECT_THAT(proof.leaf_index, Eq(123456u));
// TODO(robpercival): Enable this once MerkleAuditProof has tree_size.
// EXPECT_THAT(proof.tree_size, Eq(999999));
EXPECT_THAT(proof.tree_size, Eq(999999u));
EXPECT_THAT(proof.nodes, Eq(audit_proof));

// Check that the DNS config change was adopted.
Expand Down Expand Up @@ -654,8 +651,7 @@ TEST_P(LogDnsClientTest, CanPerformQueriesInParallel) {
SCOPED_TRACE(testing::Message() << "callbacks[" << i << "]");
EXPECT_THAT(callback.WaitForResult(), IsOk());
EXPECT_THAT(proofs[i].leaf_index, Eq(kLeafIndices[i]));
// TODO(robpercival): Enable this once MerkleAuditProof has tree_size.
// EXPECT_THAT(proofs[i].tree_size, kTreeSizes[i]);
EXPECT_THAT(proofs[i].tree_size, Eq(kTreeSizes[i]));
EXPECT_THAT(proofs[i].nodes, Eq(audit_proofs[i]));
}
}
Expand Down Expand Up @@ -704,8 +700,7 @@ TEST_P(LogDnsClientTest, CanBeThrottledToOneQueryAtATime) {
// Check that the first query succeeded.
EXPECT_THAT(callback1.WaitForResult(), IsOk());
EXPECT_THAT(proof1.leaf_index, Eq(123456u));
// TODO(robpercival): Enable this once MerkleAuditProof has tree_size.
// EXPECT_THAT(proof1.tree_size, Eq(999999));
EXPECT_THAT(proof1.tree_size, Eq(999999u));
EXPECT_THAT(proof1.nodes, Eq(audit_proof));

// Try a third query, which should succeed now that the first is finished.
Expand All @@ -730,8 +725,7 @@ TEST_P(LogDnsClientTest, CanBeThrottledToOneQueryAtATime) {
// Check that the third query succeeded.
EXPECT_THAT(callback3.WaitForResult(), IsOk());
EXPECT_THAT(proof3.leaf_index, Eq(666u));
// TODO(robpercival): Enable this once MerkleAuditProof has tree_size.
// EXPECT_THAT(proof3.tree_size, Eq(999999));
EXPECT_THAT(proof3.tree_size, Eq(999999u));
EXPECT_THAT(proof3.nodes, Eq(audit_proof));
}

Expand Down

0 comments on commit 5bfec4f

Please sign in to comment.