Skip to content

add --password-on-stdin#1279

Open
mafgh wants to merge 1 commit into
adrienverge:masterfrom
mafgh:stdin_master
Open

add --password-on-stdin#1279
mafgh wants to merge 1 commit into
adrienverge:masterfrom
mafgh:stdin_master

Conversation

@mafgh
Copy link
Copy Markdown

@mafgh mafgh commented Apr 23, 2025

This allows the password to be read from stdin, e.g. from a password manager. Closes #1063.

Comment thread src/main.c Outdated
log_warn("Could not read password from stdin");
break;
}
strncpy(cli_cfg.password, password, PASSWORD_SIZE);

Check failure

Code scanning / CodeQL

Cleartext storage of sensitive information in buffer

This write into buffer 'password' may contain unencrypted data from [user input (buffer read by read)](1).
@DimitriPapadopoulos
Copy link
Copy Markdown
Collaborator

DimitriPapadopoulos commented Apr 28, 2025

Not sure if this PR is the place to address these issues, but:

  1. What should happen if we pass --password-on-stdin multiple times? Right now openfortivpn attempts to read the password multiple times. I believe it should only try once.
  2. When omitting the server or passing invalid arguments, I believe openfortivpn should exit before asking for the password.
$ openfortivpn --password-on-stdin --password-on-stdin
<password1>
<password2>
WARN:   Could not load configuration file "/volatile/local/opefortivpn/etc/openfortivpn/config" (No such file or directory).
ERROR:  Specify a valid host:port couple.
Usage: openfortivpn [<host>[:<port>]] [-u <user>] [-p <pass>] [--password-on-stdin]
[...]
$ 

The --cookie-on-stdin option has the same issues, so I am OK with fix both options in a different PR.

@mafgh
Copy link
Copy Markdown
Author

mafgh commented Apr 29, 2025

How about we just set a flag during argument parsing:

if (strcmp(long_options[option_index].name, "password-on-stdin") == 0) {
  password_from_stdin = 1;
  break;
}

And then, just before this line:

if (cli_cfg.password_set) {

read from stdin (if that flag is set), etc.

@DimitriPapadopoulos
Copy link
Copy Markdown
Collaborator

DimitriPapadopoulos commented May 2, 2025

The flag will fix issue 1, but not issue 2. And there's an additional issue:
3. Option --password is not compatible with option --password-on-stdin, using both should raise an error.

Again, --cookie-on-stdin shares this issue. We don't have to fix everything here, I'll let you decide which issues you're willing to fix in this PR.

@mafgh
Copy link
Copy Markdown
Author

mafgh commented May 4, 2025

I have tried to fix 1. and 3.

@rempairamore
Copy link
Copy Markdown

If you want to store passwords in the macOS Keychain, check out the GUI client I built on top of openfortivpn:
rempairamore/OpenFortiMac

Hope you find it useful!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[EHN] Read secret from stdin

4 participants