Add a script that can sign a file based on an ssh key #948
Conversation
|
Instance
|
|
Instance
|
|
Instance
|
|
We can do clever things with the allowed signers file, https://man.openbsd.org/ssh-keygen.1#ALLOWED_SIGNERS |
| - name: test sign_verify_file_ssh.sh script | ||
| run: | | ||
| # Create a PEM format ssh identity | ||
| ssh-keygen -t rsa -b 4096 -m PEM -f id_rsa.pem -N "" | ||
| # Find a file to sign | ||
| export TARBALL="$(ls *.tar.gz | tail -1)" | ||
| # Sign the file | ||
| ./sign_verify_file_ssh.sh sign "$TARBALL" id_rsa.pem | ||
| # Create an allowed_signers file based on the public key | ||
| echo -n "allowed_signer " > allowed_signers | ||
| cat id_rsa.pem.pub >> allowed_signers | ||
| # Verify the signature | ||
| ./sign_verify_file_ssh.sh verify "$TARBALL" allowed_signers | ||
There was a problem hiding this comment.
A bit unsure what this actually tests? To me it looks like if the script exists in the current directory, is executable and exits with 0 the test will succeed?
There was a problem hiding this comment.
This tests the workflow end-to-end, it creates a key (PEM format, same as a GitHub App key), signs a file with it, and then verifies the signature.
Basically it is testing core functionality, if there is a problem the code will error out and the CI will fail.
There was a problem hiding this comment.
The test succeeds if the last run of sign_verify_file_ssh.sh exits with 0, no? All the rest that is done in the test doesn't really matter. Wouldn't it be better to use predefined input (data file, pem key) & output (pub key, signature) instead of generating them on the fly? Thus you would have input and output under control and could verify if the created pub key and signature match what you expect.
There was a problem hiding this comment.
Not quite, the test will also fail if any of the previous commands fail. The first command tests the signing, the second command tests the verification. The specific data file and PEM key are not particularly relevant (we're not processing the data itself, only the signature file that gets created)
sign_verify_file_ssh.sh
Outdated
|
|
||
| # Usage message | ||
| usage() { | ||
| echo "Usage: $0 sign <file> <private_key> | verify <file> <public_key> [signature_file]" |
There was a problem hiding this comment.
Do we need verification here?
There was a problem hiding this comment.
Usage info is very short. Maybe some information about the supported format of they private key would be good.
sign_verify_file_ssh.sh
Outdated
| # Ensure the file exists | ||
| if [ ! -f "$FILE_TO_SIGN" ]; then | ||
| echo "Error: File '$FILE_TO_SIGN' not found." | ||
| exit 1 |
There was a problem hiding this comment.
Maybe named constants for exit values (defined/declared at the beginning) would be nice. One could then use different exit codes for different cases and use these in other scripts to report back what went wrong.
There was a problem hiding this comment.
I made a stab at this but they are a bit useless unless they are somehow consistent across scripts
sign_verify_file_ssh.sh
Outdated
|
|
||
| if grep -q "BEGIN RSA PRIVATE KEY" "$key_file" || grep -q "BEGIN OPENSSH PRIVATE KEY" "$key_file"; then | ||
| # Ensure cleanup on exit | ||
| trap 'rm -f "$output_file" "$output_file.pub"' EXIT |
There was a problem hiding this comment.
That (trap ...) looks a bit fancy to me. First time we use it in any script in EESSI? Maybe not bad ... but then other scripts should be modernised too.
There was a problem hiding this comment.
I added it because I don't want to leave these files lying around even if the script is interrupted
There was a problem hiding this comment.
Not in scope to look at modernising scripts in this PR, but it could be looked at in general.
There's a couple of style comments in the review, and I don't think we ever discussed styling for our scripts. It may be time we should? The obvious thing to do is to just use shellcheck, otherwise we will have to come up with "rules".
sign_verify_file_ssh.sh
Outdated
| cp "$key_file" "$output_file" && ssh-keygen -c -C "Converted from PEM" -f "$output_file" || { | ||
| echo "Error: Failed to convert PEM key to OpenSSH." | ||
| exit 1 | ||
| } |
There was a problem hiding this comment.
Again, a bit unusual style looking at other scripts in EESSI. Personally, I think using if ... else ... fi makes the code more easy to read.
There was a problem hiding this comment.
What are the options for ssh-keygen? A small comment may help others to understand the code without checking a man page.
There was a problem hiding this comment.
So the general advice I see around is
- Use
ifblocks for clarity and when you expect to add more conditions later. - Use the
&&shorthand for brief checks where compactness is preferred.
I think it's marginal here, and if additional commenting is also needed then better to go with if
sign_verify_file_ssh.sh
Outdated
| if [ ! -f "$PRIVATE_KEY_PEM" ]; then | ||
| echo "Error: Private key '$PRIVATE_KEY_PEM' not found." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Here the traditional style for conditional execution is used. Why not
[ ! -f "${PRIVATE_KEY_PEM} ] && {
echo "..."
exit 1
}
sign_verify_file_ssh.sh
Outdated
| while read -r identity pub_key; do | ||
| # Check if identity and public key are not empty | ||
| if [[ -n "$identity" && -n "$pub_key" ]]; then | ||
| echo "Verifying signature for identity: $identity" | ||
|
|
||
| # Use ssh-keygen to verify the signature (example with RSA) | ||
| if ssh-keygen -Y verify -f "$ALLOWED_SIGNERS_FILE" -n file -I "$identity" -s "$SIG_FILE" < "$FILE_TO_SIGN"; then | ||
| echo "Signature is valid for identity: $identity" | ||
| exit 0 # Exit once we find a valid signature | ||
| else | ||
| echo "Invalid signature for identity: $identity" | ||
| fi | ||
| fi | ||
| done < "$ALLOWED_SIGNERS_FILE" |
There was a problem hiding this comment.
Is this the equivalent to importing GPG public keys?
There was a problem hiding this comment.
I haven't used GPG keys in 20 years so I'm not sure 😉
This cycles through a given authorised signers file, checking to see if the tarball has been signed by any of the identities. Looking it up now, it seems you can give a wildcard '*' to the -I option, so the loop here may not be necessary.
There was a problem hiding this comment.
Wildcards don't work so the loop remains
trz42
left a comment
trz42
left a comment
There was a problem hiding this comment.
Overall a solid PR. Thanks @ocaisa !
A few thoughts:
- where should we put "this"? maybe under
bot/sign...if we put it into thesoftware-layer repo - is this unique to the software-layer repo? how about putting this into the
scripts/...directory of theeessi-bot-software-layerrepository? - should it really have both functions to sign file and to verify the signature of a file? the only reason for not separating them would be that through tests we make sure that both work together, i.e., if we change the signing, it shouldn't break the verification
- the "bot" only really needs the sign part (unless the signature check uses the verification part)
- the Stratum-0 needs the verification part
- the bot could use the script (ideally via
bot/sign...), but the Stratum-0 would need to get it elsewhere ... another option could be to put this (and maybe other) scripts into a shared repository (EESSI/common-tools)
- good idea to add a test (although I think it doesn't really test much functionality right now)
- a little bit too experimental/exotic with use of "fancy" bash capabilities (
trap, not really fancy but unusual for EESSI so far; plus it seems what the trap should execute is then repeatedly done explicitly) and unusual syntax for conditional execution (also not consistently used throughout the script)
|
About where to put this, the creation of tarballs is done in this repo so I thought it made sense here. Now I think about it though, the bot is the one uploading the tarballs to S3, and the one who is aware of the GitHub App PEM key, so it may indeed be better off there. |
trz42
left a comment
trz42
left a comment
There was a problem hiding this comment.
Looks good. I'd like to test this in action with the bot. This will require a bit coding on my side. It should not really delay the use of this PR because we have to add the signing capability on the bot side anyhow.
trz42
left a comment
trz42
left a comment
There was a problem hiding this comment.
Added one more change that was necessary when I tested this. No other changes were needed.
|
This is now moved over the Bot repository, see EESSI/eessi-bot-software-layer#304 |
|
PR merged! Moved |
1 similar comment
|
PR merged! Moved |
2 participants
And can also verify a signature based on an allowed signers file.
The allowed signers file has the format