Add Notify MFA module by awarecan · Pull Request #16314 · home-assistant/core

awarecan · 2018-08-31T08:02:52Z

Description:

Add a multi-factor authentication module that using notify service to delivery one-time password to user during the login process

Related issue (if applicable): fixes #

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#6131

Dev document: home-assistant/developers.home-assistant#80

Example entry for `configuration.yaml` (if applicable):

homeassistant:
  auth_mfa_modules:
     - type: notify

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code does not interact with devices:

Tests have been added to verify that the new code works.

awarecan · 2018-08-31T08:03:51Z

Almost done. Need add translation, tweak some wording, and do more tests on real notify service

houndci-bot · 2018-09-01T10:00:20Z

+
+async def test_setup_user_no_notify_service(hass):
+    """Test setup flow abort if there is no avilable notify service."""
+    notify_calls = async_mock_service(


local variable 'notify_calls' is assigned to but never used

balloob · 2018-09-04T08:19:18Z

        """Return True if validation passed."""
        raise NotImplementedError

+    async def async_generate(self, user_id: str) -> Optional[str]:


This should not be part of the MFA module interface. Modules need to generate their own codes and ask for that. There is no need to share this. We can make a reusable function available to generate 6 digit codes.

balloob · 2018-09-04T08:19:43Z

+                                notify_service, target)
+
+    @callback
+    def aync_get_aviliable_notify_services(self) -> List[str]:


balloob · 2018-09-04T08:21:13Z

+
+    def _validate_one_time_password(self, user_id: str, code: str) -> bool:
+        """Validate one time password."""
+        ota_secret, counter, notify_service, target = \


We should not use tuples to store data. Use dicts instead.

balloob · 2018-09-04T08:21:36Z

+        ota_secret, counter, notify_service, target = \
+            self._users.get(user_id, (None, 0, None, None))  # type: ignore
+        if ota_secret is None:
+            # even we cannot find user, we still do verify


I don't think that this is necessary, as a user has to pick a MFA module to use.

In case the HTTP request got pollution

It doesn't matter, we just can return right away without doing any computation. We only need to care about this if we are verifying a username/password combo and want to have a similar runtime between username not found and incorrect password.

balloob · 2018-09-04T08:22:29Z

+
+        result = _verify_otp(ota_secret, code, counter)
+
+        # move counter no matter if passed validation


Does this mean that if a user makes a typo, the sent code is no longer valid? I think that it's not necessary to use pyotp for this. We just need a 6 digit code that is stored for the duration of the config flow.

Then we need add either retry limit or time delay between failed attempts.

MFA login flows expire after 5 minutes, wouldn't that be enough?

5 min is not enough. I tried direct call pyotp on RPi 3B+, it take 3 mintues 50 seconds to brute force all combination of 6 digital for TOTP and only 2 mintues 40 seconds for HOTP.

Hmm. What about 3 tries? After that we send a new one.

For notify limit to 3 times. After limit flow will abort
For totp limit to 5 times

balloob · 2018-09-04T08:25:10Z

        mfa_conf = config.get(CONF_AUTH_MFA_MODULES, [
-            {'type': 'totp', 'id': 'totp', 'name': 'Authenticator app'}
+            {'type': 'totp', 'id': 'totp', 'name': 'Authenticator app'},
+            {'type': 'notify', 'id': 'notify'}


I don't think that we should make it available as a default MFA Module from the get go. SMS notifications are known to be able to be hijacked.

houndci-bot · 2018-09-05T02:31:32Z

+        await self.async_notify_user(user_id, code)
+
+
+    def _generate_and_send_one_time_password(self, user_id: str) -> str:


too many blank lines (2)

awarecan · 2018-09-05T06:09:29Z

Change to increase counter before generate code, allow user try 3 times before login flow abort

balloob · 2018-09-05T10:15:10Z

+CONFIG_SCHEMA = MULTI_FACTOR_AUTH_MODULE_SCHEMA.extend({
+    vol.Optional(CONF_INCLUDE): vol.All(cv.ensure_list, [cv.string]),
+    vol.Optional(CONF_EXCLUDE): vol.All(cv.ensure_list, [cv.string]),
+    vol.Optional(CONF_MESSAGE, default='Your Home Assistant One-time Password'


Let's change default message around, as notifications usually just show first couple of words.

{} is your Home Assistant login code.

balloob · 2018-09-05T13:11:27Z

+    import pyotp
+
+    ota_secret = pyotp.random_base32()
+    counter = SystemRandom().randint(0, 2 << 31)


Can't we just always use a random input for counter value?

Also, do we really need to use pyotp to generate a random code between 100000 and 999999 ? What does it do that makes it more secure?

For the random counter, strength is same, pyotp internal use SystemRandom as well. However I will change it to use pyotp just looks consistent.

pyotp implemented RFC 4226 for the one-time password generation, I thought it will be secure that we write a routine by ourselves.

We can use 8 digit number for the code, looks better than 6 digit

I prefer 6 digits because it's easier to remember when you are typing it from your phone.

balloob · 2018-09-10T07:40:33Z

+STORAGE_KEY = 'auth_module.notify'
+STORAGE_USERS = 'users'
+STORAGE_USER_ID = 'user_id'
+STORAGE_OTA_SECRET = 'ota_secret'


balloob · 2018-09-10T07:41:19Z

+def _generate_otp(secret: str, count: int) -> str:
+    """Generate one time password."""
+    import pyotp
+    return str(pyotp.HOTP(secret, digits=8).at(count))


I really think we should stick with 6 digits. We are already limiting it at 3 tries and 6 is a) what every other service uses and b) is easier to remember.

balloob · 2018-09-10T07:41:55Z

+class NotifySetting:
+    """Store notify setting for one user."""
+
+    secret = attr.ib(type=str, factory=_generate_secret)   # not persistent


If it's not persistent, we shouldn't put it on the storage class.

Because instantiating it will have the random side effect of generating the secret / random. Meaning during load, all these things get generated without maybe not being needed.

balloob · 2018-09-10T07:42:46Z

+    """Auth module send hmac-based one time password by notify service."""
+
+    DEFAULT_TITLE = 'Notify One-Time Password'
+    DUMMY_SECRET = '7Z5EFWI4RFLVV67G'


balloob · 2018-09-10T07:48:49Z

+    @callback
+    def aync_get_available_notify_services(self) -> List[str]:
+        """Return list of notify services."""
+        unordered_services = list(self.hass.services.async_services().get(


unordered_services = [] for service in self.hass.services.async_services().get('notify', {}): if service not in self._exclude: unordered_services.append(service)

balloob · 2018-09-10T07:49:57Z

+            if exclude_service in unordered_services:
+                unordered_services.remove(exclude_service)
+
+        if self._include:


If this was a set and unorded_services too, you could just do unordered_services &= self._include

balloob · 2018-09-10T08:00:21Z

+            return False
+
+        # user_input has been validate in caller
+        return await self.hass.async_add_executor_job(


I wonder if we did the right thing with MFA architecture. Would it be possible to get an instance of a flow which can store instance variables instead of storing temp values in our storage object. The latter feels hacky.

I even wrote a version of that somewhere in the history. The reason I give it up because consider following scenario:

User login, send a notify, for some reason he didn't received the code, so he opens another browser, login, send the second notify, now two code arrived together, can he still use the first code to login in first login session?

I had seen this on other systems, most of them won't allow you to use the first code anyway, for that case, the code is associated with user, not login flow instance.

I think it's fine to have a code be bound to a login flow. That's what I would have expected.

balloob · 2018-09-10T08:04:09Z

        self._auth_manager = auth_provider.hass.auth  # type: ignore
        self.available_mfa_modules = {}  # type: Dict[str, str]
        self.created_at = dt_util.utcnow()
+        self.invalid_mfa_times = 0


This too could be moved to a MFA flow if we had it, allowing each MFA implementation to decide it on their own.

awarecan · 2018-09-23T18:19:13Z

I resolved several easy-fix code review comment. @balloob are you okay we merge in current state, and leave the mfa re-structure in a separate PR, e.g. create a separated MFA flow

balloob · 2018-09-24T08:25:10Z

+            return
+
+        await self._user_store.async_save({STORAGE_USERS: {
+            user_id: attr.asdict(


It's shorter code to just write the dict out

balloob

Ok let's merge and do the refactor in another PR.

homeassistant added the cla-signed label Aug 31, 2018

ghost assigned awarecan Aug 31, 2018

ghost added the in progress label Aug 31, 2018

homeassistant added the core label Aug 31, 2018

awarecan force-pushed the mfa-notify branch from a8ba101 to adb298e Compare August 31, 2018 14:19

homeassistant added the cla-signed label Aug 31, 2018

This was referenced Sep 1, 2018

Add MultiFactorAuthModule.async_initialize_login_mfa_step home-assistant/developers.home-assistant#80

Merged

Add notify MFA module home-assistant/home-assistant.io#6131

Merged

awarecan force-pushed the mfa-notify branch from adb298e to 399bbc7 Compare September 1, 2018 10:00

awarecan requested a review from a team as a code owner September 1, 2018 10:00

homeassistant added the cla-signed label Sep 1, 2018

houndci-bot reviewed Sep 1, 2018

View reviewed changes

awarecan changed the title ~~WIP: Add Notify MFA module~~ Add Notify MFA module Sep 1, 2018

awarecan force-pushed the mfa-notify branch from 399bbc7 to 41bb4ab Compare September 1, 2018 10:01

homeassistant added the cla-signed label Sep 1, 2018

balloob reviewed Sep 4, 2018

View reviewed changes

houndci-bot reviewed Sep 5, 2018

View reviewed changes

awarecan force-pushed the mfa-notify branch from 88d0362 to f6c4635 Compare September 5, 2018 02:32

homeassistant added the cla-signed label Sep 5, 2018

awarecan force-pushed the mfa-notify branch from d3df950 to 40fa6c0 Compare September 5, 2018 03:46

homeassistant added the cla-signed label Sep 5, 2018

balloob reviewed Sep 5, 2018

View reviewed changes

balloob reviewed Sep 10, 2018

View reviewed changes

awarecan added 13 commits September 23, 2018 10:54

Add Notify MFA

08af92d

Fix unit test

516671e

Address review comment, change storage implementation

6c5536b

Add retry limit to mfa module

9984fa5

Fix loading

2d8e6da

Fix invalaid login log processing

0bf7bc6

Typing

e2da768

Change default message template

fd65a1b

Change one-time password to 8 digit

471b73a

Refactoring to not save secret

942573d

Bug fixing

8a87465

Change async_initialize method name to aysnc_initialize_login_mfa_step

24f10cd

Address some simple fix code review comment

fd20a9b

awarecan force-pushed the mfa-notify branch from 34a1455 to fd20a9b Compare September 23, 2018 18:16

homeassistant added the cla-signed label Sep 23, 2018

balloob reviewed Sep 24, 2018

View reviewed changes

balloob approved these changes Sep 24, 2018

View reviewed changes

balloob merged commit 7a77951 into home-assistant:dev Sep 24, 2018

ghost removed the in progress label Sep 24, 2018

balloob mentioned this pull request Sep 28, 2018

0.79.0 #16940

Merged

home-assistant locked and limited conversation to collaborators Feb 5, 2019


		result = _verify_otp(ota_secret, code, counter)

		# move counter no matter if passed validation

		await self.async_notify_user(user_id, code)


		def _generate_and_send_one_time_password(self, user_id: str) -> str:

Uh oh!

Conversation

awarecan commented Aug 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

awarecan commented Aug 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awarecan commented Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awarecan commented Sep 23, 2018

Uh oh!

Choose a reason for hiding this comment

awarecan commented Aug 31, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

awarecan commented Sep 5, 2018 •

edited

Loading