Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2557803002:
Add disabled certificate check support to IceServer PeerConnection API. (Closed)
DescriptionAdd disabled certificate check support to IceServer PeerConnection API.
Refactor "OPT_SSLTCP" renaming it to "OPT_TLS_FAKE", making it clear
that it's not actually some kind of SSL over TCP. Also making it clear
that it's mutually exclusive with OPT_TLS.
Add "OPT_TLS_INSECURE" that implements the new certificate-check
disabled TLS mode, which is also mutually exclusive with the other
TLS options.
PortAllocator: Add a new TLS policy enum TlsCertPolicy which defines
the new insecure mode and added it as a RelayCredentials member.
TurnPort: Add new TLS policy member with appropriate getter and setter
to avoid constructor bloat. Initialize it from the RelayCredentials
after the TurnPort is created.
Expose the new feature in the PeerConnection API via
IceServer.tls_certificate_policy as well as via the Android JNI
PeerConnection API.
For security reasons we ensure that:
1) The policy is always explicitly initialized to secure.
2) API users have to explicitly integrate with the feature to
use it, and will otherwise get no change in behavior.
3) The feature is not immediately exposed in non-native
contexts. For example, disabling of certificate validation
is not implemented via URI parsing since this would
immediately allow it to be used from a web page.
BUG=webrtc:6840
Review-Url: https://codereview.webrtc.org/2557803002
Cr-Commit-Position: refs/heads/master@{#15670}
Committed: https://chromium.googlesource.com/external/webrtc/+/b0f04fdb9eb8b28424b7f80e38f4936e5c501b7b
Patch Set 1 #Patch Set 2 : Add disabled certificate check support to IceServer PeerConnection API. #
Total comments: 38
Patch Set 3 : Address review comments. #Patch Set 4 : New insecure check support patch set with no PROTO refactoring. #Patch Set 5 : Fix unused tlsOpts with no assert with new atMostOneTLSOptionIsSet() fn. #Patch Set 6 : Fix unused tlsOpts with no assert, second try with ATTRIBUTE_UNUSED. #
Total comments: 5
Patch Set 7 : JNI API: rename TlsCertificatePolicy -> TlsCertPolicy. #Patch Set 8 : Get rid of tlsOpts unused warning once and for all by actually using it. #
Messages
Total messages: 58 (29 generated)
The CQ bit was checked by hnsl@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/13161) ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/13172) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/15518) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/15376) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/19459)
Description was changed from ========== Add disabled certificate check support to IceServer PeerConnection API. Refactor "secure bool" to new ProtocolType PROTO_TLS. It makes more sense since port.h already has a PROTO_SSLTCP and TLS is a separate, very specific protocol. In the future we may also want to add PROTO_DTLS. Add new ProtocolFlags type for setting flags that configure a chosen protocol. We use ProtocolFlags to support a new protocol option "PROTO_FLAG_INSECURE_CERT_CHECK" which disables certificate validation. It can be set through the PeerConnection API via IceServer.tls_certificate_policy as well as via the Android JNI PC API. It is not possible to set this flag via the URL string. PC API users have to actively integrate with the feature to use it for security reasons. Integrated ParseIceServerUrl with new PROTO_TLS and new PROTO_FLAG_INSECURE_CERT_CHECK. BUG=webrtc:6840 ========== to ========== Add disabled certificate check support to IceServer PeerConnection API. Refactor "secure bool" to new ProtocolType PROTO_TLS. It makes more sense since port.h already has a PROTO_SSLTCP and TLS is a separate, very specific protocol. In the future we may also want to add PROTO_DTLS. Add new ProtocolFlags type for setting flags that configure a chosen protocol. We use ProtocolFlags to support a new protocol option "PROTO_FLAG_INSECURE_CERT_CHECK" which disables certificate validation. It can be set through the PeerConnection API via IceServer.tls_certificate_policy as well as via the Android JNI PC API. It is not possible to set this flag via the URL string. PC API users have to actively integrate with the feature to use it for security reasons. Integrated ParseIceServerUrl with new PROTO_TLS and new PROTO_FLAG_INSECURE_CERT_CHECK. BUG=webrtc:6840 ==========
hnsl@webrtc.org changed reviewers: + pthatcher@webrtc.org
Hi, please review this. Note that try bots are failing because they can't patch webrtc/api/android/ ...I'm investigating.
The CQ bit was checked by hnsl@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
hnsl@webrtc.org changed reviewers: + deadbeef@webrtc.org
Adding deadbeef as reviewer due to timeout.
https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:316: turn_transport_type, flags)); Attaching flags onto relay servers feels like the wrong way to do this. It should instead be a PortAllocator flag. See all the other flags we have here: https://cs.chromium.org/chromium/src/third_party/webrtc/p2p/base/portallocato.... https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:183: TlsCertificatePolicy tls_certificate_policy; Again, this shouldn't be an ICE server attribute. It should be something passed in from PeerConnection's RTCConfig that then goes down into the PortAllocator. There are a lot of similar such things, as you can see here: https://cs.chromium.org/chromium/src/third_party/webrtc/api/peerconnection.cc... https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/webrtcsdp.cc File webrtc/api/webrtcsdp.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/webrtcsdp.cc#n... webrtc/api/webrtcsdp.cc:1036: protocol == cricket::PROTO_TLS) { It seems like we should have StringToProtocol and StringToRelayProtocol, since they apparently do different things. That would be more clear here. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/packetsoc... File webrtc/p2p/base/packetsocketfactory.h (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/packetsoc... webrtc/p2p/base/packetsocketfactory.h:28: OPT_INSECURE_CERT_CHECK = 0x08, // Disable certificate validation. I think this would be cleaner as: OPT_FAKE_SSL = 0x01, // Fake SSL OPT_TLS = 0x02, ... OPT_INSECURE_TLS = 0x04 And OPT_INSECURE_TLS is OPT_TLS with insecure CERT check. Instead of an option on TLS, it's just a different mode of TLS (like FAKE_SSL) is. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:96: TLS_PROTOCOL_NAME}; We should probably document what these things mean. First, that they are used for ICE candidate protocol and ICE candidate relay protocol. UDP and TCP are clear. SSLTCP means to use a "fake SSL" connection over TCP (see AsyncSSLSocket). This new thing (TLS) is real TLS over TCP. But it's only used by the ICE candidate relay protocol, not ICE candidate protocol. In other words, we have it implemented in turnport.cc, but not tcpport.cc. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/port_unit... File webrtc/p2p/base/port_unittest.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/port_unit... webrtc/p2p/base/port_unittest.cc:560: return "turn(tls)"; Can you make the style here consistent? And below. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/portinter... File webrtc/p2p/base/portinterface.h (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/portinter... webrtc/p2p/base/portinterface.h:34: PROTO_TLS, Same here with PROTO_INSECURE_TLS and PROTO_FAKE_SSL. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/relayport.cc File webrtc/p2p/base/relayport.cc (left): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/relayport... webrtc/p2p/base/relayport.cc:220: } Why did you both changing this? RelayPort is deprecated anyway and only used by one thing that will hopefully not be using it soon. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/turnport.... webrtc/p2p/base/turnport.cc:332: } I like the cleanup/refactoring going on here (the addition of PROTO_TLS and ICE_TYPE_PREFERENCE_RELAY_X). I think it would be easier to make that into a separate CL with no behavior change, and then a second CL that's the behavior change. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/peerconnection_jni.cc:1574: server.tls_certificate_policy = tls_certificate_policy; If my idea works, you shouldn't any of this new Java/JNI code.
Looks good. You have my l-g-t-m after you add comments, add the old RelayServerConfig constructor and optionally split the CL. I don't have an opinion about flags/enums/etc.; I'm happy with what you did so I'll let you and Peter hash it out. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:316: turn_transport_type, flags)); On 2016/12/07 21:29:35, pthatcher1 wrote: > Attaching flags onto relay servers feels like the wrong way to do this. > > It should instead be a PortAllocator flag. See all the other flags we have > here: > https://cs.chromium.org/chromium/src/third_party/webrtc/p2p/base/portallocato.... > I disagree. "Verify certificate or not?" is fundamentally a property of an individual TLS connection, not the whole PeerConnection/PortAllocator. I think this is a more intuitive and flexible interface, and doesn't have any downsides I can see. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:174: kTlsCertPolicyInsecureNoCheck, Add comments explaining what each of these values implies. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:183: TlsCertificatePolicy tls_certificate_policy; I'd have this default to kTlsCertPolicySecure, as an extra precaution. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/webrtcsdp.cc File webrtc/api/webrtcsdp.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/webrtcsdp.cc#n... webrtc/api/webrtcsdp.cc:1036: protocol == cricket::PROTO_TLS) { On 2016/12/07 21:29:35, pthatcher1 wrote: > It seems like we should have StringToProtocol and StringToRelayProtocol, since > they apparently do different things. That would be more clear here. I don't follow this comment. What does different things? https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:96: TLS_PROTOCOL_NAME}; On 2016/12/07 21:29:35, pthatcher1 wrote: > We should probably document what these things mean. First, that they are used > for ICE candidate protocol and ICE candidate relay protocol. UDP and TCP are > clear. SSLTCP means to use a "fake SSL" connection over TCP (see > AsyncSSLSocket). This new thing (TLS) is real TLS over TCP. But it's only used > by the ICE candidate relay protocol, not ICE candidate protocol. In other > words, we have it implemented in turnport.cc, but not tcpport.cc. I agree about documenting what these things mean. But we should document how they're used in the places they're used, separately. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:86: ICE_TYPE_PREFERENCE_RELAY_UDP = 2, Thanks for doing this; preferably should be in a small standalone CL though. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/portalloc... File webrtc/p2p/base/portallocator.h (left): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/portalloc... webrtc/p2p/base/portallocator.h:123: bool secure) You should keep this constructor around for backwards compatibility. Even though PortAllocator isn't in /api/, it's effectively part of the public API since it's referenced by PeerConnectionInterface. I think things would break if you removed it. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/turnport.... webrtc/p2p/base/turnport.cc:332: } On 2016/12/07 21:29:36, pthatcher1 wrote: > I like the cleanup/refactoring going on here (the addition of PROTO_TLS and > ICE_TYPE_PREFERENCE_RELAY_X). I think it would be easier to make that into a > separate CL with no behavior change, and then a second CL that's the behavior > change. I agree, if it's not too hard to pull the two apart. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/sdk/android/api/or... File webrtc/sdk/android/api/org/webrtc/PeerConnection.java (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/sdk/android/api/or... webrtc/sdk/android/api/org/webrtc/PeerConnection.java:1: /* May be easier to do the Java part in a separate CL.
https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:316: turn_transport_type, flags)); Perhaps I misunderstood the use case we're trying to support. I thought the point of the flag was to allow a given PeerConnection to not check TLS certs for all TURN connections. But you're saying you want to allow a PeerConnection to allow some TURN servers to be checked and others to not be checked. Is that really a needed use case? It seems odd to want to do that, and it adds more complexity to the API and code. Perhaps the benefit of that use case is worth the cost. But it doesn't seem so to me. Also, where does the flag come from anyway? It doesn't come from parsing the TURN URL. Is there going to be some other mechanism for passing in TURN server configs that includes the policy? On 2016/12/08 01:36:40, Taylor Brandstetter wrote: > On 2016/12/07 21:29:35, pthatcher1 wrote: > > Attaching flags onto relay servers feels like the wrong way to do this. > > > > It should instead be a PortAllocator flag. See all the other flags we have > > here: > > > https://cs.chromium.org/chromium/src/third_party/webrtc/p2p/base/portallocato.... > > > > I disagree. "Verify certificate or not?" is fundamentally a property of an > individual TLS connection, not the whole PeerConnection/PortAllocator. I think > this is a more intuitive and flexible interface, and doesn't have any downsides > I can see. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/webrtcsdp.cc File webrtc/api/webrtcsdp.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/webrtcsdp.cc#n... webrtc/api/webrtcsdp.cc:1036: protocol == cricket::PROTO_TLS) { On 2016/12/08 01:36:40, Taylor Brandstetter wrote: > On 2016/12/07 21:29:35, pthatcher1 wrote: > > It seems like we should have StringToProtocol and StringToRelayProtocol, since > > they apparently do different things. That would be more clear here. > > I don't follow this comment. What does different things? Nevermind. I misunderstood this code.
https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:316: turn_transport_type, flags)); On 2016/12/08 02:36:13, pthatcher1 wrote: > Perhaps I misunderstood the use case we're trying to support. I thought the > point of the flag was to allow a given PeerConnection to not check TLS certs for > all TURN connections. But you're saying you want to allow a PeerConnection to > allow some TURN servers to be checked and others to not be checked. Is that > really a needed use case? It seems odd to want to do that, and it adds more > complexity to the API and code. Perhaps the benefit of that use case is worth > the cost. But it doesn't seem so to me. Maybe there's no obvious use case right now. But: - It's a more intuitive API to me, since a flag that controls a property of the connection to the TURN server is in the same place as the TURN server URL. - The addition to the API surface is the same (one new flag in one structure). - The complexity/maintenance cost is lower in my estimation (PortAllocator is a really unfortunate part of the API, I'd like to redo it). > Also, where does the flag come from anyway? It doesn't come from parsing the > TURN URL. Is there going to be some other mechanism for passing in TURN server > configs that includes the policy? It comes from the ICE server structure in RTCConfiguration. Even if we decide that doing this per-server isn't an API we want to support: from an implementation perspective, I'd prefer for the flag to be on the RelayServerConfig, and not use the PortAllocator flags.
Please review split PROTO_TLS refactor CL here: https://codereview.webrtc.org/2568833002/ This CL will be updated with a new rebased patchset after that CL has been landed. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:316: turn_transport_type, flags)); On 2016/12/08 18:51:39, Taylor Brandstetter wrote: > On 2016/12/08 02:36:13, pthatcher1 wrote: > > Perhaps I misunderstood the use case we're trying to support. I thought the > > point of the flag was to allow a given PeerConnection to not check TLS certs > for > > all TURN connections. But you're saying you want to allow a PeerConnection to > > allow some TURN servers to be checked and others to not be checked. Is that > > really a needed use case? It seems odd to want to do that, and it adds more > > complexity to the API and code. Perhaps the benefit of that use case is worth > > the cost. But it doesn't seem so to me. > > Maybe there's no obvious use case right now. But: > > - It's a more intuitive API to me, since a flag that controls a property of the > connection to the TURN server is in the same place as the TURN server URL. > - The addition to the API surface is the same (one new flag in one structure). > - The complexity/maintenance cost is lower in my estimation (PortAllocator is a > really unfortunate part of the API, I'd like to redo it). > > > Also, where does the flag come from anyway? It doesn't come from parsing the > > TURN URL. Is there going to be some other mechanism for passing in TURN > server > > configs that includes the policy? > > It comes from the ICE server structure in RTCConfiguration. > > Even if we decide that doing this per-server isn't an API we want to support: > from an implementation perspective, I'd prefer for the flag to be on the > RelayServerConfig, and not use the PortAllocator flags. Imagine a scenario where you have a RTCConfiguration TURNS server with a sensitive TURN username and password which is expected to validate the TLS certificate, and you also have a TURNS server with a non-sensitive TURN username and password which is expected to have a self-signed TLS certificate. If you moved this flag to PeerConnection/PortAllocator level you would force users to either send a sensitive TURN username and password insecurely or not use them in the same RTCConfiguration. That design would be strictly worse and shows that this flags fundamentally an IceServer level configuration. I will opt to keep this flag on IceServer and RelayServerConfig level. And I'd say that yes, this is what we want to do and the above scenario is already likely in the short-term today. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:174: kTlsCertPolicyInsecureNoCheck, On 2016/12/08 01:36:40, Taylor Brandstetter wrote: > Add comments explaining what each of these values implies. Done. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:183: TlsCertificatePolicy tls_certificate_policy; On 2016/12/08 01:36:40, Taylor Brandstetter wrote: > I'd have this default to kTlsCertPolicySecure, as an extra precaution. Done. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:183: TlsCertificatePolicy tls_certificate_policy; On 2016/12/07 21:29:35, pthatcher1 wrote: > Again, this shouldn't be an ICE server attribute. It should be something passed > in from PeerConnection's RTCConfig that then goes down into the PortAllocator. > There are a lot of similar such things, as you can see here: > > https://cs.chromium.org/chromium/src/third_party/webrtc/api/peerconnection.cc... Nack, see my other response about API design. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/webrtcsdp.cc File webrtc/api/webrtcsdp.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/webrtcsdp.cc#n... webrtc/api/webrtcsdp.cc:1036: protocol == cricket::PROTO_TLS) { On 2016/12/08 02:36:13, pthatcher1 wrote: > On 2016/12/08 01:36:40, Taylor Brandstetter wrote: > > On 2016/12/07 21:29:35, pthatcher1 wrote: > > > It seems like we should have StringToProtocol and StringToRelayProtocol, > since > > > they apparently do different things. That would be more clear here. > > > > I don't follow this comment. What does different things? > > Nevermind. I misunderstood this code. Acknowledged. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/packetsoc... File webrtc/p2p/base/packetsocketfactory.h (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/packetsoc... webrtc/p2p/base/packetsocketfactory.h:28: OPT_INSECURE_CERT_CHECK = 0x08, // Disable certificate validation. On 2016/12/07 21:29:35, pthatcher1 wrote: > I think this would be cleaner as: > > OPT_FAKE_SSL = 0x01, // Fake SSL > OPT_TLS = 0x02, > ... > OPT_INSECURE_TLS = 0x04 > > > And OPT_INSECURE_TLS is OPT_TLS with insecure CERT check. Instead of an option > on TLS, it's just a different mode of TLS (like FAKE_SSL) is. If you want to rename OPT_SSLTCP to OPT_FAKE_SSL, I think that could be done in a separate CL. I disagree that OPT_SSLTCP/OPT_FAKE_SSL is a different mode of TLS. OPT_FAKE_SSL is a different mode of TCP, it's not TLS at all. Although you could obviously also argue that TLS is a different mode of TCP as well, but SSLTCP is closer to a TCP mode than a TLS mode. I agree OPT_INSECURE_TLS is a better name, changing. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:96: TLS_PROTOCOL_NAME}; On 2016/12/08 01:36:40, Taylor Brandstetter wrote: > On 2016/12/07 21:29:35, pthatcher1 wrote: > > We should probably document what these things mean. First, that they are used > > for ICE candidate protocol and ICE candidate relay protocol. UDP and TCP are > > clear. SSLTCP means to use a "fake SSL" connection over TCP (see > > AsyncSSLSocket). This new thing (TLS) is real TLS over TCP. But it's only > used > > by the ICE candidate relay protocol, not ICE candidate protocol. In other > > words, we have it implemented in turnport.cc, but not tcpport.cc. > > I agree about documenting what these things mean. But we should document how > they're used in the places they're used, separately. I agree there are different meanings here that should be documented and possibly cleaned up. If tcpport ends up having some other meaning of "tls" it can fork the ProtocolType type into a new type. For now this should not affect tcpport since SupportsProtocol() will return false for "tls" candidates. But I think this cleanup and documentation can be added in a future CL. The different meanings where already there before this CL (candidate protocol vs relay client protocol). https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:86: ICE_TYPE_PREFERENCE_RELAY_UDP = 2, On 2016/12/08 01:36:40, Taylor Brandstetter wrote: > Thanks for doing this; preferably should be in a small standalone CL though. Acknowledged. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/port_unit... File webrtc/p2p/base/port_unittest.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/port_unit... webrtc/p2p/base/port_unittest.cc:560: return "turn(tls)"; On 2016/12/07 21:29:35, pthatcher1 wrote: > Can you make the style here consistent? And below. Done in separate CL. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/portalloc... File webrtc/p2p/base/portallocator.h (left): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/portalloc... webrtc/p2p/base/portallocator.h:123: bool secure) On 2016/12/08 01:36:40, Taylor Brandstetter wrote: > You should keep this constructor around for backwards compatibility. Even though > PortAllocator isn't in /api/, it's effectively part of the public API since it's > referenced by PeerConnectionInterface. I think things would break if you removed > it. Done. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/portinter... File webrtc/p2p/base/portinterface.h (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/portinter... webrtc/p2p/base/portinterface.h:34: PROTO_TLS, On 2016/12/07 21:29:35, pthatcher1 wrote: > Same here with PROTO_INSECURE_TLS and PROTO_FAKE_SSL. Acknowledged. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/relayport.cc File webrtc/p2p/base/relayport.cc (left): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/relayport... webrtc/p2p/base/relayport.cc:220: } On 2016/12/07 21:29:35, pthatcher1 wrote: > Why did you both changing this? RelayPort is deprecated anyway and only used by > one thing that will hopefully not be using it soon. Done. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/turnport.... webrtc/p2p/base/turnport.cc:332: } On 2016/12/08 01:36:40, Taylor Brandstetter wrote: > On 2016/12/07 21:29:36, pthatcher1 wrote: > > I like the cleanup/refactoring going on here (the addition of PROTO_TLS and > > ICE_TYPE_PREFERENCE_RELAY_X). I think it would be easier to make that into a > > separate CL with no behavior change, and then a second CL that's the behavior > > change. > > I agree, if it's not too hard to pull the two apart. Okay, I'll prepare a new CL where I do the PROTO_TLS refactoring, which I will land first. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/peerconnection_jni.cc:1574: server.tls_certificate_policy = tls_certificate_policy; On 2016/12/07 21:29:36, pthatcher1 wrote: > If my idea works, you shouldn't any of this new Java/JNI code. Acknowledged.
https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:316: turn_transport_type, flags)); On 2016/12/12 16:08:13, hnsl1 wrote: > On 2016/12/08 18:51:39, Taylor Brandstetter wrote: > > On 2016/12/08 02:36:13, pthatcher1 wrote: > > > Perhaps I misunderstood the use case we're trying to support. I thought > the > > > point of the flag was to allow a given PeerConnection to not check TLS certs > > for > > > all TURN connections. But you're saying you want to allow a PeerConnection > to > > > allow some TURN servers to be checked and others to not be checked. Is > that > > > really a needed use case? It seems odd to want to do that, and it adds more > > > complexity to the API and code. Perhaps the benefit of that use case is > worth > > > the cost. But it doesn't seem so to me. > > > > Maybe there's no obvious use case right now. But: > > > > - It's a more intuitive API to me, since a flag that controls a property of > the > > connection to the TURN server is in the same place as the TURN server URL. > > - The addition to the API surface is the same (one new flag in one structure). > > - The complexity/maintenance cost is lower in my estimation (PortAllocator is > a > > really unfortunate part of the API, I'd like to redo it). > > > > > Also, where does the flag come from anyway? It doesn't come from parsing > the > > > TURN URL. Is there going to be some other mechanism for passing in TURN > > server > > > configs that includes the policy? > > > > It comes from the ICE server structure in RTCConfiguration. > > > > Even if we decide that doing this per-server isn't an API we want to support: > > from an implementation perspective, I'd prefer for the flag to be on the > > RelayServerConfig, and not use the PortAllocator flags. > > Imagine a scenario where you have a RTCConfiguration TURNS server with a > sensitive TURN username and password which is expected to validate the TLS > certificate, and you also have a TURNS server with a non-sensitive TURN username > and password which is expected to have a self-signed TLS certificate. If you > moved this flag to PeerConnection/PortAllocator level you would force users to > either send a sensitive TURN username and password insecurely or not use them in > the same RTCConfiguration. That design would be strictly worse and shows that > this flags fundamentally an IceServer level configuration. I will opt to keep > this flag on IceServer and RelayServerConfig level. > > And I'd say that yes, this is what we want to do and the above scenario is > already likely in the short-term today. OK, I think I understand the use case. Let's talk about the API. I think we have a few options: A. Put it in the protocol, like TURN vs. TURNS: server = { uri: "TURNSU://...", } B. Put it in a URL parameter: server = { uri: "TURNS://...?verify_cert=false", } C. Put it in a separate field server = { uri: "TURNS://...", verify_cert: false } D. Put it in a flag server = { uri: "TURNS://...", flags: DONT_VERIFY_CERT | X | Y } Out of these, I don't like D. And C+D would be the most difficult to get into the web API, while A+B would be the easiest. I kind of like A, but it also feels a bit hacky. B feels like it fits into the existing structure the best. However, A+B do have one big problem: if you go get a TURN URI from a TURN server it could be evil and give you a URI with that param on there and you might not notice unless you go and check it, which is bad. So, C seems the best, which is what you have, at least for the first half. I'd suggest you push that down into the RelayServerConfig as well as tls_certificate_policy and not as a flags. https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/packetsoc... File webrtc/p2p/base/packetsocketfactory.h (right): https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/packetsoc... webrtc/p2p/base/packetsocketfactory.h:28: OPT_INSECURE_CERT_CHECK = 0x08, // Disable certificate validation. On 2016/12/12 16:08:13, hnsl1 wrote: > On 2016/12/07 21:29:35, pthatcher1 wrote: > > I think this would be cleaner as: > > > > OPT_FAKE_SSL = 0x01, // Fake SSL > > OPT_TLS = 0x02, > > ... > > OPT_INSECURE_TLS = 0x04 > > > > > > And OPT_INSECURE_TLS is OPT_TLS with insecure CERT check. Instead of an > option > > on TLS, it's just a different mode of TLS (like FAKE_SSL) is. > > If you want to rename OPT_SSLTCP to OPT_FAKE_SSL, I think that could be done in > a separate CL. > > I disagree that OPT_SSLTCP/OPT_FAKE_SSL is a different mode of TLS. OPT_FAKE_SSL > is a different mode of TCP, it's not TLS at all. Although you could obviously > also argue that TLS is a different mode of TCP as well, but SSLTCP is closer to > a TCP mode than a TLS mode. > > I agree OPT_INSECURE_TLS is a better name, changing. The point I was trying to make is that flattening the modes (Real TLS, Fake SSL, Unverified TLS) into one dimension would be cleaner than having multiple dimensions (Real TLS, Fake SSL) + (Unverified TLS). The names were secondary.
On 2016/12/12 23:12:29, pthatcher1 wrote:
>
> OK, I think I understand the use case. Let's talk about the API. I think we
> have a few options:
>
> A. Put it in the protocol, like TURN vs. TURNS:
>
> server = {
> uri: "TURNSU://...",
> }
>
> B. Put it in a URL parameter:
>
> server = {
> uri: "TURNS://...?verify_cert=false",
> }
>
> C. Put it in a separate field
> server = {
> uri: "TURNS://...",
> verify_cert: false
> }
>
>
> D. Put it in a flag
> server = {
> uri: "TURNS://...",
> flags: DONT_VERIFY_CERT | X | Y
> }
>
>
>
> Out of these, I don't like D. And C+D would be the most difficult to get into
> the web API, while A+B would be the easiest. I kind of like A, but it also
> feels a bit hacky. B feels like it fits into the existing structure the
>
> However, A+B do have one big problem: if you go get a TURN URI from a TURN
> server it could be evil and give you a URI with that param on there and you
> might not notice unless you go and check it, which is bad.
>
> So, C seems the best, which is what you have, at least for the first half.
I'd
> suggest you push that down into the RelayServerConfig as well as
> tls_certificate_policy and not as a flags.
My idea was actually not adding this feature to the Web API for now because I
feel like that would have a whole bunch of security implications I don't want to
go in to right now. Sorry if this was unclear. This is why I avoided A and B,
because that would integrate the feature immediately with the Web API. So to me
that is a separate discussion that would be part of a future proposal.
Initially, it would just be a flag that native WebRTC applications can use.
Having tls_certificate_policy in RelayServerConfig instead of more generic flags
makes sense, I'll change that.
>
>
https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/packetsoc...
> File webrtc/p2p/base/packetsocketfactory.h (right):
>
>
https://codereview.webrtc.org/2557803002/diff/20001/webrtc/p2p/base/packetsoc...
> webrtc/p2p/base/packetsocketfactory.h:28: OPT_INSECURE_CERT_CHECK = 0x08, //
> Disable certificate validation.
> On 2016/12/12 16:08:13, hnsl1 wrote:
> > On 2016/12/07 21:29:35, pthatcher1 wrote:
> > > I think this would be cleaner as:
> > >
> > > OPT_FAKE_SSL = 0x01, // Fake SSL
> > > OPT_TLS = 0x02,
> > > ...
> > > OPT_INSECURE_TLS = 0x04
> > >
> > >
> > > And OPT_INSECURE_TLS is OPT_TLS with insecure CERT check. Instead of an
> > option
> > > on TLS, it's just a different mode of TLS (like FAKE_SSL) is.
> >
> > If you want to rename OPT_SSLTCP to OPT_FAKE_SSL, I think that could be done
> in
> > a separate CL.
> >
> > I disagree that OPT_SSLTCP/OPT_FAKE_SSL is a different mode of TLS.
> OPT_FAKE_SSL
> > is a different mode of TCP, it's not TLS at all. Although you could
obviously
> > also argue that TLS is a different mode of TCP as well, but SSLTCP is closer
> to
> > a TCP mode than a TLS mode.
> >
> > I agree OPT_INSECURE_TLS is a better name, changing.
>
> The point I was trying to make is that flattening the modes (Real TLS, Fake
SSL,
> Unverified TLS) into one dimension would be cleaner than having multiple
> dimensions (Real TLS, Fake SSL) + (Unverified TLS). The names were secondary.
Ah, I see what you mean. You want to strengthen the existing either-or
relationships and make them more explicit. I saw the existing either-or
constraints more like "unimplemented by design" rather than "undefined by
design", and continued the design in that fashion. But I can change that.
Hi, please review my latest patch set which does not include PROTO refactoring and which has replaced the flag implementation with one that stores the certificate policy on RelayServerConfig level.
Description was changed from
==========
Add disabled certificate check support to IceServer PeerConnection API.
Refactor "secure bool" to new ProtocolType PROTO_TLS. It makes more
sense since port.h already has a PROTO_SSLTCP and TLS is a separate,
very specific protocol. In the future we may also want to add
PROTO_DTLS.
Add new ProtocolFlags type for setting flags that configure a chosen
protocol.
We use ProtocolFlags to support a new protocol option
"PROTO_FLAG_INSECURE_CERT_CHECK" which disables certificate validation.
It can be set through the PeerConnection API via
IceServer.tls_certificate_policy as well as via the Android JNI PC API.
It is not possible to set this flag via the URL string. PC API users
have to actively integrate with the feature to use it for security
reasons.
Integrated ParseIceServerUrl with new PROTO_TLS and new
PROTO_FLAG_INSECURE_CERT_CHECK.
BUG=webrtc:6840
==========
to
==========
Add disabled certificate check support to IceServer PeerConnection API.
Refactor "OPT_SSLTCP" renaming it to "OPT_TLS_FAKE", making it clear
that it's not actually some kind of SSL over TCP. Also making it clear
that it's mutually exclusive with OPT_TLS.
Add "OPT_TLS_INSECURE" that implements the new certificate-check
disabled TLS mode, which is also mutually exclusive with the other
TLS options.
PortAllocator: Add a new TLS policy enum TlsCertPolicy which defines
the new insecure mode and added it as a RelayCredentials member.
TurnPort: Add new TLS policy member with appropriate getter and setter
to avoid constructor bloat. Initialize it from the RelayCredentials
after the TurnPort is created.
Expose the new feature in the PeerConnection API via
IceServer.tls_certificate_policy as well as via the Android JNI
PeerConnection API.
For security reasons we ensure that:
1) The policy is always explicitly initialized to secure.
2) API users have to explicitly integrate with the feature to
use it, and will otherwise get no change in behavior.
3) The feature is not immediately exposed in non-native
contexts. For example, disabling of certificate validation
is not implemented via URI parsing since this would
immediately allow it to be used from a web page.
BUG=webrtc:6840
==========
lgtm
The CQ bit was checked by hnsl@webrtc.org
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: ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/19750)
The CQ bit was checked by hnsl@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_compile_mips_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_mips_db...)
The CQ bit was checked by hnsl@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2557803002/#ps100001 (title: "Fix unused tlsOpts with no assert, second try with ATTRIBUTE_UNUSED.")
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/11479)
hnsl@webrtc.org changed reviewers: + magjed@webrtc.org
Magnus, please review changes to webrtc/sdk/android/*.
lgtm % comments https://codereview.webrtc.org/2557803002/diff/100001/webrtc/sdk/android/src/j... File webrtc/sdk/android/src/jni/classreferenceholder.cc (right): https://codereview.webrtc.org/2557803002/diff/100001/webrtc/sdk/android/src/j... webrtc/sdk/android/src/jni/classreferenceholder.cc:85: LoadClass(jni, "org/webrtc/PeerConnection$TlsCertificatePolicy"); There is no need to add this unless you want to hold global class references to TlsCertificatePolicy. https://codereview.webrtc.org/2557803002/diff/100001/webrtc/sdk/android/src/j... File webrtc/sdk/android/src/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2557803002/diff/100001/webrtc/sdk/android/src/j... webrtc/sdk/android/src/jni/peerconnection_jni.cc:1569: static PeerConnectionInterface::TlsCertificatePolicy I think the type name should be TlsCertPolicy here.
magjed: PTAL https://codereview.webrtc.org/2557803002/diff/100001/webrtc/sdk/android/src/j... File webrtc/sdk/android/src/jni/classreferenceholder.cc (right): https://codereview.webrtc.org/2557803002/diff/100001/webrtc/sdk/android/src/j... webrtc/sdk/android/src/jni/classreferenceholder.cc:85: LoadClass(jni, "org/webrtc/PeerConnection$TlsCertificatePolicy"); On 2016/12/16 11:57:54, magjed_webrtc wrote: > There is no need to add this unless you want to hold global class references to > TlsCertificatePolicy. Without this line I get this runtime JNI crash: # Fatal error in ***/webrtc/sdk/android/src/jni/classreferenceholder.cc, line 115 # last system error: 11 # Check failed: it != classes_.end() # Unexpected GetClass() call for: org/webrtc/PeerConnection$TlsCertPolicy # A/libc: Fatal signal 6 (SIGABRT), code -6 in tid 23781 (Thread-16) *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** Revision: '0' ABI: 'arm' signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr -------- backtrace: ... #04 pc 00017014 /system/lib/libc.so (abort+4) #05 pc 00ebbd97 (_ZN3rtc12FatalMessageD2Ev+226) #06 pc 00243519 (_ZN10webrtc_jni20ClassReferenceHolder8GetClassERKNSt3__112basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEE+280) #07 pc 00243b31 (_ZN10webrtc_jni9FindClassEP7_JNIEnvPKc+80) #08 pc 00247c13 (_ZN10webrtc_jni15GetJavaEnumNameEP7_JNIEnvRKNSt3__112basic_stringIcNS2_11char_traitsIcEENS2_9allocatorIcEEEEP8_jobject+130) #09 pc 00254d85 (_ZN10webrtc_jniL33JavaTlsCertPolicyTypeToNativeTypeEP7_JNIEnvP8_jobject+76) #10 pc 0025513b (_ZN10webrtc_jniL30JavaIceServersToJsepIceServersEP7_JNIEnvP8_jobjectPNSt3__16vectorIN6webrtc23PeerConnectionInterface9IceServerENS4_9allocatorIS8_EEEE+330) #11 pc 00255a71 (_ZN10webrtc_jniL42JavaRTCConfigurationToJsepRTCConfigurationEP7_JNIEnvP8_jobjectPN6webrtc23PeerConnectionInterface16RTCConfigurationE+464) #12 pc 002568d9 (Java_org_webrtc_PeerConnection_setConfiguration+28) https://codereview.webrtc.org/2557803002/diff/100001/webrtc/sdk/android/src/j... File webrtc/sdk/android/src/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2557803002/diff/100001/webrtc/sdk/android/src/j... webrtc/sdk/android/src/jni/peerconnection_jni.cc:1569: static PeerConnectionInterface::TlsCertificatePolicy On 2016/12/16 11:57:54, magjed_webrtc wrote: > I think the type name should be TlsCertPolicy here. Done.
lgtm
https://codereview.webrtc.org/2557803002/diff/100001/webrtc/sdk/android/src/j... File webrtc/sdk/android/src/jni/classreferenceholder.cc (right): https://codereview.webrtc.org/2557803002/diff/100001/webrtc/sdk/android/src/j... webrtc/sdk/android/src/jni/classreferenceholder.cc:85: LoadClass(jni, "org/webrtc/PeerConnection$TlsCertificatePolicy"); On 2016/12/16 14:18:39, hnsl1 wrote: > On 2016/12/16 11:57:54, magjed_webrtc wrote: > > There is no need to add this unless you want to hold global class references > to > > TlsCertificatePolicy. > > Without this line I get this runtime JNI crash: > > # Fatal error in ***/webrtc/sdk/android/src/jni/classreferenceholder.cc, line > 115 > # last system error: 11 > # Check failed: it != classes_.end() > # Unexpected GetClass() call for: org/webrtc/PeerConnection$TlsCertPolicy > # > A/libc: Fatal signal 6 (SIGABRT), code -6 in tid 23781 (Thread-16) > *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** > Revision: '0' > ABI: 'arm' > signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr -------- > backtrace: > ... > #04 pc 00017014 /system/lib/libc.so (abort+4) > #05 pc 00ebbd97 (_ZN3rtc12FatalMessageD2Ev+226) > #06 pc 00243519 > (_ZN10webrtc_jni20ClassReferenceHolder8GetClassERKNSt3__112basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEE+280) > #07 pc 00243b31 (_ZN10webrtc_jni9FindClassEP7_JNIEnvPKc+80) > #08 pc 00247c13 > (_ZN10webrtc_jni15GetJavaEnumNameEP7_JNIEnvRKNSt3__112basic_stringIcNS2_11char_traitsIcEENS2_9allocatorIcEEEEP8_jobject+130) > #09 pc 00254d85 > (_ZN10webrtc_jniL33JavaTlsCertPolicyTypeToNativeTypeEP7_JNIEnvP8_jobject+76) > #10 pc 0025513b > (_ZN10webrtc_jniL30JavaIceServersToJsepIceServersEP7_JNIEnvP8_jobjectPNSt3__16vectorIN6webrtc23PeerConnectionInterface9IceServerENS4_9allocatorIS8_EEEE+330) > #11 pc 00255a71 > (_ZN10webrtc_jniL42JavaRTCConfigurationToJsepRTCConfigurationEP7_JNIEnvP8_jobjectPN6webrtc23PeerConnectionInterface16RTCConfigurationE+464) > #12 pc 002568d9 (Java_org_webrtc_PeerConnection_setConfiguration+28) You are still using the globally cached FindClass function in classreferenceholder.h from the GetJavaEnumName function. Can you try changing: jclass enumClass = FindClass(jni, className.c_str()); to: jclass enumClass = jni->FindClass(className.c_str()); in GetJavaEnumName in webrtc/sdk/android/src/jni/jni_helpers.cc? If that doesn't work, then it's ok to do it like this, and we can clean it up some other day. It's not a big deal, but I don't like the current design where we load almost every class at start time and store them globally here.
On 2016/12/19 08:54:05, magjed_webrtc wrote: > You are still using the globally cached FindClass function in > classreferenceholder.h from the GetJavaEnumName function. Can you try changing: > jclass enumClass = FindClass(jni, className.c_str()); > to: > jclass enumClass = jni->FindClass(className.c_str()); > in GetJavaEnumName in webrtc/sdk/android/src/jni/jni_helpers.cc? > > If that doesn't work, then it's ok to do it like this, and we can clean it up > some other day. It's not a big deal, but I don't like the current design where > we load almost every class at start time and store them globally here. I see what you mean, and I agree, but I don't think this is the right CL to touch jni_helpers.cc. Let's do it like this for now because that's how jni_helpers currently works and refactor in a new CL?
On 2016/12/19 08:59:36, hnsl1 wrote: > On 2016/12/19 08:54:05, magjed_webrtc wrote: > > > You are still using the globally cached FindClass function in > > classreferenceholder.h from the GetJavaEnumName function. Can you try > changing: > > jclass enumClass = FindClass(jni, className.c_str()); > > to: > > jclass enumClass = jni->FindClass(className.c_str()); > > in GetJavaEnumName in webrtc/sdk/android/src/jni/jni_helpers.cc? > > > > If that doesn't work, then it's ok to do it like this, and we can clean it up > > some other day. It's not a big deal, but I don't like the current design where > > we load almost every class at start time and store them globally here. > > I see what you mean, and I agree, but I don't think this is the right CL to > touch jni_helpers.cc. > > Let's do it like this for now because that's how jni_helpers currently works and > refactor in a new CL? Alright, go ahead and land it.
The CQ bit was checked by hnsl@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2557803002/#ps120001 (title: "JNI API: rename TlsCertificatePolicy -> TlsCertPolicy.")
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/17104)
The CQ bit was checked by hnsl@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, deadbeef@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2557803002/#ps140001 (title: "Get rid of tlsOpts unused warning once and for all by actually using it.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1482148254079340,
"parent_rev": "b6c456b7721529cea59078273bba365d64548825", "commit_rev":
"b0f04fdb9eb8b28424b7f80e38f4936e5c501b7b"}
Message was sent while issue was closed.
Description was changed from
==========
Add disabled certificate check support to IceServer PeerConnection API.
Refactor "OPT_SSLTCP" renaming it to "OPT_TLS_FAKE", making it clear
that it's not actually some kind of SSL over TCP. Also making it clear
that it's mutually exclusive with OPT_TLS.
Add "OPT_TLS_INSECURE" that implements the new certificate-check
disabled TLS mode, which is also mutually exclusive with the other
TLS options.
PortAllocator: Add a new TLS policy enum TlsCertPolicy which defines
the new insecure mode and added it as a RelayCredentials member.
TurnPort: Add new TLS policy member with appropriate getter and setter
to avoid constructor bloat. Initialize it from the RelayCredentials
after the TurnPort is created.
Expose the new feature in the PeerConnection API via
IceServer.tls_certificate_policy as well as via the Android JNI
PeerConnection API.
For security reasons we ensure that:
1) The policy is always explicitly initialized to secure.
2) API users have to explicitly integrate with the feature to
use it, and will otherwise get no change in behavior.
3) The feature is not immediately exposed in non-native
contexts. For example, disabling of certificate validation
is not implemented via URI parsing since this would
immediately allow it to be used from a web page.
BUG=webrtc:6840
==========
to
==========
Add disabled certificate check support to IceServer PeerConnection API.
Refactor "OPT_SSLTCP" renaming it to "OPT_TLS_FAKE", making it clear
that it's not actually some kind of SSL over TCP. Also making it clear
that it's mutually exclusive with OPT_TLS.
Add "OPT_TLS_INSECURE" that implements the new certificate-check
disabled TLS mode, which is also mutually exclusive with the other
TLS options.
PortAllocator: Add a new TLS policy enum TlsCertPolicy which defines
the new insecure mode and added it as a RelayCredentials member.
TurnPort: Add new TLS policy member with appropriate getter and setter
to avoid constructor bloat. Initialize it from the RelayCredentials
after the TurnPort is created.
Expose the new feature in the PeerConnection API via
IceServer.tls_certificate_policy as well as via the Android JNI
PeerConnection API.
For security reasons we ensure that:
1) The policy is always explicitly initialized to secure.
2) API users have to explicitly integrate with the feature to
use it, and will otherwise get no change in behavior.
3) The feature is not immediately exposed in non-native
contexts. For example, disabling of certificate validation
is not implemented via URI parsing since this would
immediately allow it to be used from a web page.
BUG=webrtc:6840
Review-Url: https://codereview.webrtc.org/2557803002
Cr-Commit-Position: refs/heads/master@{#15670}
Committed:
https://chromium.googlesource.com/external/webrtc/+/b0f04fdb9eb8b28424b7f80e3...
==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/b0f04fdb9eb8b28424b7f80e3...
Message was sent while issue was closed.
The SSLTCP rename broke chromium, fix on it's way: https://codereview.chromium.org/2585343002 I wonder what those p2p things are doing there and why it's directly using rtc:: internal types.
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.webrtc.org/2590153002/ by magjed@webrtc.org. The reason for reverting is: This CL broke all Chromium WebRTC FYI bots. A roll+fix was attempted here: https://codereview.chromium.org/2590783003/, but failed to land. I'm reverting this CL now to make the tree green again. Make the API change gradual when you reland so that we can update Chromium between..
Message was sent while issue was closed.
On 2016/12/20 10:21:55, magjed_webrtc_OOO_Jan_5 wrote: > A revert of this CL (patchset #8 id:140001) has been created in > https://codereview.webrtc.org/2590153002/ by mailto:magjed@webrtc.org. > > The reason for reverting is: This CL broke all Chromium WebRTC FYI bots. A > roll+fix was attempted here: https://codereview.chromium.org/2590783003/, but > failed to land. I'm reverting this CL now to make the tree green again. Make the > API change gradual when you reland so that we can update Chromium between.. FWIW, OPT_TLS_INSECURE also seems like a confusing name. OPT_TLS_NO_CERT_VALIDATION (or OPT_TLS_OPPORTUNISTIC, as in "opportunistic encryption") would be more clear. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
