IoT unsigned custom authentication builder broken

[jawilson]

Describe the bug

AwsIotMqttConnectionConfigBuilder.with_custom_authorizer() is broken, signing isn't required for custom authorizers, yet it enforces it.

Expected Behavior

Using with_custom_authorizer() with only the authorizer_name, token_key_name, and token_value successfully builds a configuration.

Current Behavior

Using with_custom_authorizer() with only the authorizer_name, token_key_name, and token_value throws error Signing-based custom authentication requires all token-related properties to be set.

Reproduction Steps

const { iot } = require('aws-crt');
iot.AwsIotMqttConnectionConfigBuilder.new_default_builder()
    .with_custom_authorizer(null, 'test-authorizer', null, null, 'x-api-key', '1234')
    .build();

Possible Solution

Change

aws-crt-nodejs/lib/common/aws_iot_shared.ts

Lines 85 to 93 in d1729bf

	if (is_string_and_not_empty(input_signature) || is_string_and_not_empty(input_token_value) || is_string_and_not_empty(input_token_key_name)) {
	if (!input_token_value || !input_token_key_name || !input_signature) {
	throw new Error("Signing-based custom authentication requires all token-related properties to be set");
	}
	}
	if (is_string_and_not_empty(input_signature) && input_signature) {
	username_string = add_to_username_parameter(username_string, input_signature, "x-amz-customauthorizer-signature=");
	}

to

 if (is_string_and_not_empty(input_signature) && input_signature) { 
     if (!is_string_and_not_empty(input_token_value) || !is_string_and_not_empty(input_token_key_name)) {
         throw new Error("Signing-based custom authentication requires all token-related properties to be set"); 
     }
     username_string = add_to_username_parameter(username_string, input_signature, "x-amz-customauthorizer-signature="); 
 } 

Additional Information/Context

No response

aws-crt-nodejs version used

1.21.0

nodejs version used

v20.10.0

Operating System and version

Ubuntu 22.04.3 LTS

[bretambrose]

token key name and token key value are only relevant to signed authorizers.

[bretambrose]

It looks like you're using the 311 client.

• Node is checked in CI here: https://github.com/awslabs/aws-crt-nodejs/blob/main/lib/native/aws_iot.spec.ts#L16
• Browser is checked in CI here: https://github.com/awslabs/aws-crt-nodejs/blob/main/lib/browser/aws_iot.spec.ts#L15

[jawilson]

token key name and token key value are only relevant to signed authorizers.

@bretambrose that is not true. You can have a non-signed authorizer configured with a token so that IoT Core populates the event.token value in the Lambda regardless of connection method (HTTPS, WebSocket, etc).

[bretambrose]

The only purpose for token key name and token key value is to communicate to a signing-enabled authorizer what the signature's unsigned value is. The fact that some field in the lambda context gets populated with it is not really relevant.

Username and password are the supported ways of passing auxiliary data into the authorizer lambda

[jawilson]

Fair enough. The IoT dev guide isn't exactly clear on that, and all of the tools (cli, console, CDK, etc) let you create authorizers with a token key name configured and signing disabled. So why should this lib not allow you to pass a token to a custom authorizer without a signing key?

[bretambrose]

The 311 interface is old and umm, not ideal from a clarity standpoint. There was an existing function that was missing parameters, so the missing parameters were just glommed onto the end, leading to a situation where you can accidentally pass in invalid (from our perspective) combinations.

It probably would have been better to explicitly model signed and unsigned authorizer configurations as two separate data types, making it clear what's expected and allowed.

[bretambrose]

Thinking about it more, this is kind of bothering me. Custom auth is complicated enough as is and neither the 311 nor the 5 APIs are as clear as they should be. Tempted to deprecate the current API (still works of course) and add

interface UnsignedCustomAuthOptions { authorizer_name, username?, password? }

and

interface SignedCustomAuthOptions {authorizer_name, token_key_name, token_key_value, token_signature, username?, password?}

and new builder constructor APIs for

newSignedCustomAuth(...)

newUnsignedCustomAuth(...)

Let's leave this open as a reminder.

[jawilson]

We'd been focusing on 311 since 5 had all of the "developer preview" warnings around it. I see that's finally gone, so we'll switch to MQTT 5 then.

[jmklix]

Are you having any problems with using mqtt5? If not the we can close this issue

[jawilson]

Per @bretambrose comment I went ahead and switched the custom authorizer to validate using the MQTT password unless a token is present and the signatureVerified field for the event was true as it sounds like this is more of the "correct" way. Eventually we plan to switch to signed tokens anyway. FWIW, mqtt5 seems to be working quite well.

So while I still think either the ticket is valid or the docs/cli tools should be updated to reflect the intention, this is no longer an issue for me.