Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trusted Networks Auth Provider enhancement #169

Closed
awarecan opened this issue Mar 8, 2019 · 7 comments · Fixed by home-assistant/core#22478
Closed

Trusted Networks Auth Provider enhancement #169

awarecan opened this issue Mar 8, 2019 · 7 comments · Fixed by home-assistant/core#22478

Comments

@awarecan
Copy link

awarecan commented Mar 8, 2019

Current

Since 0.89, Trusted Networks Auth Provider will load its own config.

homeassistant:
  auth_providers:
    - type: trusted_networks
      trusted_networks:
        - 127.0.0.1
        - ::1
        - 192.168.0.0/24
        - fd00::/8

Proposed Change

homeassistant:
  auth_providers:
    - type: trusted_networks
      trusted_networks:
        - 127.0.0.1
        - ::1
        - 192.168.0.0/24
        - fd00::/8
      trusted_users:
        192.168.0.0/24:
          - user1_id
          - user2_id
        192.168.0.1: user1_id
        fd00::/8: 
           - group: group_1
      bypass_login: false

The changes around the user list provided in the login form, depends on where the request is coming from, the user list could be different

trusted_users key in the example explain
192.168.0.0/24 two users allow to choice
192.168.0.1 only one user allow to choice, this has overlap with above entry, we will do AND logic
fd00::/8 all users in group_1
rest all active, non-system users are in the option list

Especially if bypass_login is enabled and only one user could be chosen, the login form could be skipped.

Migration / Breaking Changes
No issue, all options are additional

EDIT: modify group base on feedback

@balloob
Copy link
Member

balloob commented Mar 8, 2019

How are you differentiating between group and user ids ?

bypass is one word.

@awarecan
Copy link
Author

awarecan commented Mar 8, 2019

First version will only support user. No good way in my mind to distinguish user and group. Maybe we can use g:some_id?

@balloob
Copy link
Member

balloob commented Mar 9, 2019

In YAML, you can actually do - group: abcdefg and it will be a list with a dictionary with 1 key 😬 Not sure if that is going to be acceptable 😉

For auth config, it needs to be crystal clear. I rather have it be too verbose, there is no room for mistakes.

@bob1de
Copy link

bob1de commented Mar 10, 2019

Maybe allow for a dictionary as value for trusted_networks as an alternative to the list?

# traditional
trusted_networks:
- 10.1.2.0/24

# alternative
trusted_networks:
  10.1.2.0/24:
    users:
    - user1
    - user2
    groups:
    - group1
    - group2

Would maintain backwards-compatibility while allowing for the new options to be used without duplicating the network cidrs under multiple keys.

@bob1de
Copy link

bob1de commented Mar 10, 2019

BTW, Do we want to sort the networks by longest prefix as in routing tables, so that smaller parts of a larger subnet can have a different config?

@awarecan
Copy link
Author

I am not the fun mixed networks and users together. Because user may choose only assign user to partial of trusted network. In that case, you can select all available users from login form.

Sort networks is a good idea. We should do it.

@bob1de
Copy link

bob1de commented Mar 12, 2019

Hmm, wouldn't sorting the networks by prefix length provide just that - the ability to assign particular users to a smaller subnet of a larger trusted network?

For a backwards-compatible syntax, I could also think of something like this:

trusted_networks:
- 10.1.2.0/24  # traditional, just a string
- network: 10.1.2.32/28  # new, dict
  users:
  - user1
  - user2
  groups: [ group1 ]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants