Relanding: Allow the DTLS fingerprint verification to occur after the handshake.

webrtc/base/opensslstreamadapter.h

 /*
  *  Copyright 2004 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 89 matching lines @@
                             const uint8_t* context,
                             size_t context_len,
                             bool use_context,
                             uint8_t* result,
                             size_t result_len) override;
 
   // DTLS-SRTP interface
   bool SetDtlsSrtpCryptoSuites(const std::vector<int>& crypto_suites) override;
   bool GetDtlsSrtpCryptoSuite(int* crypto_suite) override;
 
+  bool IsTlsConnected() override { return state_ == SSL_CONNECTED; }
+
   // Capabilities interfaces
   static bool HaveDtls();
   static bool HaveDtlsSrtp();
   static bool HaveExporter();
   static bool IsBoringSsl();
 
   static bool IsAcceptableCipher(int cipher, KeyType key_type);
   static bool IsAcceptableCipher(const std::string& cipher, KeyType key_type);
 
  protected:
   void OnEvent(StreamInterface* stream, int events, int err) override;
 
  private:
   enum SSLState {
     // Before calling one of the StartSSL methods, data flows
     // in clear text.
     SSL_NONE,
     SSL_WAIT,  // waiting for the stream to open to start SSL negotiation
     SSL_CONNECTING,  // SSL negotiation in progress
     SSL_CONNECTED,  // SSL stream successfully established
     SSL_ERROR,  // some SSL error occurred, stream is closed
     SSL_CLOSED  // Clean close
   };
 
   enum { MSG_TIMEOUT = MSG_MAX+1};
 
+  enum SSLTopology {

[mattdr-at-webrtc.org, 2016/07/21 08:04:23]

SSL really only has one topology: client-server. The real question we're
addressing is how to validate the other side's certificate.

Fun question: if we're the *server* in a "traditional" / "client-server" pairing
and enable_client_auth() is true, what function will we call to set the other
side's certificate? Which of these "topologies" would we choose?

I think we'll want to drop this enum, for these reasons and those described
below.

[Taylor Brandstetter, 2016/07/22 18:52:20]

Enum dropped, as discussed.

[pthatcher1, 2016/07/22 18:58:27]

Nothing every uses StartSSLWithServer any more, so we could probably simplify
here a lot.

[Taylor Brandstetter, 2016/07/25 23:54:53]

Can we remove StartSSLWithServer in a separate CL, to keep this one focused?

+    TOPOLOGY_CLIENT_SERVER,
+    TOPOLOGY_PEER_TO_PEER,
+    TOPOLOGY_UNKNOWN
+  };
+
   // The following three methods return 0 on success and a negative
   // error code on failure. The error code may be from OpenSSL or -1
   // on some other error cases, so it can't really be interpreted
   // unfortunately.
 
   // Go from state SSL_NONE to either SSL_CONNECTING or SSL_WAIT,
   // depending on whether the underlying stream is already open or
   // not.
   int StartSSL();
   // Prepare SSL library, state is SSL_CONNECTING.
@@ skipping 12 matching lines @@
 
   // Override MessageHandler
   void OnMessage(Message* msg) override;
 
   // Flush the input buffers by reading left bytes (for DTLS)
   void FlushInput(unsigned int left);
 
   // SSL library configuration
   SSL_CTX* SetupSSLContext();
   // SSL verification check
-  bool SSLPostConnectionCheck(SSL* ssl, const char* server_name,
-                              const X509* peer_cert,
-                              const std::string& peer_digest);
+  bool SSLPostConnectionCheck();
+  bool VerifyPeerCertificate();
   // SSL certification verification error handler, called back from
   // the openssl library. Returns an int interpreted as a boolean in
   // the C style: zero means verification failure, non-zero means
   // passed.
   static int SSLVerifyCallback(int ok, X509_STORE_CTX* store);
 
+  bool waiting_to_verify_client_cert() const {
+    // If |ssl_server_name_| is non-empty we're in client/server mode and don't
+    // need to verify a peer certificate.
+    return ssl_server_name_.empty() && client_auth_enabled() &&

[pthatcher1, 2016/07/22 18:58:27]

set_client_auth_enabled() is never called by anything other than unit tests, and
thus client_auth_enabled_ is always true, so we could probably just remove it.

If we remove that and StartSSLWithServer, then this whole method would be
!certificate_verified_, so you could just make it certificate_verified(), or
better yet, remote_certificate_verified().

[Taylor Brandstetter, 2016/07/25 23:54:53]

See above.

+           !certificate_verified_;
+  }
+
+  bool have_peer_certificate_digest() const {
+    return peer_certificate_digest_algorithm_.size() &&

[mattdr-at-webrtc.org, 2016/07/21 08:04:23]

personal preference: !xyz.empty()

[Taylor Brandstetter, 2016/07/22 18:52:20]

rtc::Buffer didn't have an "empty" method so I added one.

[pthatcher1, 2016/07/22 18:58:27]

+1

+           peer_certificate_digest_value_.size();
+  }
+
   SSLState state_;
   SSLRole role_;
   int ssl_error_code_;  // valid when state_ == SSL_ERROR or SSL_CLOSED
   // Whether the SSL negotiation is blocked on needing to read or
   // write to the wrapped stream.
   bool ssl_read_needs_write_;
   bool ssl_write_needs_read_;
 
   SSL* ssl_;
   SSL_CTX* ssl_ctx_;
 
+  SSLTopology topology_ = TOPOLOGY_UNKNOWN;

[mattdr-at-webrtc.org, 2016/07/21 08:04:23]

It's nice to be explicit, but in a way this is duplicative state.

We're in TOPOLOGY_UNKNOWN iff state_ is SSL_NONE (thanks to StartSSL). If state_
is not SSL_NONE, then we're in TOPOLOGY_CLIENT_SERVER iff server_name_ is
nonnull. Thus, we can always compute this from existing fields.

Fields with redundant state are begging to get out of sync and break invariants.
:)

[Taylor Brandstetter, 2016/07/22 18:52:20]

You're very right. I replaced every "topology" check with a method that returns
a bool. I can have one method that returns an enum instead if you prefer that
flavor.

   // Our key and certificate, mostly useful in peer-to-peer mode.
   std::unique_ptr<OpenSSLIdentity> identity_;
   // in traditional mode, the server name that the server's certificate
   // must specify. Empty in peer-to-peer mode.
   std::string ssl_server_name_;

[pthatcher1, 2016/07/22 18:58:27]

Since this is only set by StartSSLWithServer, and since StartSSLWithServer was
only ever used by the XMPP code, which is now gone, I think it would simplify
things a lot to just remove this and remove different modes and just have
"peer-to-peer mode", which is all we really care about.  

[Taylor Brandstetter, 2016/07/25 23:54:53]

See above.

   // The certificate that the peer must present or did present. Initially
   // null in traditional mode, until the connection is established.
   std::unique_ptr<OpenSSLCertificate> peer_certificate_;
   // In peer-to-peer mode, the digest of the certificate that
   // the peer must present.
   Buffer peer_certificate_digest_value_;
   std::string peer_certificate_digest_algorithm_;
+  bool certificate_verified_ = false;

[pthatcher1, 2016/07/22 18:58:27]

Since the method that changes this is VerifyPeerCertificate and the certificate
is called peer_certificate_, shouldn't this be peer_certificate_verified_? 
Also, can you move this to right below peer_certificate_?

[Taylor Brandstetter, 2016/07/25 23:54:53]

Done.

 
   // OpenSSLAdapter::custom_verify_callback_ result

[pthatcher1, 2016/07/22 18:58:27]

Would it make sense to make everything "remote" as in "remote_certificate_"
instead of "peer" as in "peer_certificate_"?  That would be a lot more
consistent with all our other code.

[Taylor Brandstetter, 2016/07/25 23:54:53]

What about the PeerConnection? :)

Anyway you're probably right, but I think the renaming can happen in a later CL.

   bool custom_verification_succeeded_;
 
   // The DtlsSrtp ciphers
   std::string srtp_ciphers_;
 
   // Do DTLS or not
   SSLMode ssl_mode_;
 
   // Max. allowed protocol version
   SSLProtocolVersion ssl_max_version_;
 };
 
 /////////////////////////////////////////////////////////////////////////////
 
 }  // namespace rtc
 
 #endif  // WEBRTC_BASE_OPENSSLSTREAMADAPTER_H__