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

MSR fails with crypto tutorial with ini as default storage #1981

Closed
petermax2 opened this issue May 12, 2018 · 25 comments · Fixed by #2635
Closed

MSR fails with crypto tutorial with ini as default storage #1981

petermax2 opened this issue May 12, 2018 · 25 comments · Fixed by #2635

Comments

@petermax2
Copy link
Member

Steps to Reproduce the Problem

Apply the patch:

diff --git a/doc/tutorials/crypto.md b/doc/tutorials/crypto.md
index 3881eb8dc..37f09d275 100644
--- a/doc/tutorials/crypto.md
+++ b/doc/tutorials/crypto.md
@@ -35,6 +35,14 @@ If you have no GPG private key available, you can generate one by entering the f
 
 The `fcrypt` plugin and the `crypto` plugin support both versions (version 1 and version 2) of GPG.
 
+In order to set up our tutorial we import the Elektra test key.
+We **DO NOT RECOMMEND** to use our key on your local machine, as it is available to the public!
+
+```sh
+gpg2 --import src/plugins/crypto/test_key.asc || gpg --import src/plugins/crypto/test_key.asc
+echo "trust-model always" > ~/.gnupg/gpg.conf
+```
+
 ## Introduction
 
 In this tutorial we explain the use of the `crypto` plugin and the `fcrypt` plugin by a simple example:
diff --git a/tests/shell/shell_recorder/tutorial_wrapper/CMakeLists.txt b/tests/shell/shell_recorder/tutorial_wrapper/CMakeLists.txt
index 60e8241fe..d991a9b25 100644
--- a/tests/shell/shell_recorder/tutorial_wrapper/CMakeLists.txt
+++ b/tests/shell/shell_recorder/tutorial_wrapper/CMakeLists.txt
@@ -12,5 +12,7 @@ add_msr_test (kdb-ls "${CMAKE_SOURCE_DIR}/doc/help/kdb-ls.md")
 
 add_msr_test (tutorial_validation "${CMAKE_SOURCE_DIR}/doc/tutorials/validation.md" REQUIRED_PLUGINS validation)
 
+add_msr_test (tutorial_crypto "${CMAKE_SOURCE_DIR}/doc/tutorials/crypto.md" REQUIRED_PLUGINS crypto_gcrypt fcrypt)
+
 # Only works with super user privileges, since it writes to `/etc/hosts`:
 # add_msr_test (tutorial_mount "${CMAKE_SOURCE_DIR}/doc/tutorials/mount.md")

Build Elektra with

cmake ..
        -DKDB_DB_FILE='default.ini' \
	-DKDB_DB_INIT='elektra.ini' \
	-DKDB_DEFAULT_STORAGE=ini

Run the test:

ctest --output-on-failure -V -R testshell_markdown_tutorial_crypto

Expected Result

Test should succeed.

Actual Result

Every execution step in the test fails, including the import of the GPG key.

System Information

  • Elektra Version: master
  • local, Docker Container. build server: elektra-ini-mergerequest

Further Log Files and Output

n/a

@petermax2
Copy link
Member Author

@ingwinlu pointed out:

hmm the msr test for crypto should probably set HOME to a tmpdir so no overwrite of config can happen on dev machines

@markus2330
Copy link
Contributor

Yes, but we could generally apply the trick with HOME to all shell tests, I proposed in #1982

But even if HOME is a safe place, we nevertheless should not have commands like echo "trust-model always" > ~/.gnupg/gpg.conf in tutorials. It is a bad practice and users might destroy their local config.

@petermax2
Copy link
Member Author

It is a bad practice and users might destroy their local config.

I agree. Nevertheless we need to instruct the build server to accept an untrusted GPG key somehow. Another way of doing so would be to enable "test mode" for the plugins.

In order to enable "test mode", /gpg/unit_test must be provided with a value of 1 in the backend/plugin configuration. This should be easy to acomplish on the build server, if I am not mistaking. Right?

@ingwinlu
Copy link
Contributor

ingwinlu commented May 12, 2018

except that this should not be done in the build server but in the test and reverted after the test is finished.

//EDIT: since I don't want to sound like I am just passing work around because I do not want to do it: Imagine developer A at home wants to run the test. He does not care about the test server setting. A will get pissed because the test broke his setting.

@petermax2
Copy link
Member Author

It's possibly easiert to generate a new GPG key for every test run instead of using the generic test key. Maybe we can get rid of the test key after all.

@markus2330
Copy link
Contributor

I agree. Nevertheless we need to instruct the build server to accept an untrusted GPG key somehow.

Isn't it possible to make the key "trustworthy" after import? (Without changing users's policies.)

It's possibly easiert to generate a new GPG key for every test run instead of using the generic test key.

Yes, that is maybe the best idea. Is it possible to make a very small GPG key so that it is done fast?

@petermax2
Copy link
Member Author

Is it possible to make a very small GPG key so that it is done fast?

Sure, that's possible.

@markus2330
Copy link
Contributor

Any progress here?

@petermax2
Copy link
Member Author

Not yet, I was just trying out

gpg2 --batch --passphrase '' --quick-generate-key elektra rsa512

to have some key for testing. MSR still fails but the key generation seems to work.

I can't promise to have progress during the week.

@petermax2
Copy link
Member Author

Is the shell recorder able to handle shell variables?

If I run the snippet

export KEY_ID=`gpg2 --batch --with-colons --fixed-list-mode --list-secret-keys | grep sec | awk '{split($0,a,":"); print a[5]}'`
echo "$KEY_ID"
kdb set /sw/elektra/kdb/#0/current/plugins ""
sudo kdb mount test.ini user/test fcrypt "encrypt/key=$KEY_ID" ini
kdb set user/test/password 1234
#> Create a new key user/test/password with string "1234"
kdb file user/test/password | xargs cat

I get the result:

194: export KEY_ID=`gpg2 --batch --with-colons --fixed-list-mode --list-secret-keys | grep sec | awk '{split($0,a,":"); print a[5]}'`
194: echo "$KEY_ID"
194: kdb set /sw/elektra/kdb/#0/current/plugins ""
194: kdb mount test.ini user/test fcrypt "encrypt/key=$KEY_ID" ini
194: 
194: ERROR - RET:
194: Return value “7” does not match “0”

@markus2330
Copy link
Contributor

markus2330 commented Mar 3, 2019

This is a good question! Afaik it does not but you can simply use KDB to store anything. So you would write something like (untested):

kdb set /tests/gpg/key/id `gpg2 --batch --with-colons --fixed-list-mode --list-secret-keys | grep sec | awk '{split($0,a,":"); print a[5]}'`
kdb get /tests/gpg/key/id
kdb set /sw/elektra/kdb/#0/current/plugins ""
sudo kdb mount test.ini user/test fcrypt "encrypt/key=`kdb get /tests/gpg/key/id`" ini
kdb set user/test/password 1234
#> Create a new key user/test/password with string "1234"
kdb file user/test/password | xargs cat

We should definitely add this information to tests/shell/shell_recorder/tutorial_wrapper/README.md

Btw. the kdb set /sw/elektra is quite dangerous, it might lead to data loss of the person executing the test. Tests should only write to /tests or, if the change is absolutely needed, restore what was in the KDB before.

@petermax2
Copy link
Member Author

Btw. the kdb set /sw/elektra is quite dangerous, it might lead to data loss of the person executing the test. Tests should only write to /tests or, if the change is absolutely needed, restore what was in the KDB before.

This work-around is documented in the README.md of fcrypt. fcrypt usually conflicts with sync.

@markus2330
Copy link
Contributor

Yes, I know that sometimes writing outside of /tests is needed. (also for mounting)

But we should make sure in tests to not change the config of the user running the test.

Maybe the backup-restore feature would fix the problem?

@petermax2
Copy link
Member Author

Before we fix that I have another problem: PGP keys that are created via

gpg2 --batch --passphrase '' --quick-generate-key elektra rsa512

are not usable by GPG. What the hug?

root@5fd7ea8f213b:/libelektra/build# gpg2 --batch --passphrase '' --quick-generate-key elektra rsa512
gpg: directory '/root/.gnupg' created
gpg: keybox '/root/.gnupg/pubring.kbx' created
gpg: /root/.gnupg/trustdb.gpg: trustdb created
gpg: key 3433691ADB9D50A9 marked as ultimately trusted
gpg: directory '/root/.gnupg/openpgp-revocs.d' created
gpg: revocation certificate stored as '/root/.gnupg/openpgp-revocs.d/4E3E00108709D96A28026AD63433691ADB9D50A9.rev'
root@5fd7ea8f213b:/libelektra/build# gpg2 --list-secret-keys
gpg: checking the trustdb
gpg: marginals needed: 3  completes needed: 1  trust model: pgp
gpg: depth: 0  valid:   1  signed:   0  trust: 0-, 0q, 0n, 0m, 0f, 1u
gpg: next trustdb check due at 2021-03-02
/root/.gnupg/pubring.kbx
------------------------
sec   rsa1024 2019-03-03 [SC] [expires: 2021-03-02]
      4E3E00108709D96A28026AD63433691ADB9D50A9
uid           [ultimate] elektra

root@5fd7ea8f213b:/libelektra/build# gpg2 -o test.gpg -r 4E3E00108709D96A28026AD63433691ADB9D50A9 -e 
gpg: 4E3E00108709D96A28026AD63433691ADB9D50A9: skipped: Unusable public key
gpg: [stdin]: encryption failed: Unusable public key
root@5fd7ea8f213b:/libelektra/build# gpg2 -o test.gpg -r elektra -e 
gpg: elektra: skipped: Unusable public key
gpg: [stdin]: encryption failed: Unusable public key
root@5fd7ea8f213b:/libelektra/build#

Does anyone have any idea about how to get a proper PGP key in the test scenario?

@markus2330
Copy link
Contributor

You are our gpg expert, I am afraid nobody can help you here! Maybe you ask on a gpg mailing list?

@petermax2
Copy link
Member Author

petermax2 commented Mar 27, 2019

UPDATE: I found a way to generate a "scripted" key that is usable, however a passphrase has to be set, otherwise the pinentry will come up asking for a passphrase.

cat >.elektra-test-key <<EOF
    Key-Type: RSA
    Key-Length: 512
    Subkey-Type: RSA
    Subkey-Length: 512
    Name-Real: libelektra test key
    Name-Comment: crypto plugin
    Name-Email: testkey@libelektra.org
    Expire-Date: 0
    Passphrase: 1234
    %commit
EOF
gpg2 --verbose --batch --gen-key .elektra-test-key

GnuPG is not meant to be "test-automated". 😆

EDIT: add missing "be" in the last sentence.

@markus2330
Copy link
Contributor

Thank you for digging deeper here! You mean that no passphrase should be set? I think we can live with that 👍

@petermax2
Copy link
Member Author

You mean that no passphrase should be set?

Having no passphrase on the test-key would make life easier on the server-side. I don't know if MSR can handle pinentry requests during the tests?

@markus2330
Copy link
Contributor

Having no passphrase on the test-key would make life easier on the server-side.

Yes, makes sense.

Can you fix this issue now?

@petermax2
Copy link
Member Author

Can you fix this issue now?

Not quite, I thought about providing a small setup tool, that generates a new GPG test-key using libgpgme. I have to investigate further.

@markus2330
Copy link
Contributor

Ok, thank you!

petermax2 added a commit to petermax2/libelektra that referenced this issue Apr 5, 2019
I discovered this bug during my work on ElektraInitiative#1981 .
If an empty recipient/signature key ID is defined, GnuPG reports an error because it receives invalid command line arguments.
petermax2 added a commit to petermax2/libelektra that referenced this issue Apr 5, 2019
I discovered this bug during my work on ElektraInitiative#1981 .
If an empty recipient is defined, GnuPG reports an error because it receives invalid command line arguments.
@petermax2
Copy link
Member Author

Status update: I have some progress to report. fcrypt is testable and running with MSR. crypto still causes troubles. I'm still at it.

I thought about providing a small setup tool, that generates a new GPG test-key using libgpgme.

This is the solution I'm currently following. It doesn't look too bad:

sudo kdb mount test.ini user/test crypto_gcrypt "crypto/key=$(elektra-gpg-testkey)" base64 ini

@petermax2
Copy link
Member Author

#2591 is blocking the progress atm.

@petermax2
Copy link
Member Author

I'm almost there... BUT

MSR fails because in order to be able to mount fcrypt, I have to manipulate the configuration outside of /tests:

kdb set dir/sw/elektra/kdb/#0/current/plugins ""
kdb mount test.ini user/tests fcrypt "encrypt/key=$(elektra-gpg-testkey)" ini
kdb set user/tests/password 1234
kdb file user/tests/password | xargs cat
kdb rm user/tests/password
kdb rm dir/sw/elektra/kdb/#0/current/plugins
kdb umount user/tests

If we leave out the first line, fcrypt can not be mounted due to the know issues with sync (see fcrypt's README):

STDERR: The command kdb mount terminated unsuccessfully with the info:
Too many plugins!
The plugin sync can't be positioned at position precommit anymore.
Try to reduce the number of plugins!

Failed because precommit with 7 is larger than 6

I see the following possibilities to solve the problem:

  1. We could fix the error that occurs during the mount of fcrypt and thus moving from permanent quick fix to sustainable solution.
  2. We provide a separate namespace for tests where we can also modify the general configuration from within a test case without destroying the entire system (which might be nice for other scenarios as well!)
  3. We only test crypto with MSR and leave out fcrypt

But maybe someone else has an idea. Your help is much appreciated!

@markus2330
Copy link
Contributor

Actually there is a bug besides the limitation of how many plugins can exist (we are already working on this one). The bug is that kdb mount even tries to add the sync plugin because fcrypt already provides sync, so adding the sync plugin is not necessary. I tried to fix it in #2631.

petermax2 added a commit to petermax2/libelektra that referenced this issue Apr 14, 2019
This tool can be utilized in unit tests whenever a valid GPG key without a passphrase is being required.

See ElektraInitiative#1981 for full discussion.
petermax2 added a commit to petermax2/libelektra that referenced this issue Apr 14, 2019
Let MSR test the crypto.md tutorial. See ElektraInitiative#1981 for full discussion.
petermax2 added a commit to petermax2/libelektra that referenced this issue Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants