Re: Issue 714: STUN/TURN OAuth token auth parameter handover

Hi,

2016-08-15 19:46 keltezéssel, Bernard Aboba írta:
>
> https://github.com/w3c/webrtc-pc/issues/714
>
>  
>
> Today we have:
>
>  
>
> dictionary *RTCIceServer*{
>     required (DOMString or sequence<DOMString>)|urls|;
>              DOMString                          |username|
> <http://w3c.github.io/webrtc-pc/#dom-rtciceserver-username>;
>              DOMString                          |credential|
> <http://w3c.github.io/webrtc-pc/#dom-rtciceserver-credential>;
>              |RTCIceCredentialType|               |credentialType|= "password";
> };
>
>  
>
>  
>
> enum *RTCIceCredentialType*{
>     "password
> <http://w3c.github.io/webrtc-pc/#dom-rtcicecredentialtype-password>",
>     "token
> <http://w3c.github.io/webrtc-pc/#dom-rtcicecredentialtype-token>"
> };
>
>  
>
> This issue asks how (or if) this can be used to support RFC 7635
> <https://tools.ietf.org/html/rfc7635> (OAuth 2.0) when credentialType
> = “token”.
>
>  
>
> An example of an access token is given in RFC 7635, Appendix B:
>
_
___may I am only confused, but _I feel there is a misunderstanding
around the access-token definition.
_
This structure containing more information not only the access token,
and IETF access token definition is not this json structure.

This example contains the access token, _but_  also other mandatory
parameters like kid, key.
So it contains all the information that needed for the ICE agent need to
know to be able to auth.
The "alg" will be important later, when the implementation will support
more then the SHA1 HMAC alg.
>
>  
>
>         {
>           "access_token":
>    "U2FsdGVkX18qJK/kkWmRcnfHglrVTJSpS6yU32kmHmOrfGyI3m1gQj1jRPsr0uBb
>    HctuycAgsfRX7nJW2BdukGyKMXSiNGNnBzigkAofP6+Z3vkJ1Q5pWbfSRroOkWBn",
>           "token_type":"pop",
>           "expires_in":1800,
>           "kid":"22BIjxU93h/IgwEb",
>           "key":"v51N62OM65kyMvfTI08O"
>           "alg":HMAC-SHA-256-128
>         }
Or do you mean that we should pass the whole structure to the ICE agent
as credential?

If yes then I still fell misleading to reference to this structure as
access_token. Your W3C access token definition split apart from IETF,
and means then different thing then the one, defined in RFC7635.

To pass in RTCIceServer.credential cause other problems, that I will
explain later. So I am not sure that it is the right way...

When you wrote that it is "an example of an access token", for me at
least, added some confusion..
Rather it is the final response structure received by Client from the
Authorization Server (according the PoP key distribution).
It contains all the informations that needed for an ICE agent to auth
not only the access_token.

The Access Token structure is defined here:
https://tools.ietf.org/html/rfc7635#section-6.2
(Most part is encrypted and some is not.)

Please understand how it works, and why this structure is more then an
access_token, to understand why passing token as rtciseserver.credential
is not the right way.

AFAIU in nutshell (and so inexactly) the way how the token based auth works:
The TURN server and the Auth server establishing a "AS-RS Auth key".
That will be used by Auth server to encrypt access token.
Access Token is created by Auth server and contains encrypted form of
the new generated mac_key lets call it as simple "key" ~="the password".
That will be used by the ICE agent to create MESSAGE-INTEGRITY.
(The usage of the "key" is during TURN auth is the same as normal
password/credential is used in password auth/LTC/.)
The ICE agents after receiving nonce sends  ----{kid  in USERNAME,
access_token in ACCESS_TOKEN attribute, and based the "key" create the
MESSAGE-INTEGRITY}
The TURN server decrypts/opens the access token. 
(with a symmetric "AS-RS Auth key", the key that is "shared/established"
between the  Authorization Server and TURN server).
And finally this is the way TURN server get access to the clear text
"key"/password. And creates and verifies the message integrity against
the one it finds in the message MESSAGE-INTEGRITY attribute.
Because (We could use multiple AS-RS "shared/established" keys)The kid
that passed in USERNAME stun/turn attribute is used to select key that
is used to decrypt ACCESS-TOKEN. .


AFAIU, three attributes kid, key, and access_token are mandatory to have
for an ICE agent to Auth against a TURN server using OAuth!
alg will be important after ICE agent's will support other than SHA1
digest alg.
>
>  
>
> Rather than adding new attributes to RTCIceServer (as proposed in
> Issue 714), is there a reason why such a token couldn’t be passed in
> RTCIceServer.credential with RTCIceServer.credentialType = “token”?
>
Yes. We have at least three mandatory parameters, but actually two
fields are in the API.

I am confused, howto pass informations because the above JSON response
is not only containing, three information (the access-token but also the
key, and kid.), but the current webrtc-pc interface  RTCIceServer only
defines two fields: username, password, that are places for kid and key.

For me it is not fully clear how the current standard imagined the pass
in RTCIceServer.credential. Could you please explain it more?
So it would be better question to where to put and pass the
access_token, (alg, expiry)?

Pass the whole result structure of the PoP key response as
RTCIceServer.credential?
I guess this is what is meant.

But, well, I believe it is not good idea from implementation point of
view, because much better to put the right values the right variables
through the interface.
Especially if username and password handling is already implemented..
So better to put "kid" in "username" and "key" in "credential", to not
bother the actual implementations, and not forcing changes in
established implementations, that even if we pass an empty username, and
pass all information in credential, should anyway sort information
behind the scenes.
But even if we mess up the normal way, anyhow and anyway behind the
scenes the "kid" must treated in STUN client as username and put it in
USERNAME STUN attribute. So why not put it through the interface and
keep the current implementation untouched.
And also not mess up the credential implementation workflows the same way..
So I think it is not the right idea to go this way.

Or it is meant to add the "kid" to the "username" field and add other
parameters to the "key" and "access_token", "alg", "expiry" and
altogether packed in one, to the "credential" field?

But it is also not right because it is confusing from implementer points
of view. Mess up the existing credential usage.
One time we use credential to pass password, other time we use to pass
multiple parameters, and also it would be not clean interface, and as I
have explained earlier later the implementation need anyhow to assign
internally to the "credential" variable the "key" value. So I believe it
is also not ideal...

I pointed out already in the issue that

  * "kid" is used as "username"
      o No change needed in already implemented internal workflow.
  * "key" is used as "credential"
      o No change needed in already implemented internal workflow.
  * access_token is a new stun parameter that need to be passed.
  * (Expires is an optional but good to know information for the ICE agent)
  * (alg if multiple digest alg will be supported)

We have to pass three new parameters.

So there is place in *RTCIceServer*to pass username=kid  and password=key,

BUT not for the access_token (and the expiry date, and alg).





My proposals:

 1. Keep RTCIceServer as it is. (Not preferred by me)
      * Document that credential could be in case of token the result of
        PoP key distribution result json. (Let's call it something, and
        find it a less confusing name than "Access Token".)
      * Add to this sentence that only check it credentialType = "password" 
          o "If scheme name is |turn| or |turns|, and either of
            |server.username| or |server.credential| are omitted, then
            throw an |InvalidAccessError| and abort these steps."
    Pros&Cons:
      * + no interface change
      * -  not clean, dumb parameter handover
      * -  adds duty for implementation to sort values later internally.
      * -  mess up the already existing implementations username and
        password/credential internal workflows as I have explained
        earlier above.
      * -  Leaving empty RTCIceServer.username, but on other hand later
        on it should anyhow put kid internally to username, and handle
        as username.
          o The same is true for "credential" and "key"
 2. Pass in credential field the key and also the access_token packed in
    one. (It is also not preferred by me.)

    dictionary TokenCredential {
        required DOMString key;
        required DOMString access_token;
        required DOMString alg;
        DOMTimeStamp expiry;
    };


    dictionary RTCIceServer {
        required (DOMString or sequence<DOMString>) urls;
                 DOMString                          username;
                 (DOMString or TokenCredential)     credential;
                 RTCIceCredentialType               credentialType =
    "password";
    };


      * + Nearly zero change needed.
      * -  It adds a not clean and ugly parameter handover in the interface.
      * -  For implementers it is more complex and confusing that some
        time credential contains the password but other times it
        contains multiple values.
            It is more clean to assign credential=key as it is
        credential=password. So not bother existing implementation and
        leave and use as it is in password case.
            Documentation and example needed that if
        credentialType="token" then the credential looks like
            credential = {
                key="base64_encoded_key",
                access_token="base64_encoded_access_token"
                alg="HMAC-SHA-1-96"
                expiry="1471528540"
            }
 3. simply extend RTCIceServer with the plus two attributes as proposed.
    (Preferred)

    dictionary RTCIceServer {
        required (DOMString or sequence<DOMString>) urls;
                 DOMString                          username;
                 DOMString                          credential;
                 DOMString                          accesstoken;
                 DOMString                          alg;
                 DOMTimeStamp                       expiry;
                 RTCIceCredentialType               credentialType =
    "password";
    };

      * + Cleanly match and reuse fields
          o username=kid
              + So kid is used in username attribute, (following the
                existing normal STUN password workflow)
          o credential=key
              + key is used as credential (following the existing normal
                STUN password workflow)
      * + Slightly more easy the backward compatibility,
          o no change needed in existing implementation
            username/credential handling workflow.
      * -  Adding more and more attributes is not ideal
 4.

    Or be more exact and define a new dictionaries and change credential
    type to multiple sequence. (Preferred)

    dictionary TokenCredential {
        required DOMString    kid;
        required DOMString    key;
        required DOMString    access_token;
        required DOMString    alg;
        DOMTimeStamp          expiry;
    };

    dictionary LongTermCredential {
        required DOMString    username;
        required DOMString    password;
    };

    dictionary RTCIceServer {
        required (DOMString or sequence<DOMString>) urls;
                 (LongTermCredential or TokenCredential)    credential;
                 RTCIceCredentialType                      
    credentialType = "password";
    };

      * + It is the most exact, and (we don't need new attributes)
      * + More Future-proff.
      * -  More complicated but more clean vs. simple change (3.)
      * -  Bigger change, need more time to implement


Please comment and correct me if I am wrong...

Just my 2 cent..
Sorry for the long email..

I am looking forward hearing your thoughts and comments...

Thanks,
Misi