Build and send HPKP violation reports

net/http/transport_security_state.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef NET_HTTP_TRANSPORT_SECURITY_STATE_H_
 #define NET_HTTP_TRANSPORT_SECURITY_STATE_H_
 
 #include <map>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/basictypes.h"

[Ryan Sleevi, 2015/06/26 20:08:58]

This should probably be <stdint.h> (noticed when you added uint16_t below)

[estark, 2015/07/09 21:45:26]

Done.

 #include "base/gtest_prod_util.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/time/time.h"
 #include "net/base/net_export.h"
 #include "net/cert/x509_cert_types.h"
 #include "net/cert/x509_certificate.h"
 
 namespace net {
 
 class SSLInfo;
@@ skipping 14 matching lines @@
   class NET_EXPORT Delegate {
    public:
     // This function may not block and may be called with internal locks held.
     // Thus it must not reenter the TransportSecurityState object.
     virtual void StateIsDirty(TransportSecurityState* state) = 0;
 
    protected:
     virtual ~Delegate() {}
   };
 
-  TransportSecurityState();
-  ~TransportSecurityState();
-
   // A DomainState describes the transport security state (required upgrade
   // to HTTPS, and/or any public key pins).
   //
   // TODO(davidben): STSState and PKPState are queried and processed
   // independently (with the exception of ShouldSSLErrorsBeFatal triggering on
   // both and on-disk storage). DomainState should be split into the
   // two. https://crbug.com/470295.
   class NET_EXPORT DomainState {
    public:
     enum UpgradeMode {
@@ skipping 102 matching lines @@
     bool HasNext() const { return iterator_ != end_; }
     void Advance() { ++iterator_; }
     const std::string& hostname() const { return iterator_->first; }
     const DomainState& domain_state() const { return iterator_->second; }
 
    private:
     std::map<std::string, DomainState>::const_iterator iterator_;
     std::map<std::string, DomainState>::const_iterator end_;
   };
 
+  class NET_EXPORT Reporter {
+   public:
+    virtual ~Reporter() {}

[Ryan Sleevi, 2015/06/26 20:08:58]

Is ownership of the reporter transferred?

If not, this should be made protected - that way implementations can be made
using refcounting (if needed)

[estark, 2015/07/09 21:45:26]

Done.

+
+    // Returns true if a violation report should be sent for the host in
+    // the given |pkp_state|, and returns the report destination URI in
+    // |report_uri|. Returns false if a report should not be sent.
+    virtual bool GetHPKPReportUri(const DomainState::PKPState& pkp_state,
+                                  GURL* report_uri) = 0;

[Ryan Sleevi, 2015/06/26 20:08:58]

It's unclear why this is a property of the Reporter, rather than being something
the TSS handles (at least, from the header)

Without having read anything else (literally, this is the first file I opened),
I suspect this is to allow the Reporter to override the reporting URL for some
state; in which case, we should document that.

Also, note that there may be multiple report URIs - another thing to consider
with this interface.

[Ryan Sleevi, 2015/06/26 20:17:15]

Bah, bad spec language. I can't tell if I'm lying or telling the truth

http://tools.ietf.org/html/rfc7469#section-2.1
   2.  With the exception of pin-directives with the same pin-directive-
       name (see below), a given directive MUST NOT appear more than
       once in a given header field.  Directives are either optional or
       required, as stipulated in their definitions.

   directive             = directive-name [ "=" directive-value ]


This leaves it ambiguous as to whether we're talking directive-name or
directive.

That is, I would expect that

report-uri="http://www.google.com/foo"; report-uri="http://www.google.com/foo"
is invalid

but, as written
report-uri="http://www.google.com/foo"; report-uri="http://www.google.com/bar"
may be seen as valid (because they're the same directive-name, but different
directive-values, ergo different directives)


It may help if you take a look at the spec, since I think I'm too close to it to
recall all the nuance, to see if you can find other language one way or the
other on the interpretation. Chris may recall as well, but language-lawyer'ing
the spec should help. 

[estark, 2015/07/09 21:45:26]

I read Section 2.1 #2 as meaning that there shouldn't be multiple report-uris
(i.e. "directive" is a typo for "directive-name"). For two reasons:

1. If we read "directive" to mean "directive", then that also implies that there
can be multiple max-age directives as long as they have distinct values, which
a.) doesn't make sense (it doesn't seem like multiple max-ages ought to be
allowed), and b.) if it were the intention, it would be a fairly big omission in
the spec to not specify how multiple max-age directives should be handled.

2. The "With the exception of pin-directives" clause wouldn't really appear to
be necessary if we read "directive" to mean "directive". That is, there doesn't
seem to be a need to explicitly allow a pin directive (pin-sha256=blah) to
repeat, whereas there is a need to allow a pin directive name (pin-sha256) to
repeat (namely, pin and backup pin).

So, I'm not opposed to doing multiple report-uris, but if we interpret the spec
to mean that multiple report-uris are allowed, then we should also assume that
multiple max-ages are allowed (and I don't particularly want to implement
multiple max-ages because it seems kinda silly).

[estark, 2015/07/09 21:45:26]

Yes, that's the reason. Done.

+
+    // Builds a serialized HPKP violation report in
+    // |serialized_report|. Returns true on success and false on
+    // failure.
+    virtual bool BuildHPKPReport(
+        const std::string& hostname,
+        uint16_t port,
+        const base::Time& expiry,
+        bool include_subdomains,
+        const std::string& effective_hostname,

[Ryan Sleevi, 2015/06/26 20:08:58]

Should provide a bit of documentation about these parameters - in particular,
this one is a little tricksy

[estark, 2015/07/09 21:45:26]

Done.

+        const scoped_refptr<X509Certificate>& served_certificate_chain,
+        const scoped_refptr<X509Certificate>& validated_certificate_chain,

[Ryan Sleevi, 2015/06/26 20:08:58]

You should be able to pass these just as naked X509Certificate*'s here - this is
a synchronous call, and you're not requiring that the callee take ownership, so
it's fine to pass the raw pointers.

[estark, 2015/07/09 21:45:26]

Done.

+        const HashValueVector& spki_hashes,
+        std::string* serialized_report) = 0;
+
+    // Sends the given serialized |report| to |report_uri|.
+    virtual void SendHPKPReport(const GURL& report_uri,
+                                const std::string& report) = 0;
+  };
+
+  TransportSecurityState();
+  ~TransportSecurityState();
+
   // These functions search for static and dynamic DomainStates, and invoke the
   // functions of the same name on them. These functions are the primary public
   // interface; direct access to DomainStates is best left to tests.
   bool ShouldSSLErrorsBeFatal(const std::string& host);
   bool ShouldUpgradeToSSL(const std::string& host);
   bool CheckPublicKeyPins(const std::string& host,
                           bool is_issued_by_known_root,
                           const HashValueVector& hashes,
                           std::string* failure_log);
   bool HasPublicKeyPins(const std::string& host);
 
   // Assign a |Delegate| for persisting the transport security state. If
   // |NULL|, state will not be persisted. The caller retains
   // ownership of |delegate|.
   // Note: This is only used for serializing/deserializing the
   // TransportSecurityState.
   void SetDelegate(Delegate* delegate);
 
+  void SetReporter(Reporter* reporter);
+
   // Clears all dynamic data (e.g. HSTS and HPKP data).
   //
   // Does NOT persist changes using the Delegate, as this function is only
   // used to clear any dynamic data prior to re-loading it from a file.
   // Note: This is only used for serializing/deserializing the
   // TransportSecurityState.
   void ClearDynamicData();
 
   // Inserts |state| into |enabled_hosts_| under the key |hashed_host|.
   // |hashed_host| is already in the internal representation.
@@ skipping 123 matching lines @@
   // The new state for |host| is persisted using the Delegate (if any).
   void EnableHost(const std::string& host, const DomainState& state);
 
   // The set of hosts that have enabled TransportSecurity. |sts.domain| and
   // |pkp.domain| will always be empty for a DomainState in this map; the domain
   // comes from the map key instead.
   DomainStateMap enabled_hosts_;
 
   Delegate* delegate_;
 
+  Reporter* reporter_;
+
   // True if static pins should be used.
   bool enable_static_pins_;
 
   DISALLOW_COPY_AND_ASSIGN(TransportSecurityState);
 };
 
 }  // namespace net
 
 #endif  // NET_HTTP_TRANSPORT_SECURITY_STATE_H_