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

Clear OTP after initial run. Fixes #1244 #1258

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yossizahn
Copy link

@yossizahn yossizahn commented Dec 23, 2024

When --persistent=>0 the OTP should be cleared from the configuration, because it's a one-time password as the name suggests and is probably not valid anymore.

Should there be an option to keep the OTP valid until a certain amount of time has lapsed, since TOTPs may still be valid?

@DimitriPapadopoulos
Copy link
Collaborator

An OTP should not appear in the configuration file in the first place.

@yossizahn
Copy link
Author

@DimitriPapadopoulos I was not referring to the configuration file but to the struct vpn_config structure that is passed around

@yossizahn
Copy link
Author

yossizahn commented Dec 24, 2024

See also discussion in #1244 for more background on the issue this PR is trying to solve

Basically, when --perpertual is set to more than 0, and the fortigate server is set to ask for OTP, subsequent attempts to connect will just keep using the OTP that was supplied for the 1st connection, whether by --otp flag, or interactively, since both the flag and interactive input will populate the otp field of struct vpn_config and once it's populated, openfortivpn doesn't prompt the user for another OTP

See relevant code:

openfortivpn/src/http.c

Lines 748 to 756 in 1d28e63

if (cfg->otp[0] == '\0') {
// Interactively ask user for 2FA token
char hint[USERNAME_SIZE + 1 + REALM_SIZE + 1 + GATEWAY_HOST_SIZE + 5];
sprintf(hint, "%s_%s_%s_2fa",
cfg->username, cfg->realm, cfg->gateway_host);
read_password(cfg->pinentry, hint,
"Two-factor authentication token: ",
cfg->otp, OTP_SIZE);

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