[OPENDJ-5902] Secrets: support retrieval of CLI arguments from environment variables and files Created: 08/Jan/19  Updated: 30/Nov/20  Resolved: 05/Nov/20

Status: Done
Project: OpenDJ
Component/s: ease of use, security, tools
Affects Version/s: 7.0.0
Fix Version/s: 7.0.1, 7.1.0

Type: Improvement Priority: Critical
Reporter: Matthew Swift Assignee: Ondrej Fuchsik
Resolution: Fixed Votes: 0
Labels: 7.0-at-risk, backport-candidate

Issue Links:
Backport
Depends
is required by OPENDJ-7485 Document new :env and :file option mo... Done
Regression
caused OPENDJ-7554 Windows: Secrets not retrieved from :... Done
Relates
Epic Link: Miscellaneous 2020.Winter
Story Points: 8
Dev Assignee: Cedric Tran-Xuan
QA Assignee: Ondrej Fuchsik
Backports: OPENDJ-7607 (7.0.1)

 Description   

DJ CLI tools often have arguments which represent secrets or credentials of some sort, such as passwords or key store PINs. It is tempting to pass the credential to the tool directly:

$ mytool --somePassword "myPassword"

However, the clear text password is exposed directly on the terminal and may be retrieved later on via the shell history or my querying the running processes by using a tool such as "ps -f".

The user may attempt to avoid this by passing the credential via an environment variable or a file:

$ mytool --somePassword $PASSWORD
$ mytool --somePassword `cat password.txt`

However, it is still possible to discover the password using a tool like "ps -f". As a workaround our tools often provide an additional argument for passing in the credential via a file. However, this leads to a proliferation of command line arguments and still doesn't make it easy to use environment variables.

It would be better if we took a similar approach to Java's keytool command:

$ man keytool
...
       --storepass[:env| :file] argument
              The password that is used to protect the integrity of the keystore.

              If the modifier env or file is not specified, then the password has the value argument, which must be at least 6
              characters long. Otherwise, the password is retrieved as follows:

              · env: Retrieve the password from the environment variable named argument.

              · file: Retrieve the password from the file named argument.

This should be relatively easy to support, especially for the long argument form, by simply updating the ArgumentParser code to detect arguments of the form "xxx:env" or "xxx:file" in com.forgerock.opendj.cli.ArgumentParser#parseArgumentWithLongId(). We may be able to also support this format for single character arguments, but only for non-boolean arguments where the argument value is separated from the argument name with a space.

Once this issue is complete it should be possible to deprecate all file based arguments as well as the com.forgerock.opendj.cli.FileBasedArgument itself.



 Comments   
Comment by Matthew Swift [ 25/Oct/19 ]

Raising the priority since it significantly impacts our CLIs, so we should address it for 7.0 rather than in a minor release.

Comment by Jean-Noël Rouvignac [ 04/Sep/20 ]

Let's take the example of a current CLI options with the famous –bindPassword.

Current CLI

Current CLI has the following options:

  • --bindPassword
  • --bindPasswordFile

Future CLI

Future CLI would have the following options:

Long argument Comment
--bindPassword No change
--bindPasswordFile Hidden argument: existing scripts continue to work, but it is no longer displayed by the online help (–help) to encourage migration
--bindPassword:file New: replacement for --bindPasswordFile
--bindPassword:env New
Comment by Jean-Noël Rouvignac [ 04/Sep/20 ]

Implementation side, I think the changes will appear in two places. Don't take these too much to the letter, they are more guidelines to show how this change may be implemented. It can be changed based on feedback from actual implementing it.

SecretResolver.java

This class can be made responsible for handling all three arguments --bindPassword, --bindPassword:file and --bindPassword:env. How much exactly remains to be determined, including whose responsibility it is for creating the arguments.
I would suggest we add two fields to this class: FileBasedArgument fileArgumentHidden and StringArgument envArgument.
This class is also responsible for actually resolving the secret.

SubCommandArgumentParser.java and ArgumentParser.java

These classes are responsible for generating the online help.
The crux of the change here will be to generate a single "entry" for the three arguments -bindPassword, bindPassword:file and -bindPassword:env instead of three different "entries".
An idea may be to look if the long arguments end with :env or :file and "aggregate" such arguments into a single "entry".

Note: I am not very far from having unified the SubCommandArgumentParser and ArgumentParser classes. My next would be to effectively drop ArgumentParser in favour of SubCommandArgumentParser. So I would suggest you make this work on SubCommandArgumentParser first, it is the most complex of the two anyway. Once it works for SubCommandArgumentParser copy / pasting (or sharing) the code into into / with ArgumentParser should be easy.

On deprecating FileBasedArgument

Let's first make the change suggested above.

Then, if the only uses for FileBasedArgument end up to be from the SecretResolver class, then I think we can remove it indeed.
In that case, we move the code doing the resolution to SecretResolver, and then we can convert all the FileBasedArgument into StringArgument.
Regardless, this is a code cleanup that should come in a second step (separate pull request).

Comment by Cedric Tran-Xuan [ 08/Sep/20 ]

Matthew Swift Ludovic Poitou For the help, what should we have:

 

 ./bin/dskeymgr create-deployment-key  --help
[...]
SubCommand Options:
-w, --deploymentKeyPassword {deploymentKeyPassword}
    The deployment key password
--deploymentKeyPassword:env {environmentVariable}
    The environment variable containing the deployment key password
--deploymentKeyPassword:file {deploymentKeyPasswordFile}
    Path to a file containing the deployment key password

or

 ./bin/dskeymgr create-deployment-key  --help
[...]
SubCommand Options:
-w, --deploymentKeyPassword[:env|:file] {deploymentKeyPassword}
    The deployment key password 
     [:env]: retrieve the password from the environment variable named argument
     [:file]: retrieve the password from the file named argument.

The first one is easier to implement

Comment by Ludovic Poitou [ 08/Sep/20 ]

My preference would go to the second option. It's simpler and shorter (and easier to sort, since I guess that the order will not be as you've outlined since -w will be listed after --deploymentKeyPassword.

Comment by Matthew Swift [ 08/Sep/20 ]

I agree with Ludo I'm afraid

Comment by Matthew Swift [ 08/Sep/20 ]

To be honest, you probably don't need all the extra detailed description. Something like this would be just fine and I'm guessing easier to implement:

 ./bin/dskeymgr create-deployment-key  --help
[...]
SubCommand Options:
-w, --deploymentKeyPassword[:env|:file] {deploymentKeyPassword}
    The deployment key password which may be obtained from an environment variable or file
Comment by Matthew Swift [ 21/Sep/20 ]

Chris Ridd - required for OPENDJ-6067. Flagging as a backport candidate because I think the changes are pretty low risk and easy to test. I'll let you decide whether the fixes for OPENDJ-6067 need backporting though.

Comment by Jean-Noël Rouvignac [ 22/Sep/20 ]

Note concerning the display of equivalent command line arguments with dsconfig, and the use of file based arguments (for example --bindPasswordFile):

Before this change, we had:

$ echo password > PASS.txt
$ dsconfig --hostname localhost -p 4444 -D "uid=admin" --bindPasswordFile PASS.txt -X     --displayCommand

...

The equivalent non-interactive command-line is:
dsconfig set-global-configuration-prop \
          --set time-limit:60\ s \
          --hostname localhost \
          --port 4444 \
          --bindDn uid=admin \
          --trustAll \
          --no-prompt

Notice that no --bindPasswordFile argument is displayed at all.

After this change, we have:

$ dsconfig --hostname localhost -p 4444 -D "uid=admin" --bindPassword:file PASS.txt -X     --displayCommand

...

The equivalent non-interactive command-line is:
dsconfig set-global-configuration-prop \
          --set time-limit:61\ s \
          --hostname localhost \
          --port 4444 \
          --bindDn uid=admin \
          --trustAll \
          --bindPassword ****** \
          --no-prompt

It's showing the obfuscated -bindPassword instead of -bindPassword:file, but this is already much better than before which was showing nothing at all 

Comment by Cedric Tran-Xuan [ 22/Sep/20 ]

Ah, good catch. May be, in a RFE?

Comment by Jean-Noël Rouvignac [ 22/Sep/20 ]

As the previous behaviour shows, I don't think it paused a problem to many people.

As a consequence I would leave it as is for now. It is already much better than what it used to be.

Comment by Cedric Tran-Xuan [ 22/Sep/20 ]

Mark Craig Doc may need to be updated?

Below to give an idea, here are some examples of use:

mkdir -p /opt/opendj/tmpo
echo "password" > /opt/opendj/tmpo/my-password.secret
export MY_PASSWORD="password"

./setup --instancePath /opt/opendj/ --serverId Anabel_Scharf --deploymentKey ADpxtE6Ini0DnIA6ZviYl_15FAbfe9Q5CBVN1bkVDJE_0OYuCh2U6Fkg --deploymentKeyPassword:file /opt/opendj/tmpo/my-password.secret --rootUserDn uid=admin --rootUserPassword:env MY_PASSWORD --hostname opendj.example.com --adminConnectorPort 4444 --ldapPort 1389 --enableStartTls --ldapsPort 1636 --httpsPort 8443 --replicationPort 8989 --profile ds-evaluation --set ds-evaluation/generatedUsers:100000

./bin/start-ds

./bin/ldapsearch --trustAll -h ctranxuan-laptop --useSsl  -p 1636 --bindDn uid=admin  --bindPassword:file  /opt/opendj/tmpo/my-password.secret -b ou=people,dc=example,dc=com "(uid=wlutz)"
./bin/ldapsearch --trustAll -h ctranxuan-laptop --useSsl  -p 1636 --bindDn uid=admin  --bindPassword:env  MY_PASSWORD -b ou=people,dc=example,dc=com "(uid=wlutz)"

./bin/stop-ds

export MY_SECRET="passwordFromEnvVar"
echo "== test :env MY_SECRET = $MY_SECRET=="
./bin/dskeymgr create-deployment-key --deploymentKeyPassword:env MY_SECRET

echo "== test :file =="
./bin/dskeymgr create-deployment-key --deploymentKeyPassword:file /opt/opendj/tmpo/my-password.secret

echo "== test backward-compatibility =="
./bin/dskeymgr create-deployment-key -j  /opt/opendj/tmpo/my-password.secret
Comment by Mark Craig [ 22/Sep/20 ]

Cedric Tran-Xuan, I expect the doc task to take some time (mainly to fix all the examples), so logged OPENDJ-7485 for that.

Comment by Ondrej Fuchsik [ 14/Oct/20 ]

Added new tests to check this and found a bug on windows.

Comment by Cedric Tran-Xuan [ 14/Oct/20 ]

What kind of bug?

Comment by Jean-Noël Rouvignac [ 14/Oct/20 ]

Cedric Tran-Xuan Look at the linked issue with a "caused" label.

Comment by Ondrej Fuchsik [ 05/Nov/20 ]

Added new tests for this improvement and related bug is verified. Tests works with 7.1.0-SNASPHOT rev. 1777c7755d4.

Generated at Fri Mar 05 06:52:40 UTC 2021 using Jira 7.13.12#713012-sha1:6e07c38070d5191bbf7353952ed38f111754533a.