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

FEATURE: add an env-sharing script #29

Merged
merged 16 commits into from
Nov 16, 2022

Conversation

amtoine
Copy link
Member

@amtoine amtoine commented Nov 10, 2022

Related to #13.

This PR adds a script, in the ./scripts/ directory, to easily share secret files to the members of the iscsc.fr team.

the script itself

usage

./scripts/gpg-share.sh $file $[list of gpg users]

source

legend

🔵 there is an info
🟢 there is a success message
🟠 there is a warning
🔴 there is an error and the script halts

in order

  • defines some tool functions
  • puts the first argument in $file and checks:
    • it was given as an argument 🔴
    • it is a true file on the filesystem 🔴
  • puts the rest of the arguments in an array and checks:
    • the array is not empty 🔴
    • each user exists, only once, in the keyring 🟠
    • shows the key if the user is valid in the keyring 🟢
    • throws an error if any of them is invalid 🔴
  • encrypts the file with all the keys with gpg 🔵
  • archives all the files in $ARCHIVE with 7z 🔵

@amtoine amtoine requested review from atxr and ctmbl November 10, 2022 13:43
@amtoine amtoine self-assigned this Nov 10, 2022
@ctmbl ctmbl added enhancement New feature or request Priority: Medium The Issue must be addressed as soon as possible Severity: Trivial The bug or Issue doesn't impact the user, is about esthetics or making things clean labels Nov 10, 2022
@ctmbl ctmbl added this to the iScsc blog v0.2.0 milestone Nov 10, 2022
Copy link
Contributor

@atxr atxr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Thanks! 🚀

Is it possible to add an option such as the script reads the users from a file?
That would be handy to avoid putting all the names of the team each time.

scripts/gpg-share.sh Outdated Show resolved Hide resolved
scripts/gpg-share.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@ctmbl ctmbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally have nothing to say about the code in itself but after trying it I have 4 points:

  • as @atxr said it please provide a dependency check for gpg and the archive software
  • the script currently doesn't work well for user in two word, I tried with mine "Clement Mabileau" --> it interprets it as two different users and then creates two encrypted file. But that's normal looking at the code and I don't think that's important once you know it, you just have to specify only ./gpg-share.sh file Mabileau!
  • currently the script isn't really clean because it leaves the user with multiple encrypted files at root without removing them after the archive is created (or 7zz does it? I could find out because I don't have this one 😕), consider either cleaning them or put them in /tmp I think!
  • you should add the script folder to the README repository structure I think!

For instance, as `npm` is not installed on my machine but required
to get the version of the project, i have
```bash
> ./scripts/gpg-share.sh .env.example "Alexandre Tullot"
./scripts/gpg-share.sh: warning: 'npm' is required
./scripts/gpg-share.sh: error: one or more dependencies are missing
```
This commit does 3 things:
- move the user check after the creation of the `users` array, to avoid
passing `users` to the function and breaking the array (bash...)
- read `stdin` when `USER_FILE` is not defined and use `readarray` when
it is set and the file exists
- remove the extra newline from the user name with `echo -n "$_user"`
(bash... it works in a shell but not in the script :shrugging:)
@amtoine
Copy link
Member Author

amtoine commented Nov 13, 2022

  • as @atxr said it please provide a dependency check for gpg and the archive software

the dependencies might change, e.g. 7zz, but have a look at 6788388

  • the script currently doesn't work well for user in two word, I tried with mine "Clement Mabileau" --> it interprets it as two different users and then creates two encrypted file. But that's normal looking at the code and I don't think that's important once you know it, you just have to specify only ./gpg-share.sh file Mabileau!

yep, that's how arrays work in bash... and i hate it 😭
i tried to fix this in b4eea59
when USER_FILE is set, the users are extracted from this file and not from stdin

# users.txt
Alexandre Tullot
amtoine

and

USER_FILE=users.txt ./scripts/gpg-share .env.production

should only try to use "Alexandre Tullot" and not "Alexandre" and "Tullot"
and the script should still work with stdin when USER_FILE is empty

  • currently the script isn't really clean because it leaves the user with multiple encrypted files at root without removing them after the archive is created (or 7zz does it? I could find out because I don't have this one confused), consider either cleaning them or put them in /tmp I think!

i moved the files to /tmp/ in 72582aa

  • you should add the script folder to the README repository structure I think!

added in 7c68495 😋

@amtoine amtoine requested review from atxr and ctmbl November 13, 2022 09:27
@amtoine
Copy link
Member Author

amtoine commented Nov 13, 2022

@atxr

Is it possible to add an option such as the script reads the users from a file?
That would be handy to avoid putting all the names of the team each time.

i've tried that in the previous comment and commits 😉

@ctmbl
Copy link
Contributor

ctmbl commented Nov 14, 2022

the dependencies might change, e.g. 7zz, but have a look at 6788388

i moved the files to /tmp/ in 72582aa

added in 7c68495 yum

love these three tks!

However the script still doesn't work as expected, but that seems normal as you couldn't test it 😕
I found two issues actually.
First running ./gpg-share.sh ../.env.example a2n-s gives me this;

./gpg-share.sh: info: encrypting '../.env.example' for 'a2n-s'...
gpg: using pgp trust model
gpg: using subkey 99A3107C6A2A29D8 instead of primary key BFDE075AC06746A5
gpg: 99A3107C6A2A29D8: There is no assurance this key belongs to the named user

sub  cv25519/99A3107C6A2A29D8 2022-07-24 a2n-s <stevan.antoine@gmail.com>
 Primary key fingerprint: FBC3 7C3E 1887 74E9 FBD5  2FB9 BFDE 075A C067 46A5
      Subkey fingerprint: 565B 938E 3C91 902C 51FD  71BF 99A3 107C 6A2A 29D8

It is NOT certain that the key belongs to the person named
in the user ID.  If you *really* know what you are doing,
you may answer the next question with yes.

Use this key anyway? (y/N) y
gpg: reading from '../.env.example'
gpg: can't create '/tmp/../.env.example.a2n-s.asc': Permission denied
gpg: ../.env.example: encryption failed: Permission denied

So why does it says weird things about the the key not belonging to the named user?
Second, at the end you'll understand that a simple sed isn't enough to properly create the key path 😕 or the user should be aware that the file he is trying to encrypt must be at $PWD! This problem of path of the file-to-encrypt can also lead to archiving gpg-share.sh instead of the encrypted files because of wrong path resolving...

./gpg-share.sh: info: storing files in archive
sed: -e expression #1, char 20: unknown option to `s'

7-Zip [64] 17.04 : Copyright (c) 1999-2021 Igor Pavlov : 2017-08-28
p7zip Version 17.04 (locale=en_US.UTF-8,Utf16=on,HugeFiles=on,64 bits,4 CPUs x64)

Scanning the drive:
1 file, 2566 bytes (3 KiB)

Creating archive: keys-0.1.0.7z

Items to compress: 1


Files read from disk: 1
Archive size: 1171 bytes (2 KiB)
Everything is Ok
./gpg-share.sh: info: content of the archive

7-Zip [64] 17.04 : Copyright (c) 1999-2021 Igor Pavlov : 2017-08-28
p7zip Version 17.04 (locale=en_US.UTF-8,Utf16=on,HugeFiles=on,64 bits,4 CPUs x64)

Scanning the drive for archives:
1 file, 1171 bytes (2 KiB)

Listing archive: keys-0.1.0.7z

--
Path = keys-0.1.0.7z
Type = 7z
Physical Size = 1171
Headers Size = 130
Method = LZMA2:12
Solid = -
Blocks = 1

   Date      Time    Attr         Size   Compressed  Name
------------------- ----- ------------ ------------  ------------------------
2022-11-14 00:06:15 ....A         2566         1041  gpg-share.sh
------------------- ----- ------------ ------------  ------------------------
2022-11-14 00:06:15               2566         1041  1 files

Or is it because of the weird sed: -e expression #1, char 20: unknown option to 's' ???

Also when using the USER_FILE variable the array creation seems to let a \n at the end of the user ids see:

./scripts/gpg-share.sh: success: key FBC37C3E188774E9FBD52FB9BFDE075AC06746A5 found for 'a2n-s'
./scripts/gpg-share.sh: success: key C4257605A0B2688C1CF2A04BB36E196507F03854 found for 'Clement Mabileau'
./scripts/gpg-share.sh: info: encrypting '.env.example' for 'a2n-s
'...
gpg: a2n-s
: skipped: No public key
gpg: .env.example: encryption failed: No public key
./scripts/gpg-share.sh: info: encrypting '.env.example' for 'Clement Mabileau
'...
gpg: Clement Mabileau
: skipped: No public key
gpg: .env.example: encryption failed: No public key

and

Scanning the drive:

WARNING: No such file or directory
/tmp/.env.example.a2n-s.asc

WARNING: No such file or directory
/tmp/.env.example..asc


WARNING: No such file or directory
/tmp/.env.example.Clement.asc


WARNING: No such file or directory
/tmp/.env.example.Mabileau.asc


WARNING: No such file or directory
/tmp/.env.example..asc

Every of these bugs would have been found by you if you could run it on your computer 😕 , maybe you can create a VM or a container? or test it through ssh on the iscsc server?

@amtoine
Copy link
Member Author

amtoine commented Nov 14, 2022

@ctmbl

i can test the script by applying

diff --git a/scripts/gpg-share.sh b/scripts/gpg-share.sh
index 3619dfd..300a4e4 100755
--- a/scripts/gpg-share.sh
+++ b/scripts/gpg-share.sh
@@ -26,7 +26,7 @@ log_info () {
 
 
 DEPENDENCIES=(
-  npm
+  # npm
   gpg
   7zz
 )
@@ -52,7 +52,8 @@ check_dependencies () {
 check_dependencies
 
 # get the version
-VERSION=$(npm pkg get version | sed 's/"//g')
+# VERSION=$(npm pkg get version | sed 's/"//g')
+VERSION="0.1.0"
 
 # build the archive name
 ARCHIVE="keys-${VERSION}.7z"

do not worry 😉

gpg output

this is because one of the following i think:

  • you never checked the fingerprint of your local BFDE075AC06746A5 against mine, and did not gpg --sign-key a2n-s, so gpg can not ensure the key is mine (see step 4 of this tutorial)
  • both a2n-s and BFDE075AC06746A5 are deprecated and lost keys, so you should update your keyring with my new official personal public key => 7C5EE50BA27B86B7F9D5A7BA37AAE9B486CFF1AB and then sign it of course

the path

the script should be ran from the root of the repo
i did not test this from ./scripts/ and that's too much work for me for this PR...
the script does not exhibit this behaviour from the root, what about keeping that for a separate issue? 😌

the USER_FILE variable

yup, you're right, i'll have a look at that newline (super annoying 😡)

@amtoine
Copy link
Member Author

amtoine commented Nov 14, 2022

if forgot a fix regarding the USER_FILE @ctmbl
that's my fault, it should have worked directly in b4eea59 😋

fix should be complete in 30f860c
arrays are terrible in bash, it's not the prettiest, with all these sed and tr manipulation and scope tricks, but i can not find a better solution for now...
i just do not get why there is a newline in the script when using readarray but not from a shell directly 🤷

@amtoine
Copy link
Member Author

amtoine commented Nov 14, 2022

so, for now, please test the script only from the root 😉
it worked locally on my machine, with either stdin or a USER_FILE 👍

@ctmbl
Copy link
Contributor

ctmbl commented Nov 14, 2022

@amtoine

gpg output

Thanks actually that was it, I'm not quite used to gpg right now that's true 😅

the path

the script should be ran from the root of the repo
i did not test this from ./scripts/ and that's too much work for me for this PR...

I totally understand but we then should document this behavior 😉, either as a warning at the beginning of the script (after checking $PWD or not) or in a wiki.

the USER_FILE variable

Actually it now works at the encryption step:

./scripts/gpg-share.sh: info: encrypting '.env.example' for 'amtoine'...
gpg: using pgp trust model
gpg: using subkey 5CE93D5885AAB235 instead of primary key 37AAE9B486CFF1AB
gpg: This key probably belongs to the named user
gpg: reading from '.env.example'
gpg: writing to '/tmp/.env.example.amtoine.asc'
gpg: ECDH/AES256 encrypted for: "5CE93D5885AAB235 amtoine <stevan.antoine@gmail.com>"
./scripts/gpg-share.sh: info: encrypting '.env.example' for 'Clement Mabileau'...
gpg: using pgp trust model
gpg: using subkey F0443EE8B1A18913 instead of primary key B36E196507F03854
gpg: This key belongs to us
gpg: reading from '.env.example'
gpg: writing to '/tmp/.env.example.Clement Mabileau.asc'
gpg: ECDH/AES256 encrypted for: "F0443EE8B1A18913 Clement Mabileau <mabileau.clement@gmail.com>"

But still doesn't at the archiving step:

./scripts/gpg-share.sh: info: storing files in archive

7-Zip [64] 17.04 : Copyright (c) 1999-2021 Igor Pavlov : 2017-08-28
p7zip Version 17.04 (locale=en_US.UTF-8,Utf16=on,HugeFiles=on,64 bits,4 CPUs x64)

Scanning the drive:

WARNING: No such file or directory
/tmp/.env.example.Clement.asc


WARNING: No such file or directory
/tmp/.env.example.Mabileau.asc

1 file, 687 bytes (1 KiB)

Creating archive: keys-0.1.0.7z

Items to compress: 1

I know... these two words user ids are really annoying... sorry for that...

Also, could we pass the file as a command line argument instead of an environment variable?
something like -u amtoine and -U users.txt to have a symetrical behavior while passing raw users or a file containing them
or is it too much work?

@amtoine
Copy link
Member Author

amtoine commented Nov 14, 2022

gpg output

Thanks actually that was it, I'm not quite used to gpg right now that's true sweat_smile

yep, this script assumes the keyring of the user is in good shape 🤔

the path

I totally understand but we then should document this behavior wink, either as a warning at the beginning of the script (after checking $PWD or not) or in a wiki.

i've added a check in b069dd5
should terminate when not in the root of the repo 😌

the USER_FILE variable

Actually it now works at the encryption step:

cool 💪

But still doesn't at the archiving step:
...
I know... these two words user ids are really annoying... sorry for that...

i've changed the way i store the files in the archive:grey_exclamation:
in f35fbd3, 8a6e589 and e8233a2, you can find the following:

  • create a unique directory in /tmp/ with mktemp -u and mkdir
  • be explicite about the path to the encrypted files in the output of encrypt
  • archive as follows
    • in a subshell, cd to the directory with encrypted files and archive the files by listing them with find
    • return automatically from the subshell
    • cp the archive back inside the root of the repo

that way

  1. only the archive ends up in the repo
  2. the files are still available in /tmp/
  3. there should not be issues with the names with find

Also, could we pass the file as a command line argument instead of an environment variable? something like -u amtoine and -U users.txt to have a symetrical behavior while passing raw users or a file containing them or is it too much work?

i can think of that after the above changes are good to go 😉

@amtoine
Copy link
Member Author

amtoine commented Nov 14, 2022

mm, for the multi-word users....
i do not know it's a mess 😅

what about not allowing them?
it will be a lot easier and safer, to avoid files with spaces and so on...

EDIT

actually it's an issue with the way find and tar interact 🤔
let me try to fix this, i have the same behaviour as you have 😉

proposition

the following diff

diff --git a/scripts/gpg-share.sh b/scripts/gpg-share.sh
index dbb4041..0482eed 100755
--- a/scripts/gpg-share.sh
+++ b/scripts/gpg-share.sh
@@ -137,7 +138,7 @@ encrypt () {
   do
       user=$(echo -n "$_user")
       log_info "encrypting '$file' for '$user' inside '$DUMP_DIR'..."
-      gpg --verbose --recipient "$user" --encrypt --armor --output "$DUMP_DIR/$file.$user.asc" "$file"
+      gpg --verbose --recipient "$user" --encrypt --armor --output $(echo "$DUMP_DIR/$file.$user.asc" | sed 's/\s\+/-/g') "$file"
   done
   log_success "all encrypted file have been saved inside '$DUMP_DIR'"
 }
@@ -149,7 +150,7 @@ archive () {
   # subshell here to avoid archiving the paths to the .asc files
   (
     cd "$DUMP_DIR"
-    tar cvf "$ARCHIVE" $(find . -type f)
+    tar cvf "$ARCHIVE" $(find . -type f | sed 's/\s\+/-/g')
   )
   cp "$DUMP_DIR/$ARCHIVE" .
 }

works with USER_FILE equal to

amtoine
Alexandre tullot

i simply replace the whitespaces in the filenames with "-" 😏
is that ok for you 😋

@ctmbl
Copy link
Contributor

ctmbl commented Nov 14, 2022

@amtoine

the path when running

i've added a check in b069dd5
should terminate when not in the root of the repo relieved

Exactly what I thought about! Tks a lot!

multiple works user ids/ whitespaces problem

create a unique directory in /tmp/ with mktemp -u and mkdir

I really love that! It's really prettier I think and also more reliable 😉

actually it's an issue with the way find and tar interact thinking

I was just going to ping you about that...
But I'm was also going to propose the same fix 😉, replacing whitespaces by - or _ seems like the best option 👍

However you don't need this

-    tar cvf "$ARCHIVE" $(find . -type f)
+    tar cvf "$ARCHIVE" $(find . -type f | sed 's/\s\+/-/g')

because you already saved the encrypted files without whitspaces 😉, but it sure doesn't harm 👍

Overall I agree with your proposition

flags -u and -U

let me know what you want to do!

Copy link
Contributor

@atxr atxr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You did an amazing work thank you for your contribution!
As @ctmbl said, it works fine for me with names of multiple words with this diff

- gpg --verbose --recipient "$user" --encrypt --armor --output "$DUMP_DIR/$file.$user.asc" "$file"
+ gpg --verbose --recipient "$user" --encrypt --armor --output $(echo "$DUMP_DIR/$file.$user.asc" | sed 's/\s\+/-/g') "$file"

Regarding the user option with -u and -U it would be great but that's up to you. This script solves the initial issue, so I'll approve this merge! 🚀

@amtoine
Copy link
Member Author

amtoine commented Nov 15, 2022

@atxr @ctmbl

glad it works 😌

i've added the fix in the latest a04938a 👍
without the extra sed on find as you suggested 👌

flags -u and -U

let me know what you want to do!

if that's ok with you both, i'd like to close this PR, as @atxr said it solves the original issue, and think of improvements in an issue i can open right after the merge 😋

@amtoine
Copy link
Member Author

amtoine commented Nov 15, 2022

also, please solve the threads if they have been addressed 😉
otherwise, we won't be able to merge 👀

@atxr
Copy link
Contributor

atxr commented Nov 15, 2022

i've added the fix in the latest a04938a +1 without the extra sed on find as you suggested ok_hand

Perfect 👌

flags -u and -U

if that's ok with you both, i'd like to close this PR, as @atxr said it solves the original issue, and think of improvements in an issue i can open right after the merge 😋

Totally fine for me 😉
I resolved the remaining threads, we just need @ctmbl approval now.

Copy link
Contributor

@ctmbl ctmbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works perfectly tks a lot @amtoine for your amazing work!!
As for the improvements of course it can be address in other PRs

You'll just have to resolve some conflicts (I guess in the README) because I merged #27 before this one (ahah I'm evil 👿)

@amtoine
Copy link
Member Author

amtoine commented Nov 15, 2022

@ctmbl, there it is 👌

Copy link
Contributor

@ctmbl ctmbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Ready to merge this one 🚀

@amtoine amtoine merged commit cc1ee22 into iScsc:main Nov 16, 2022
@amtoine amtoine deleted the feature/env-sharing-script branch November 16, 2022 08:11
@ctmbl ctmbl linked an issue Jun 2, 2023 that may be closed by this pull request
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Priority: Medium The Issue must be addressed as soon as possible Severity: Trivial The bug or Issue doesn't impact the user, is about esthetics or making things clean
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Automation and enhancement of the dev workflow
3 participants