Skip to content

Add multi-factor authentication modules#15489

Merged
balloob merged 3 commits intohome-assistant:devfrom
awarecan:auth-2fa-v2
Aug 22, 2018
Merged

Add multi-factor authentication modules#15489
balloob merged 3 commits intohome-assistant:devfrom
awarecan:auth-2fa-v2

Conversation

@awarecan
Copy link
Copy Markdown
Contributor

@awarecan awarecan commented Jul 16, 2018

Description:

Add multi-factor authentication support for new auth system.

Auth module

Multi-factor auth modules has been added to auth/modules folder.

~~totp is Time-based One Time Password module which compatible with Google Authenticator and Authy (not test yet). ~~ will be included in separate PR

insecure_example is a Example designed for demo and unit testing purpose, should not be used in any production system.

Multi-facot auth module can be used mixed-match with auth providers. After normal auth provider validate, if there are auth modules enabled for the specific user, the login flow will direct to auth module input form. Auth module use separated storage, can be hardening it in future.

Related issue (if applicable): fixes #

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

Example entry for configuration.yaml (if applicable):

# configuration.yaml
homeassistant:
  # Auth Provider
  auth_providers:
    - type: homeassistant
  auth_mfa_modules:
    - type: totp

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:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.

If the code does not interact with devices:

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'tests.common.MockUser' imported but unused

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'homeassistant.auth.models.Credentials' imported but unused

@balloob
Copy link
Copy Markdown
Member

balloob commented Jul 17, 2018

For architecture, if we need to resolve the flow result to credentials, we should just do that. We can't be making things up. Maybe we need a config flow that wraps the provider config flow instead of using inheritance. That way we can forward calls and then intercept the call to async_create_entry. But that too is a complicated solution. We should focus on the rest of the auth system before picking this up again.

@awarecan
Copy link
Copy Markdown
Contributor Author

Sure, I am going to finish change user.mfa_modules design tomorrow, then I will put this PR on the shelf for a while.

The way mfa modules work, is that we persist in the user which mfa are enabled, and then we ask the mfa module to show a form. I feel like this violates the single source of truth. We should probably not store which mfa modules are enabled for a user. Instead:

  • MFA modules will get a method async_is_enabled_for(user)
  • We can add a method to auth manager to gather enabled MFA modules for a user. It then asks each MFA module.

@balloob
Copy link
Copy Markdown
Member

balloob commented Jul 17, 2018

I was thinking about that comment, and with credentials we do the exact opposite. We store the data that connects a user to an auth provider in the user. We should probably do that for MFA too. That way we can ask an MFA module: given this config and this value, is this ok? The only downside is if an MFA module wants to update the data 🤔

@awarecan
Copy link
Copy Markdown
Contributor Author

awarecan commented Jul 17, 2018

That is current design:

in mfa data store:

{
    "data": {
        "users": [
            {
                "ota_secret": "some_secret",
                "user_id": "some_id"
            }
        ]
    },
    "key": "auth_module.totp",
    "version": 1
}

In auth

        "users": [
            {
                "id": "some_id",
                "is_active": true,
                "is_owner": true,
                "mfa_modules": ["totp"],
                "name": null,
                "system_generated": false
            }
        ]

@awarecan
Copy link
Copy Markdown
Contributor Author

awarecan commented Jul 17, 2018

I can just delete mfa_modules from users since it is redundant, e.g. do not touch current auth data store in dev branch

@balloob
Copy link
Copy Markdown
Member

balloob commented Jul 17, 2018

Yeah, single source of truth 👍

@awarecan awarecan changed the title WIP: [Recreated PR] Add multi-factor authentication plug-able modules [Recreated PR] Add multi-factor authentication plug-able modules Jul 18, 2018
@awarecan
Copy link
Copy Markdown
Contributor Author

Rebase against dev

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add a lot more documentation to this method . What is happening, when and why.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I am slightly confused why we are even making changes for this method?

All auth provider login flows will create async_start_mfa(flow_result). The MFA flow will show more forms if necessary and eventually will pass the original flow result on if MFA has passed. That means that this method will not see any new data?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed design, now the auth provider (not the base class) will not know about mfa, they just call async_finish after validate credential. The _async_finish_login_flow will do the heave lift, if need mfa then change the result type to form by calling async_step_select_mfa_module

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constructors should never have side effects. Add this to auth_mfa_module_from_config

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would this not be the case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For trusted networks auth provider

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah interesting.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is async_start_mfa defined?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect async_start_mfa to not do anything if the context is type=link_user.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, that is from old design, missed to update document. It should change to
Return await self.async_finish(flow_result) if login init step pass

@awarecan
Copy link
Copy Markdown
Contributor Author

Rebase done, clean it up as much as possible, test should be able to pass, typing still need fix. And this PR is still 900+ lines need review 😄

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'..models.User' imported but unused

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'typing.Callable' imported but unused

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'collections.OrderedDict' imported but unused

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉

@balloob balloob merged commit 7e7f9bc into home-assistant:dev Aug 22, 2018
@ghost ghost removed the in progress label Aug 22, 2018
@awarecan awarecan deleted the auth-2fa-v2 branch August 22, 2018 12:05
@balloob balloob mentioned this pull request Aug 29, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants