IoT MQTT5 MqttConnectCustomAuthConfig has invalid typing for custom authorizer + browser

omer-feinberg-island commented 4 months ago

Describe the bug

iot.AwsIotMqtt5ClientConfigBuilder.newWebsocketMqttBuilderWithCustomAuth receives iot_shared.MqttConnectCustomAuthConfig as a second argument. this in turn has a password property, which is claimed to be of type mqtt5_packet.BinaryData = ArrayBuffer | ArrayBufferView This works completely fine in node, but when running inside a browser the connect message is never sent, resulting in connectionFailure. Surprisingly, passing a string instead of and encoded buffers does work.

Expected Behavior

Either declare MqttConnectCustomAuthConfig.password should be a string, or support BinaryData in browser.

Current Behavior

when passing a binary array, an mqtt connection is initialized but no message is ever sent. after ~30 seconds a I get a connectionFailure event with no other data.

Reproduction Steps

no need to setup an authorizer. any IoT core endpoint will do.
npm install + open index.html in browser to view behavior.
run again with password as a string to see proper behavior ("Connection refused: Not authorized")

heavily inspired by aws-iot-devices-sdk-v2-js

index.js

const iotsdk = require("aws-iot-device-sdk-v2");
const iot = iotsdk.iot;
const mqtt5 = iotsdk.mqtt5;

const $ = require("jquery");

function log(msg) {
    $("#console").append(`<pre>${msg}</pre>`);
}

let config =
    iot.AwsIotMqtt5ClientConfigBuilder.newWebsocketMqttBuilderWithCustomAuth(
        "<insert AWS IoT core endpoint>",
        {password: new TextEncoder().encode("password")}
        //// uncomment to see proper behavior
        // {password: "password"}
    ).build()

log("Connecting custom authorizer...")
const client = new mqtt5.Mqtt5Client(config)
client.on('connectionFailure', (eventData) => {
  log(`connection failed - ${eventData.error.toString()}`)
})

client.start()

package.json

{
    "name": "pub_sub",
    "version": "1.0.0",
    "description": "Publish/Subscribe sample for AWS IoT Browser SDK",
    "homepage": "https://github.com/aws/aws-iot-device-sdk-js-v2",
    "repository": {
        "type": "git",
        "url": "git+https://github.com/aws/aws-iot-device-sdk-js-v2.git"
    },
    "contributors": [
        "AWS SDK Common Runtime Team <aws-sdk-common-runtime@amazon.com>"
    ],
    "license": "Apache-2.0",
    "bugs": {
        "url": "https://github.com/awslabs/aws-iot-device-sdk-js-v2/issues"
    },
    "browser": "./dist/index.js",
    "scripts": {
        "webpack": "webpack --mode=development",
        "prepare": "npm run webpack",
        "build": "webpack --mode=production --node-env=production",
        "build:dev": "webpack --mode=development",
        "build:prod": "webpack --mode=production --node-env=production",
        "watch": "webpack --watch"
    },
    "main": "./index.js",
    "devDependencies": {
        "@types/jquery": "^3.3.31",
        "node-polyfill-webpack-plugin": "^1.1.4",
        "source-map-loader": "^4.0.0",
        "ts-loader": "^9.3.1",
        "typescript": "^4.7.4",
        "webpack": "^5.73.0",
        "webpack-cli": "^4.10.0"
    },
    "dependencies": {
        "aws-iot-device-sdk-v2": "^1.19.1",
        "aws-sdk": "^2.585.0",
        "jquery": "^3.5.0"
    }
}

webpack.config.js

const NodePolyfillPlugin = require("node-polyfill-webpack-plugin");

module.exports = {
  entry: "./index.js",
  devtool: "source-map",
  target: "web",
  output: {
    filename: "index.js",
  },
  resolve: {
    extensions: [".tsx", ".ts", ".js", ".json"],
  },
  module: {
    rules: [
      // all files with a '.ts' or '.tsx' extension will be handled by 'ts-loader'
      { test: /\.tsx?$/, use: ["ts-loader"], exclude: /node_modules/ },
    ],
  },
  plugins: [new NodePolyfillPlugin()],
};

index.html

<html>

<head>
    <meta charset="UTF-8">
</head>

<body>
    <div id="console"></div>
    <script src="dist/index.js"></script>
</body>

</html>

Possible Solution

No response

Additional Information/Context

No response

aws-crt-nodejs version used

1.21.1

nodejs version used

v21.4.0

Operating System and version

macOS 13.5.2

bretambrose commented 4 months ago

I do not think there is a problem with binary data and the password field. All of the integration tests that use binary data explicitly convert to ArrayBuffer via Buffer.from, including the custom auth tests: https://github.com/awslabs/aws-crt-nodejs/blob/main/lib/browser/aws_iot_mqtt5.spec.ts#L40-L165

A UInt8Array is not an ArrayBuffer, but Buffer.from will give you one (web search seems to indicate that the actual ArrayBuffer is a field inside the Uint8Array).

omer-feinberg-island commented 4 months ago

That is 100% correct. So first of all thanks, and I'm a bit embarrassed I didn't think of looking at the tests before asking. However, probably worth considering making the failure a bit more apparent. Current behavior is that nothing happens until 30 secs after trying to connect when there is an empty error - not very easy to deal with. But it's your call, and thanks again!

bretambrose commented 4 months ago

That's a totally reasonable request; we get used to typescript telling us when we mess up at compile time but pure JS users don't have that luxury. Do you happen to know what kind of runtime check we could use to verify something was ArrayBuffer polymorphic?

omer-feinberg-island commented 4 months ago

I didn't realize it earlier, but it's only half an answer: Buffer.from is a node function, not browser. And after all, this is also a browser API. So although there are workarounds, it could be pretty difficult for a user to get going. Seems to me like we should probably look at it the other way around: start with a working mqtt5 custom authorizer sample in the IoT SDK repo (Should I open a different issue there?), and work out a solution from there. wdyt?

omer-feinberg-island commented 4 months ago

I kept playing with the sdk and reached a similar behavior when I tried using .withSessionBehvior: if I use anything but the default no message is being sent, and 30 seconds after using hitting client.start() I receive an empty connectionFailure. Again - works perfectly fine with node, not with browser. I'm starting to feel the browser SDK is not as mature for MQTT5 and I should probably use the MQTT3.11 client, but would love to hear your take on this

bretambrose commented 4 months ago

You're running into an ugly aspect of MQTT.js.

Without a client id, mqtt.js will refuse to connect in MQTT5 if clean session is false: https://github.com/mqttjs/mqtt-packet/blob/v6.8.0/writeToStream.js#L114-L126

And unfortunately, when an error is raised on the ws stream, MQTT.js won't reraise on the client in the browser version for reasons I have no insight into: https://github.com/mqttjs/MQTT.js/blob/v4.3.7/lib/client.js#L462

Digging around a bit yields the following comment wrt error propagation:

    // error.code will only be set on NodeJS env, browse don't allow to detect erros on sockets
    // also emitting errors on browser seems to create issues

The second link also shows why you don't see any error emission from the client in any case, despite the fact that it errors out immediately.

We don't have any control over this behavior. We can add a validation check to auto-assign a client id in the browser case (and we should since IoT Core has broken auto-assign client id behavior).

We may eventually look into a major version upgrade of MQTT.js if it fixes this plus a number of other issues that we've uncovered and worked around over time. But it's not a high priority.

bretambrose commented 4 months ago

I didn't realize it earlier, but it's only half an answer: Buffer.from is a node function, not browser. And after all, this is also a browser API. So although there are workarounds, it could be pretty difficult for a user to get going. Seems to me like we should probably look at it the other way around: start with a working mqtt5 custom authorizer sample in the IoT SDK repo (Should I open a different issue there?), and work out a solution from there. wdyt?

Buffer is essentially a required polyfill for MQTT.js to work

omer-feinberg-island commented 4 months ago

First and foremost, thank you for your patience and informative replies. I really appreciate it. My main remaining question is where would I find this kind of information? I feel like I keep hitting these speed bumps although I'm trying to read the docs pretty thoroughly, is there some data source I'm missing regarding AWS IoT, especially usage as a pubsub infrastructure?

And another note, for future readers: I'm pretty sure this is also needed to connect with clean_session=false. Took me a while to find.

bretambrose commented 4 months ago

Honestly I've never encountered anyone actually discriminating clean vs. non-clean in their IAM policy, but I guess it's possible and might have some use.

bretambrose commented 4 months ago

As to the larger question, you're not missing anything really; things can be a bit murky. I didn't know about those sharp edges on mqtt.js until you brought this up and I reproed what you were experiencing. JS can be tough because there's so many different execution environments, packaging processes, module/export approaches, and so forth, and they all interact with the node/browser split in the CRT which can get painful.

We can definitely improve Troubleshooting/FAQ docs as we discover issues like these.

omer-feinberg-island commented 4 months ago

Honestly I've never encountered anyone actually discriminating clean vs. non-clean in their IAM policy, but I guess it's possible and might have some use.

Whatever works 🤷

So do you want me to keep reporting corners I find? I made tons of progress so far but it would be very optimistic to think I'm done with all hurdles.

bretambrose commented 4 months ago

While frustrating for you, reporting issues is critical to improving the overall experience/product. So I'm all for more issues.

jmklix commented 3 months ago

@omer-feinberg-island did you have any other questions/concerns pertaining to MqttConnectCustomAuthConfig?

github-actions[bot] commented 3 months ago

Greetings! It looks like this issue hasn’t been active in longer than a week. We encourage you to check if this is still an issue in the latest release. Because it has been longer than a week since the last update on this, and in the absence of more information, we will be closing this issue soon. If you find that this is still a problem, please feel free to provide a comment or add an upvote to prevent automatic closure, or if the issue is already closed, please feel free to open a new one.

awslabs / aws-crt-nodejs