Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding key management interface #28

Closed
wants to merge 4 commits into from

Conversation

martinthomson
Copy link
Member

Exposes local keying material and a control for it.

This doesn't do anything about presenting a view of the remote party keys.

For https://www.w3.org/Bugs/Public/show_bug.cgi?id=21880

@fluffy
Copy link
Contributor

fluffy commented Nov 5, 2014

Overall I like this but want to think about it more in the case where there is no bundle and we have multiple DTLS sessions.

@martinthomson
Copy link
Member Author

Since this is on the sending side only and we have a requirement to have a single consistent set of keys for the entire session, we can't possibly have a problem in that regard. If you want to have separate DTLS sessions with different credentials, you need multiple RTCPeerConnection instances anyway. See jsep:

For DTLS, all m= sections MUST use the certificate for the identity that has been specified for the PeerConnection; as a result, they MUST all have the same [RFC4572] fingerprint value, or this value MUST be a session-level attribute.

I don't know why there is a "For DTLS" there still, but whatever.

@@ -3463,6 +3524,49 @@ <h2 id="sec.stats-model">Statistics Model</h2>
</section>

<section>
<h2 id="sec.key-generation">Key Generation</h2>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not get really why this part could not be in a web crypto document we reference. That would be my preference.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feedback we got from the web cryptography files was that we were better off defining this and owning it. They were reluctant to take on an effectively useless key usage.

@sleevi
Copy link

sleevi commented Dec 3, 2014

From the WebCrypto side, this seems very much an inappropriate use of WebCrypto, and is not something that was at all intended with the WebCrypto interface.

  1. These keys are NOT usable with any of window.crypto.subtle. This violates nearly every principle of good API design to create a type, defined by an API, that is not usable at all with that API. Indeed, the only extent that it IS usable is via exportKey(), which seems to just be due to spec preference more than any reason that aids developers.

  2. The use of WebCrypto Keys and Algorithms is inappropriate, because it is intentionally not designed as a general "key type" interface. That is, you do NOT create "RSA keys", but you create keys for specific algorithms and hash requirements. For a DTLS exchange using RSA keys, you nominally use PKCS#1v1.5. However, your hash algorithm selection is contextually dependent upon the negotiated SignatureAndAlgorithms exchange, rather than the WebCrypto key. This itself violates the WebCrypto security design and considerations.

2.a) The choice of "Allow RSA-PSS even though it can't be used" is further example of the exceptional strangeness re: "future proofing"
2.b) The absence of ECDH, in light of the ongoing TLS 1.3 discussions regarding static server ECDH keys (c.f. OPTLS) is an example of how this "future proofing" would already fail.

  1. Your choice of "webrtc" usage creates an inconsistent set of security expectations. That is, it is entirely possible to export the (private) key from the user agent for one usage, and modify the usage, and re-import it. This creates a new set of security considerations (e.g. key reuse) that are not addressed in the specification. Indeed, you try to mitigate this with a normative requirement of "extractable" being false, but this further creates a set of impedance mismatch for authors when it's not that the API really expects a WebCrypto CryptoKey (as a general type), but only a CryptoKey that was created with the right invocations. That sort of requirement wholly violates principle of least surprise.

  2. Implementors may not be able to expose a general purpose set of algorithms to application authors for reasons such as regulatory frameworks or patents. However, they MAY be able to use these algorithms in use for systems built upon DTLS. While I cannot provide legal advice to anyone, within the US alone, there are different export controls regarding general purpose APIs and internal APIs, and there are different IPR disclosures within the IETF regarding RF usage of some algorithms for certain IETF protocols that are not extended to general purpose libraries.

To be clear, as a member of the WebCrypto WG, I am not just opposed to "including this in the WebCrypto API", as suggested in https://github.com/w3c/webrtc-pc/pull/28/files#r20125077 , but I am opposed to this use of WebCrypto entirely.

The WebCrypto WG spent significant time haggling over naming and terminology in order to try to establish that the WebCrypto CryptoKey object was truly intended for explicit use with the low-level window.crypto.subtle interface. It is NOT meant to be a substitute for a "high level" key object, such as for a high-level key/identity API that has been discussed both in the WebCrypto WG and in various IGs, nor was/is it meant as a substitute for use with other protocols/APIs such as WebRTC. This isn't merely about turf, it's about providing spec and application authors - and the broader community - a set of reasonable paths to review the security and privacy implications of the APIs exposed. Indeed, the WebCrypto WG discussed quite a bit how unintentional combining of APIs can provide disastrous security experiences.

There is nothing preventing the WebRTC WG from describing a new object that meets their use and security requirements. Indeed, WebCrypto's CryptoKey's are hardly the first object of this type - the File APIs' File object is a prime example. It would be better for application authors and security reviewers to consider introducing your own type here.

@fluffy
Copy link
Contributor

fluffy commented Dec 4, 2014

Think this needs discussion on the mailing list not the bug tracker.

@sleevi
Copy link

sleevi commented Dec 5, 2014

Right, except there was no discussion I could find over the past four weeks
on this approach, beyond the bug being filed that there should be a way to
manage certs (sure, all agreed as good) and the slides from TPAC.

Since this seemed just like one draft proposal, and because there was no
list activity on it, felt better on the pull request.

If you can direct me to where this is being discussed, fantastic.
On Dec 4, 2014 10:30 AM, "Cullen Jennings" notifications@github.com wrote:

Think this needs discussion on the mailing list not the bug tracker.


Reply to this email directly or view it on GitHub
#28 (comment).

@stefhak
Copy link
Contributor

stefhak commented Dec 5, 2014

Sleevi,

there is not an active discussion right now. It was discussed during the f2f at TPAC, but not since. And that was the first time, so I think that many are not up to speed.
Could you take your feedback to the public-webrtc list? I think it is there we should discuss.
Br,
Stefan

@alvestrand
Copy link
Contributor

List discussion needed.

@alvestrand
Copy link
Contributor

This PR has caused considerable controversy on the mailing list. Apparently some people are calling for discussion of the use of the crypto API at the TAG. This PR is put on hold awaiting the outcome of that discussion.

@martinthomson
Copy link
Member Author

I'm going to close this and provide a new PR.

dontcallmedom pushed a commit that referenced this pull request Aug 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants