1168298 - [Bluetooth] Support OBEX authentication procedure

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Jamin Liu [:jaliu][:Jamin]]

According to PBAP spec., the OBEX authentication procedure is required by PSE.

Gecko have to handle related OBEX headers, including 'Authentication Challenge' and 'Authentication Response'.

Comment 1

[Jamin Liu [:jaliu][:Jamin]]

Attachment: bug1168298_ObexAuth_v0.patch

Comment 2

[Ben Tian (inactive)]

Mark feature-b2g: 2.2r+ since PBAP is required bluetooth feature.

Comment 3

[Ben Tian (inactive)]

Stead this bug from Will.

Comment 4

[Ben Tian (inactive)]

Attachment: [m-c] Patch 1 (WIP): OBEX authentication challenge - webidl change

Comment 5

[Ben Tian (inactive)]

Attachment: [m-c] Patch 2 (WIP): OBEX authentication challenge - dom/bluetooth change

Rebased on the latest m-c with some revision. Need tests on m-c and 2.2r.

Comment 6

[Ben Tian (inactive)]

Attachment: Patch 1 (v1): Support OBEX authentication procedure - webidl change

Comment 7

[Ben Tian (inactive)]

Attachment: Patch 2 (v1): OBEX authentication challenge - dom/bluetooth change

Comment 8

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8672957 [details] [diff] [review]
Patch 1 (v1): Support OBEX authentication procedure - webidl change

Review of attachment 8672957 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. But a few comments addressed before I r+ this.

::: dom/base/nsGkAtomList.h
@@ +771,5 @@
>  GK_ATOM(ondrain, "ondrain")
>  GK_ATOM(ondrop, "ondrop")
>  GK_ATOM(oneitbroadcasted, "oneitbroadcasted")
>  GK_ATOM(onenabled, "onenabled")
> +GK_ATOM(onenterobexpincodereq, "onenterobexpincodereq")

It's a bit strange for me to call onenterobexpincodereq.
Following IrObex 3.5.1, this is actually related to 'password' instead of pin code that GAP defined, right?
I think this makes people confused pin code and Authenticate Challenge password because it's OBEX protocol not Bluetooth.
How about onobexpasswordreq? Since your later patch contain user id, that means you're trying to align with obex spec. Any reason you called it pin code?

::: dom/webidl/BluetoothAdapter.webidl
@@ +29,5 @@
>    // playing time (ms)
>    long long   position = -1;
>    // one of 'STOPPED'/'PLAYING'/'PAUSED'/'FWD_SEEK'/'REV_SEEK'/'ERROR'
>    DOMString   playStatus = "";
>  };

Too many unrelated changes (and this is even webidl change). I don't like too much unrelated change if it's not related to your main purpose. Please remove BluetoothAdapter.webidl from this patch and open new bug for format/style change.

::: dom/webidl/BluetoothObexAuthHandle.webidl
@@ +10,5 @@
> +   * Reply pin code for enterobexpincodereq. The promise will be rejected if the
> +   * operation fails.
> +   */
> +  [NewObject]
> +  Promise<void> setPinCode(DOMString aPinCode);

Promise<void> setPassword(DOMString aPassword);

Comment 9

[Ben Tian (inactive)]

Attachment: Patch 1 (v2): Support OBEX authentication procedure - webidl change

Revise per reviewer's comment 8.

Comment 10

[Ben Tian (inactive)]

Attachment: Patch 2 (v2): OBEX authentication challenge - dom/bluetooth change

Revise per patch 1 (v2) change.

Comment 11

[Ben Tian (inactive)]

Comment on attachment 8673443 [details] [diff] [review]
Patch 1 (v2): Support OBEX authentication procedure - webidl change

Blake, can you help this bluetooth webidl change?

The change adds an event handler to inform gaia app of password request for authentication. Gaia app can then call BluetoothObexAuthHandle.setPassword/reject to set password for or reject authentication. According gecko implementation is in patch 2 of this bug.

Comment 12

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8673444 [details] [diff] [review]
Patch 2 (v2): OBEX authentication challenge - dom/bluetooth change

Review of attachment 8673444 [details] [diff] [review]:
-----------------------------------------------------------------

Some comments addressed, I have some questions that need your response. Thanks!

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +741,5 @@
>    index += AppendHeaderWho(&res[index], kObexLeastMaxSize,
>                             kPbapObexTarget.mUuid, sizeof(BluetoothUuid));
>    index += AppendHeaderConnectionId(&res[index], 0x01);
>  
> +  // authentication response

We shall add some comments just like we did for other AppParameters.
Such as "[headerId:1][length:2][appParam:var]" in order to give the clear picture of response packet layout.

@@ +768,5 @@
> +                                 mObexRemoteNonce, DIGEST_LENGTH);
> +
> +    index += AppendAuthResponse(&res[index], kObexLeastMaxSize - index,
> +                                digestResponse, offset);
> +  }

What happened when |aPassword.IsEmpty()|?
Looking into IrOBEX 1.2, 3.5 Authentication Procedure.

"The server authenticates the client using the same basic algorithm. When a client attempts an operation
to the server which requires authentication, the server sends an Authenticate Challenge header along
with an UNAUTHORIZED response code in a response packet."

@@ +774,1 @@
>    SendObexData(res, ObexResponseCode::Success, index);

We shall handle reject case with UNAUTHORIZED response code in a response packet for failure case.


Response with opcode "UNAUTHORIZED" and Authenticate Challenge, as 3.5.4 Authentication Examples shows.

::: dom/bluetooth/bluedroid/BluetoothPbapManager.h
@@ +16,2 @@
>  #include "ObexBase.h"
> + 

nit: remove the trailing space.

@@ +82,5 @@
> +  /**
> +   * Reply vCard objects to IPC / in-process 'pullphonebook' request.
> +   *
> +   * @param aActor         [in]  (IPC) blob actor of the vCard objects
> +   * @param aBlob          [in]  (in-process) blob of the vCard objects

The comment doesn't seems to be consistent with declaration. Perhaps typo?

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1651,5 @@
> +      NS_LITERAL_STRING("Failed to reject OBEX authentication request"));
> +    return;
> +  }
> +
> +  pbap->ReplyToAuthChallenge(EmptyString());

A bit strange here for me. If we want to reject ObexAuth, do we still send obex response code success, after checking ReplyToAuthChallenge?

::: dom/bluetooth/common/ObexBase.h
@@ +283,5 @@
> +      }
> +    }
> +  }
> +
> +  void GetAuthResponse(uint8_t** aRetData, int* aRetDataLength) const

I checked Patch1 and Patch2, I did not see |GetAuthResponse| been used. Is it for future usage? I would like to see some comments for purpose of this function.

::: dom/bluetooth/common/webapi/BluetoothAdapter.cpp
@@ +1357,5 @@
> +  init.mHandle = BluetoothObexAuthHandle::Create(GetOwner());
> +
> +  // The optional userId of authenticate challenge
> +  if (arr.Length() == 1 && arr[0].value().type() == BluetoothValue::TnsString) {
> +    init.mUserId = arr[0].value().get_nsString();

I don't see where we actually pass userid in other places (in BluetoothPbapManager). Do we expect to add userid related code in BluetoothPbapManager?

Comment 13

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8673443 [details] [diff] [review]
Patch 1 (v2): Support OBEX authentication procedure - webidl change

Review of attachment 8673443 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Ben,
Sorry for late notice.
But I think you missed test_interfaces.html and that may break some mochitest test cases.
Please add events into test_interfaces.html.

Comment 14

[Ben Tian (inactive)]

Comment on attachment 8673443 [details] [diff] [review]
Patch 1 (v2): Support OBEX authentication procedure - webidl change

Cancel review first for comment 13.

Comment 15

[Ben Tian (inactive)]

Attachment: Patch 1 (v3): Support OBEX authentication procedure - webidl change

Add tests per comment 13.

Comment 16

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8673568 [details] [diff] [review]
Patch 1 (v3): Support OBEX authentication procedure - webidl change

Review of attachment 8673568 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, Ben!

Comment 17

[Ben Tian (inactive)]

Comment on attachment 8673568 [details] [diff] [review]
Patch 1 (v3): Support OBEX authentication procedure - webidl change

Blake, can you help this bluetooth webidl change?

The change adds an event handler to inform gaia app of password request for authentication. Gaia app can then call BluetoothObexAuthHandle.setPassword/reject methods to set password for or reject authentication. Corresponding gecko implementation is in patch 2 of this bug.

Comment 18

[Ben Tian (inactive)]

Attachment: Patch 2 (v3): OBEX authentication challenge - dom/bluetooth change

Comment 19

[Ben Tian (inactive)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #12)
> Looking into IrOBEX 1.2, 3.5 Authentication Procedure.
> 
> "The server authenticates the client using the same basic algorithm. When a
> client attempts an operation
> to the server which requires authentication, the server sends an
> Authenticate Challenge header along
> with an UNAUTHORIZED response code in a response packet."

The paragraph above is for "server to authenticate client", in contrast to "client to authenticate server" that we want to support. I revised patch in comment 18 to respond "unauthorized" to reject authentication as IrOBEX doesn't specify how server rejects authentication from client.

Let me know for any further thoughts.

Comment 20

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #18)
> Created attachment 8681183 [details] [diff] [review]
> Patch 2 (v3): OBEX authentication challenge - dom/bluetooth change

Note I've verified this patch on MecApp tool to connect with authentication challenge from client to server.

Comment 21

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8681183 [details] [diff] [review]
Patch 2 (v3): OBEX authentication challenge - dom/bluetooth change

Review of attachment 8681183 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +750,5 @@
> +  if (!aPassword.IsEmpty()) {
> +    // Section 3.5.2.1 "Request-digest", PBAP 1.2
> +    // The request-digest is required and calculated as follows:
> +    //   H(nonce ":" password)
> +    char hashString[kObexLeastMaxSize];

Is it true that we should use kObexLeastMaxSize? Just wonder IrObex defined the size of hash string.

@@ +757,5 @@
> +    hashString[offset++] = ':';
> +
> +    memcpy(&hashString[offset], NS_ConvertUTF16toUTF8(aPassword).get(),
> +           aPassword.Length());
> +    offset += aPassword.Length();

Should check |aPassword.Length()| less than |kObexLeastMaxSize|.

Comment 22

[Ben Tian (inactive)]

Attachment: Patch 2 (v4): OBEX authentication challenge - dom/bluetooth change

Changes:
- allocate |hashString| with dynamic length
- precisify length of |digestResonse|
- rebase on bug 1221477

Comment 23

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8683535 [details] [diff] [review]
Patch 2 (v4): OBEX authentication challenge - dom/bluetooth change

Review of attachment 8683535 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +520,5 @@
> +
> +  // Get authentication challenge data
> +  uint8_t* dataPtr;
> +  int dataLength;
> +  aHeader.GetAuthChallenge(&dataPtr, &dataLength);

GetAuthChallenge, this function leaks memory. 
nsAutoArrayPtr<uint8_t> dataPtr;

@@ +521,5 @@
> +  // Get authentication challenge data
> +  uint8_t* dataPtr;
> +  int dataLength;
> +  aHeader.GetAuthChallenge(&dataPtr, &dataLength);
> +

Please add comments to point out the format of packets for fetching nonce.

::: dom/bluetooth/common/ObexBase.h
@@ +275,5 @@
> +
> +    for (int i = 0; i < length; ++i) {
> +      if (mHeaders[i]->mId == ObexHeaderId::AuthChallenge) {
> +        uint8_t* ptr = mHeaders[i]->mData.get();
> +        *aRetData = new uint8_t[mHeaders[i]->mDataLength];

When will aRetData be released?

Comment 24

[Ben Tian (inactive)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #23)
> ::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
> @@ +520,5 @@
> > +
> > +  // Get authentication challenge data
> > +  uint8_t* dataPtr;
> > +  int dataLength;
> > +  aHeader.GetAuthChallenge(&dataPtr, &dataLength);
> 
> GetAuthChallenge, this function leaks memory. 
> nsAutoArrayPtr<uint8_t> dataPtr;

nsAutoArrayPtr cannot be passed directly into GetAuthChallenge. Need more time to revise the code.

../../../../gecko/central/dom/bluetooth/bluedroid/BluetoothPbapManager.cpp:524:49: error: no matching function for call to 'mozilla::dom::bluetooth::ObexHeaderSet::GetAuthChallenge(nsAutoArrayPtr<unsigned char>*, int*) const'

Comment 25

[Ben Tian (inactive)]

Attachment: Patch 2 (v5): OBEX authentication challenge - dom/bluetooth change

Revise per comment 23.

Change:
- use nsAutoArrayPtr to get authentication challenge data
- add comment to explain nonce format

Comment 26

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8684743 [details] [diff] [review]
Patch 2 (v5): OBEX authentication challenge - dom/bluetooth change

Review of attachment 8684743 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/common/ObexBase.cpp
@@ +77,5 @@
>                        aWho, aLength);
>  }
>  
>  int
> +AppendAuthChallenge(uint8_t* aRetBuf, int aBufferSize, const uint8_t* aDigest,

Do you have any reason to keep this function? It seems to be unused now.

@@ +85,5 @@
> +                      aDigest, aLength);
> +}
> +
> +int
> +AppendAuthResponse(uint8_t* aRetBuf, int aBufferSize, const uint8_t* aDigest,

It seems to be unused now.

Comment 27

[Ben Tian (inactive)]

Attachment: Patch 2 (v6): OBEX authentication challenge - dom/bluetooth change

Comment 28

[Ben Tian (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=092b053c5954

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/9a942f34debf

Comment 30

[Ben Tian (inactive)]

Attachment: [Test patch] 2.2r gaia patch to verify authentication challenge

set password = 0000

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a942f34debf