Azure / azure-iot-sdk-node

A Node.js SDK for connecting devices to Microsoft Azure IoT services
https://docs.microsoft.com/en-us/azure/iot-hub/
Other
261 stars 227 forks source link

Inconsistent SAS expiration time Registry vs. JobClient, DeviceClient #855

Closed Suizi11 closed 4 years ago

Suizi11 commented 4 years ago

Context

Description of the issue

The C SDK describes that the expiration time/expiry date of a SAS token is set to one hour. E.g. for a DeviceClient using MQTT refreshed after 48 minutes. https://github.com/Azure/azure-iot-sdk-c/blob/master/doc/connection_and_messaging_reliability.md#connection-authentication

Code sample exhibiting the issue

JobClient.fromConnectionString() uses also one hour (as described in the C SDK): var sas = SharedAccessSignature.create(cn.HostName, cn.SharedAccessKeyName, cn.SharedAccessKey, azure_iot_common_1.anHourFromNow());

But this behavior is inconsistent with the Registry.fromConnectionString(): azure_iot_common_1.SharedAccessSignature.create(cn.HostName, cn.SharedAccessKeyName, cn.SharedAccessKey, Date.now()) Unfortunately, I can't find any documentation about the default expiration time in the Node SDK reference or somewhere else. Further, what does Date.now() mean - expiring never?

What's the intended solution to refresh the SAS token? Would be nice if a method for that will be provided. Or should I create the JobClient, etc. instance with the method "fromSharedAccessSignature()" and set the expiration time by myself?

Thanks!

AB#7748273

anthonyvercolano commented 4 years ago

@Suizi11 Perhaps it's just early in the morning for me. I'm not exactly sure what the issue is here.

Does registry.fromConnectionString not work?

What behavior is not consistent between the clients? The code at various points may not be the same but what happens later when a request is actually submitted to the service is what is important.

Suizi11 commented 4 years ago

Sorry, maybe I was a bit too quick ...

Both clients (JobClient & Registry) are working "as expected" relating to the response of subsequent calls.

In my backend I've an instance of a JobClient and an instance of the Registry (both are clients - just as term - with respect to the IoT Hub API). IMPORTANT: I'm reusing the clients for several requests against the API, I don't re-create a new client for every new request (should I?). After one hour the JobClient stops working (as described e.g. in the C API), because the SAS token is expired and I get an Unauthorized response. However, the Registry client is still working because the SAS token never expires.

For me, as a developer, I expect same behavior of both clients:

Or is this an intended behavior. But then I'd suggest to document this behavior somewhere.

anthonyvercolano commented 4 years ago

"IMPORTANT: I'm reusing the clients for several requests against the API, I don't re-create a new client for every new request (should I?)."

@Suizi11 You should not recreate the client for each request. Although hold that thought. Perhaps if there is a bug in the JobClient, it may be your best work around.

"After one hour the JobClient stops working (as described e.g. in the C API), because the SAS token is expired and I get an Unauthorized response. However, the Registry client is still working because the SAS token never expires."

Please provide a more precise description of JobClient stops working.

Are you saying that you invoked a method 1 hour after the JobClient was created and that request got an UNAUTH? Are you saying that you invoked a method that was still executing after 1 hour and that invocation of that method failed with an UNAUTH?

A little explanation on Authorization with these service side clients. Most of these clients are sending HTTP requests to the service. Each time these HTTP requests are made there is the VERB - POST, GET, etc. The URL that goes with the verb (and more stuff with the URL). There are a series of headers and then there is the request body.

Headers typically look like key/value pairs.

One of those headers in an Authorization header. The word "Authorization" in this case is the "key". The value in this case is (I believe) the Shared Access Signature. The SAS is a string with "SharedAccessSignature ". The sas token is what contains the authentication information which also includes an expiration time for the validity of that sas token. It's been quite a while since I was involved in the dirty details of the HTTP request itself, but when last I visited this code it was actually generating the sas token as it was building that particular request.

In the case of the registry manager if two requests to create a device identity were with 1 second of time elapsing between the requests, each request would have a different sas token.

I'll take a look at the JobClient to see how it is dealing with multiple requests. Look forward to your further description of the failure.

Suizi11 commented 4 years ago

You should not recreate the client for each request. Perhaps if there is a bug in the JobClient, it may be your best work around. Well, then I'm doing it correctly. That's true regarding work around in case of a bug.

I'll try to explain it by code:

const jobClient = JobClient.fromConnectionString(<connection-string>);
setInterval(() => {
    jobClient.scheduleDeviceMethod(...) 
}, 5 * 60000);

jobClient.scheduleDeviceMethod is called every 5 minutes - after one hour I'm getting Unauthorized response, because the SAS token is valid for one hour only as shown above in the initial post (snippet from the source code of the SDK).

Are you saying that you invoked a method 1 hour after the JobClient was created and that request got an UNAUTH? Partially. I'm invoking a method every 5 minutes (so also after one hour after creation).

Are you saying that you invoked a method that was still executing after 1 hour and that invocation of that method failed with an UNAUTH? No!

Thanks for the detailed explanation. But the SAS token isn't built for every particular request, it's built when the JobClient instance (or Registry instance) are created:

JobClient.fromConnectionString = function (connectionString) {
        /*Codes_SRS_NODE_JOB_CLIENT_16_002: [The `fromConnectionString` method shall throw a `ReferenceError` if `connectionString` is falsy.]*/
        if (!connectionString)
            throw new ReferenceError('connectionString cannot be \'' + connectionString + '\'');
        var cn = ConnectionString.parse(connectionString);
        var sas = SharedAccessSignature.create(cn.HostName, cn.SharedAccessKeyName, cn.SharedAccessKey, azure_iot_common_1.anHourFromNow());
        var config = {
            host: cn.HostName,
            sharedAccessSignature: sas.toString()
        };
        /*Codes_SRS_NODE_JOB_CLIENT_16_003: [The `fromConnectionString` method shall return a new `JobClient` instance.]*/
        return new JobClient(new azure_iot_http_base_1.RestApiClient(config, packageJson.name + '/' + packageJson.version));
    };

In the end, the inconsistency is about create SDK clients with different expiration times.

anthonyvercolano commented 4 years ago

Thank you for the bug report. We've checked the fix in. Will probably release in a week or so.

az-iot-builder-01 commented 4 years ago

@Suizi11, thank you for your contribution to our open-sourced project! Please help us improve by filling out this 2-minute customer satisfaction survey

Suizi11 commented 4 years ago

Hey @anthonyvercolano , can you tell me or can you estimate the upcoming date of release? In your last post you mentioned ~ beginning of August, some time has passed now.

Thank you!

anthonyvercolano commented 4 years ago

@Suizi11 Unless life gets really bad in the next couple of hours (which I don't anticipate at all), I will release within about 2 hours.