refactor(azure-iot-device): Auth Revisions #555

cartertinney · 2020-04-29T00:04:57Z

Removed auth providers and refactored entire auth flow
Unified auth between provisioning and hub clients
Refactored pipeline to store configuration details on the root
Refactored translation tests to move to new infrastructure
Removed all code no longer necessary due to these changes

cartertinney · 2020-04-29T21:00:47Z

./azp run Azure.azure-iot-sdk-python-horton-e2e

cartertinney · 2020-04-30T16:11:38Z

./azp run

cartertinney · 2020-05-04T21:51:02Z

./azp run

olivakar · 2020-05-16T00:12:19Z

/azp run Azure.azure-iot-sdk-python-dps-e2e

azure-pipelines · 2020-05-16T00:12:27Z

Azure Pipelines successfully started running 1 pipeline(s).

olivakar · 2020-05-18T23:28:47Z

azure-iot-device/azure/iot/device/common/auth/sastoken.py

+    ttl (int): Time to live for the token, in seconds
+    """
+
+    _auth_rule_token_format = (


_auth_rule_token_format [](start = 4, length = 23)

why not keyname_token_format ? #ByDesign

Because apparently for some reason the official name of of the skn field is not... shared key name as you might expect, and is actually.... for some reason... auth rule.

I don't get it either.

In reply to: 426946266 [](ancestors = 426946266)

I'm wondering if this was the name in the azure portal at one point. Right now, the key names (iothubowner, etc) show up in the portal under "Shared Access Policy" as "Access policy name". We had this same thing happen with one of the DPS names -- I think ProvisioningHostName shows up in the portal with one name and in the SDK as another.

In reply to: 427500301 [](ancestors = 427500301,426946266)

olivakar · 2020-05-18T23:30:42Z

azure-iot-device/azure/iot/device/common/auth/sastoken.py

+
+    @property
+    def expiry_time(self):
+        "Expiry Time is READ ONLY"


" [](start = 8, length = 1)

are read only doc strings with single quotes instead of 3 ? #Resolved

Nope, that's just a mistake.

In reply to: 426946876 [](ancestors = 426946876)

olivakar · 2020-05-18T23:39:42Z

azure-iot-device/azure/iot/device/common/pipeline/config.py

+        :type sastoken: :class:`azure.iot.device.common.auth.SasToken`
+        :param x509: X509 to be used for authentication. Mutually exclusive with sastoken.
+        :type x509: :class:`azure.iot.device.models.X509`
+        :param str server_verification_cert: The trusted certificate chain.


:param str server_verification_cert: The trusted certificate chain. [](start = 8, length = 67)

this has nothing to do with you PR...but i just wanted to mention should we update the doc string to say that it is the content of the cert chain...i have faced ppl with some confusion #Pending

At some point we'll change the functionality to take both the contents or filepath

In reply to: 426949443 [](ancestors = 426949443)

olivakar · 2020-05-19T00:01:25Z

azure-iot-device/azure/iot/device/common/pipeline/pipeline_stages_base.py

+        self._token_renewal_timer = None
+        if timer:
+            logger.debug("Cancelling SAS Token renewal timer")
+            timer.cancel()


timer.cancel() [](start = 12, length = 14)

can you also put timer = None? For reasons that I don't quite remember i see this pattern in all timers in our stack #ByDesign

This happens on line 303

In reply to: 426955668 [](ancestors = 426955668)

olivakar · 2020-05-19T00:25:46Z

azure-iot-device/azure/iot/device/iothub/abstract_clients.py

 from . import pipeline

+# TODO: different package?
+from azure.iot.device.common.auth import connection_string as cs


cs [](start = 62, length = 2)

may be import as connection and token...just to make more meaningful in code. #ByDesign

I have mixed feelings about renaming to cs and st. Yes, it's shorter, but this is the only place we do this and I don't want it to get out of control.

In reply to: 426962229 [](ancestors = 426962229)

So for context, the reason that we do this is that connection_string and sastoken are kind of abnormal modules (for us, not in general) in that there are associated values in the modules, such as the constants. Thus we need to import the whole module, not just the object in it.

The problem is that connection_string as a term easily gets overloaded, and I didn't want to have to worry about accidentally doing that, so I aliased the package to be something that is out of the way and not likely to be used in a variable name by accident.

In reply to: 427502200 [](ancestors = 427502200,426962229)

I'm OK with that, I've seen that problem before and never came up with a better answer. I wouldn't be offended if you left this as-is

In reply to: 427504005 [](ancestors = 427504005,427502200,426962229)

olivakar · 2020-05-19T01:21:22Z

azure-iot-device/azure/iot/device/iothub/pipeline/pipeline_stages_iothub_mqtt.py

@@ -263,4 +220,4 @@ def _handle_pipeline_event(self, event):

        else:
            # all other messages get passed up
-            super(IoTHubMQTTTranslationStage, self)._handle_pipeline_event(event)
+            self.send_event_up(event)


self.send_event_up(event) [](start = 12, length = 25)

oh we missed this #Resolved

BertKleewein · 2020-05-19T03:37:51Z

azure-iot-device/azure/iot/device/product_info.py

@@ -3,6 +3,9 @@
 # Licensed under the MIT License. See License.txt in the project root for
 # license information.
 # --------------------------------------------------------------------------
+"""This module is for creating product identifiers or agent strings for IotHub as well as Provisioning."""
+
+# TODO: Should this be renamed to user_agent.py?


yes #Closed

BertKleewein · 2020-05-19T03:42:55Z

azure-iot-device/azure/iot/device/common/callable_weak_method.py


    def __call__(self, *args, **kwargs):
        return self._get_method()(*args, **kwargs)

    def __eq__(self, other):
        return self._get_method() == other
+        # if inspect.isfunction(other):


if inspect.isfunction(other): [](start = 8, length = 31)

I've been debating this in my head. I think you should leave the getattr change and revert the other 2 changes. #Closed

I agree, especially since the getattr is the only thing required to fix the issue.

In reply to: 427012869 [](ancestors = 427012869)

BertKleewein · 2020-05-19T03:49:02Z

azure-iot-device/azure/iot/device/common/auth/__init__.py

+from .signing_mechanism import SymmetricKeySigningMechanism
+
+# from .sastoken import SasToken
+# from .connection_string import ConnectionString


please remove #Closed

BertKleewein · 2020-05-19T04:20:32Z

azure-iot-device/azure/iot/device/common/pipeline/pipeline_stages_base.py

+# Perhaps some kind of... Pipeline Daemon?
+class SasTokenRenewalStage(PipelineStage):
+    # Amount of time, in seconds, prior to token expiration, when the renewal process will begin
+    DEFAULT_TOKEN_RENEWAL_MARGIN = 120


DEFAULT_TOKEN_RENEWAL_MARGIN = 120 [](start = 4, length = 34)

why is this set here instead of setting this in the pipeline config with the renewal interval? part of the purpose of this is to account for clock skew between the client and the server and it's not had to imagine needing a bigger margin if you know that your device clock is going to be inaccurate for some reason or another. #Pending

to be clear are you saying this would be user input kwarg ?

In reply to: 427021277 [](ancestors = 427021277)

yes, margin should be a config kwarg that the user can supply

In reply to: 428953125 [](ancestors = 428953125,427021277)

BertKleewein · 2020-05-19T15:52:56Z

azure-iot-device/azure/iot/device/common/pipeline/pipeline_stages_base.py

+        # a few seconds given processing time. We could make this more accurate if SasToken
+        # objects had a live TTL value rather than a static one (i.e. "there are n seconds
+        # remaining in the lifespan of this token", rather than "this token was intended to live
+        # for n seconds")


Given the possibility of clock skew, TTL is so vague that you're really splitting hairs here. :) #Closed

I agree, that's why I didn't upend the implementation to account for this. Figured it would be a good information to document inline however.

In reply to: 427411587 [](ancestors = 427411587)

BertKleewein · 2020-05-19T15:57:47Z

I love the clean relationships between sastoken, signing_mechanism, and the renewal logic! This is a huge improvement! #Closed

BertKleewein · 2020-05-19T16:19:37Z

azure-iot-device/azure/iot/device/common/pipeline/pipeline_stages_base.py

+                this._start_renewal_timer()
+
+            self._token_renewal_timer = threading.Timer(seconds_until_renewal, renew_token)
+            self._token_renewal_timer.daemon = True


self._token_renewal_timer.daemon = True [](start = 12, length = 39)

Mental exercise: Consider different token lifetime rules. why do we create a token on pipeline init and not on connect? Why do we keep the token renewal timer going after we disconnect? We don't need the token before we connect and we don't need it after we disconnect.

This is why I'm asking the question: daemon=True protects us for shutdown scenarios. In other words, if a daemon timer (thread) is still running when we shut down, it won't stop the app from exiting. That's good.

But, daemon=True doesn't stop the timer from living longer than the pipeline. Consider an app that makes multiple clients, with each client sending a message then dying. For each client instance, there would be a thread that lives for up to a full hour past the client. Then, when each timer fires, it silently raises an exception when it tries to dereference this and dies. No harm, but still sloppy and sort-of a memory leak. #Pending

BertKleewein · 2020-05-19T16:54:46Z

azure-iot-device/azure/iot/device/common/auth/sastoken.py

+        self.refresh()
+
+    def __str__(self):
+        return self._token


Mental exercise :#2 (written after the one in pipeline_stages_base.py).

postulate: The notion that a SasToken has a value and that we need to periodically "refresh" that value is not quite right. It's better to think about the SasToken object as a "password generator" object. When a consumer of this class needs to talk to a network service, he can use this object to generate a password that gives him access for some period of time. When that password is set to expire, the consumer is responsible for generating a new one. This is how the code is written and I like it.

Saying token.refresh(); password=str(token); as we do, accomplishes this, but it's not quite semantically valid(). Rather, what we want is password=token.generate_new_password()

why this matters: It doesn't really matter that much, but, there an odd ownership relationship with the root owning the SasToken, object and 2 different pipelines controlling it. Both MQTT and HTTP can refresh the token. If we think of the SasToken object as having a value, having someone else change the value feels wrong. But if we think of each pipeline as owning it's own password that it gets from the generator object, it just feels better. #Pending

BertKleewein · 2020-05-19T18:17:31Z

azure-iot-device/azure/iot/device/iothub/edge_hsm.py

-
-
-class IoTEdgeHsm(object):
+class IoTEdgeHsm(SigningMechanism):


don't know the solution, but it seems odd that SigningMechanism is in the common.auth sub-package, but this one is not in an auth sub-package #Closed

Yeah, I'm not thrilled about it, but I didn't really want to make a package just for one module

In reply to: 427506098 [](ancestors = 427506098)

BertKleewein · 2020-05-19T20:23:49Z

azure-iot-device/tests/common/pipeline/test_pipeline_ops_http.py

@@ -151,7 +78,7 @@ def test_reason(self, cls_type, init_kwargs):

 pipeline_ops_test.add_operation_tests(
    test_module=this_module,
-    op_class_under_test=pipeline_ops_http.SetHTTPConnectionArgsOperation,
+    op_class_under_test=pipeline_ops_http.HTTPRequestAndResponseOperation,


op_class_under_test=pipeline_ops_http.HTTPRequestAndResponseOperation, [](start = 4, length = 70)

huh. apparently this attribute isn't that important #Closed

I mean.... it is important... it's just that it doesn't necessarily cause failure. If you pass the wrong one, you test the wrong one. If that wrong one still passes all tests, this isn't going to fail

In reply to: 427577323 [](ancestors = 427577323)

BertKleewein · 2020-05-22T14:53:19Z

azure-iot-device/tests/common/pipeline/config_test.py

+
+
+@six.add_metaclass(abc.ABCMeta)
+class PipelineConfigInstantiationTestBase(object):


PipelineConfigInstantiationTestBase [](start = 6, length = 35)

<eyes crossing at 7 AM> Man, this "InstantiationTestBase" convention makes for some long identifiers. No change requested. Just murmuring to myself. Get off my lawn.

Some of these test class names could be confused for parody of Java tbh 😂

In reply to: 429295058 [](ancestors = 429295058)

BertKleewein

olivakar

🐳 🐋

Initial revisions

2fb091a

cartertinney force-pushed the ct-auth-revision branch from 935075a to 2fb091a Compare May 18, 2020 20:14

Renamed SendOutputEvent

0e4007b

olivakar reviewed May 18, 2020

View reviewed changes

olivakar reviewed May 19, 2020

View reviewed changes

BertKleewein reviewed May 19, 2020

View reviewed changes

cartertinney and others added 2 commits May 19, 2020 12:41

First pass revisions

56640bf

Merge branch 'master' into ct-auth-revision

51115ff

BertKleewein reviewed May 19, 2020

View reviewed changes

further cleanup

79800a3

BertKleewein reviewed May 22, 2020

View reviewed changes

BertKleewein approved these changes May 22, 2020

View reviewed changes

olivakar approved these changes May 27, 2020

View reviewed changes

cartertinney merged commit fd27e8c into Azure:master May 27, 2020

cartertinney deleted the ct-auth-revision branch May 27, 2020 18:10



		class IoTEdgeHsm(object):
		class IoTEdgeHsm(SigningMechanism):



		@six.add_metaclass(abc.ABCMeta)
		class PipelineConfigInstantiationTestBase(object):

refactor(azure-iot-device): Auth Revisions #555

refactor(azure-iot-device): Auth Revisions #555

Conversation

cartertinney commented Apr 29, 2020 • edited Loading

cartertinney commented Apr 29, 2020

cartertinney commented Apr 30, 2020

cartertinney commented May 4, 2020

olivakar commented May 16, 2020

azure-pipelines bot commented May 16, 2020

olivakar May 18, 2020 • edited by cartertinney Loading

Choose a reason for hiding this comment

cartertinney May 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivakar May 18, 2020 • edited by cartertinney Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivakar May 18, 2020 • edited by cartertinney Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivakar May 19, 2020 • edited by cartertinney Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivakar May 19, 2020 • edited by cartertinney Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cartertinney May 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivakar May 19, 2020 • edited by cartertinney Loading

Choose a reason for hiding this comment

BertKleewein May 19, 2020 • edited Loading

Choose a reason for hiding this comment

BertKleewein May 19, 2020 • edited Loading

Choose a reason for hiding this comment

if inspect.isfunction(other): [](start = 8, length = 31)

Choose a reason for hiding this comment

BertKleewein May 19, 2020 • edited Loading

Choose a reason for hiding this comment

BertKleewein May 19, 2020 • edited by cartertinney Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BertKleewein May 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BertKleewein commented May 19, 2020 • edited Loading

BertKleewein May 19, 2020 • edited by cartertinney Loading

Choose a reason for hiding this comment

BertKleewein May 19, 2020 • edited by cartertinney Loading

Choose a reason for hiding this comment

BertKleewein May 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BertKleewein May 19, 2020 • edited Loading

Choose a reason for hiding this comment

cartertinney May 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BertKleewein left a comment

Choose a reason for hiding this comment

olivakar left a comment

Choose a reason for hiding this comment

cartertinney commented Apr 29, 2020 •

edited

Loading

olivakar May 18, 2020 •

edited by cartertinney

Loading

cartertinney May 19, 2020 •

edited

Loading

olivakar May 18, 2020 •

edited by cartertinney

Loading

olivakar May 18, 2020 •

edited by cartertinney

Loading

olivakar May 19, 2020 •

edited by cartertinney

Loading

olivakar May 19, 2020 •

edited by cartertinney

Loading

cartertinney May 19, 2020 •

edited

Loading

olivakar May 19, 2020 •

edited by cartertinney

Loading

BertKleewein May 19, 2020 •

edited

Loading

BertKleewein May 19, 2020 •

edited

Loading

BertKleewein May 19, 2020 •

edited

Loading

BertKleewein May 19, 2020 •

edited by cartertinney

Loading

BertKleewein May 19, 2020 •

edited

Loading

BertKleewein commented May 19, 2020 •

edited

Loading

BertKleewein May 19, 2020 •

edited by cartertinney

Loading

BertKleewein May 19, 2020 •

edited by cartertinney

Loading

BertKleewein May 19, 2020 •

edited

Loading

BertKleewein May 19, 2020 •

edited

Loading

cartertinney May 19, 2020 •

edited

Loading