-
Notifications
You must be signed in to change notification settings - Fork 442
totp auto generation #797
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
totp auto generation #797
Conversation
|
How did you come up with all the translations? I assume they were autogenerated by some machine translation? |
| c->hwndStatus, | ||
| ID_TXT_STATUS, | ||
| LoadLocalizedString(IDS_NFO_STATE_ROUTE_ERROR)); | ||
| return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unecessary white space changes in a lot of places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right, but this is the result of reformatting according to the rules of .clang-format in the repository. After making some modifications I have reformatted all the modified .h/.c and this is the result.
| ID_DLG_AUTH_CHALLENGE DIALOGEX 6, 18, 260, 146 | ||
| STYLE WS_POPUP | WS_VISIBLE | WS_CAPTION | DS_CENTER | DS_SETFOREGROUND | ||
| EXSTYLE WS_EX_TOPMOST | ||
| CAPTION "OpenVPN – Benutzer-Authentifizierung" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are some of the existing texts changed? If translation is to be improved, please do that in a separate PR.
| @@ -0,0 +1,307 @@ | |||
| #include "totp.h" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing license text and also source where this implementation originates from. I find highly doubtful that you wrote this from scratch just for OpenVPN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that part was completely written from scratch just for OpenVPN, basically the skeleton was vibecoded and fixed manually a bit, after that it was partly covered with testing code. But, of course, the license text similar to other files should be added, you are right.
| @@ -0,0 +1,28 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't #pragma once is a valid C11 statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, I will replace it with conventional include guards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm totally not in favour of having this as a feature in GUI. You may want to reconsider spending time on this.
|
All the stylistic questions aside, I have serious doubts that this is a good addition. The whole point of 2FA is not to use software that gives you a trivial workaround - the result is likely to get openvpn banned from your network. If the security admin adds TOTP requirements, they want you to use a second factor, and that is usually for a good reason. |
|
I also wanted also to say that I am skeptical about this feature. In scenarios where you need the authentication without two factor, the server admin should be able to disable TOTP for you. So this feature seems to only valid for scenarios where the user disregards the policies set by the server admin unless I am missing some scenario. It can be useful as a debug build only feature but feels to be too much code for a that. |
Exactly my thinking as well. |
Yes, this is the result from chatgpt 5.2 Pro |
Thanks for the feedback — I agree with the concern in the general case: storing a TOTP seed on the same workstation reduces separation of factors compared to a phone/hardware token. The intent of this change is narrower: it targets deployments where the shared TOTP seed is already present on the endpoint (e.g. users already generate the OTP locally via KeePassXC/desktop authenticator and then copy/paste it into the “Response” field). In that situation, the second factor is already effectively on the machine; this PR does not enable a new bypass, it mainly eliminates manual copy/paste and can even reduce exposure via the clipboard. If an admin’s policy requires the second factor to live on a separate device/channel, the typical way to enforce that is to not provision the seed to the endpoint (or use push/WebAuthn/SMS). In that scenario this feature cannot be used and therefore does not weaken the policy. |
This allows the user to bypass most (if not all) TOTP implementations where the user is asked to scan a QR code into an authenticator app. The QR code contains the secret in plain text. Extracting that secret and saving it in the GUI would be trivial. I'm not saying users should not do this -- its not our place to facilitate this in a widely used application like OpenVPN-GUI. So NAK from me. |
|
Thanks for the candid feedback. I understand the “facilitation” concern: even if a user can extract an otpauth secret from a QR code on their own, baking seed storage + OTP generation into OpenVPN-GUI lowers the bar and may conflict with MFA policies. Given that stance, it sounds like this feature is not a good fit for upstream. I can close this PR and keep it as a downstream patch/fork for environments where OTP seeds are already endpoint-managed. So let's make a decision do we need this feature? If so I will fix other issues, if not - we need to decline this. |
|
Selva said NAK, and he's the one who does most of the work on openvpn-gui. Since none of the other developers are chiming in with strong arguments in favour, the question is settled already :-) This is a bit of an unhappy situation, refusing work that has already been done and is offered to us. But for us it's always a trade-off, maintenance effort, code size, political statements, security, ... |
Summary
This pull request adds optional, per-profile TOTP auto-generation for OpenVPN static challenge/response authentication flows in OpenVPN GUI. If a profile uses a challenge “Response” (OTP) field and the user has already saved:
…then the GUI will generate the current TOTP automatically and complete authentication without prompting. If any of the required values are missing, the GUI behaves as before and shows the existing authentication dialog (now extended with a button to set/clear the OTP secret).
Motivation
Many deployments use “password + TOTP” where the OTP is entered into the Response field of the GUI’s auth dialog. Today, users must copy/paste the TOTP from an external app (KeePassXC, Authenticator, etc.) every time they connect. This is inconvenient and error-prone, and it prevents a true “one-click” or “silent” connection experience even when username/password are already saved.
This PR makes that workflow optional and streamlined by letting users store the OTP seed once and then letting the GUI generate the correct TOTP on demand.
User-visible behavior
When the OTP secret is not set
When username + password + OTP secret are all present
Clearing the stored secret
Protocol / OpenVPN compatibility
Security considerations
This feature is explicitly opt-in. The OTP secret is not stored unless the user actively sets it.
Important trade-off: storing a TOTP seed on the same machine as the saved username/password reduces the “separation of factors” property of 2FA if the host/user profile is compromised. This PR treats the OTP secret as a credential-class secret (similar sensitivity to a saved password):
Implementation overview
Core TOTP module
Credential storage
UI integration
Extends the existing username/password/challenge auth dialog:
Implements “skip auth dialog” behavior when all required values exist and a Response is required:
Localization
Updates resource files for all supported languages so the new UI controls exist for all locales (text may be translated or temporarily mirrored from English, depending on the locale content in this PR).
Testing
Unit test utility (no external framework)
Manual test checklist
Profile without OTP secret:
Profile with username/password/secret:
Clearing secret:
Backward compatibility
Notes for reviewers