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

multi factor authentication for ssh #1037

Open
wants to merge 25 commits into
base: devel
Choose a base branch
from

Conversation

theCalcaholic
Copy link
Collaborator

@theCalcaholic theCalcaholic commented Dec 14, 2019

As proposed in #1035 I implemented an ncp app to manage multi (and single) factor authentication methods. Currently supported are:

  • password only (default)
  • public key authentication
  • public key + password
  • TOTP (google-authenticator based) + password

If multiple options are enabled, they will act as alternatives. However, if at least one mfa method is enabled, all single factor methods will be disabled automatically.
Also, it is unfortunately impossible to use totp (or any PAM based) authentication methods with non-PAM password authentication. That makes some combinations invalid, e.g. having pubkey+pw and totp+pw both enabled.

Functionality wise I'm satisfied with the current state of the ncp app now. The only missing things that I'm aware of are documentation and localization at this point.

TODO:

  • Implement provision of ssh public key
  • Prevent invalid/broken combinations of parameters/authentication methods
  • Fix terminal version (when run via ncp-config)
  • Provide QR-Code in web interface (?)
  • Add localization
  • Test with debian buster

image

image

@theCalcaholic
Copy link
Collaborator Author

theCalcaholic commented Dec 14, 2019

Somehow the ncp app is currently broken, but only in the terminal (ncp-config). I'll have to investigate that...

@theCalcaholic
Copy link
Collaborator Author

theCalcaholic commented Dec 14, 2019

@nachoparker I have now implemented the ability to supply a public key. However, it get's caught by the sanitization, because SSH public keys contain spaces (specifically at this line).
I know that it's hard to properly handle them in bash without security risks, but in some cases spaces are entirely valid.
I have two ideas on how to approach this, please let me know what you think about it:

  1. Mark parameters in the nc-app config if they should be able to receive unsafe inputs. That way we only needed to pay attention when handling specific parameters and could apply extra caution.

  2. Sanitize parameters with spaces by replacing them (e.g. with %SPACE%). That way, if a script needs access to the unescaped variable it could just reverse the process with a simple search & replace (maybe a function for that should be provided via the library.sh).

  3. A combination of the two could also make sense, because it would probably be a good idea if a script knew when to revert potential sanitization.

@theCalcaholic
Copy link
Collaborator Author

I have taken the liberty to implement the 3rd option as an example. If you are not fine with it, it can always be reverted.

@theCalcaholic theCalcaholic changed the base branch from master to devel December 15, 2019 14:15
@theCalcaholic theCalcaholic changed the base branch from devel to master December 15, 2019 14:25
@theCalcaholic theCalcaholic force-pushed the feature/multi-factor-ssh branch 4 times, most recently from 0c038af to 9f7aa2b Compare December 17, 2019 12:43
@theCalcaholic theCalcaholic changed the title [WIP] multi factor authentication for ssh multi factor authentication for ssh Dec 29, 2019
@theCalcaholic
Copy link
Collaborator Author

theCalcaholic commented Jan 2, 2020

Hm, maybe it would be a good idea to add a few more text fields for ssh public keys...

I'll implement that in a bit (don't merge yet 😉)

@theCalcaholic
Copy link
Collaborator Author

theCalcaholic commented Jan 10, 2020

@nachoparker Alright, it's feature complete now

EDIT: Please note, that spaces (for fields which have 'allow_unsafe' set to true) will be shown as %SPACE% in ncp-config, because we can't pass parameters with spaces to dialog atm (possibly related to #1038).

@nachoparker
Copy link
Member

Thanks! I will take a look soon, but I don't know when since I have family visits for the following weeks.

@theCalcaholic
Copy link
Collaborator Author

Sure, take your time :)

theCalcaholic and others added 12 commits January 21, 2020 15:20
- Fix misspelled variable names

Signed-off-by: Tobias K <[email protected]>
Signed-off-by: Tobias Knöppler <[email protected]>
Signed-off-by: Tobias K <[email protected]>
Signed-off-by: Tobias Knöppler <[email protected]>
Signed-off-by: Tobias K <[email protected]>
Signed-off-by: Tobias Knöppler <[email protected]>
Signed-off-by: Tobias K <[email protected]>
Signed-off-by: Tobias Knöppler <[email protected]>
…l quotes)

Signed-off-by: Tobias K <[email protected]>
Signed-off-by: Tobias Knöppler <[email protected]>
Signed-off-by: Tobias K <[email protected]>
Signed-off-by: Tobias Knöppler <[email protected]>
…er was retrieved correctly.

Signed-off-by: Tobias K <[email protected]>
Signed-off-by: Tobias Knöppler <[email protected]>
Signed-off-by: Tobias K <[email protected]>
Signed-off-by: Tobias Knöppler <[email protected]>
…ssword reliant methods (not possible due to limitations of sshd configuration)

Signed-off-by: Tobias K <[email protected]>
Signed-off-by: Tobias Knöppler <[email protected]>
…p is enabled

Signed-off-by: Tobias K <[email protected]>
Signed-off-by: Tobias Knöppler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants