feat: add early MFA functionality #188
Conversation
|
|
||
| # See if we have password and MFA, or just MFA | ||
|
|
||
| echo "$pass" | grep -q -i : | ||
|
|
||
| if [ $? -eq 0 ]; | ||
| then | ||
| realpass=$(echo "$pass" | cut -d: -f1) | ||
| mfatoken=$(echo "$pass" | cut -d: -f2) | ||
|
|
||
| # put code here to verify $realpass, the code below the if validates $mfatoken or $pass if false | ||
| # exit 0 if the password is correct, the exit below will deny access otherwise | ||
| fi | ||
|
|
|
|
||
| - name: delete clients in db those are not in clients list | ||
| set_fact: | ||
| cleaned_db: '{{ oath_secrets_db.list | selectattr("username", "in", clients) | list }}' |
There was a problem hiding this comment.
Please prefix this with openvpn_mfa_
| - name: prepare list of username and secret to be written to secret file | ||
| set_fact: | ||
| username_list: '{{ clients_merged | map(attribute="username") | list }}' | ||
| secret_list: '{{ clients_merged | map(attribute="secret") | list }}' | ||
|
|
||
| - name: write lines to file | ||
| copy: | ||
| content: "{{ username_list | zip(secret_list) | map('join', ':') | join('\n') }}" | ||
| dest: "{{ openvpn_base_dir }}/{{ openvpn_mfa_oathsecrets_file }}" | ||
| owner: "{{ openvpn_service_uesr }}" | ||
| mode: 0700 |
There was a problem hiding this comment.
Should this be a template that loops over clients_merged instead?
| secret=$(grep -i -m 1 "$user:" {{ openvpn_mfa_oathsecrets_file }} | cut -d: -f2) | ||
|
|
||
| # Calculate the code we should expect | ||
| code=$(oathtool --totp $secret) | ||
|
|
||
| if [ "$code" = "$pass" ]; | ||
| then | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
This is safe-ish. Only user gets interpolated and that has some character sanitization:
To protect against a client passing a maliciously formed username or password string, the username string must consist only of these characters: alphanumeric, underbar (''), dash ('-'), dot ('.'), or at ('@'). The password string can consist of any printable characters except for CR or LF. Any illegal characters in either the username or password string will be converted to underbar ('').
auth-user-pass-verify in https://openvpn.net/community-resources/reference-manual-for-openvpn-2-0/
I'd much prefer this to be in a script to keep the values as values instead of potentially executing them.
| # Calculate the code we should expect | ||
| code=$(oathtool --totp $secret) | ||
|
|
||
| if [ "$code" = "$pass" ]; |
There was a problem hiding this comment.
$pass should be validated to be all numeric, 6 digits
| pass=$(tail -1 $passfile) | ||
|
|
||
| # Find the entry in our oath.secrets file, ignore case | ||
| secret=$(grep -i -m 1 "$user:" {{ openvpn_mfa_oathsecrets_file }} | cut -d: -f2) |
There was a problem hiding this comment.
Should the path have {{ openvpn_base_dir }}/ as a prefix to make it absolute?
There was a problem hiding this comment.
In general:
- anything that uses set_fact should have
openvpn_mfa_as a prefix (functionally a namespace) - ansible-lint will want fully qualified names
- the validation script being in bash makes me worry about CLI injection. But other then validating the attacker-controlled secret is actually numeric I don't see another attack route because OpenVPN does character sanitization on the username field. Having it in a script would be preferable but not blocking
No description provided.