Return enum from TransportSecurityState::CheckPublicKeyPins

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 <stdint.h>
 
 #include <map>
@@ skipping 89 matching lines @@
     bool HasNext() const { return iterator_ != end_; }
     void Advance() { ++iterator_; }
     const std::string& hostname() const { return iterator_->first; }
     const STSState& domain_state() const { return iterator_->second; }
 
    private:
     std::map<std::string, STSState>::const_iterator iterator_;
     std::map<std::string, STSState>::const_iterator end_;
   };
 
+  // PKPStatus describes the result of a pinning check.
+  enum class PKPStatus { VIOLATED = -1, OK = 0, BYPASSED = 1 };

[Ryan Sleevi, 2016/06/15 02:04:03]

STYLE: No need to assign explicit values.
STYLE: No benefit from making it an "enum class" here because it doesn't allow
forward declaration
STYLE: Add documentation explaining what these statuses mean.

Example

enum PKPStatus {
  // Pinning was enabled and the necessary pins were not present.
  VIOLATED,

  // Pinning was not enabled, or it was and the certificate satisfied
  // the pins.
  OK,

  // Pinning was enabled, but ignored due to local policy, such as
  // a local trust anchor.
  BYPASSED,
};

NAMING: Is "violated" the right status here? RFC 7469 doesn't call it violated,
nor does our code (I guess, not until report_uri was introduced). In the context
of "CheckPublicKeyPins", what are natural ways of reading the possible results?

[dadrian, 2016/06/15 02:47:55]

I went with violated because that's what I've heard in conversation, both in and
out of Google, although I have no particular attachment to the name.

[dadrian, 2016/06/15 18:58:28]

I left it with enum class because I think we want the scoping benefits?

[Ryan Sleevi, 2016/06/15 19:02:23]

Scoping benefits? We don't gain anything in terms of scoping/naming resolution,
since we're a nested class, just in type safety. Was that what you meant?

[dadrian, 2016/06/15 19:33:16]

Both? We're trying to represent the result of PKP actions, not
TransportSecurityState as a whole, so saying TransportSecurityState::OK (vs
TransportSecurityState::PKPStatus::OK) shouldn't have any meaning unless we
convert every TransportSecurityState operation to use the same enum.

+
   // A PKPState describes the public key pinning state.
   class NET_EXPORT PKPState {
    public:
     PKPState();
     PKPState(const PKPState& other);
     ~PKPState();
 
     // The absolute time (UTC) when the |spki_hashes| (and other state) were
     // observed.
     base::Time last_observed;
@@ skipping 132 matching lines @@
   TransportSecurityState();
   ~TransportSecurityState();
 
   // These functions search for static and dynamic STS and PKP states, and
   // invoke the functions of the same name on them. These functions are the
   // primary public interface; direct access to STS and PKP states is best
   // left to tests. The caller needs to handle the optional pinning override
   // when is_issued_by_known_root is false.
   bool ShouldSSLErrorsBeFatal(const std::string& host);
   bool ShouldUpgradeToSSL(const std::string& host);
-  bool CheckPublicKeyPins(const HostPortPair& host_port_pair,
-                          bool is_issued_by_known_root,
-                          const HashValueVector& hashes,
-                          const X509Certificate* served_certificate_chain,
-                          const X509Certificate* validated_certificate_chain,
-                          const PublicKeyPinReportStatus report_status,
-                          std::string* failure_log);
+  PKPStatus CheckPublicKeyPins(
+      const HostPortPair& host_port_pair,
+      bool is_issued_by_known_root,
+      const HashValueVector& hashes,
+      const X509Certificate* served_certificate_chain,
+      const X509Certificate* validated_certificate_chain,
+      const PublicKeyPinReportStatus report_status,
+      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 SetReportSender(ReportSenderInterface* report_sender);
@@ skipping 125 matching lines @@
   // representation of first-class DomainStates, and exposing the preloads
   // to the caller with |GetStaticDomainState|.
   static void ReportUMAOnPinFailure(const std::string& host);
 
   // IsBuildTimely returns true if the current build is new enough ensure that
   // built in security information (i.e. HSTS preloading and pinning
   // information) is timely.
   static bool IsBuildTimely();
 
   // Helper method for actually checking pins.
-  bool CheckPublicKeyPinsImpl(
+  PKPStatus CheckPublicKeyPinsImpl(
       const HostPortPair& host_port_pair,
       bool is_issued_by_known_root,
       const HashValueVector& hashes,
       const X509Certificate* served_certificate_chain,
       const X509Certificate* validated_certificate_chain,
       const PublicKeyPinReportStatus report_status,
       std::string* failure_log);
 
   // If a Delegate is present, notify it that the internal state has
   // changed.
@@ skipping 20 matching lines @@
   void EnableSTSHost(const std::string& host, const STSState& state);
   void EnablePKPHost(const std::string& host, const PKPState& state);
 
   // Returns true if a request to |host_port_pair| with the given
   // SubjectPublicKeyInfo |hashes| satisfies the pins in |pkp_state|,
   // and false otherwise. If a violation is found and reporting is
   // configured (i.e. there is a report URI in |pkp_state| and
   // |report_status| says to), this method sends an HPKP violation
   // report containing |served_certificate_chain| and
   // |validated_certificate_chain|.
-  bool CheckPinsAndMaybeSendReport(
+  PKPStatus CheckPinsAndMaybeSendReport(
       const HostPortPair& host_port_pair,
       bool is_issued_by_known_root,
       const TransportSecurityState::PKPState& pkp_state,
       const HashValueVector& hashes,
       const X509Certificate* served_certificate_chain,
       const X509Certificate* validated_certificate_chain,
       const TransportSecurityState::PublicKeyPinReportStatus report_status,
       std::string* failure_log);
 
   // Returns true and updates |*expect_ct_result| iff there is a static
@@ skipping 36 matching lines @@
   // rate-limiting.
   ExpiringCache<std::string, bool, base::TimeTicks, std::less<base::TimeTicks>>
       sent_reports_cache_;
 
   DISALLOW_COPY_AND_ASSIGN(TransportSecurityState);
 };
 
 }  // namespace net
 
 #endif  // NET_HTTP_TRANSPORT_SECURITY_STATE_H_