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

Crypto++ and Commit Signing #290

Closed
anonimal opened this issue Sep 20, 2016 · 20 comments
Closed

Crypto++ and Commit Signing #290

anonimal opened this issue Sep 20, 2016 · 20 comments

Comments

@anonimal
Copy link
Contributor

In addition to #272, I think the project would benefit from having regular commit signing. I'd be happy to argue the benefits but I think that they're self-evident (especially within this community). Maybe also include a small contributing guide so others can start signing too (or is that asking for too much)?

@DevJPM
Copy link
Contributor

DevJPM commented Sep 21, 2016

While I can agree that commit signing sounds like a good idea, there are questions left to be asked.

  • Who should sign their commits? (only Jeff? only the regulars? everybody?)
  • Should we enforce commit signing? (is this even possible with GitHub?)
  • How do you even sign commits? (that's important for a lot of people who never used this feature before, including me)

@mouse07410
Copy link
Collaborator

mouse07410 commented Sep 21, 2016

  1. Whoever makes the commit, signs it. Obviously, including Jeff.
  2. I don't know if GitHub supports enforcing signatures. But one can accept only commits whose signature is verified: https://github.com/blog/2144-gpg-signature-verification.
  3. You sign commits with PGP (or rather GPG). How - is described here. The description seems very clear, concise and precise. (Don't forget to add your GPG key to your GitHub account: https://help.github.com/articles/adding-a-new-gpg-key-to-your-github-account/)

The following is a good read that I recommend all, especially the maintainers, to familiarize themselves with: https://mikegerwitz.com/papers/git-horror-story

@noloader
Copy link
Collaborator

noloader commented Sep 21, 2016

This seems kind of relevant right around now: Commit by Root (Commit d21248b995ec2ae0). I probably should have been signing commits from the beginning. I think I've avoided it out of laziness and/or [lack of] ease of use to add the identity assurances.

I'm guessing making it a requirement for everyone is too firm. Code signing should not be a barrier to entry to frame it in economic theory. The code is either good or its not; and its pedigree does not matter.

I try to remove most barriers to entry. I'll take partial patches, ill-formatted submissions, etc. I don't nit-pick the commits. Whether you prefer spaces or tabs is irrelevant to me and the compiler. I'll format it later if it bothers me.

@mouse07410
Copy link
Collaborator

I'm guessing making it a requirement is too aggressive.

I'm not sure I agree here. Especially considering the low "cost of entry".

The code is either good or its not; and its pedigree does not matter.

I rather disagree with the above, and the pedigree may be related to whether the code is good or not.

In any case, it's good when you know who to blame. :-)

@noloader
Copy link
Collaborator

In any case, it's good when you know who to blame. :-)

Lol... That one is easy; its usually me :)

@mouse07410
Copy link
Collaborator

In any case, it's good when you know who to blame. :-)

Lol... That one is easy; its usually me :)

Ah, but we want to know for sure! :-)

@MarcelRaad
Copy link
Contributor

I'm guessing making it a requirement is too aggressive.

I'm not sure I agree here. Especially considering the low "cost of entry".

I just set this up a few days ago under Windows using TortoiseGit and it was not trivial, so I guess the cost is not that low for an occasional git user. Signing single commits is not even possible with TortoiseGit, you can only sign tags or sign all commits via editing the git config text file by hand (something I've never had to do before). Then the version of GPG that comes with git for Windows is so old that you cannot use keys protected by a passphrase and the resulting error message (something about tty) doesn't make that clear.

@mouse07410
Copy link
Collaborator

just set this up a few days ago under Windows using TortoiseGit and it was not trivial

I hear you. Many development-related (such as full utilization of Git) things are not trivial on Windows.

I guess the cost is not that low for an occasional git user

I don't expect occasional git users to submit commits to the main repo.

The way I envision this to work is:

  • The main point of Git is to facilitate collaboration between the maintainers, secondary benefit is the ability of everybody else to look and use the code stored there
  • Only the maintainers can alter stuff in the main repo
  • When a maintainer makes a change, he signs that change
  • When a user wants to download anything from the main repo, he can see what change was made by whom (unauthorized changes, if any, would stand up like a sore thumb)

@anonimal
Copy link
Contributor Author

Should we enforce commit signing? (is this even possible with GitHub?)

I don't think we should enforce but rather ask. With Kovri, all commits are signed when possible. Some people forget to sign, some people don't know how to sign (and don't RTFM), some people don't care to sign, and some people want to sign but their OS or setup restricts them from doing so. This ends up being OK because we sign merges; so the merger takes full responsibility of any PR's that have unsigned commits.

I agree with @noloader: code signing should not be a barrier to entry though I do think more responsibility would be beneficial.

Maybe also include a small contributing guide so others can start signing too (or is that asking for too much)?

I still think there should be a contributing guide in place so, at the very least, the option to sign (and links on how to sign) is encouraged.

@mouse07410
Copy link
Collaborator

I don't think we should enforce but rather ask. With Kovri, all commits are signed when possible. Some people forget to sign, some people don't know how to sign (and don't RTFM), some people don't care to sign, and some people want to sign but their OS or setup restricts them from doing so.

Look, right now no commit is signed at all, so anything would be an improvement over status quo. But this project has very few committers, and practically one (maybe two) who actually commit at a regular basis. So who are those "some people" who contribute but cannot sign (for whatever reason)? Frankly, I see none.

And I still think that for the maintainers (those who are supposed to be able to modify the main repo) it is between highly desirable to mandatory to figure out once how to sign commits, do the setup once, and be done with it. *If you can figure out how to fix a bug in Crypto++ or add a working enhancement to it - you better be able to figure how to get GPG working. :-) *

...ends up being OK because we sign merges; so the merger takes full responsibility of any PR's that have unsigned commits.

Perhaps there's terminology difference. I agree with you, and in fact we seem to be saying the same thing: those who can directly modify the main repo ("committers" in my lingo, "mergers" in yours) should take responsibility for what they merge, and sign it. Those who fork and provide PRs are welcome to sign, but do not have to (at this point).

I strongly recommend keeping keys on hardware tokens, such as YubiKey NEO ($50 retail per key, preferable if NFC access desired - like using for PGP/MIME with Android devices (as I'm doing often enough :) )) or YubiKey 4 ($40 retail per key, preferable if stronger crypto is desired), as I've had plenty of good experience with both. There are other OpenPGP-compatible tokens, which I'm sure are great - but I haven't dealt with them, so can't comment.

Another advantage of hardware tokens is - you can increase security of your crypto keys (for me - both PIV and OpenPGP) across the entire board of your applications and use cases (S/MIME, Web login, smart card login to computer, etc. etc.).

@DevJPM
Copy link
Contributor

DevJPM commented Sep 25, 2016

On a side note a few things about yubikeys:

If you only need OpenPGP functionality (and no fancy ECC support) and already have a smartcard reader, than the OpenPGP Smartcard is the cheapest option (with <17€ / card + 5-10€ shipping). Especially if you want to consider using something GnuPG to encrypt your offsite backups (for example using duplicati )

As for key backup: It appears that these cards are import-only, so generate on a trusted laptop and import to multiple cards (or yubikeys).


As for my personal strategy (as I'm one of the "more regular" PR-creators), I plan on setting up a software-backed key until I have the money to spare to order multiple OpenPGP-Smartcards.

@mouse07410
Copy link
Collaborator

Yubikeys offer U2F support for our chrome and edge users here

And Firefox with U2F plugin too. ;-)
I've tested it successfully.

Yubikeys are supported by OpenSC, allowing access to the (three) keys via PKCS#11 and Microsoft's CAPI

Make it four PIV keys supported by the PIV applet, accessible via PKCS#11 (and, I assume, via MS CAPI - based on my limited user experience with Windows devices). Which are independent from GPG subkeys that are supported by the OpenPGP applet, making the same token usable as PIV, OpenPGP, or both.

I'll leave alone the question of what's the most cost-effective solution. For myself I considered tokens in smart card form (and was issued one by my employer :) ), but in the end decided that a USB-style approach offers better functionality, the price difference not high enough to play a role in the token quantities I'm dealing with.

As for key backup: It appears that these cards are import-only, so generate on a trusted laptop and import to multiple cards (or yubikeys).

You mean - you cannot generate a key on an OpenPGP smart card? It wouldn't worry me for encryption keys (we're escrowing them anyway, for multiple reasons - token damage or loss being one of them), but I really don't like it for signing keys (which, coincidentally, we do not escrow).

On Yubikey you certainly can generate keys, though some people do prefer to generate in software (for backup purposes) and then import. That adds a degree in convenience, and removes a degree in security. :-)

According to https://gnupg.org/howtos/card-howto/en/smartcard-howto-single.html#id2506175, GPG can generate the key on the card (I haven't done that myself), or add subkeys (by generating them on the card) to a given key (that I have done).

I plan on setting up a software-backed key until I have the money to spare to order multiple OpenPGP-Smartcards

If these cards are import-only, I don't think you have an option - except for "generate, import to several cards, delete from disk". For more expensive tokens - you can generate on the card, and if the card is lost of damaged - forget the old key, and arrange with the other maintainers to replace your old key by your newly-generated key that would replace the lost one. I don't see why we should make a big deal of signing keys - just revoke the old ones and replace...

@noloader
Copy link
Collaborator

noloader commented Sep 26, 2016

@everyone,

I'll [try] to start signing commits when I have time to look up the procedures. I'm kind of buried at the moment, so it may take a few days to get to it.

If anyone wants to provide a reference, then I have PGP keys and I use the command line. Here's the current process I use:

  1. Determine files that changed
  2. Perform a git commit <file 1> <file 2> ... -m "A message"
  3. Perform a git push
  4. When needed, merge master into all dev-branches

I do (4) because Git does an awful job at merging. It does a worse job of allowing me to continue after a failed merge (it often results in me deleting the directory and performing a fresh clone). So I try to keep all dev-branches up to date with master.

Its been my experience Git makes every simple task difficult. If this turns out to be a 5 or a 10 step process, then I'll likely shy away from signing commits.

@mouse07410
Copy link
Collaborator

mouse07410 commented Sep 26, 2016

If anyone wants to provide a reference, then I have PGP keys and I use the command line

I found this reference to be very good: https://help.github.com/articles/signing-commits-using-gpg/

According to it, you need two things:

  1. Make GitHub aware of your GPG signing key (here) shown how);
  2. Add "-S" to your commit command, aka git commit -S -m "signed commit for ..." <file 1> <file 2> ...

The rest should be handled for you automatically. I've just tested the above, and it worked with a key on a hardware token out of box.

Note: make sure email in your global or local Git config matches the one on the key.

@noloader
Copy link
Collaborator

noloader commented Sep 26, 2016

Thanks Uri. I have one question. Below is my script to merge Master into all dev-branches. How can I automate the -S so that I only get prompted once for the password on the private key?

#!/usr/bin/env bash

if [[ (-z $(git rev-parse HEAD 2>/dev/null)) ]]; then
    echo "$PWD is not a Git repository"
    [[ "$0" = "$BASH_SOURCE" ]] && exit 1 || return 1
fi

current=$(git rev-parse --abbrev-ref HEAD 2>/dev/null)
git fetch --all &>/dev/null &>/dev/null
if [[ "$?" -ne "0" ]]; then
    echo "git fetch --all failed"
    [[ "$0" = "$BASH_SOURCE" ]] && exit 1 || return 1
fi

for branch in $(git branch -a | cut -b 2- | grep -v "remotes/origin" | awk '{print $1}');
do
    # Skip anything that looks like Master
    if [[ (! -z "$branch") && (("$branch" = "master") || ("$branch" = "HEAD")) ]]; then
        continue;
    fi

    # Skip anything that looks like a release, like CRYPTOPP_5_6_3
    if [[ (! -z $(echo -n "$branch" | grep "CRYPTOPP_")) ]]; then
        continue;
    fi

    echo "**************** $branch *******************"

    git checkout "$branch" &>/dev/null
    if [[ "$?" -ne "0" ]]; then
        echo "git checkout $branch failed"
        continue;          
    fi

    git rebase "origin/$branch" &>/dev/null
    if [[ "$?" -ne "0" ]]; then
        echo "git rebase $branch failed"
        continue;          
    fi

    git merge master -m "Merge branch 'master' into dev-branch '$branch'" &>/dev/null
    if [[ "$?" -ne "0" ]]; then
        echo "git merge $branch failed"
        continue;          
    fi

    git push
    if [[ "$?" -ne "0" ]]; then
        echo "git push $branch failed"
        continue;          
    fi

done

if [[ ! -z  "$current" ]]; then
    git checkout "$current" &>/dev/null
fi

[[ "$0" = "$BASH_SOURCE" ]] && exit 0 || return 0

@noloader
Copy link
Collaborator

noloader commented Sep 27, 2016

After the protracted debate, the signed commits do not even show up in the logs:

$ git log
commit 88b2e70582754b02f65b57a421f9f37c21e986a5
Author: Jeffrey Walton <noloader@gmail.com>
Date:   Mon Sep 26 19:59:20 2016 -0400

    Add validat4.cpp to project files

commit af3cd90450799c06485137e711c7141c70ced940
Merge: fd2a304 62ca476
Author: Jeffrey Walton <noloader@gmail.com>
Date:   Mon Sep 26 11:54:18 2016 -0400

    Merge branch 'master' into dev-branch 'ecies'

This tool is so defective by design...

@noloader
Copy link
Collaborator

noloader commented Sep 27, 2016

@mouse07410, @anonimal,

The script above is now failing with a useless error message: _not something we can merge git merge integer failed_. The only change was to change the options from git merge master -m "Merge branch... to git merge master -Sm "Merge branch.... Stack Overflow wisdom says its the wrong branch name, which I'm fairly certain is not the case.

$ ./master-merge.sh 
**************** detsig *******************
merge: Merge branch 'master' into dev-branch 'detsig' - not something we can merge
git merge detsig failed
**************** ecies *******************
merge: Merge branch 'master' into dev-branch 'ecies' - not something we can merge
git merge ecies failed
**************** integer *******************
merge: Merge branch 'master' into dev-branch 'integer' - not something we can merge
git merge integer failed
**************** skein *******************
merge: Merge branch 'master' into dev-branch 'skein' - not something we can merge
git merge skein failed

Any ideas how to fix it?

@mouse07410
Copy link
Collaborator

mouse07410 commented Sep 27, 2016

Hmm... First, I don't think you can use -Sm flag, trying to combine -S and -m. I think they have to be at least space-separated, like -S -m.

How can I automate the -S so that I only get prompted once for the password on the private key?

I believe (though I don't have much experience with it yet) that if you use GPG Agent (gpg-agent executable), it can be configured to cache the PIN (or a key password?) for the amount of time you choose (I believe it's the default-ttl xxx parameter of ~/.gnupg/gpg-agent.conf that gives value in seconds, default should be default-ttl 600 meaning 10 minutes). That would be enough for the script to complete.

@mouse07410
Copy link
Collaborator

mouse07410 commented Sep 27, 2016

It appears that to see the signatures you have to use git log --show-signature:

$ git log --show-signature
commit 514d6a79e74eb892dab4b8db53d99d4cef27a769
Merge: 7fe8f0f bbefa10
Author: Mouse <uri@mit.edu>
Date:   Mon Sep 26 21:54:11 2016 -0400

    Merge branch 'master' of https://github.com/mouse07410/cryptopp

commit 7fe8f0f195662e6484a8b76c5a38cbf8766543a3
gpg: Signature made Mon Sep 26 21:53:33 2016 EDT
gpg:                using RSA key 0x6C34A49741E90902
gpg: Good signature from "Uri Blumenthal (MIT) <uri@mit.edu>" [ultimate]
Primary key fingerprint: 706C 9639 92B2 4F0A 4EED  B75E 9BAD 9629 C89B F6E5
     Subkey fingerprint: 7ACC 2166 010F CD10 AAB7  5465 6C34 A497 41E9 0902
Author: Mouse <mouse008@gmail.com>
Date:   Mon Sep 26 11:05:51 2016 -0400

    Changed ECIES parameters length to bits from bytes.
    Experimenting with GPG commit signature.

commit bbefa10f7a32f4e048054b577c2d378e98f32fea
gpg: Signature made Mon Sep 26 11:06:31 2016 EDT
gpg:                using RSA key 0x6C34A49741E90902
gpg: Good signature from "Uri Blumenthal (MIT) <uri@mit.edu>" [ultimate]
Primary key fingerprint: 706C 9639 92B2 4F0A 4EED  B75E 9BAD 9629 C89B F6E5
     Subkey fingerprint: 7ACC 2166 010F CD10 AAB7  5465 6C34 A497 41E9 0902
Author: Mouse <mouse008@gmail.com>
Date:   Mon Sep 26 11:05:51 2016 -0400
. . . . .

See commit history on https://github.com/mouse07410/cryptopp/commits/master

This may not be the most convenient or the easiest to figure out tool, but once the process is set, signing should go smoothly. I think there are only a couple of kinks to address before that happens.

@anonimal
Copy link
Contributor Author

@mouse07410

So who are those "some people" who contribute but cannot sign (for whatever reason)? Frankly, I see none.

I said "With Kovri". That means with The Kovri I2P Router Project; not Crypto++.

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

No branches or pull requests

5 participants