Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2163683003:
Relanding: Allow the DTLS fingerprint verification to occur after the handshake. (Closed)
|
Created:
4 years, 5 months ago by Taylor Brandstetter Modified:
4 years, 2 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRelanding: Allow the DTLS fingerprint verification to occur after the handshake.
This means the DTLS handshake can make progress while the SDP answer
containing the fingerprint is still in transit. If the signaling path
if significantly slower than the media path, this can have a moderate
impact on call setup time.
Of course, until the fingerprint is verified no media can be sent. Any
attempted write will result in SR_BLOCK.
This essentially fulfills the requirements of RFC 4572, Section 6.2:
Note that when the offer/answer model is being used, it is possible
for a media connection to outrace the answer back to the offerer.
Thus, if the offerer has offered a 'setup:passive' or 'setup:actpass'
role, it MUST (as specified in RFC 4145 [2]) begin listening for an
incoming connection as soon as it sends its offer. However, it MUST
NOT assume that the data transmitted over the TLS connection is valid
until it has received a matching fingerprint in an SDP answer. If
the fingerprint, once it arrives, does not match the client's
certificate, the server endpoint MUST terminate the media connection
with a bad_certificate error, as stated in the previous paragraph.
BUG=webrtc:6387
Committed: https://crrev.com/89824f6fe05b43876550812c35195cf0f1237ccc
Cr-Commit-Position: refs/heads/master@{#14461}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixing comment line wrapping. #
Total comments: 20
Patch Set 3 : Code cleanup based on comments from Matt. Setting DTLS state to "failed" on bad fingerprint. #
Total comments: 23
Patch Set 4 : Responding to second round of comments. #Patch Set 5 : Responding to Peter's comments. #
Total comments: 21
Patch Set 6 : Adding a bunch more tests, wiring up fatal alerts. #Patch Set 7 : Removing files that were caught by "git merge master" and "git cl format". #Patch Set 8 : . #Patch Set 9 : Merge with master. #Patch Set 10 : Getting rid of obsolete unit test (someone else added tests). #Patch Set 11 : Rename "NO_ERROR" to "NONE", since "NO_ERROR" is a Windows #define... #Patch Set 12 : Making SetPeerCertificateDigest's new signature backwards compatible. Also adding unit tests. #
Total comments: 6
Patch Set 13 : Fix name of method. #Patch Set 14 : Fixing comment grammar. #
Messages
Total messages: 56 (24 generated)
deadbeef@webrtc.org changed reviewers: + mattdr@webrtc.org, pthatcher@webrtc.org
Peter: Please take a look. Matt: I realize you're unfamiliar with this code, but I thought you'd be the best person to comment on the overall concept. Review as much as you feel comfortable, or we can just talk about it when Peter's back. https://codereview.webrtc.org/2163683003/diff/1/webrtc/p2p/base/dtlstransport... File webrtc/p2p/base/dtlstransportchannel_unittest.cc (left): https://codereview.webrtc.org/2163683003/diff/1/webrtc/p2p/base/dtlstransport... webrtc/p2p/base/dtlstransportchannel_unittest.cc:946: TEST_F(DtlsTransportChannelTest, TestReceiveClientHelloBeforeWritable) { These tests are now just two of the possible permutations covered by TestEventPermutation. They're functionally equivalent to permutations 5 and 6. https://codereview.webrtc.org/2163683003/diff/1/webrtc/p2p/base/dtlstransport... File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right): https://codereview.webrtc.org/2163683003/diff/1/webrtc/p2p/base/dtlstransport... webrtc/p2p/base/dtlstransportchannel_unittest.cc:1030: TEST_F(DtlsTransportChannelTest, EventPermutation1) { "PermutationN" isn't the best name I realize. If you have a better suggestion, feel free, but "TestAThenBThenCThenD" seemed too verbose.
https://codereview.webrtc.org/2163683003/diff/20001/webrtc/base/opensslstream... File webrtc/base/opensslstreamadapter.cc (right): https://codereview.webrtc.org/2163683003/diff/20001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:340: Error("SetPeerCertificateDigest", -1, false); instead of -1, X509_V_ERR_CERT_UNTRUSTED might be reasonable. Should the "signal" argument here be true? https://codereview.webrtc.org/2163683003/diff/20001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:524: if (client_auth_enabled() && !certificate_verified_) { We repeat this expression more than a few times. Maybe give it a name? if (waiting_for_client_cert()) ... https://codereview.webrtc.org/2163683003/diff/20001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:855: // If the certificate is not verified (because we're in peer-to-peer Make it a bit more obvious here that you're talking about what *isn't* happening here. e.g. We have everything we need to start the connection, so signal SE_OPEN. If we need a client certificate fingerprint and don't have it yet, we'll instead signal SE_OPEN in SetPeerCertificateDigest. https://codereview.webrtc.org/2163683003/diff/20001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:1123: if (stream->peer_certificate_digest_algorithm_.empty()) { do we really want to use the digest *algorithm* variable everywhere as a sentinel to whether we have the *digest*? The former would be something like "SHA256", while the latter will be a binary or hex string. https://codereview.webrtc.org/2163683003/diff/20001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:1128: return stream->VerifyPeerCertificate() ? 1 : 0; bool coerces to 1 or 0, so this should be redundant. https://codereview.webrtc.org/2163683003/diff/20001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:1135: const X509* peer_cert) { This function doesn't really need the peer certificate; it just needs to know if we *have* a peer certificate. Maybe consider a bool? Then again, it seems like the class assumes an invariant where either we get a real server name or we're using peer-to-peer auth and have a peer cert. Is that the case? https://codereview.webrtc.org/2163683003/diff/20001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:1143: custom_verification_succeeded_); Let's avoid chained assignment. In fact, I'd prefer if rely on the *caller* of SSLPostConnectionCheck to set certificate_verified_. Setting this member variable is sort of a nonobvious side effect of calling this. https://codereview.webrtc.org/2163683003/diff/20001/webrtc/p2p/base/dtlstrans... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/2163683003/diff/20001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel.cc:259: LOG_J(LS_ERROR, this) << "Couldn't set DTLS certificate digest."; should we set DTLS state here too? https://codereview.webrtc.org/2163683003/diff/20001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel.cc:506: if (!dtls_ && local_certificate_) { What does checking local_certificate protect against? https://codereview.webrtc.org/2163683003/diff/20001/webrtc/p2p/base/dtlstrans... File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right): https://codereview.webrtc.org/2163683003/diff/20001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel_unittest.cc:1027: // The tests below cover all possible permutations of these events, where by "all possible" you mean "all causally consistent", since 3 must precede 4.
Many thanks for the prompt and valuable feedback! By the way, so I don't forget, "NOT LGTM" until I extend the unit tests as mentioned below. https://codereview.webrtc.org/2163683003/diff/20001/webrtc/base/opensslstream... File webrtc/base/opensslstreamadapter.cc (right): https://codereview.webrtc.org/2163683003/diff/20001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:340: Error("SetPeerCertificateDigest", -1, false); On 2016/07/20 00:50:27, mattdr-at-webrtc.org wrote: > instead of -1, X509_V_ERR_CERT_UNTRUSTED might be reasonable. > > Should the "signal" argument here be true? No, in this case the caller can check the return value so no signal is necessary. The signal seems to be meant for asynchronous events. https://codereview.webrtc.org/2163683003/diff/20001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:524: if (client_auth_enabled() && !certificate_verified_) { On 2016/07/20 00:50:27, mattdr-at-webrtc.org wrote: > We repeat this expression more than a few times. Maybe give it a name? > > if (waiting_for_client_cert()) ... Good idea, done. https://codereview.webrtc.org/2163683003/diff/20001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:855: // If the certificate is not verified (because we're in peer-to-peer On 2016/07/20 00:50:27, mattdr-at-webrtc.org wrote: > Make it a bit more obvious here that you're talking about what *isn't* happening > here. > > e.g. > > We have everything we need to start the connection, so signal SE_OPEN. If we > need a client certificate fingerprint and don't have it yet, we'll instead > signal SE_OPEN in SetPeerCertificateDigest. Done. https://codereview.webrtc.org/2163683003/diff/20001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:1123: if (stream->peer_certificate_digest_algorithm_.empty()) { On 2016/07/20 00:50:27, mattdr-at-webrtc.org wrote: > do we really want to use the digest *algorithm* variable everywhere as a > sentinel to whether we have the *digest*? > > The former would be something like "SHA256", while the latter will be a binary > or hex string. Both should always be set at the same time. But this could change in the future, so I added a "have_peer_certificate_digest" function. https://codereview.webrtc.org/2163683003/diff/20001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:1128: return stream->VerifyPeerCertificate() ? 1 : 0; On 2016/07/20 00:50:27, mattdr-at-webrtc.org wrote: > bool coerces to 1 or 0, so this should be redundant. I always forget this. Done. https://codereview.webrtc.org/2163683003/diff/20001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:1135: const X509* peer_cert) { On 2016/07/20 00:50:27, mattdr-at-webrtc.org wrote: > This function doesn't really need the peer certificate; it just needs to know if > we *have* a peer certificate. Maybe consider a bool? > > Then again, it seems like the class assumes an invariant where either we get a > real server name or we're using peer-to-peer auth and have a peer cert. Is that > the case? In fact I'm not sure why this method takes any parameters since they're all member variables. Maybe historical reasons. I went ahead and cleaned it up. As for the invariant: it definitely seems to be the case, though it's only strictly enforced by ASSERTs. If you call "StartSSLWithServer" and then "StartSSLWithPeer" and asserts are disabled you'll be left in a weird state. I went ahead and made the enforcement (and "which topology am I using" check) a bit more explicit. https://codereview.webrtc.org/2163683003/diff/20001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:1143: custom_verification_succeeded_); On 2016/07/20 00:50:27, mattdr-at-webrtc.org wrote: > Let's avoid chained assignment. > > In fact, I'd prefer if rely on the *caller* of SSLPostConnectionCheck to set > certificate_verified_. Setting this member variable is sort of a nonobvious side > effect of calling this. This is no longer necessary, since I have "waiting_to_verify_client_cert()" now which returns false in client/server mode. https://codereview.webrtc.org/2163683003/diff/20001/webrtc/p2p/base/dtlstrans... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/2163683003/diff/20001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel.cc:259: LOG_J(LS_ERROR, this) << "Couldn't set DTLS certificate digest."; On 2016/07/20 00:50:27, mattdr-at-webrtc.org wrote: > should we set DTLS state here too? If this has an immediate effect and succeeds, we'll asynchronously see "SE_OPEN" in OnDtlsEvent and that will update the state. But if it fails we'll see no such event and should update the state here. Good catch. I should probably update the unit tests adding another dimension: whether or not the fingerprint is valid. If I had done that earlier I would have caught this problem. https://codereview.webrtc.org/2163683003/diff/20001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel.cc:506: if (!dtls_ && local_certificate_) { On 2016/07/20 00:50:27, mattdr-at-webrtc.org wrote: > What does checking local_certificate protect against? SetupDtls requires the local certificate. You might then wonder, "What happens if we receive a client hello, *then* get our local certificate? Should we call SetupDtls then?" However this is currently not possible under the PeerConnection offer/answer model, so I'm not concerned about this optimization yet. https://codereview.webrtc.org/2163683003/diff/20001/webrtc/p2p/base/dtlstrans... File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right): https://codereview.webrtc.org/2163683003/diff/20001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel_unittest.cc:1027: // The tests below cover all possible permutations of these events, On 2016/07/20 00:50:27, mattdr-at-webrtc.org wrote: > where by "all possible" you mean "all causally consistent", since 3 must precede > 4. Actually 2 and 3, since we won't "write" packets until the caller is writable. Reducing the permutations from 24 to 8. I'll update the comment to reflect this.
deadbeef@webrtc.org changed reviewers: + deadbeef@webrtc.org
On 2016/07/20 17:37:38, Taylor Brandstetter wrote: Hmm, guess you can't do "not lgtm" on your own CL. Was worth a try.
Partial comments for the next pass. https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... File webrtc/base/opensslstreamadapter.h (right): https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:138: enum SSLTopology { 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. https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:192: return peer_certificate_digest_algorithm_.size() && personal preference: !xyz.empty() https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:207: SSLTopology topology_ = TOPOLOGY_UNKNOWN; 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. :)
https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... File webrtc/base/opensslstreamadapter.h (right): https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:138: enum SSLTopology { On 2016/07/21 08:04:23, mattdr-at-webrtc.org wrote: > 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. Enum dropped, as discussed. https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:192: return peer_certificate_digest_algorithm_.size() && On 2016/07/21 08:04:23, mattdr-at-webrtc.org wrote: > personal preference: !xyz.empty() rtc::Buffer didn't have an "empty" method so I added one. https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:207: SSLTopology topology_ = TOPOLOGY_UNKNOWN; On 2016/07/21 08:04:23, mattdr-at-webrtc.org wrote: > 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. > :) 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.
More comments to come. https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... File webrtc/base/opensslstreamadapter.cc (right): https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:318: ASSERT(topology_ != TOPOLOGY_CLIENT_SERVER); Should we make these DCHECKS instead of crashing in production? https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:333: if (VerifyPeerCertificate()) { I'd prefer some early returns: if (!peer_certificate_) { return true; } if (!VerifyPeerCertificate()) { Error(...); return false; } if (state_ == SSL_CONNECTED) { ... } return true; https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:1152: } We can probably just delete all of this. https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... File webrtc/base/opensslstreamadapter.h (right): https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:138: enum SSLTopology { On 2016/07/21 08:04:23, mattdr-at-webrtc.org wrote: > 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. Nothing every uses StartSSLWithServer any more, so we could probably simplify here a lot. https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:187: return ssl_server_name_.empty() && client_auth_enabled() && 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(). https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:192: return peer_certificate_digest_algorithm_.size() && On 2016/07/21 08:04:23, mattdr-at-webrtc.org wrote: > personal preference: !xyz.empty() +1 https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:212: std::string ssl_server_name_; 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. https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:220: bool certificate_verified_ = false; 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_? https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:222: // OpenSSLAdapter::custom_verify_callback_ result 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.
Description was changed from ========== Allow the DTLS fingerprint verification to occur after the handshake. This means the DTLS handshake can make progress while the SDP answer containing the fingerprint is still in transit. If the signaling path if significantly slower than the media path, this can have a moderate impact on call setup time. Of course, until the fingerprint is verified no media can be sent. Any attempted write will result in SR_BLOCK. ========== to ========== Allow the DTLS fingerprint verification to occur after the handshake. This means the DTLS handshake can make progress while the SDP answer containing the fingerprint is still in transit. If the signaling path if significantly slower than the media path, this can have a moderate impact on call setup time. Of course, until the fingerprint is verified no media can be sent. Any attempted write will result in SR_BLOCK. This essentially fulfills the requirements of RFC 4572, Section 6.2: Note that when the offer/answer model is being used, it is possible for a media connection to outrace the answer back to the offerer. Thus, if the offerer has offered a 'setup:passive' or 'setup:actpass' role, it MUST (as specified in RFC 4145 [2]) begin listening for an incoming connection as soon as it sends its offer. However, it MUST NOT assume that the data transmitted over the TLS connection is valid until it has received a matching fingerprint in an SDP answer. If the fingerprint, once it arrives, does not match the client's certificate, the server endpoint MUST terminate the media connection with a bad_certificate error, as stated in the previous paragraph. ==========
Responded to comments. Note that I'm still adding unit tests for "bad fingerprint received after handshake is done". I found an issue, which is that we need a way of alerting the other peer that it gave us a bad certificate. I'm going to talk to davidben@ about this, since I don't see a way to do this with BoringSSL currently. https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... File webrtc/base/opensslstreamadapter.cc (right): https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:318: ASSERT(topology_ != TOPOLOGY_CLIENT_SERVER); On 2016/07/22 18:58:27, pthatcher1 wrote: > Should we make these DCHECKS instead of crashing in production? ASSERTS are still only run in debug builds. I think the only difference is how the error is recorded. I'll change the ASSERTS to DCHECKS though. https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:333: if (VerifyPeerCertificate()) { On 2016/07/22 18:58:27, pthatcher1 wrote: > I'd prefer some early returns: > > if (!peer_certificate_) { > return true; > } > > if (!VerifyPeerCertificate()) { > Error(...); > return false; > } > > if (state_ == SSL_CONNECTED) { > ... > } > > return true; Done. https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... File webrtc/base/opensslstreamadapter.h (right): https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:138: enum SSLTopology { On 2016/07/22 18:58:27, pthatcher1 wrote: > On 2016/07/21 08:04:23, http://mattdr-at-webrtc.org wrote: > > 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. > > Nothing every uses StartSSLWithServer any more, so we could probably simplify > here a lot. Can we remove StartSSLWithServer in a separate CL, to keep this one focused? https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:187: return ssl_server_name_.empty() && client_auth_enabled() && On 2016/07/22 18:58:27, pthatcher1 wrote: > 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(). See above. https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:212: std::string ssl_server_name_; On 2016/07/22 18:58:27, pthatcher1 wrote: > 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. See above. https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:220: bool certificate_verified_ = false; On 2016/07/22 18:58:27, pthatcher1 wrote: > 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_? > Done. https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:222: // OpenSSLAdapter::custom_verify_callback_ result On 2016/07/22 18:58:27, pthatcher1 wrote: > 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. What about the PeerConnection? :) Anyway you're probably right, but I think the renaming can happen in a later CL.
On 2016/07/25 23:54:54, Taylor Brandstetter wrote: > Responded to comments. > > Note that I'm still adding unit tests for "bad fingerprint received after > handshake is done". I found an issue, which is that we need a way of alerting > the other peer that it gave us a bad certificate. I'm going to talk to davidben@ > about this, since I don't see a way to do this with BoringSSL currently. I wouldn't worry about this too much -- just write something to the log and tear down the connection.
https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... File webrtc/base/opensslstreamadapter.cc (right): https://codereview.webrtc.org/2163683003/diff/40001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:318: ASSERT(topology_ != TOPOLOGY_CLIENT_SERVER); On 2016/07/25 23:54:53, Taylor Brandstetter wrote: > On 2016/07/22 18:58:27, pthatcher1 wrote: > > Should we make these DCHECKS instead of crashing in production? > > ASSERTS are still only run in debug builds. I think the only difference is how > the error is recorded. I'll change the ASSERTS to DCHECKS though. Oh, right. I got confused with CHECK. We have too many similar things :). https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... File webrtc/base/opensslstreamadapter.cc (right): https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:341: return false; This would cause SetRemoteDescription to fail based upon whether or not the handshake has completed. That seems like a problem. Wouldn't instead we want SetRemoteDescription to succeed, but have the transport go into a failed state? https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:347: PostEvent(SE_OPEN | SE_READ | SE_WRITE, 0); It's not very clear what's going on here. It appears that ContinueSSL(), which sets the state_ = SSL_CONNECTED sets the state of the adapter to open/read/write, but synchronously, but that would be bad for some callers of SetPeerCertificateDigest, so we post an even that amounts to "call ContinueSSL() later". Is that accurate? If so, can you make that a little more clear? https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:791: verify_certificate_using_peer_digest()); Doesn't this equate to the following? RTC_DCHECK(!ssl_server_name_.empty() != ssl_server_name_.empty()); That doesn't seem like a useful DCHECK. Or am I missing something? https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:793: << (verify_certificate_using_server_name() ? ssl_server_name_ We already verified the state_, so why not just use "in_peer_mode" here? https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... File webrtc/base/opensslstreamadapter.h (right): https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:178: bool waiting_to_verify_client_cert() const { Whould this be waiting_to_verify_peer_certificate() to match VerifyPeerCertificate and peer_certificate_verified_? Or would needs_to_verify_peer_certificate() be a better name? https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:183: } If we're going to keep the ssl_server_name_ stuff, can we at least avoid calling ssl_server_name_.empty() with an explanation of what it means? Perhaps have something like this: bool in_peer_mode() { return ssl_server_name_.empty(); } Then you can have this: bool waiting_to_verify_peer_certificate() { return in_peer_mode() && client_auth_enabled() && !peer_certificate_verified_; } https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:185: bool have_peer_certificate_digest() const { Should this be has_peer_certificate_digest()? https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:194: return state_ != SSL_NONE && !ssl_server_name_.empty(); Here in_peer_mode() would be nice as well. https://codereview.webrtc.org/2163683003/diff/80001/webrtc/p2p/base/dtlstrans... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/2163683003/diff/80001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel.cc:268: } Would this be more clear as this flow? if (dtls_ && remote_fingerprint_value_.empty()) { // This can occur if DTLS is set up before a remote fingerprint is if (!dtls_->SetPeerCertificateDigest(...)) { ... return false; } return true; } if (dtls_) { // If the fingerprint is changing, we'll tear down the DTLS association and ... } if (!SetupDtls()) { return false; } return true; https://codereview.webrtc.org/2163683003/diff/80001/webrtc/p2p/base/dtlstrans... File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right): https://codereview.webrtc.org/2163683003/diff/80001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel_unittest.cc:1029: // finishes, but otherwise any ordering is possible. Can you point out why there are only 8 permutations (The handshake can't finish before the ClientHello and the caller is writable)? https://codereview.webrtc.org/2163683003/diff/80001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel_unittest.cc:1038: }); Would it make sense to just have these be methods? Setup(); CallerReceivesFingerprint(); CallerWritable(); CallerReceivesClientHello(); HandshakeFinishes(); Finish();
deadbeef@webrtc.org changed reviewers: - deadbeef@webrtc.org
I finally got around to updating this CL. Main changes are: * Started sending the "bad_certificate" alert (where an API is available) * Started handling remote DTLS closures and updating the DTLS channel's state (it wasn't checking the return code, so this has been an issue for a while). * Added a new parameter to the "event permutation" test: whether or not the certificate matches the digest. * Merged with master, where StartSSLWithServer was removed * Responded to comments https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... File webrtc/base/opensslstreamadapter.cc (right): https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:341: return false; On 2016/07/27 19:32:35, pthatcher1 wrote: > This would cause SetRemoteDescription to fail based upon whether or not the > handshake has completed. That seems like a problem. Wouldn't instead we want > SetRemoteDescription to succeed, but have the transport go into a failed state? I think you meant for this comment to be in DtlsTransportChannel? But yes I agree that having variable results from SetRemoteDescription depending on timing is something applications won't expect (and isn't mentioned by JSEP). I'll fix this and update the test. https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:347: PostEvent(SE_OPEN | SE_READ | SE_WRITE, 0); On 2016/07/27 19:32:35, pthatcher1 wrote: > It's not very clear what's going on here. It appears that ContinueSSL(), which > sets the state_ = SSL_CONNECTED sets the state of the adapter to > open/read/write, but synchronously, but that would be bad for some callers of > SetPeerCertificateDigest, so we post an even that amounts to "call ContinueSSL() > later". Is that accurate? If so, can you make that a little more clear? ContinueSSL should have been posting the event asynchronously as well. It's just a bug no one noticed (until you did, thanks) and happened to not have any consequences. Fixed now. https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:791: verify_certificate_using_peer_digest()); On 2016/07/27 19:32:35, pthatcher1 wrote: > Doesn't this equate to the following? > > RTC_DCHECK(!ssl_server_name_.empty() != ssl_server_name_.empty()); > > > That doesn't seem like a useful DCHECK. Or am I missing something? This checks that one and only one "mode" has been determined. Of course, right now the DCHECK will always pass. The point of DCHECKS isn't to validate the external inputs of a class, but to enforce and document implicit assumptions, both now and into the future. If we later changed the implementation of "verify_certificate_using_*", and this assumption no longer held, this DCHECK would catch the problem early. Of course, this is invalidated now that StartSSLWithServer is gone. But I thought I'd explain the reasoning. https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.cc:793: << (verify_certificate_using_server_name() ? ssl_server_name_ On 2016/07/27 19:32:35, pthatcher1 wrote: > We already verified the state_, so why not just use "in_peer_mode" here? I don't know what you mean. I'm using "verify_certificate_using_server_name()" which is equivalent to "!in_peer_mode()". https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... File webrtc/base/opensslstreamadapter.h (right): https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:178: bool waiting_to_verify_client_cert() const { On 2016/07/27 19:32:35, pthatcher1 wrote: > Whould this be waiting_to_verify_peer_certificate() to match > VerifyPeerCertificate and peer_certificate_verified_? > > Or would needs_to_verify_peer_certificate() be a better name? I'll change it to "peer". I prefer "waiting" to "needs" because "waiting" implies "something needs to happen but hasn't happened yet" whereas "needs" doesn't. At least in my opinion. https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:185: bool have_peer_certificate_digest() const { On 2016/07/27 19:32:35, pthatcher1 wrote: > Should this be has_peer_certificate_digest()? Sure. https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... webrtc/base/opensslstreamadapter.h:194: return state_ != SSL_NONE && !ssl_server_name_.empty(); On 2016/07/27 19:32:35, pthatcher1 wrote: > Here in_peer_mode() would be nice as well. These functions *were* the equivalent of in_peer_mode (we need to check that state_ isn't SSL_NONE, because in that state we don't know what mode we'll use). However this is obsoleted now that StartSSLWithServer is removed. https://codereview.webrtc.org/2163683003/diff/80001/webrtc/p2p/base/dtlstrans... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/2163683003/diff/80001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel.cc:268: } On 2016/07/27 19:32:35, pthatcher1 wrote: > Would this be more clear as this flow? > > if (dtls_ && remote_fingerprint_value_.empty()) { > // This can occur if DTLS is set up before a remote fingerprint is > if (!dtls_->SetPeerCertificateDigest(...)) { > ... > return false; > } > return true; > } > > if (dtls_) { > // If the fingerprint is changing, we'll tear down the DTLS association and > ... > } > > if (!SetupDtls()) { > return false; > } > return true; Done. https://codereview.webrtc.org/2163683003/diff/80001/webrtc/p2p/base/dtlstrans... File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right): https://codereview.webrtc.org/2163683003/diff/80001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel_unittest.cc:1029: // finishes, but otherwise any ordering is possible. On 2016/07/27 19:32:35, pthatcher1 wrote: > Can you point out why there are only 8 permutations (The handshake can't finish > before the ClientHello and the caller is writable)? Isn't that exactly what the above paragraph points out? What would you add? https://codereview.webrtc.org/2163683003/diff/80001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel_unittest.cc:1038: }); On 2016/07/27 19:32:35, pthatcher1 wrote: > Would it make sense to just have these be methods? > > Setup(); > CallerReceivesFingerprint(); > CallerWritable(); > CallerReceivesClientHello(); > HandshakeFinishes(); > Finish(); I changed how this works; I'm using parameterized tests now.
On 2016/08/13 00:09:53, Taylor Brandstetter wrote: > I finally got around to updating this CL. Main changes are: > > * Started sending the "bad_certificate" alert (where an API is available) > * Started handling remote DTLS closures and updating the DTLS channel's state > (it wasn't checking the return code, so this has been an issue for a while). > * Added a new parameter to the "event permutation" test: whether or not the > certificate matches the digest. > * Merged with master, where StartSSLWithServer was removed > * Responded to comments > > https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... > File webrtc/base/opensslstreamadapter.cc (right): > > https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... > webrtc/base/opensslstreamadapter.cc:341: return false; > On 2016/07/27 19:32:35, pthatcher1 wrote: > > This would cause SetRemoteDescription to fail based upon whether or not the > > handshake has completed. That seems like a problem. Wouldn't instead we want > > SetRemoteDescription to succeed, but have the transport go into a failed > state? > > I think you meant for this comment to be in DtlsTransportChannel? But yes I > agree that having variable results from SetRemoteDescription depending on timing > is something applications won't expect (and isn't mentioned by JSEP). I'll fix > this and update the test. Everything looks fine to me except for this. Let me know when this is fixed. > > https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... > webrtc/base/opensslstreamadapter.cc:347: PostEvent(SE_OPEN | SE_READ | SE_WRITE, > 0); > On 2016/07/27 19:32:35, pthatcher1 wrote: > > It's not very clear what's going on here. It appears that ContinueSSL(), > which > > sets the state_ = SSL_CONNECTED sets the state of the adapter to > > open/read/write, but synchronously, but that would be bad for some callers of > > SetPeerCertificateDigest, so we post an even that amounts to "call > ContinueSSL() > > later". Is that accurate? If so, can you make that a little more clear? > > ContinueSSL should have been posting the event asynchronously as well. It's just > a bug no one noticed (until you did, thanks) and happened to not have any > consequences. Fixed now. > > https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... > webrtc/base/opensslstreamadapter.cc:791: > verify_certificate_using_peer_digest()); > On 2016/07/27 19:32:35, pthatcher1 wrote: > > Doesn't this equate to the following? > > > > RTC_DCHECK(!ssl_server_name_.empty() != ssl_server_name_.empty()); > > > > > > That doesn't seem like a useful DCHECK. Or am I missing something? > > This checks that one and only one "mode" has been determined. Of course, right > now the DCHECK will always pass. The point of DCHECKS isn't to validate the > external inputs of a class, but to enforce and document implicit assumptions, > both now and into the future. If we later changed the implementation of > "verify_certificate_using_*", and this assumption no longer held, this DCHECK > would catch the problem early. > > Of course, this is invalidated now that StartSSLWithServer is gone. But I > thought I'd explain the reasoning. > > https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... > webrtc/base/opensslstreamadapter.cc:793: << > (verify_certificate_using_server_name() ? ssl_server_name_ > On 2016/07/27 19:32:35, pthatcher1 wrote: > > We already verified the state_, so why not just use "in_peer_mode" here? > > I don't know what you mean. I'm using "verify_certificate_using_server_name()" > which is equivalent to "!in_peer_mode()". > > https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... > File webrtc/base/opensslstreamadapter.h (right): > > https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... > webrtc/base/opensslstreamadapter.h:178: bool waiting_to_verify_client_cert() > const { > On 2016/07/27 19:32:35, pthatcher1 wrote: > > Whould this be waiting_to_verify_peer_certificate() to match > > VerifyPeerCertificate and peer_certificate_verified_? > > > > Or would needs_to_verify_peer_certificate() be a better name? > > I'll change it to "peer". I prefer "waiting" to "needs" because "waiting" > implies "something needs to happen but hasn't happened yet" whereas "needs" > doesn't. At least in my opinion. > > https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... > webrtc/base/opensslstreamadapter.h:185: bool have_peer_certificate_digest() > const { > On 2016/07/27 19:32:35, pthatcher1 wrote: > > Should this be has_peer_certificate_digest()? > > Sure. > > https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... > webrtc/base/opensslstreamadapter.h:194: return state_ != SSL_NONE && > !ssl_server_name_.empty(); > On 2016/07/27 19:32:35, pthatcher1 wrote: > > Here in_peer_mode() would be nice as well. > > These functions *were* the equivalent of in_peer_mode (we need to check that > state_ isn't SSL_NONE, because in that state we don't know what mode we'll use). > > However this is obsoleted now that StartSSLWithServer is removed. > > https://codereview.webrtc.org/2163683003/diff/80001/webrtc/p2p/base/dtlstrans... > File webrtc/p2p/base/dtlstransportchannel.cc (right): > > https://codereview.webrtc.org/2163683003/diff/80001/webrtc/p2p/base/dtlstrans... > webrtc/p2p/base/dtlstransportchannel.cc:268: } > On 2016/07/27 19:32:35, pthatcher1 wrote: > > Would this be more clear as this flow? > > > > if (dtls_ && remote_fingerprint_value_.empty()) { > > // This can occur if DTLS is set up before a remote fingerprint is > > if (!dtls_->SetPeerCertificateDigest(...)) { > > ... > > return false; > > } > > return true; > > } > > > > if (dtls_) { > > // If the fingerprint is changing, we'll tear down the DTLS association and > > ... > > } > > > > if (!SetupDtls()) { > > return false; > > } > > return true; > > Done. > > https://codereview.webrtc.org/2163683003/diff/80001/webrtc/p2p/base/dtlstrans... > File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right): > > https://codereview.webrtc.org/2163683003/diff/80001/webrtc/p2p/base/dtlstrans... > webrtc/p2p/base/dtlstransportchannel_unittest.cc:1029: // finishes, but > otherwise any ordering is possible. > On 2016/07/27 19:32:35, pthatcher1 wrote: > > Can you point out why there are only 8 permutations (The handshake can't > finish > > before the ClientHello and the caller is writable)? > > Isn't that exactly what the above paragraph points out? What would you add? > > https://codereview.webrtc.org/2163683003/diff/80001/webrtc/p2p/base/dtlstrans... > webrtc/p2p/base/dtlstransportchannel_unittest.cc:1038: }); > On 2016/07/27 19:32:35, pthatcher1 wrote: > > Would it make sense to just have these be methods? > > > > Setup(); > > CallerReceivesFingerprint(); > > CallerWritable(); > > CallerReceivesClientHello(); > > HandshakeFinishes(); > > Finish(); > > I changed how this works; I'm using parameterized tests now.
On 2016/08/23 00:25:54, pthatcher1 wrote: > On 2016/08/13 00:09:53, Taylor Brandstetter wrote: > > I finally got around to updating this CL. Main changes are: > > > > * Started sending the "bad_certificate" alert (where an API is available) > > * Started handling remote DTLS closures and updating the DTLS channel's state > > (it wasn't checking the return code, so this has been an issue for a while). > > * Added a new parameter to the "event permutation" test: whether or not the > > certificate matches the digest. > > * Merged with master, where StartSSLWithServer was removed > > * Responded to comments > > > > > https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... > > File webrtc/base/opensslstreamadapter.cc (right): > > > > > https://codereview.webrtc.org/2163683003/diff/80001/webrtc/base/opensslstream... > > webrtc/base/opensslstreamadapter.cc:341: return false; > > On 2016/07/27 19:32:35, pthatcher1 wrote: > > > This would cause SetRemoteDescription to fail based upon whether or not the > > > handshake has completed. That seems like a problem. Wouldn't instead we > want > > > SetRemoteDescription to succeed, but have the transport go into a failed > > state? > > > > I think you meant for this comment to be in DtlsTransportChannel? But yes I > > agree that having variable results from SetRemoteDescription depending on > timing > > is something applications won't expect (and isn't mentioned by JSEP). I'll fix > > this and update the test. > > Everything looks fine to me except for this. Let me know when this is fixed. Is it not fixed? I thought it was; the tests with "EXPECT_TRUE(SetRemoteTransportDescription)" are passing.
lgtm Sorry, I thought your comment meant you had more to do.
lgtm
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from mattdr@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2163683003/#ps160001 (title: "Merge with master.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/14393) win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)
Description was changed from ========== Allow the DTLS fingerprint verification to occur after the handshake. This means the DTLS handshake can make progress while the SDP answer containing the fingerprint is still in transit. If the signaling path if significantly slower than the media path, this can have a moderate impact on call setup time. Of course, until the fingerprint is verified no media can be sent. Any attempted write will result in SR_BLOCK. This essentially fulfills the requirements of RFC 4572, Section 6.2: Note that when the offer/answer model is being used, it is possible for a media connection to outrace the answer back to the offerer. Thus, if the offerer has offered a 'setup:passive' or 'setup:actpass' role, it MUST (as specified in RFC 4145 [2]) begin listening for an incoming connection as soon as it sends its offer. However, it MUST NOT assume that the data transmitted over the TLS connection is valid until it has received a matching fingerprint in an SDP answer. If the fingerprint, once it arrives, does not match the client's certificate, the server endpoint MUST terminate the media connection with a bad_certificate error, as stated in the previous paragraph. ========== to ========== Allow the DTLS fingerprint verification to occur after the handshake. This means the DTLS handshake can make progress while the SDP answer containing the fingerprint is still in transit. If the signaling path if significantly slower than the media path, this can have a moderate impact on call setup time. Of course, until the fingerprint is verified no media can be sent. Any attempted write will result in SR_BLOCK. This essentially fulfills the requirements of RFC 4572, Section 6.2: Note that when the offer/answer model is being used, it is possible for a media connection to outrace the answer back to the offerer. Thus, if the offerer has offered a 'setup:passive' or 'setup:actpass' role, it MUST (as specified in RFC 4145 [2]) begin listening for an incoming connection as soon as it sends its offer. However, it MUST NOT assume that the data transmitted over the TLS connection is valid until it has received a matching fingerprint in an SDP answer. If the fingerprint, once it arrives, does not match the client's certificate, the server endpoint MUST terminate the media connection with a bad_certificate error, as stated in the previous paragraph. BUG=webrtc:6387 ==========
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from mattdr@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2163683003/#ps200001 (title: "Rename "NO_ERROR" to "NONE", since "NO_ERROR" is a Windows #define...")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Allow the DTLS fingerprint verification to occur after the handshake. This means the DTLS handshake can make progress while the SDP answer containing the fingerprint is still in transit. If the signaling path if significantly slower than the media path, this can have a moderate impact on call setup time. Of course, until the fingerprint is verified no media can be sent. Any attempted write will result in SR_BLOCK. This essentially fulfills the requirements of RFC 4572, Section 6.2: Note that when the offer/answer model is being used, it is possible for a media connection to outrace the answer back to the offerer. Thus, if the offerer has offered a 'setup:passive' or 'setup:actpass' role, it MUST (as specified in RFC 4145 [2]) begin listening for an incoming connection as soon as it sends its offer. However, it MUST NOT assume that the data transmitted over the TLS connection is valid until it has received a matching fingerprint in an SDP answer. If the fingerprint, once it arrives, does not match the client's certificate, the server endpoint MUST terminate the media connection with a bad_certificate error, as stated in the previous paragraph. BUG=webrtc:6387 ========== to ========== Allow the DTLS fingerprint verification to occur after the handshake. This means the DTLS handshake can make progress while the SDP answer containing the fingerprint is still in transit. If the signaling path if significantly slower than the media path, this can have a moderate impact on call setup time. Of course, until the fingerprint is verified no media can be sent. Any attempted write will result in SR_BLOCK. This essentially fulfills the requirements of RFC 4572, Section 6.2: Note that when the offer/answer model is being used, it is possible for a media connection to outrace the answer back to the offerer. Thus, if the offerer has offered a 'setup:passive' or 'setup:actpass' role, it MUST (as specified in RFC 4145 [2]) begin listening for an incoming connection as soon as it sends its offer. However, it MUST NOT assume that the data transmitted over the TLS connection is valid until it has received a matching fingerprint in an SDP answer. If the fingerprint, once it arrives, does not match the client's certificate, the server endpoint MUST terminate the media connection with a bad_certificate error, as stated in the previous paragraph. BUG=webrtc:6387 R=mattdr@webrtc.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/042041bf9585f92e962387c59... ==========
Message was sent while issue was closed.
Description was changed from ========== Allow the DTLS fingerprint verification to occur after the handshake. This means the DTLS handshake can make progress while the SDP answer containing the fingerprint is still in transit. If the signaling path if significantly slower than the media path, this can have a moderate impact on call setup time. Of course, until the fingerprint is verified no media can be sent. Any attempted write will result in SR_BLOCK. This essentially fulfills the requirements of RFC 4572, Section 6.2: Note that when the offer/answer model is being used, it is possible for a media connection to outrace the answer back to the offerer. Thus, if the offerer has offered a 'setup:passive' or 'setup:actpass' role, it MUST (as specified in RFC 4145 [2]) begin listening for an incoming connection as soon as it sends its offer. However, it MUST NOT assume that the data transmitted over the TLS connection is valid until it has received a matching fingerprint in an SDP answer. If the fingerprint, once it arrives, does not match the client's certificate, the server endpoint MUST terminate the media connection with a bad_certificate error, as stated in the previous paragraph. BUG=webrtc:6387 R=mattdr@webrtc.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/042041bf9585f92e962387c59... ========== to ========== Allow the DTLS fingerprint verification to occur after the handshake. This means the DTLS handshake can make progress while the SDP answer containing the fingerprint is still in transit. If the signaling path if significantly slower than the media path, this can have a moderate impact on call setup time. Of course, until the fingerprint is verified no media can be sent. Any attempted write will result in SR_BLOCK. This essentially fulfills the requirements of RFC 4572, Section 6.2: Note that when the offer/answer model is being used, it is possible for a media connection to outrace the answer back to the offerer. Thus, if the offerer has offered a 'setup:passive' or 'setup:actpass' role, it MUST (as specified in RFC 4145 [2]) begin listening for an incoming connection as soon as it sends its offer. However, it MUST NOT assume that the data transmitted over the TLS connection is valid until it has received a matching fingerprint in an SDP answer. If the fingerprint, once it arrives, does not match the client's certificate, the server endpoint MUST terminate the media connection with a bad_certificate error, as stated in the previous paragraph. BUG=webrtc:6387 R=mattdr@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/042041bf9585f92e962387c59ca805f1218338f9 Cr-Commit-Position: refs/heads/master@{#14296} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/042041bf9585f92e962387c59ca805f1218338f9 Cr-Commit-Position: refs/heads/master@{#14296}
Message was sent while issue was closed.
Committed patchset #11 (id:200001) manually as 042041bf9585f92e962387c59ca805f1218338f9 (presubmit successful).
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.webrtc.org/2352863003/ by deadbeef@webrtc.org. The reason for reverting is: Broke a downstream user of SSLStreamAdapter. Need to add the new interface (returning error code instead of bool) in a backwards compatible way..
Message was sent while issue was closed.
Description was changed from ========== Allow the DTLS fingerprint verification to occur after the handshake. This means the DTLS handshake can make progress while the SDP answer containing the fingerprint is still in transit. If the signaling path if significantly slower than the media path, this can have a moderate impact on call setup time. Of course, until the fingerprint is verified no media can be sent. Any attempted write will result in SR_BLOCK. This essentially fulfills the requirements of RFC 4572, Section 6.2: Note that when the offer/answer model is being used, it is possible for a media connection to outrace the answer back to the offerer. Thus, if the offerer has offered a 'setup:passive' or 'setup:actpass' role, it MUST (as specified in RFC 4145 [2]) begin listening for an incoming connection as soon as it sends its offer. However, it MUST NOT assume that the data transmitted over the TLS connection is valid until it has received a matching fingerprint in an SDP answer. If the fingerprint, once it arrives, does not match the client's certificate, the server endpoint MUST terminate the media connection with a bad_certificate error, as stated in the previous paragraph. BUG=webrtc:6387 R=mattdr@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/042041bf9585f92e962387c59ca805f1218338f9 Cr-Commit-Position: refs/heads/master@{#14296} ========== to ========== Allow the DTLS fingerprint verification to occur after the handshake. This means the DTLS handshake can make progress while the SDP answer containing the fingerprint is still in transit. If the signaling path if significantly slower than the media path, this can have a moderate impact on call setup time. Of course, until the fingerprint is verified no media can be sent. Any attempted write will result in SR_BLOCK. This essentially fulfills the requirements of RFC 4572, Section 6.2: Note that when the offer/answer model is being used, it is possible for a media connection to outrace the answer back to the offerer. Thus, if the offerer has offered a 'setup:passive' or 'setup:actpass' role, it MUST (as specified in RFC 4145 [2]) begin listening for an incoming connection as soon as it sends its offer. However, it MUST NOT assume that the data transmitted over the TLS connection is valid until it has received a matching fingerprint in an SDP answer. If the fingerprint, once it arrives, does not match the client's certificate, the server endpoint MUST terminate the media connection with a bad_certificate error, as stated in the previous paragraph. BUG=webrtc:6387 R=mattdr@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/042041bf9585f92e962387c59ca805f1218338f9 Cr-Commit-Position: refs/heads/master@{#14296} ==========
Description was changed from ========== Allow the DTLS fingerprint verification to occur after the handshake. This means the DTLS handshake can make progress while the SDP answer containing the fingerprint is still in transit. If the signaling path if significantly slower than the media path, this can have a moderate impact on call setup time. Of course, until the fingerprint is verified no media can be sent. Any attempted write will result in SR_BLOCK. This essentially fulfills the requirements of RFC 4572, Section 6.2: Note that when the offer/answer model is being used, it is possible for a media connection to outrace the answer back to the offerer. Thus, if the offerer has offered a 'setup:passive' or 'setup:actpass' role, it MUST (as specified in RFC 4145 [2]) begin listening for an incoming connection as soon as it sends its offer. However, it MUST NOT assume that the data transmitted over the TLS connection is valid until it has received a matching fingerprint in an SDP answer. If the fingerprint, once it arrives, does not match the client's certificate, the server endpoint MUST terminate the media connection with a bad_certificate error, as stated in the previous paragraph. BUG=webrtc:6387 R=mattdr@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/042041bf9585f92e962387c59ca805f1218338f9 Cr-Commit-Position: refs/heads/master@{#14296} ========== to ========== Relanding: Allow the DTLS fingerprint verification to occur after the handshake. This means the DTLS handshake can make progress while the SDP answer containing the fingerprint is still in transit. If the signaling path if significantly slower than the media path, this can have a moderate impact on call setup time. Of course, until the fingerprint is verified no media can be sent. Any attempted write will result in SR_BLOCK. This essentially fulfills the requirements of RFC 4572, Section 6.2: Note that when the offer/answer model is being used, it is possible for a media connection to outrace the answer back to the offerer. Thus, if the offerer has offered a 'setup:passive' or 'setup:actpass' role, it MUST (as specified in RFC 4145 [2]) begin listening for an incoming connection as soon as it sends its offer. However, it MUST NOT assume that the data transmitted over the TLS connection is valid until it has received a matching fingerprint in an SDP answer. If the fingerprint, once it arrives, does not match the client's certificate, the server endpoint MUST terminate the media connection with a bad_certificate error, as stated in the previous paragraph. BUG=webrtc:6387 R=mattdr@webrtc.org, pthatcher@webrtc.org ==========
deadbeef@webrtc.org changed reviewers: - mattdr@webrtc.org, pthatcher@webrtc.org
Would like a review of the final patchset. I'm making the error enum an optional output parameter, so that existing callers expecting a bool aren't disappointed.
skvlad@webrtc.org changed reviewers: + skvlad@webrtc.org
I like the tests! https://codereview.webrtc.org/2163683003/diff/220001/webrtc/base/opensslstrea... File webrtc/base/opensslstreamadapter.cc (right): https://codereview.webrtc.org/2163683003/diff/220001/webrtc/base/opensslstrea... webrtc/base/opensslstreamadapter.cc:1146: return 1; Just to be sure - we won't end up accepting ANY certificate chain here, will we? https://codereview.webrtc.org/2163683003/diff/220001/webrtc/base/opensslstrea... File webrtc/base/opensslstreamadapter.h (right): https://codereview.webrtc.org/2163683003/diff/220001/webrtc/base/opensslstrea... webrtc/base/opensslstreamadapter.h:176: bool waiting_to_verify_peer_certificate_cert() const { The name has both "certificate" and "cert" - are both necessary? Or should it be "waiting_to_verify_peer_certificate()"? https://codereview.webrtc.org/2163683003/diff/220001/webrtc/base/sslstreamada... File webrtc/base/sslstreamadapter.h (right): https://codereview.webrtc.org/2163683003/diff/220001/webrtc/base/sslstreamada... webrtc/base/sslstreamadapter.h:110: NONE, I think using kConstantCase for enum values would have avoided the name collision, but either way is fine.
lgtm
https://codereview.webrtc.org/2163683003/diff/220001/webrtc/base/opensslstrea... File webrtc/base/opensslstreamadapter.cc (right): https://codereview.webrtc.org/2163683003/diff/220001/webrtc/base/opensslstrea... webrtc/base/opensslstreamadapter.cc:1146: return 1; On 2016/09/29 20:56:29, skvlad wrote: > Just to be sure - we won't end up accepting ANY certificate chain here, will we? Based on this bug, it looks like we'd end up trying to verify the root certificate and fail: https://bugs.chromium.org/p/webrtc/issues/detail?id=3383 https://codereview.webrtc.org/2163683003/diff/220001/webrtc/base/opensslstrea... File webrtc/base/opensslstreamadapter.h (right): https://codereview.webrtc.org/2163683003/diff/220001/webrtc/base/opensslstrea... webrtc/base/opensslstreamadapter.h:176: bool waiting_to_verify_peer_certificate_cert() const { On 2016/09/29 20:56:29, skvlad wrote: > The name has both "certificate" and "cert" - are both necessary? Or should it be > "waiting_to_verify_peer_certificate()"? Good catch, I think that was a "find/replace" gone awry. https://codereview.webrtc.org/2163683003/diff/220001/webrtc/base/sslstreamada... File webrtc/base/sslstreamadapter.h (right): https://codereview.webrtc.org/2163683003/diff/220001/webrtc/base/sslstreamada... webrtc/base/sslstreamadapter.h:110: NONE, On 2016/09/29 20:56:29, skvlad wrote: > I think using kConstantCase for enum values would have avoided the name > collision, but either way is fine. The Chromium style guide says to use this style unfortunately; https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
Description was changed from ========== Relanding: Allow the DTLS fingerprint verification to occur after the handshake. This means the DTLS handshake can make progress while the SDP answer containing the fingerprint is still in transit. If the signaling path if significantly slower than the media path, this can have a moderate impact on call setup time. Of course, until the fingerprint is verified no media can be sent. Any attempted write will result in SR_BLOCK. This essentially fulfills the requirements of RFC 4572, Section 6.2: Note that when the offer/answer model is being used, it is possible for a media connection to outrace the answer back to the offerer. Thus, if the offerer has offered a 'setup:passive' or 'setup:actpass' role, it MUST (as specified in RFC 4145 [2]) begin listening for an incoming connection as soon as it sends its offer. However, it MUST NOT assume that the data transmitted over the TLS connection is valid until it has received a matching fingerprint in an SDP answer. If the fingerprint, once it arrives, does not match the client's certificate, the server endpoint MUST terminate the media connection with a bad_certificate error, as stated in the previous paragraph. BUG=webrtc:6387 R=mattdr@webrtc.org, pthatcher@webrtc.org ========== to ========== Relanding: Allow the DTLS fingerprint verification to occur after the handshake. This means the DTLS handshake can make progress while the SDP answer containing the fingerprint is still in transit. If the signaling path if significantly slower than the media path, this can have a moderate impact on call setup time. Of course, until the fingerprint is verified no media can be sent. Any attempted write will result in SR_BLOCK. This essentially fulfills the requirements of RFC 4572, Section 6.2: Note that when the offer/answer model is being used, it is possible for a media connection to outrace the answer back to the offerer. Thus, if the offerer has offered a 'setup:passive' or 'setup:actpass' role, it MUST (as specified in RFC 4145 [2]) begin listening for an incoming connection as soon as it sends its offer. However, it MUST NOT assume that the data transmitted over the TLS connection is valid until it has received a matching fingerprint in an SDP answer. If the fingerprint, once it arrives, does not match the client's certificate, the server endpoint MUST terminate the media connection with a bad_certificate error, as stated in the previous paragraph. BUG=webrtc:6387 ==========
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from mattdr@webrtc.org, pthatcher@webrtc.org, skvlad@webrtc.org Link to the patchset: https://codereview.webrtc.org/2163683003/#ps260001 (title: "Fixing comment grammar.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by deadbeef@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Relanding: Allow the DTLS fingerprint verification to occur after the handshake. This means the DTLS handshake can make progress while the SDP answer containing the fingerprint is still in transit. If the signaling path if significantly slower than the media path, this can have a moderate impact on call setup time. Of course, until the fingerprint is verified no media can be sent. Any attempted write will result in SR_BLOCK. This essentially fulfills the requirements of RFC 4572, Section 6.2: Note that when the offer/answer model is being used, it is possible for a media connection to outrace the answer back to the offerer. Thus, if the offerer has offered a 'setup:passive' or 'setup:actpass' role, it MUST (as specified in RFC 4145 [2]) begin listening for an incoming connection as soon as it sends its offer. However, it MUST NOT assume that the data transmitted over the TLS connection is valid until it has received a matching fingerprint in an SDP answer. If the fingerprint, once it arrives, does not match the client's certificate, the server endpoint MUST terminate the media connection with a bad_certificate error, as stated in the previous paragraph. BUG=webrtc:6387 ========== to ========== Relanding: Allow the DTLS fingerprint verification to occur after the handshake. This means the DTLS handshake can make progress while the SDP answer containing the fingerprint is still in transit. If the signaling path if significantly slower than the media path, this can have a moderate impact on call setup time. Of course, until the fingerprint is verified no media can be sent. Any attempted write will result in SR_BLOCK. This essentially fulfills the requirements of RFC 4572, Section 6.2: Note that when the offer/answer model is being used, it is possible for a media connection to outrace the answer back to the offerer. Thus, if the offerer has offered a 'setup:passive' or 'setup:actpass' role, it MUST (as specified in RFC 4145 [2]) begin listening for an incoming connection as soon as it sends its offer. However, it MUST NOT assume that the data transmitted over the TLS connection is valid until it has received a matching fingerprint in an SDP answer. If the fingerprint, once it arrives, does not match the client's certificate, the server endpoint MUST terminate the media connection with a bad_certificate error, as stated in the previous paragraph. BUG=webrtc:6387 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Relanding: Allow the DTLS fingerprint verification to occur after the handshake. This means the DTLS handshake can make progress while the SDP answer containing the fingerprint is still in transit. If the signaling path if significantly slower than the media path, this can have a moderate impact on call setup time. Of course, until the fingerprint is verified no media can be sent. Any attempted write will result in SR_BLOCK. This essentially fulfills the requirements of RFC 4572, Section 6.2: Note that when the offer/answer model is being used, it is possible for a media connection to outrace the answer back to the offerer. Thus, if the offerer has offered a 'setup:passive' or 'setup:actpass' role, it MUST (as specified in RFC 4145 [2]) begin listening for an incoming connection as soon as it sends its offer. However, it MUST NOT assume that the data transmitted over the TLS connection is valid until it has received a matching fingerprint in an SDP answer. If the fingerprint, once it arrives, does not match the client's certificate, the server endpoint MUST terminate the media connection with a bad_certificate error, as stated in the previous paragraph. BUG=webrtc:6387 ========== to ========== Relanding: Allow the DTLS fingerprint verification to occur after the handshake. This means the DTLS handshake can make progress while the SDP answer containing the fingerprint is still in transit. If the signaling path if significantly slower than the media path, this can have a moderate impact on call setup time. Of course, until the fingerprint is verified no media can be sent. Any attempted write will result in SR_BLOCK. This essentially fulfills the requirements of RFC 4572, Section 6.2: Note that when the offer/answer model is being used, it is possible for a media connection to outrace the answer back to the offerer. Thus, if the offerer has offered a 'setup:passive' or 'setup:actpass' role, it MUST (as specified in RFC 4145 [2]) begin listening for an incoming connection as soon as it sends its offer. However, it MUST NOT assume that the data transmitted over the TLS connection is valid until it has received a matching fingerprint in an SDP answer. If the fingerprint, once it arrives, does not match the client's certificate, the server endpoint MUST terminate the media connection with a bad_certificate error, as stated in the previous paragraph. BUG=webrtc:6387 Committed: https://crrev.com/89824f6fe05b43876550812c35195cf0f1237ccc Cr-Commit-Position: refs/heads/master@{#14461} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/89824f6fe05b43876550812c35195cf0f1237ccc Cr-Commit-Position: refs/heads/master@{#14461} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
