Skip to content
Snippets Groups Projects

GPG signed commits

Merged Alexis Reigel requested to merge siemens/gitlab-ce:feature/gpg-signed-commits into master
2 unresolved threads

What does this MR do?

  1. Shows gpg signed commits (excl. tags)
  2. If the gpg key is verified (= the gpg was added to GitLab and the user's email matches the key's email) a "Verified" batch is displayed
  3. If the gpg key is not verified or does not exist on GitLab an "Unverified" batch is displayed
  4. If the commit is not signed, the behaviour is unchanged
  5. Allows the user to add gpg keys to his profile

Are there points in the code the reviewer needs to double check?

Why was this MR needed?

Git allows to signed commits. This PR enables displaying those commits.

The development of this MR is sponsored by @siemens (/cc @bufferoverflow).

Screenshots (if relevant)

Commits list:

Commits___signed_commits___nannie_bernhard___gitlab_test___GitLab_3_

Commit details:

signed_commit_by_nannie_bernhard__0f44cd1d____Commits___nannie_bernhard___gitlab_test___GitLab

Badge popovers:

Commits___signed_commits___nannie_bernhard___gitlab_test___GitLab_5_

Commits___signed_commits___nannie_bernhard___gitlab_test___GitLab_4_

User settings > GPG key:

GPG_Keys___User_Settings___GitLab_2_

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/20268.

Edited by Dmytro Zaporozhets (DZ)

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Alexis Reigel added 456 commits

    added 456 commits

    • 806d3385...348dff0a - 437 commits from branch gitlab-org:master
    • 8bfe6278 - Prototype key verification
    • 19588e46 - remove commit actions width constraint
    • 5a3bb9ab - commit signature with spec
    • 4db37594 - add gpg key model
    • 31836b0e - add emails method to GgpKey
    • d89b6c39 - only validate gpg_key#fingerprint "internally"
    • da6464b9 - add profile gpg key page to manage gpg keys
    • 80a97d8c - extract gpg functionality to lib class
    • c0b9b3eb - add / remove gpg keys to / from system keychain
    • c0a981b7 - add second gpg key for specs
    • 2e6ba6ed - feature spec for gpg signed commits
    • fbda179b - use example gpg key instead of my own
    • da601265 - test with a gpg key with multiple emails
    • bd3848f9 - email handling for gpg keys
    • 50260622 - move current keychain methods to namespace
    • 65b2c2e3 - gpg email verification
    • 89d30f5c - notification email on add new gpg key
    • f6713667 - remove gpg from keychain when user's email changes
    • c4ed04c1 - fixup! add profile gpg key page to manage gpg keys

    Compare with previous version

  • Author Contributor

    @ayufan

    [...] but I think that we should not use KeyChain.

    I thought about other solutions as well. While this may be a slower solution as it requires disk access, it's the safest one, as it doesn't try to reinvent something about gpg.

    We should parse signature in order to figure out the key fingerprint that we would look in the database.

    When would this happen? We'd have to do this whenever...

    • ... a key gets added or removed
    • ... a commit gets added
    • ... a key expires
    • ...

    The key verification to some extent should be cached, as this is a time-consuming operation.

    ... which makes invalidation (esp. based on date) more difficult.

    Furthermore the verification happens only when displaying commits, where the initial number is limited. Potentially this could be solved by the rails view caching...

    We should be careful about date validity.

    That's the advantage of the keychain solution which already validates this.

  • Author Contributor

    @smcgivern

    [...] is that this keychain won't be written to often, once it has the most common intermediaries. Is that right?

    The keychain is written to only in the following situations:

    • when a user adds or removes a keychain
    • when a user changes his email address
  • I thought about other solutions as well. While this may be a slower solution as it requires disk access, it's the safest one, as it doesn't try to reinvent something about gpg.

    Right, but this also doesn't scale. And at gitlab.com scale it will probably not work :)

  • Author Contributor

    @ayufan

    We should parse signature in order to figure out the key fingerprint that we would look in the database.

    Can you elaborate a bit on the solution you have in mind? If I understand correctly you don't mean to do the verification on commit create time instead of read time, but to wholly leave out the verification using the gpg keychain.

  • @koffeinfrei I added some lower-level CR comments. I'll let @ayufan advise on the overall direction :slight_smile:

  • assigned to @ayufan

  • @ayufan skipping the keychain sounds dangerous from a security pov, if you are worried about hitting the disk maybe we could use FuseFS or something..?

  • @johnou @koffeinfrei

    Can you elaborate a bit on the solution you have in mind? If I understand correctly you don't mean to do the verification on commit create time instead of reading time, but to wholly leave out the verification using the gpg keychain.

    Nope. It's not about skipping keychain, it is about implementing it in a way that can use DB for that instead of file storage. Having to keep in sync FS and DB is usually pain, and if we can avoid it, it is great.

    This is my thoughts what we need:

    1. Read signature of the commit,
    2. Decode signature and figure out the fingerprint or another identifier of the key,
    3. Get public part of the key from DB by doing fingerprint lookup,
    4. Doing regular OpenSSL verify that verifies signature cryptographically, but also if needed verifies certificate validity time and usage (this can also be done with dynamically constructed KeyChain).
    Edited by Kamil Trzciński
  • Author Contributor

    @ayufan (/cc @johnou)

    I'm still a bit unsure about what your solution would look like.

    Would 1. and 2. happen when creating a commit? The keychain would still be accessed in this case, just on commit-write and not on commit-read time?

    I assume that 3. (and 4.) will happen on commit-read time?

    What's the purpose of 4.? If we verify the signature on commit-write time, there's no need for an additional ssl verification. As I see it, the verification has to be the same as the current solution, just not on read but on write time. Are you trying to replace a part of the gpg verification with openssl? Does that even work?

  • No, we should not do verification at commit write, but on read/present time, as the key could be revoked.

    Yes, 3. and 4. would happen on-line when "verifying" commit status when presenting it.

    I will take a look at gpgme code and see how we can get a key fingerprint from signature as this is the missing piece here I think.

  • @ayufan did you get a chance to take a look at the gpgme code?

  • Not trying to discourage anyone but as far as I know there is no way to "skip" KeyChain for GPGME, as there is no way to provide an alternative "KeyChain" implementation (like getting key from DB and providing it as reference, etc).

    There are some lengthy discussion around how "bad" GPGME is about the decision to not provide "library friendly" APIs (I can't find the links right now)

    One alternative way to mitigate some of the downsides of syncing keychains to disk is to have a keychain per-user/project (https://github.com/ueno/ruby-gpgme/blob/af5df129103c83f6e75183d39154616fcacf1a04/lib/gpgme/engine.rb#L63-L73), so we don't have to sync a huge file with every possible key as we do for SSH

    Another alternative would require tmpfs path (we don't actually need ramfs to prevent keys to be disclosed in disk, as we can generate a random password for the keyring that exists only during "session" usage), and do the same thing proposed above, create a temporary keyring for the validation and purges it after.

  • @brodock Interesting, could you take a look how to improve the implementation to make it scale?

  • Author Contributor

    so we don't have to sync a huge file with every possible key as we do for SSH

    Is this a scalability problem? Is having a huge keychain slower for the verification process? Writing to the keychain should not be the main issue, as this doesn't happen very frequently.

    Another alternative would require tmpfs path (we don't actually need ramfs

    The scalability concern stems from the fact that for the verification process disk access is required (which is slow).

    @ayufan Would using ramfs actually solve the scalability issues? Maybe we could do something like this for the verification process:

    1. Read all the relevant keys from the database and store them to a temporary keychain in ramfs

    OR

    1. Copy the main keychain on disk to a temporary keychain in ramfs. This would require only 1 disk read per verification "session".

    Otherwise, did you have a chance to get any further with your partial keychain/database verification?

  • @ayufan I can only dig into this next week, but a few thoughts with trade-offs:

    If we go into tmpfs territory, we need to recreate the keychain everytime we need to "check" data. If we move into creating specific keychains for every project, we only need to "update" that keychain whenever there is a change into required keys.

    Checking required keys can be little tricky as we have to figure how to deal with removal (should we ever let someone remove a key that has already been used before? how do we "re-authenticate" old data or handle key expiration/revocation etc).

    I'm under impression that we never "delete" a key that was used to authenticate some content, we can "disable" it after a specific date or for new commits after that date etc, but if we just remove we will not be able to handle authentication for old data.

    I'm not 100% familiar with how this works with git, @briann may probably have things to contribute.

    Edited by Gabriel Mazetto
  • Could keychain per-user/project be enough? Instead of trying to pre-optimize, are we certain there will be scalability issues? What is / are the actual bottleneck(s)?

  • Author Contributor

    Instead of trying to pre-optimize, are we certain there will be scalability issues? What is / are the actual bottleneck(s)?

    That's exactly how I see it too, although I lack the experience of e.g. @ayufan with the scale of gitlab.com and can't really assess the gravity of this. @ayufan raised the performance concerns, so I'd be really grateful if he helped move this forward!

  • I want this feature to ship into GitLab soon but I agree with @ayufan that we should look for a way to avoid keychain usage. 2 reasons why I don't like keychain idea:

    • we duplicate info in database and keychain and we depends on code to sync those
    • keychain is another file we need to share between all those servers of GitLab.com

    @brodock you mentioned there is no way to skip keychain from gpgme. Are we talking about ruby implementation or original C library. I was thinking maybe we can invoke certain gpgme C functions directly from our code to achieve this (and not use ruby-gpgme gem). From what I see the whole check happens somewhere here https://github.com/dephekt/gpgme/blob/903bf16a416b1bf608b1e647937c9b06864b0141/src/engine.c#L919. Maybe worth investigating

    Edited by Dmytro Zaporozhets (DZ)
  • @dzaporozhets I'm not aware of the implementation details in the C part ( we can probably hack it ), but it was by design made in a way to always rely on the keychain.

  • @brodock yep I get it. But if we can skip this part (keychain) - why not? it saves us a lot of headache

  • I take this one from @ayufan. He already has enough on his plate. Also I am interested in having it merged.

    I think before considering this merge request I will spend some time looking for option to compare key and signature without using keychain. If found - I will create a new merge request based on this one.

    Then we can proceed to polishing details and describing use scope of the feature. For example: expire date works, revoke does not etc

    Edited by Dmytro Zaporozhets (DZ)
  • I am working on https://gitlab.com/gitlab-org/gitlab-ce/issues/4044 right now, but I will return to this issue once I have spare minute there.

  • Author Contributor

    @dzaporozhets As far as I can tell the main verification is in the gnupg library at https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=sm/verify.c;h=6c034e69292e309df855f4a9730e552e46e65b24;hb=HEAD#l90. The function is quite complex and does a lot of things, mostly interacting with the low-level library KSBA. We could maybe try to copy the function while stripping away unneeded cornercases and replacing the calls to keydb with our own.

    We could possibly also write our own database adapter replacing the file based keydb implementation (https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=sm/keydb.c;h=87fc12d0ee8cba36cf2c8abbca4b5ea65dbc75e5;hb=HEAD).

  • @koffeinfrei thanks for looking into it. You already give us 2 ways to go - that's a good start.

    The function is quite complex and does a lot of things, mostly interacting with the low-level library KSBA. We could maybe try to copy the function while stripping away unneeded cornercases and replacing the calls to keydb with our own.

    I think we should try this first. It feels like it's easier to write own function without half of those code than to monkey-patch existing db adapter.

  • I spend some time on it and come up with next plan:

    1. The user adds public key -> we save fingerprint to db.
    2. We read signed commit and extract signature and signed text using rugged.
    3. We extract a fingerprint from signature and signed text (no need for public key). For example, it can be done with gpg --verify signature signed_text.
    4. We use a fingerprint to find the public key in the database (single sql query).
    5. We import public key to the keychain and do a verification (I believe we can't just trust fingerprint match?).
    6. We save the result of the verification in the database table with a structure like:
      project_id: integer
      sha: string
      fingerprint: string
      gpg_key_id: integer
    7. We treat keychain as temp storage to get verification entry in the database created. We don't sync it between application servers.

    Meta code can look like:

    class Commit
      def signature
        fingerprint = gpg_verify(signature, signed_text)
        gpg_key = GpgKey.find_by(fingerprint: fingerprint) 
    
        if gpg_key 
          import_key
          do_verification
          save_result
        else
          'Unverified'
        end 
      end
    end

    @koffeinfrei what do you think?

  • Author Contributor

    @dzaporozhets Ok that's actually not very far from the current solution. I'm much more comfortable with a solution where we don't need to patch GPG :relieved:.

    \1. The user adds public key -> we save fingerprint to db.

    :ballot_box_with_check: we do that already.

    \2. We read signed commit and extract signature and signed text using rugged.

    :ballot_box_with_check: we do that already, unless you mean we should do this not on commit read-time.

    \3. We extract a fingerprint from signature and signed text (no need for public key). For example, it can be done with gpg --verify signature signed_text.

    We can do this already in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9546/diffs#d1c10892daedb4d4dd3d4b12b6d071091eea83df_242_245. signature.fpr returns the fingerprint that we can use to query the user's public key from the db.

    \4. We use a fingerprint to find the public key in the database (single sql query).

    See above (3.)

    \5. We import public key to the keychain and do a verification (I believe we can't just trust fingerprint match?).

    We could do this at https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9546/diffs#d1c10892daedb4d4dd3d4b12b6d071091eea83df_242_247.

    \6. We save the result of the verification in the database table with a structure like:

    So the idea is to cache the verification and use the verified result on consecutive reads of the commit?

    I guess it's actually fine to assume a commit is verified forever once it has been verified. This simplifies cache key invalidation:

    • we don't need to check the expire date of a key after the commit has been validated
    • we don't need to handle removal of keys

    \7. We treat keychain as temp storage to get verification entry in the database created. We don't sync it between application servers.

    One downside of this is that we need to write and read on every commit verification process. This may conflict with the concerns issued by @ayufan even more than the current solution, which requires writing to the keychain only when adding / removing a key, and the verification only needs to read.

  • Ok that's actually not very far from the current solution.

    @koffeinfrei yes

    I'm much more comfortable with a solution where we don't need to patch GPG

    yes, me too,

    One downside of this is that we need to write and read on every commit verification process. This may conflict with the concerns issued by @ayufan even more than the current solution, which requires writing to the keychain only when adding / removing a key, and the verification only needs to read.

    @koffeinfrei I think the main concern was keychain that needs to be synced between app servers. Which is solved with my proposal. write and read can be done in async job by sidekiq. And since we don't maintain huge keychain those operations should be fast enough. As a first step, we can even start with fingerprint match and don't use keychain at all.

  • @koffeinfrei what's our next step? do you want to finish MR according to my proposal at https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9546#note_31943767 or you want me to do it?

  • Author Contributor

    what's our next step? do you want to finish MR according to my proposal at https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9546#note_31943767 or you want me to do it?

    @dzaporozhets I'd like to continue working on this. I'm actually already at it.

    There's a problem with the fetching of the fingerprint from the temporary keychain:

    When the keychain doesn't contain the key that was used to sign the commit, the verify call returns a different fingerprint. This means we can't get the fingerprint to query the key from the database. We actually need to add the appropriate key to the keychain first. Maybe I'm missing something...

    I'll try if I can extract the email address or some other info from the signed commit...

    Edited by Alexis Reigel
  • Author Contributor

    @dzaporozhets I tried to benchmark whether we could use a temporary keychain with all the keys, assuming the query from the database is fast enough:

    using tmp dir /tmp/d20170612-30111-4b1fus
    Testing 10,000 keys
    This took 2.15525157 seconds
    
    using tmp dir /tmp/d20170612-30111-1jf22oe
    Testing 100,000 keys
    This took 13.122465633 seconds
    
    using tmp dir /tmp/d20170612-30111-1p9o4al
    Testing 1,000,000 keys
    This took 121.265232966 seconds

    10,000 keys already take 2 seconds, so this doesn't seem to be an option. Maybe we can only insert the potentially relevant keys, maybe only from the users from the same organization / group. But it still seems like an imminent scalability issue.

  • When the keychain doesn't contain the key that was used to sign the commit, the verify call returns a different fingerprint. This means we can't get the fingerprint to query the key from the database. We actually need to add the appropriate key to the keychain first. Maybe I'm missing something...

    @koffeinfrei Try gpg --verify signature_file signed_text_file. It should give you a valid fingerprint

  • @koffeinfrei Hmm at least it always gives a correct key id. Can we search the database by key id then?

  • Some debugging:

    From: /Users/dzaporozhets/Projects/gitlab-development-kit/gitlab/app/models/commit.rb @ line 256 Commit#signature:
    
        246: def signature
        247:   return @signature if defined?(@signature)
        248: 
        249:   @signature = nil
        250: 
        251:   signature, signed_text = @raw.signature(project.repository)
        252: 
        253:   if signature && signed_text
        254:     GPGME::Crypto.new.verify(signature, signed_text: signed_text) do |sign|
        255:       binding.pry
     => 256:       @signature = sign
        257:     end
        258:   end
        259: 
        260:   @signature
        261: end
    
    [1] pry(#<Commit>)> signature
    => "-----BEGIN PGP SIGNATURE-----\nVersion: GnuPG v2\n\niQEcBAABCAAGBQJZDw5xAAoJEEUF9cfhLGpaKtgH/0aJ+U1cXl4RMwuMXVqXemXy\nMDm0moOz7wP6gDJvu5dg6NgGK9vMWBsMwP0c9EItZWofUddi10fR4XoQlvMI7UJr\nDaBtKU7pbEzokof2lYjHqLlbDaW0c49zzTrJff0guyaeIQleE883cPQBhw1kyUHm\n1ktKK7gC1+m8fcY7doAIJfqiOK11aD811wK4JVmFL/+54YkSN9B5Q0VORt4O5SRg\n65nlrPuXwkmy0HBaWLxoCvDrFa3qiUvmcYgeiwNa4B+E3kDIMmd99U/dYrj2Ypyw\nC2y2tS95JDELKzi3jrgDt4qK8LhfgkQChtLVwcN0nzF/mZ9g7vNYAo/S7MI5604=\n=zvBG\n-----END PGP SIGNATURE-----"
    [2] pry(#<Commit>)> File.write('/tmp/sig3.sig', signature)
    => 472
    [3] pry(#<Commit>)> File.write('/tmp/sig3.text', signed_text)
    => 264
    [4] pry(#<Commit>)> `gpg --verify /tmp/sig3.sig /tmp/sig3.text`
    gpg: Signature made Sun May  7 15:09:21 2017 EEST using RSA key ID E12C6A5A
    gpg: requesting key E12C6A5A from hkps server hkps.pool.sks-keyservers.net
    gpgkeys: key 4505F5C7E12C6A5A not found on keyserver
    gpg: no valid OpenPGP data found.
    gpg: Total number processed: 0
    gpg: keyserver communications error: Not found
    gpg: keyserver communications error: Bad public key
    gpg: Can't check signature: No public key

    Which gives us a correct key id: 4505F5C7E12C6A5A Screen_Shot_2017-06-13_at_00.34.23

  • @koffeinfrei what do you think about ^ ?

  • Author Contributor

    @dzaporozhets You seem to have the public key in your keyring. If you remove the key from keyring or use the --homedir option to use the temporary test keyring the output is not as helpful anymore:

    [5] pry(#<Commit>)> `gpg --homedir /tmp/d20170613-19496-q0sdoy --verify /tmp/sig3.sig /tmp/sig3.text`
    gpg: Signature made Don 23 Feb 2017 13:55:10 CET
    gpg:                using RSA key CCFBE19F00AC8B1D
    gpg: Can't check signature: No public key
    => ""
  • Author Contributor

    what do you think about ^ ?

    @dzaporozhets Looks nice!

  • @koffeinfrei good, so we have key id even if key is not in keychain. can you proceed with that?

  • Alexis Reigel added 6944 commits

    added 6944 commits

    • c4ed04c1...9cb3510a - 6923 commits from branch gitlab-org:master
    • c5644818 - Prototype key verification
    • f59c8639 - commit signature with spec
    • dac132c1 - add gpg key model
    • 05ed6b67 - add emails method to GgpKey
    • eabc292c - only validate gpg_key#fingerprint "internally"
    • cb7ba5b0 - add profile gpg key page to manage gpg keys
    • 93def3da - extract gpg functionality to lib class
    • a5bddda4 - add / remove gpg keys to / from system keychain
    • 9978740f - add second gpg key for specs
    • 782af120 - feature spec for gpg signed commits
    • 1a2f70b1 - use example gpg key instead of my own
    • b34bc918 - test with a gpg key with multiple emails
    • da115e1d - email handling for gpg keys
    • cb42d7b2 - move current keychain methods to namespace
    • f49fdbe8 - gpg email verification
    • 08e18d86 - notification email on add new gpg key
    • ad8e50f7 - remove gpg from keychain when user's email changes
    • b97bfb21 - fixup! add profile gpg key page to manage gpg keys
    • 31134861 - use more descriptive variable names
    • 2b18dc7e - don't sync to keychain file
    • c6c7aa3d - add primary keyid attribute to gpg keys

    Compare with previous version

  • Alexis Reigel added 2 commits

    added 2 commits

    • 685e4883 - add primary keyid attribute to gpg keys
    • 25e02c50 - verify gpg commit using tmp keyring and db query

    Compare with previous version

  • Author Contributor

    good, so we have key id even if key is not in keychain. can you proceed with that?

    @dzaporozhets yes, the code is now updated to use the keyring only temporarily for verification.

    Should I add the verification caching (storing the verification result in the db) also in this MR? Or should I implement this in a follow-up MR? Could we potentially merge this without the caching?

  • Should I add the verification caching (storing the verification result in the db) also in this MR? Or should I implement this in a follow-up MR? Could we potentially merge this without the caching?

    @koffeinfrei I think verification process on commits page for 20+ commits without caching is not ok. I see 2 options for us:

    1. Add caching to this merge request so commits page is performant.
    2. Remove verification process from commits page (keep it only for commit#show page) and bring it back with caching in in follow-up MR.

    Which one do you prefer?

  • Author Contributor

    \1. Add caching to this merge request so commits page is performant.

    @dzaporozhets Just to be sure: Caching means storing the initial verification process, meaning that the first call of a commits page still takes long. Or do you think we should/can generate the verification results earlier, e.g. in a background job or at commit write time?

  • @koffeinfrei caching means storing result of verification (key id, verified/unverified, commit_sha, project_id, gpg_key_id) in the database after first call. Additionally, we can add verification for new commits in post receive worker when new code is pushed. So all new commits will be verified before user visit commits page and we have no slow down in performance

  • Alexis Reigel added 107 commits

    added 107 commits

    • 25e02c50...9cc126ea - 83 commits from branch gitlab-org:master
    • e9d39133 - Prototype key verification
    • 5627ad25 - commit signature with spec
    • 3b5e479d - add gpg key model
    • de1a52c0 - add emails method to GgpKey
    • e162055d - only validate gpg_key#fingerprint "internally"
    • a1ffe58d - add profile gpg key page to manage gpg keys
    • 795b5dff - extract gpg functionality to lib class
    • 831f13b8 - add / remove gpg keys to / from system keychain
    • bf8eab85 - add second gpg key for specs
    • cd45a74f - feature spec for gpg signed commits
    • f3ee5c8c - use example gpg key instead of my own
    • eb75031e - test with a gpg key with multiple emails
    • 8ae2d232 - email handling for gpg keys
    • 36177230 - move current keychain methods to namespace
    • 9512873a - gpg email verification
    • 5815e91b - notification email on add new gpg key
    • 513cd142 - remove gpg from keychain when user's email changes
    • d8a5529f - fixup! add profile gpg key page to manage gpg keys
    • e67370c5 - use more descriptive variable names
    • 455d53a7 - don't sync to keychain file
    • bccffd77 - add primary keyid attribute to gpg keys
    • 25a9e699 - verify gpg commit using tmp keyring and db query
    • 402d4afb - gpg signature model for gpg verification caching
    • 63010d8e - cache the gpg commit signature

    Compare with previous version

  • @koffeinfrei I looked at the code. The direction looks good. I am wondering about caching behaviour if there is no GPG key in the database associated. I think we should create cache anyway with gpg_key_id: nil and then when user adds a new GPG key we just do something like:

    key = current_user.gpg_keys.new(gpg_key_params)
    
    GpgSignarure.where(gpg_key_primary_keyid: key.primary_keyid, gpg_key_id: nil).
      update_all(gpg_key_id: key.id)

    What do you think?

    Edited by Dmytro Zaporozhets (DZ)
  • Author Contributor

    I think we should create cache anyway with gpg_key_id: nil [...]

    It currently works like this already, the cache entry gets created whenever a commit has a signature, whether valid or invalid.

    [...] and then when user adds a new GPG key we just do something like

    I agree, that's actually what I'm currently working on (although we need to do the whole gpg verification ceremony at this point).

  • Alexis Reigel added 4 commits

    added 4 commits

    • a92f1260 - cache the gpg commit signature
    • f78ac30f - bail if the commit has no signature
    • 8b0889fb - gpg signature is only valid when key is verified
    • 0b3f0190 - move signature cache read to Gpg::Commit

    Compare with previous version

  • @koffeinfrei awesome, please tell me if you need help or when code is ready for review.

  • changed milestone to %9.4

  • Alexis Reigel added 10 commits

    added 10 commits

    • a4468866 - gpg signature model for gpg verification caching
    • 1c1b12cf - cache the gpg commit signature
    • ca53b25a - bail if the commit has no signature
    • 8046bce5 - gpg signature is only valid when key is verified
    • 3460ace4 - move signature cache read to Gpg::Commit
    • 905912c5 - store gpg_key_primary_keyid for unknown gpg keys
    • 1651527b - memoize verified_signature call
    • eba1b77d - allow updating of gpg signature through gpg commit
    • 9ce3cf7d - update invalid gpg signatures when key is created
    • 7b9d5c5f - update invalid gpg signatures when email changes

    Compare with previous version

  • Author Contributor

    @dzaporozhets So far the caching is ready:

    • cache key (gpg signature) invalidation:
      • when a user adds a key (gpg commit signature was invalid because key was missing)
      • when a user changes his email address (gpg commit signature was invalid because email didn't match)

    Once a signature is valid it's not invalidated anymore. I believe that when a commit was valid at some specific point it should stay valid, even if a key is removed later or expires. Tell me if you think otherwise.

    Now I'll update the feature specs. Concerning https://gitlab.com/siemens/gitlab-ce/blob/feature/gpg-signed-commits/spec/features/commits_spec.rb#L218-227, should I add the commits to https://gitlab.com/gitlab-org/gitlab-test? Or should I create a separate repo?

    Edited by Alexis Reigel
  • Alexis Reigel added 30 commits

    added 30 commits

    • f7ba6260 - commit signature with spec
    • e39029b2 - add gpg key model
    • f6c86348 - add emails method to GgpKey
    • b8201311 - only validate gpg_key#fingerprint "internally"
    • c0a8b5da - add profile gpg key page to manage gpg keys
    • aced00cd - extract gpg functionality to lib class
    • 8536ef2d - add / remove gpg keys to / from system keychain
    • e99cde7b - add second gpg key for specs
    • 56c368ce - feature spec for gpg signed commits
    • 7b3b3259 - use example gpg key instead of my own
    • 6a3888a8 - test with a gpg key with multiple emails
    • 2222958b - email handling for gpg keys
    • e142e94d - move current keychain methods to namespace
    • c8e370b5 - gpg email verification
    • 5b1b1967 - notification email on add new gpg key
    • 29f76dc8 - remove gpg from keychain when user's email changes
    • 36bfa9c4 - use more descriptive variable names
    • 5694e34e - don't sync to keychain file
    • 52cbdecb - add primary keyid attribute to gpg keys
    • 80cbafed - verify gpg commit using tmp keyring and db query
    • df10fde7 - gpg signature model for gpg verification caching
    • 3c015b84 - cache the gpg commit signature
    • e9ac7621 - bail if the commit has no signature
    • 1f629410 - gpg signature is only valid when key is verified
    • 3f3dd798 - move signature cache read to Gpg::Commit
    • cfecb56c - store gpg_key_primary_keyid for unknown gpg keys
    • ef08746f - memoize verified_signature call
    • 643ed70e - allow updating of gpg signature through gpg commit
    • d82e9884 - update invalid gpg signatures when key is created
    • 844f9839 - update invalid gpg signatures when email changes

    Compare with previous version

  • Alexis Reigel added 17 commits

    added 17 commits

    • aea90f4b - notification email on add new gpg key
    • 0b17796b - remove gpg from keychain when user's email changes
    • 269a8fdf - use more descriptive variable names
    • fd9fdce9 - don't sync to keychain file
    • a6169547 - add primary keyid attribute to gpg keys
    • 4ed93fe7 - verify gpg commit using tmp keyring and db query
    • 816e9132 - gpg signature model for gpg verification caching
    • d0f45b28 - cache the gpg commit signature
    • 70c1ebee - bail if the commit has no signature
    • b99df965 - gpg signature is only valid when key is verified
    • 3c1f59be - move signature cache read to Gpg::Commit
    • 6184b2bc - store gpg_key_primary_keyid for unknown gpg keys
    • f9a1247d - memoize verified_signature call
    • 90c75a13 - allow updating of gpg signature through gpg commit
    • 48976835 - update invalid gpg signatures when key is created
    • 630667b4 - update invalid gpg signatures when email changes
    • 46808a84 - ui: place the gpg badge before the commit hash

    Compare with previous version

  • Alexis Reigel added 27 commits

    added 27 commits

    • 4b0c1836 - add profile gpg key page to manage gpg keys
    • 9e0fe173 - extract gpg functionality to lib class
    • 602153f3 - add / remove gpg keys to / from system keychain
    • a6b9ed7f - add second gpg key for specs
    • 607df8fa - feature spec for gpg signed commits
    • 0c325edb - use example gpg key instead of my own
    • c30a1990 - test with a gpg key with multiple emails
    • 25327962 - email handling for gpg keys
    • 37b056f9 - move current keychain methods to namespace
    • eabf90a5 - gpg email verification
    • d6823364 - notification email on add new gpg key
    • 11b97d29 - remove gpg from keychain when user's email changes
    • 39abf506 - use more descriptive variable names
    • 5b61e4a3 - don't sync to keychain file
    • 7c83c73c - add primary keyid attribute to gpg keys
    • 519837db - verify gpg commit using tmp keyring and db query
    • d5a9102f - gpg signature model for gpg verification caching
    • b417aef2 - cache the gpg commit signature
    • 12ad2f39 - bail if the commit has no signature
    • 6ed84e02 - gpg signature is only valid when key is verified
    • e9309b7f - move signature cache read to Gpg::Commit
    • a9c9da8a - store gpg_key_primary_keyid for unknown gpg keys
    • 2ef8d4c2 - memoize verified_signature call
    • 8dacbe46 - allow updating of gpg signature through gpg commit
    • 61e2771b - update invalid gpg signatures when key is created
    • 6e611e25 - update invalid gpg signatures when email changes
    • b32bb78c - ui: place the gpg badge before the commit hash

    Compare with previous version

  • Alexis Reigel added 12 commits

    added 12 commits

    • efe55ba8 - verify gpg commit using tmp keyring and db query
    • f5c9c20a - gpg signature model for gpg verification caching
    • 5549b01f - cache the gpg commit signature
    • 316f2b43 - bail if the commit has no signature
    • 3a09566c - gpg signature is only valid when key is verified
    • feee15ca - move signature cache read to Gpg::Commit
    • e0bfb5d1 - store gpg_key_primary_keyid for unknown gpg keys
    • b6f365a6 - memoize verified_signature call
    • 9905f52c - allow updating of gpg signature through gpg commit
    • b0397019 - update invalid gpg signatures when key is created
    • 4cc20f29 - update invalid gpg signatures when email changes
    • 3dfc55ed - ui: place the gpg badge before the commit hash

    Compare with previous version

  • Awesome work! I have some concerns, though:

    • Is it possible to remove my PGP keys (eg. due to compromisation, or revocation)?
    • I if so, will that invalidate the cache?
    • What about expired PGP keys?
  • Author Contributor

    @gergelypolonkai

    Is it possible to remove my PGP keys (eg. due to compromisation, or revocation)?

    Yes, keys can be added and removed.

    I if so, will that invalidate the cache?

    No.

    What about expired PGP keys?

    See my comment above https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9546#note_32422731, my assumption is as follows:

    Once a signature is valid it's not invalidated anymore. I believe that when a commit was valid at some specific point it should stay valid, even if a key is removed later or expires.

    This means that

    1. when a commit is verified and the key is expired, the signature will be invalid.
    2. when a commit is verified and the key is valid, the signature will be valid. This commit will stay verified, even when the key expires later.
  • Oh sorry, I somehow missed that comment. Thanks for the clarification.

  • should I add the commits to https://gitlab.com/gitlab-org/gitlab-test

    @koffeinfrei yes

    I believe that when a commit was valid at some specific point it should stay valid, even if a key is removed later or expires

    makes sense

  • @koffeinfrei please mark resolved discussions as resolved :)

  • Alexis Reigel mentioned in merge request !9646 (merged)

    mentioned in merge request !9646 (merged)

  • Author Contributor

    @dzaporozhets There's a bug in rugged https://github.com/libgit2/rugged/issues/608 that's affecting the reading of signature. The bug seems to be fixed starting with 0.26.0b4. Can I upgrade rugged to that version? It's a beta-version, so I'm not sure if that's fine.

  • Author Contributor

    please mark resolved discussions as resolved :)

    @dzaporozhets I was under the impression that discussions should be resolved by the original poster. Should I resolve comments as I see fit?

  • Can I upgrade rugged to that version? It's a beta-version, so I'm not sure if that's fine.

    @koffeinfrei let's see if it break existing tests. If not then fine. Otherwise, we might do this in separate merge request

    Should I resolve comments as I see fit?

    @koffeinfrei please do

  • Let me ask a few corner case scenarios that I believe are relevant:

    What about expired PGP keys?

    How we will deal with rebase? suppose a commit that was previously valid, and after got a GPG key expired or revoked, do we consider the rewritten commits as valid or not? This is important decision because in EE one of the merge algorithms does a rebase, which will rewrite the commit hashes (which I believe we will use for as key to store the signatures status).

    About key invalidation, will we rely on the keyserver for that or will this require manual input from the "key owner" in gitlab itself? if we rely on key servers, how often will we ping it for updates, etc?

    If we rely on keyservers for invalidation, what should we do if we can't ping the keyserver (like connection issues, etc)?

    Edited by Gabriel Mazetto
  • Alexis Reigel added 4 commits

    added 4 commits

    • 30714d53 - no need for passing parameter
    • 3e028e1c - need to wrap the raw commit in a commit model
    • 3f7a3247 - update rugged
    • d2254c8c - update features specs for gpg commits

    Compare with previous version

  • How we will deal with rebase?

    @brodock rebase means new sha means its different commit means we treat it just like any other new commit -> verify and save result to the database.

    About key invalidation, will we rely on the keyserver for that

    @brodock no, only local keychain where we import user keys. If they want to invalidate key they should remove it from GitLab.

  • Alexis Reigel added 2 commits

    added 2 commits

    • bf30f501 - update features specs for gpg commits
    • 94705f59 - perform signature update in sidekiq worker

    Compare with previous version

  • Alexis Reigel changed the description

    changed the description

  • Alexis Reigel added 1 commit

    added 1 commit

    • 46f7447f - perform signature update in sidekiq worker

    Compare with previous version

  • mentioned in issue #33958 (moved)

  • rebase means new sha means its different commit means we treat it just like any other new commit -> verify and save result to the database.

    Ok so as a side comment we need to detect if anyone in a MR is signing commits and prevent the MR to be merged with the rebase strategy, which will invalidate the signature. (or rewrite/remote it)

    Edited by Gabriel Mazetto
  • Ok so as a side comment we need to detect if anyone in a MR is signing commits and prevent the MR to be merged with the rebase strategy, which will invalidate the signature. (or rewrite/remote it)

    @brodock I believe we think different about it. For me, it's obvious that if GitLab rebases your commits there will be no signature because rebase means creating new commits. And there is no private key on GitLab server to create those new commits with your signature. So I would leave it up to the user to decide what to do. However, some info/warning message will be good.

  • Alexis Reigel added 3 commits

    added 3 commits

    • 70b0b7e9 - allow removal of gpg key by nullifying signatures
    • 2b79dd1e - add gpg commit popover badges
    • 85038dd2 - we need to update the gpg_key as well

    Compare with previous version

  • Alexis Reigel added 1 commit

    added 1 commit

    • 83e93e38 - linkify the whole user badge part, not only avatar

    Compare with previous version

  • mentioned in merge request gitlab-test!23 (merged)

  • Alexis Reigel added 1 commit

    added 1 commit

    Compare with previous version

  • Alexis Reigel added 2 commits

    added 2 commits

    • 8d565588 - use updated gitlab-test repo for signed commits
    • 6134f14d - popover trigger needs to be defined in js init

    Compare with previous version

  • Alexis Reigel added 834 commits

    added 834 commits

    • 6134f14d...ad3843ae - 790 commits from branch gitlab-org:master
    • 8372b7b0 - Prototype key verification
    • a34afc88 - commit signature with spec
    • 00061501 - add gpg key model
    • b39e2d05 - add emails method to GgpKey
    • 76e8568e - only validate gpg_key#fingerprint "internally"
    • 0c829641 - add profile gpg key page to manage gpg keys
    • b30085c7 - extract gpg functionality to lib class
    • 8f0fe2e1 - add / remove gpg keys to / from system keychain
    • b8813f84 - add second gpg key for specs
    • 33f99249 - feature spec for gpg signed commits
    • c9415593 - use example gpg key instead of my own
    • 7b3664c9 - test with a gpg key with multiple emails
    • 5eb0cacc - email handling for gpg keys
    • 9b974203 - move current keychain methods to namespace
    • 7cbe04cc - gpg email verification
    • d132e516 - notification email on add new gpg key
    • 89b18858 - remove gpg from keychain when user's email changes
    • 520d1911 - use more descriptive variable names
    • d6c8dc76 - don't sync to keychain file
    • a0851bd1 - add primary keyid attribute to gpg keys
    • ce9c14bd - verify gpg commit using tmp keyring and db query
    • 218d3915 - gpg signature model for gpg verification caching
    • d4c8da6f - cache the gpg commit signature
    • c0b43e54 - bail if the commit has no signature
    • 27ec69ca - gpg signature is only valid when key is verified
    • 691769ab - move signature cache read to Gpg::Commit
    • a3a63b01 - store gpg_key_primary_keyid for unknown gpg keys
    • 8d12e0de - memoize verified_signature call
    • a891bac4 - allow updating of gpg signature through gpg commit
    • d9bdb513 - update invalid gpg signatures when key is created
    • 1959ef41 - update invalid gpg signatures when email changes
    • dfa507e9 - ui: place the gpg badge before the commit hash
    • 1069f29c - no need for passing parameter
    • edd9ecf7 - need to wrap the raw commit in a commit model
    • 49ff27ba - update rugged
    • 3725bcd8 - update features specs for gpg commits
    • 17dbba8b - perform signature update in sidekiq worker
    • 1fede54e - allow removal of gpg key by nullifying signatures
    • e555efcd - add gpg commit popover badges
    • 3dec5fb4 - we need to update the gpg_key as well
    • e24ea08e - linkify the whole user badge part, not only avatar
    • 3c8514b5 - extract variable
    • e0772d43 - use updated gitlab-test repo for signed commits
    • d83acb1d - popover trigger needs to be defined in js init

    Compare with previous version

  • Alexis Reigel marked the checklist item Update styling, add tooltip as completed

    marked the checklist item Update styling, add tooltip as completed

  • Alexis Reigel changed the description

    changed the description

  • Alexis Reigel marked the checklist item Update the feature spec and add the signed and unsigned commits directly to https://gitlab.com/gitlab-org/gitlab-test as completed

    marked the checklist item Update the feature spec and add the signed and unsigned commits directly to https://gitlab.com/gitlab-org/gitlab-test as completed

  • Alexis Reigel marked the checklist item Changelog entry added as completed

    marked the checklist item Changelog entry added as completed

  • Alexis Reigel marked the checklist item Conform by the merge request performance guides as completed

    marked the checklist item Conform by the merge request performance guides as completed

  • Alexis Reigel changed the description

    changed the description

  • Alexis Reigel marked the checklist item Conform by the merge request performance guides as incomplete

    marked the checklist item Conform by the merge request performance guides as incomplete

  • Author Contributor

    let's see if it break existing tests. If not then fine. Otherwise, we might do this in separate merge request

    @dzaporozhets I had to update one small thing due to the rugged update, see 6d24ee41

  • Alexis Reigel added 187 commits

    added 187 commits

    • d83acb1d...a2e13104 - 140 commits from branch gitlab-org:master
    • 22c6f2c3 - Prototype key verification
    • 0795f9ef - commit signature with spec
    • 04436204 - add gpg key model
    • 556ad331 - add emails method to GgpKey
    • 0eb043ae - only validate gpg_key#fingerprint "internally"
    • b460fa58 - add profile gpg key page to manage gpg keys
    • 89b9d9fe - extract gpg functionality to lib class
    • ff951704 - add / remove gpg keys to / from system keychain
    • 2aec7765 - add second gpg key for specs
    • 83429d6a - feature spec for gpg signed commits
    • 9946bb02 - use example gpg key instead of my own
    • 93797652 - test with a gpg key with multiple emails
    • a11f2fdb - email handling for gpg keys
    • 923c55e7 - move current keychain methods to namespace
    • d3ece26f - gpg email verification
    • 6abc73f6 - notification email on add new gpg key
    • f31f7eea - remove gpg from keychain when user's email changes
    • d59e6adc - use more descriptive variable names
    • 7a81506f - don't sync to keychain file
    • 17793137 - add primary keyid attribute to gpg keys
    • 923bea8f - verify gpg commit using tmp keyring and db query
    • 597ddd49 - gpg signature model for gpg verification caching
    • d3e655eb - cache the gpg commit signature
    • a6b03d31 - bail if the commit has no signature
    • 2289f608 - gpg signature is only valid when key is verified
    • cb17f5b2 - move signature cache read to Gpg::Commit
    • 4855239c - store gpg_key_primary_keyid for unknown gpg keys
    • 23cc85f1 - memoize verified_signature call
    • f8532f5c - allow updating of gpg signature through gpg commit
    • fcd020c3 - update invalid gpg signatures when key is created
    • 37f57ce4 - update invalid gpg signatures when email changes
    • b1aae10b - ui: place the gpg badge before the commit hash
    • 23963764 - no need for passing parameter
    • 5d21841f - need to wrap the raw commit in a commit model
    • 6d24ee41 - update rugged
    • ab923de5 - update features specs for gpg commits
    • 17d79fc0 - perform signature update in sidekiq worker
    • 1a730432 - allow removal of gpg key by nullifying signatures
    • c896990e - add gpg commit popover badges
    • 6b34f6f7 - we need to update the gpg_key as well
    • 5735f2b1 - linkify the whole user badge part, not only avatar
    • f254bc2a - extract variable
    • 5716f022 - use updated gitlab-test repo for signed commits
    • 59c5b389 - popover trigger needs to be defined in js init
    • 061b375c - add changelog
    • fa09eb3b - remove the :gpg rspec tag
    • 5dd39999 - use sign_in instead of login_with

    Compare with previous version

  • Author Contributor

    @dzaporozhets what's in your opinion left to do? Maybe add documentation?

  • mentioned in issue #4232 (closed)

  • Maybe add documentation?

    @koffeinfrei yes please, its important for considering feature as complete. I will review your recent changes in meantime.

  • Alexis Reigel added 28 commits

    added 28 commits

    • 6f9d2cc8 - verify gpg commit using tmp keyring and db query
    • a28b4eca - gpg signature model for gpg verification caching
    • b65531b8 - cache the gpg commit signature
    • 6a6263bc - bail if the commit has no signature
    • bb173686 - gpg signature is only valid when key is verified
    • d4a88463 - move signature cache read to Gpg::Commit
    • b7e608c6 - store gpg_key_primary_keyid for unknown gpg keys
    • e8f0f865 - memoize verified_signature call
    • a6dd6233 - allow updating of gpg signature through gpg commit
    • 00541a64 - update invalid gpg signatures when key is created
    • 7a15dc44 - update invalid gpg signatures when email changes
    • 0191d75c - ui: place the gpg badge before the commit hash
    • 6aabdbe5 - no need for passing parameter
    • 217ce5ec - need to wrap the raw commit in a commit model
    • 7c50423c - update rugged
    • 747e0d20 - update features specs for gpg commits
    • f460c993 - perform signature update in sidekiq worker
    • 72b10893 - allow removal of gpg key by nullifying signatures
    • d50b2a60 - add gpg commit popover badges
    • 6a037491 - we need to update the gpg_key as well
    • bf2cfe46 - linkify the whole user badge part, not only avatar
    • 286dac25 - extract variable
    • 3ba2c97a - use updated gitlab-test repo for signed commits
    • e19f471d - popover trigger needs to be defined in js init
    • dc3fef66 - add changelog
    • 45ef4f1f - remove the :gpg rspec tag
    • d61f87da - use sign_in instead of login_with
    • 18e83f75 - documentation for gpg signed commits

    Compare with previous version

  • Alexis Reigel added 1 commit

    added 1 commit

    • 06aa5d7f - documentation for gpg signed commits

    Compare with previous version

  • Alexis Reigel added 1 commit

    added 1 commit

    • bdf0e575 - documentation for gpg signed commits

    Compare with previous version

  • Alexis Reigel added 1 commit

    added 1 commit

    • 30e9dfe8 - documentation for gpg signed commits

    Compare with previous version

  • Could someone UX please provide me with some suggestions / hints on how to style the badges in a nice way?

    @koffeinfrei I am chiming in for UX here :), so cool this feature is already being worked on!

    Badges shouldn't need the icon, as we chose this same direction with "protected branches".. also this seems done similarly like this on GH

    As this is more of a status on the commit, let's use a status badge similar as what can be found on the pipeline list

    As for the commit list view:

    Group

    As for the commit view titlebar, can we position the label like this:

    image image

    I like what GH has done with the rich tooltips (https://gitlab.com/gitlab-org/gitlab-ce/issues/30495) upon the verified status badges.

    @dzaporozhets Is it correct that a commit can be signed by someone else, created by someone else, but not authored by that same person? Therefore we need a similar solution.. where we show the more information about the signer of that commit? See the following gif:

    GH


    Is "warning" color in place here? or just the white/grey enough? cc: @dzaporozhets

    image

  • Is "warning" color in place here? or just the white/grey enough?

    White/grey is more appropriate. Unverified means it is signed, but the identity of the signer is not verified. This is not dangerous, it's just not "green".

  • @dimitrieh Screenshots in the merge request description are outdated. Some things are already improved. If possible I would recommend checkout the branch and try it.

    @koffeinfrei can you please update MR with latest screenshots?

  • Author Contributor

    @dzaporozhets Screenshots updated (/cc @dimitrieh)

  • Alexis Reigel changed the description

    changed the description

  • Alexis Reigel marked the checklist item Documentation created/updated as completed

    marked the checklist item Documentation created/updated as completed

  • Alexis Reigel added 1 commit

    added 1 commit

    • d5496ac7 - position gpg badge first on commit line

    Compare with previous version

  • Alexis Reigel added 288 commits

    added 288 commits

    • d5496ac7...b071317c - 238 commits from branch gitlab-org:master
    • 9eacab3a - Prototype key verification
    • 28b1565e - commit signature with spec
    • be45050c - add gpg key model
    • bc5c5f20 - add emails method to GgpKey
    • 75943113 - only validate gpg_key#fingerprint "internally"
    • 4bcb6469 - add profile gpg key page to manage gpg keys
    • 9cf63797 - extract gpg functionality to lib class
    • eb27a0c8 - add / remove gpg keys to / from system keychain
    • b2b5a446 - add second gpg key for specs
    • a00f8369 - feature spec for gpg signed commits
    • ce529146 - use example gpg key instead of my own
    • 961aa566 - test with a gpg key with multiple emails
    • 09047e17 - email handling for gpg keys
    • 347cc387 - move current keychain methods to namespace
    • 2101447e - gpg email verification
    • e70c1151 - notification email on add new gpg key
    • 04a1df8a - remove gpg from keychain when user's email changes
    • ed01aec6 - use more descriptive variable names
    • b3fbcf22 - don't sync to keychain file
    • 9e9e1375 - add primary keyid attribute to gpg keys
    • e3524580 - verify gpg commit using tmp keyring and db query
    • cbf4ab1f - gpg signature model for gpg verification caching
    • f8d786fc - cache the gpg commit signature
    • 4f6711d1 - bail if the commit has no signature
    • 9d239e17 - gpg signature is only valid when key is verified
    • ee1f38af - move signature cache read to Gpg::Commit
    • ec3e3636 - store gpg_key_primary_keyid for unknown gpg keys
    • 49fe658f - memoize verified_signature call
    • d4603acf - allow updating of gpg signature through gpg commit
    • 0903ae5f - update invalid gpg signatures when key is created
    • 68d7a599 - update invalid gpg signatures when email changes
    • dd0c9f84 - ui: place the gpg badge before the commit hash
    • aef2340f - no need for passing parameter
    • 05cf5480 - need to wrap the raw commit in a commit model
    • aa740ad9 - update rugged
    • 17d126c0 - update features specs for gpg commits
    • 3ccfc2df - perform signature update in sidekiq worker
    • dd6079ef - allow removal of gpg key by nullifying signatures
    • bf0411ed - add gpg commit popover badges
    • 0af025b7 - we need to update the gpg_key as well
    • 139c61aa - linkify the whole user badge part, not only avatar
    • f1ab1ed2 - extract variable
    • ab33f877 - use updated gitlab-test repo for signed commits
    • d30df07a - popover trigger needs to be defined in js init
    • 5d57b591 - add changelog
    • 07eec0a1 - remove the :gpg rspec tag
    • 1ca150af - use sign_in instead of login_with
    • f4b57336 - documentation for gpg signed commits
    • 00092e40 - position gpg badge first on commit line
    • b1d28cac - add help links to gpg commits / gpg settings

    Compare with previous version

  • Alexis Reigel changed the description

    changed the description

  • Author Contributor

    @dzaporozhets Small update: I added links from the popover and the gpg settings page to the documentation (i.e. help page). Also updated the screenshots accordingly.

    Did you already have the chance to have a look and the latest updates?

  • Alexis Reigel added 8 commits

    added 8 commits

    • 61df1e5e - use updated gitlab-test repo for signed commits
    • c9d47315 - popover trigger needs to be defined in js init
    • 498ae078 - add changelog
    • 935da775 - remove the :gpg rspec tag
    • 336a43e6 - use sign_in instead of login_with
    • e588bae8 - documentation for gpg signed commits
    • b2036cba - position gpg badge first on commit line
    • b2b74d7d - add help links to gpg commits / gpg settings

    Compare with previous version

  • @DouweM can you please take a look at the code too. I might missed something after looking on it for too long.

  • @koffeinfrei from user perspective feature works great. The only question left is performance. I believe we do signature verification on go when open commits page, right? After first visit its cached to db so its good. But first load might take a while. I think we should have cached version created before first load where we can. For example for new commits pushed we can execute commit.signature method somewhere in GitPushService. This way signature verified in async job before user opens the page

    Edited by Dmytro Zaporozhets (DZ)
  • @koffeinfrei btw great work done here. Can't wait to see this feature shipped

  • Alexis Reigel added 1 commit

    added 1 commit

    Compare with previous version

  • Author Contributor

    @dzaporozhets

    I believe we do signature verification on go when open commits page, right?

    Yes.

    I think we should have cached version created before first load where we can. For example for new commits pushed we can execute commit.signature method somewhere in GitPushService

    I agree. I'll update this.

    Edited by Alexis Reigel
  • @dzaporozhets I'll review the code. Does this also need some UX attention? The screenshots look identical to GitHub's implementation (https://github.com/blog/2144-gpg-signature-verification) down to the text, so we shouldn't ship like this.

  • Douwe Maan
  • @dzaporozhets @koffeinfrei @DouweM can we make these changes still:

    • Make the badge, if verified, also have a green border: image
    • Can we give the badges some side padding.. to make the badges look similar to badges used in https://gitlab.com/gitlab-org/gitlab-ce/pipelines
    • Can we get some margin in between the badge and Commit wording in the commit detail view titlebar image
    • Group
      • adjust the tooltip arrow to also be grey
      • vertically align the avatar with the name
      • give the grey and white areas in the tooltip some more vertical breathing/room/space == more padding
      • the following icons should be available already in the codebase image (@lbennett correct? :smiley_cat: )
    • image
      • List your gpg keys without key icon
      • Put the information on 1 line and in columns

    let me know if anything is unclear


    Edit:

    I made some mockups of the rich tooltips. Please see to using the same colors and other characteristics:

    Group_5

    Group_6

    Edited by Dimitrie Hoekstra
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • 1 module BadgesHelper
    2 def verified_email_badge(email, verified)
    3 css_classes = %w(btn btn-xs disabled)
    4
    5 css_classes << 'btn-success' if verified
    6
    7 content_tag 'span', class: css_classes do
    8 "#{email} #{verified ? 'Verified' : 'Unverified'}"
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • @koffeinfrei With what we have here, would it be possible to add an option automatically verify all incoming commits on-push, and reject if any are not verified? That's definitely something for another MR, of course.

  • Douwe Maan
  • mentioned in issue #30495 (closed)

  • Author Contributor

    With what we have here, would it be possible to add an option automatically verify all incoming commits on-push, and reject if any are not verified? That's definitely something for another MR, of course.

    @DouweM Yes this would be possible. I'll update the GitPushService anyway to create the signatures on push, so we should be able to use something similar to accept/reject pushed commits.

  • Author Contributor

    @dimitrieh On your mockups, did you leave out the user's profile info on purpose? I believe that this is a pretty important info for verified signatures.

    Edited by Alexis Reigel
  • Alexis Reigel added 24 commits

    added 24 commits

    • 6d51f755 - perform signature update in sidekiq worker
    • cbdbfb4f - allow removal of gpg key by nullifying signatures
    • 8835ddff - add gpg commit popover badges
    • 0731b4af - we need to update the gpg_key as well
    • e392a574 - linkify the whole user badge part, not only avatar
    • 824d52c9 - extract variable
    • ca661be7 - use updated gitlab-test repo for signed commits
    • 264e8f83 - popover trigger needs to be defined in js init
    • 5fe45aba - add changelog
    • c2069ff6 - remove the :gpg rspec tag
    • 865feaa4 - use sign_in instead of login_with
    • ef63cd96 - documentation for gpg signed commits
    • 3991c606 - position gpg badge first on commit line
    • 49818f8f - add help links to gpg commits / gpg settings
    • 150d4f54 - no need for html_safe
    • 1d9c3454 - find_by_id -> find_by(:id, ...)
    • 0ac466ca - use hash instead of 2d array
    • a8c92313 - validate presence of user on gpg_key
    • c46fd388 - use after_commit instead of AfterCommitQueue
    • c8cffd96 - convert gpg badge helper methods to partials
    • a7ae315e - fix memoization
    • 176d37e3 - simplify fetching of commit
    • 6f45796a - remove duplicate statement
    • f324856b - also update gpg_signatures when gpg_key is null

    Compare with previous version

  • Alexis Reigel added 1 commit

    added 1 commit

    Compare with previous version

  • @koffeinfrei oops sorry! updating in a sec with improved mockups for thos

  • Alexis Reigel added 2 commits

    added 2 commits

    • 7e7860f5 - use the correct flex classes on the commits list
    • cd5f4fbf - use existing status-box css class for gpg badge

    Compare with previous version

  • @koffeinfrei @dzaporozhets updated mockups compared to previous one, now including avatar and name/username references

    Group_5

    Group_6

    updated mockups in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9546#note_34186656 ;)

    Edited by Dimitrie Hoekstra
  • @koffeinfrei please ping me once comments from @DouweM are addressed. Once UI approved by @dimitrieh we will merge it.

  • changed milestone to %9.5

  • @koffeinfrei I'll wait for the adjustments presented at https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9546#note_34186656 :smile:

    Let me know if you need any help! :star:

  • Alexis Reigel added 10 commits

    added 10 commits

    • cbf1de06 - use after_commit instead of AfterCommitQueue
    • e3f03641 - convert gpg badge helper methods to partials
    • 6f9aa175 - fix memoization
    • 2df75532 - simplify fetching of commit
    • 5f4f2bee - remove duplicate statement
    • e6cb00ef - also update gpg_signatures when gpg_key is null
    • 2dbc04d9 - extract common method
    • c4f1a958 - use the correct flex classes on the commits list
    • 705c4f5e - use existing status-box css class for gpg badge
    • 8312840c - use ShaAttribute for commit_sha column

    Compare with previous version

  • Author Contributor

    the following icons should be available already in the codebase image

    @dimitrieh I can't find the question mark icon. Could you point me to it please? Otherwise I'll leave the FA icons.

    Could you also please address the questions in the comments?

    Edited by Alexis Reigel
  • Author Contributor

    @dimitrieh

    Make the badge, if verified, also have a green border

    Updated

    Can we give the badges some side padding.. to make the badges look similar to badges used in https://gitlab.com/gitlab-org/gitlab-ce/pipelines

    Updated

    Can we get some margin in between the badge and Commit wording in the commit detail view titlebar

    adjust the tooltip arrow to also be grey

    This is handled by bootstrap.

    vertically align the avatar with the name

    Updated

    give the grey and white areas in the tooltip some more vertical breathing/room/space == more padding

    Updated

    List your gpg keys without key icon

    The styling is kept similar to the already existing ssh keys section.

    Put the information on 1 line and in columns

    The styling is kept similar to the already existing ssh keys section.

  • Alexis Reigel added 30 commits

    added 30 commits

    • 794781bf - perform signature update in sidekiq worker
    • 995acf27 - allow removal of gpg key by nullifying signatures
    • 29fa09e0 - add gpg commit popover badges
    • da255e8f - we need to update the gpg_key as well
    • 7e1f5cf5 - linkify the whole user badge part, not only avatar
    • 04d2cff2 - extract variable
    • daaa004a - use updated gitlab-test repo for signed commits
    • 99b04bec - popover trigger needs to be defined in js init
    • b669ddaa - add changelog
    • 4652171f - remove the :gpg rspec tag
    • a2341d85 - use sign_in instead of login_with
    • ef19df22 - documentation for gpg signed commits
    • fd204dd0 - position gpg badge first on commit line
    • e198a865 - add help links to gpg commits / gpg settings
    • 39ad90a9 - no need for html_safe
    • 4a0b1907 - find_by_id -> find_by(:id, ...)
    • 87a34e43 - use hash instead of 2d array
    • 0bf4590a - validate presence of user on gpg_key
    • 0ebe5bbe - use after_commit instead of AfterCommitQueue
    • ad7d6fe9 - convert gpg badge helper methods to partials
    • bce79fcc - fix memoization
    • 1f52ba0e - simplify fetching of commit
    • f26c3a7f - remove duplicate statement
    • ee40d0ae - also update gpg_signatures when gpg_key is null
    • 3a82bf6b - extract common method
    • ac52b58a - use the correct flex classes on the commits list
    • 053f8f20 - use existing status-box css class for gpg badge
    • cfcd76a4 - use ShaAttribute for commit_sha column
    • 7c208f38 - improve spacing / alignments in gpg popup
    • 1d9521af - generate gpg signature on push

    Compare with previous version

  • Alexis Reigel added 30 commits

    added 30 commits

    • 794781bf - perform signature update in sidekiq worker
    • 995acf27 - allow removal of gpg key by nullifying signatures
    • 29fa09e0 - add gpg commit popover badges
    • da255e8f - we need to update the gpg_key as well
    • 7e1f5cf5 - linkify the whole user badge part, not only avatar
    • 04d2cff2 - extract variable
    • daaa004a - use updated gitlab-test repo for signed commits
    • 99b04bec - popover trigger needs to be defined in js init
    • b669ddaa - add changelog
    • 4652171f - remove the :gpg rspec tag
    • a2341d85 - use sign_in instead of login_with
    • ef19df22 - documentation for gpg signed commits
    • fd204dd0 - position gpg badge first on commit line
    • e198a865 - add help links to gpg commits / gpg settings
    • 39ad90a9 - no need for html_safe
    • 4a0b1907 - find_by_id -> find_by(:id, ...)
    • 87a34e43 - use hash instead of 2d array
    • 0bf4590a - validate presence of user on gpg_key
    • 0ebe5bbe - use after_commit instead of AfterCommitQueue
    • ad7d6fe9 - convert gpg badge helper methods to partials
    • bce79fcc - fix memoization
    • 1f52ba0e - simplify fetching of commit
    • f26c3a7f - remove duplicate statement
    • ee40d0ae - also update gpg_signatures when gpg_key is null
    • 3a82bf6b - extract common method
    • ac52b58a - use the correct flex classes on the commits list
    • 053f8f20 - use existing status-box css class for gpg badge
    • cfcd76a4 - use ShaAttribute for commit_sha column
    • 7c208f38 - improve spacing / alignments in gpg popup
    • 1d9521af - generate gpg signature on push

    Compare with previous version

  • Author Contributor

    @DouweM / @dzaporozhets I created a worker that generates the signatures on push (added to GitPushService).

  • Alexis Reigel added 1 commit

    added 1 commit

    • d427dcb1 - generate gpg signature on push

    Compare with previous version

  • @koffeinfrei seems good to me. I am still in doubt about calculating signature for existing commits during page load, but I don't have a good solution for this problem. We could do it async too but it means user does not see result immediately which destroys the value of feature.

  • adjust the tooltip arrow to also be grey

    @koffeinfrei can you make the rich tooltips to spawn on top? this way the icon is automatically at the white bottom and doesn't need a color change.

  • @koffeinfrei can you post updated screenshots? i have a hard time inspecting this within the gdk ;)

  • I can't find the question mark icon

    @koffeinfrei icons can be found in https://gitlab.com/gitlab-org/gitlab-ce/tree/master/app/views/shared/icons . You need to use the borderless ones and create a border of 1px with css. The question mark icon was not yet in, could you place it in that directory and use it here similarly?

    _icon_status_notfound_borderless.svg _icon_status_notfound_borderless.svg

  • Alexis Reigel changed the description

    changed the description

  • Author Contributor

    @dimitrieh

    The question mark icon was not yet in, could you place it in that directory and use it here similarly?

    I updated the gpg popovers to use the icons.

    can you post updated screenshots?

    Updated.

    can you make the rich tooltips to spawn on top?

    Done.

    One thing left with the popovers is the change regarding the name / username. I'll update this next, as I'll have to handle the case when a profile doesn't exist anymore. We could link to the email address in that case (as it's already done with the commits). What should be shown in place of the "@ username" in that case? Nothing? The email address?

  • Alexis Reigel added 6 commits

    added 6 commits

    • 6a99bc12 - improve spacing / alignments in gpg popup
    • 39b0514e - generate gpg signature on push
    • 7c105d86 - don't use assignment in if condition
    • a28554d5 - add notfound icon (question mark)
    • 9f5121df - use svg icons for gpg popovers
    • 9f09d114 - use lighter gray for unverified gpg signature

    Compare with previous version

  • @koffeinfrei nice!

    small detail: why is there a white border on the grey area?:

    image

    also can you make use of the normal focus styling for badges, (see pipeline list for an example)

    Done.

    One thing left with the popovers is the change regarding the name / username. I'll update this next, as I'll have to handle the case when a profile doesn't exist anymore. We could link to the email address in that case (as it's already done with the commits). What should be shown in place of the "@ username" in that case? Nothing? The email address?

    i would only show the email in that case ;)

  • @koffeinfrei seems good to me. I am still in doubt about calculating signature for existing commits during page load, but I don't have a good solution for this problem. We could do it async too but it means user does not see result immediately which destroys the value of feature.

    @dzaporozhets @koffeinfrei What about getting the verified states in a separate AJAX request that can be a bit slower than the initial page load, just like we do with "Last commit" on the tree page (https://gitlab.com/gitlab-org/gitlab-ce/tree/master)? We can do this both on the commits index and the show page.

  • This ad-hock AJAX verification just needs to happen once for every signature right? So first page load you backfill the verification state and on next you don't need to make the ajax request for that commit again, just for the "pending verification" ones.

    So commits with signature can have these states: "pending verification", "valid", "invalid".

    On first page load the ones with "pending" status will trigger the AJAX request once.

  • Alexis Reigel added 8 commits

    added 8 commits

    • c9998f9e - use ShaAttribute for commit_sha column
    • b92be2b2 - improve spacing / alignments in gpg popup
    • 8d06a1b0 - generate gpg signature on push
    • 145005ee - don't use assignment in if condition
    • 85e5252d - add notfound icon (question mark)
    • 9a8c4d59 - use svg icons for gpg popovers
    • 05547dbb - use lighter gray for unverified gpg signature
    • 70b47807 - user may now revoke a gpg key

    Compare with previous version

  • Alexis Reigel added 7 commits

    added 7 commits

    • a32bf669 - improve spacing / alignments in gpg popup
    • f994b0ea - generate gpg signature on push
    • b11ccc4f - don't use assignment in if condition
    • 1a1ce56f - add notfound icon (question mark)
    • 452e0678 - use svg icons for gpg popovers
    • acd86506 - use lighter gray for unverified gpg signature
    • ec23cb59 - user may now revoke a gpg key

    Compare with previous version

  • Alexis Reigel added 8 commits

    added 8 commits

    • 02f418d3 - use ShaAttribute for commit_sha column
    • 8973a0d2 - improve spacing / alignments in gpg popup
    • 5f17e06b - generate gpg signature on push
    • a6f0974c - don't use assignment in if condition
    • 05827207 - add notfound icon (question mark)
    • eb2defc6 - use svg icons for gpg popovers
    • f600da09 - use lighter gray for unverified gpg signature
    • d8f56a50 - user may now revoke a gpg key

    Compare with previous version

  • @koffeinfrei I agree with @DouweM - let's make is with AJAX like we do with "Last commit" on the tree page.

  • This ad-hock AJAX verification just needs to happen once for every signature right?

    @brodock no just one AJAX call per 20 commits on the page. Like we do for tree view.

    So commits with signature can have these states: "pending verification", "valid", "invalid".

    @brodock I believe we don't need to complicate this with "pending verification" state. If signature has DB record - we return it, otherwise do verification and create one.

  • On first page load the ones with "pending" status will trigger the AJAX request once.

    @brodock Nope. Just use same mechanism as we have for tree view. One AJAX call for all visible elements (with signatures).

    Edited by Dmytro Zaporozhets (DZ)
  • With pseudo code it will look like

    Controller

    def commits
      commits = Commit.where...
    end
    
    def signatures
      signatures = commits.map(&:signatures)
    end

    View

    %div{id: commit.id}
      = commit.sha
      ...
      - if commit.has_signature?
        .signature-info Loading spinner

    JS

    ajax(
      'commits/signatures', 
      ids: [commit_ids],
      success: function(signatures) { 
        signatures.each -> find(signature.commit_id).html(...)
      }
    )

    What do you think?

  • Author Contributor

    I agree with @DouweM - let's make is with AJAX like we do with "Last commit" on the tree page.

    @DouweM / @dzaporozhets Well I'm not sure this is necessary. The async loading would only make sense for non-processed commits, which is only the case for...

    1. signed commits that were pushed before this feature gets shipped and...
    2. signed commits that were never viewed on the commits or commit page

    As we have an after push hook that processes all newly pushed commits the AJAX call doesn't make sense anymore. And for the existing commits the loading is slower only once. All consecutive calls don't require any re-verification. This AJAX-feature actually slows down the loading of the page in the long run.

  • Alexis Reigel added 6 commits

    added 6 commits

    • b73a4254 - unify commit signature colors with pipeline status
    • 5915844e - store gpg user name and email on the signature
    • e59987e8 - show gpg key's user info when no profile exists
    • 5c9f40c3 - swap user's name and the user's username
    • 47d392fb - display gpg key in the popover with monospace font
    • f7383551 - nicer email badges on the profile gpg page

    Compare with previous version

  • Author Contributor

    @dimitrieh

    small detail: why is there a white border on the grey area?:

    This is how bootstrap does it :)

    also can you make use of the normal focus styling for badges, (see pipeline list for an example)

    Updated. The same sass mixin as for the pipeline badges is used now.

    i would only show the email in that case ;)

    Updated.

  • Alexis Reigel added 1011 commits

    added 1011 commits

    • f7383551...6658c706 - 934 commits from branch gitlab-org:master
    • f38a8a69 - Prototype key verification
    • dab1d26b - commit signature with spec
    • b914af5f - add gpg key model
    • 5484afdc - add emails method to GgpKey
    • 2dbe165d - only validate gpg_key#fingerprint "internally"
    • c6305e33 - add profile gpg key page to manage gpg keys
    • ba94133e - extract gpg functionality to lib class
    • 3839d6ac - add / remove gpg keys to / from system keychain
    • 66bf1fcf - add second gpg key for specs
    • 43145531 - feature spec for gpg signed commits
    • 062c2212 - use example gpg key instead of my own
    • ced68151 - test with a gpg key with multiple emails
    • 89db0a85 - email handling for gpg keys
    • 002e0ed8 - move current keychain methods to namespace
    • 36d3fedd - gpg email verification
    • 8b999bb6 - notification email on add new gpg key
    • 00ef00ca - remove gpg from keychain when user's email changes
    • fcc36ad3 - use more descriptive variable names
    • 1d32f614 - don't sync to keychain file
    • 4e53876a - add primary keyid attribute to gpg keys
    • 75cb43a8 - verify gpg commit using tmp keyring and db query
    • e17e404a - gpg signature model for gpg verification caching
    • 4534184c - cache the gpg commit signature
    • efff9732 - bail if the commit has no signature
    • bfc8bf7a - gpg signature is only valid when key is verified
    • 5dd58be5 - move signature cache read to Gpg::Commit
    • 06dcf5f9 - store gpg_key_primary_keyid for unknown gpg keys
    • 1bb74c07 - memoize verified_signature call
    • 3edeb3ee - allow updating of gpg signature through gpg commit
    • 8682d621 - update invalid gpg signatures when key is created
    • 0a442c96 - update invalid gpg signatures when email changes
    • 36b0d030 - ui: place the gpg badge before the commit hash
    • 6e4b3129 - no need for passing parameter
    • 1f64f9ce - need to wrap the raw commit in a commit model
    • 7d4830bd - update rugged
    • 5cf9a968 - update features specs for gpg commits
    • 134b0494 - perform signature update in sidekiq worker
    • 4027aa29 - allow removal of gpg key by nullifying signatures
    • d840b7c7 - add gpg commit popover badges
    • 62b521a1 - we need to update the gpg_key as well
    • be914d46 - linkify the whole user badge part, not only avatar
    • b00cebae - extract variable
    • f10ee728 - use updated gitlab-test repo for signed commits
    • e71819dc - popover trigger needs to be defined in js init
    • 64483fe4 - add changelog
    • 74dd358d - remove the :gpg rspec tag
    • 570f319c - use sign_in instead of login_with
    • f7461281 - documentation for gpg signed commits
    • 32fa350a - position gpg badge first on commit line
    • 905c31f6 - add help links to gpg commits / gpg settings
    • 1e487c1f - no need for html_safe
    • bfb81505 - find_by_id -> find_by(:id, ...)
    • ad6228c9 - use hash instead of 2d array
    • 244d5d60 - validate presence of user on gpg_key
    • d76cedce - use after_commit instead of AfterCommitQueue
    • 9892dd7d - convert gpg badge helper methods to partials
    • 26ee4fb3 - fix memoization
    • 017923f9 - simplify fetching of commit
    • 8d48b295 - remove duplicate statement
    • dcdfdb7b - also update gpg_signatures when gpg_key is null
    • a5405995 - extract common method
    • c5b36cef - use the correct flex classes on the commits list
    • 33db52c9 - use existing status-box css class for gpg badge
    • b7f4339d - use ShaAttribute for commit_sha column
    • 98f4141c - improve spacing / alignments in gpg popup
    • 62619fd0 - generate gpg signature on push
    • 48e6a92c - don't use assignment in if condition
    • b1bfb2f0 - add notfound icon (question mark)
    • 901e9bb3 - use svg icons for gpg popovers
    • 71af65d7 - use lighter gray for unverified gpg signature
    • b0af28b9 - user may now revoke a gpg key
    • 4258e055 - unify commit signature colors with pipeline status
    • 8154ee33 - store gpg user name and email on the signature
    • fa2c83ae - show gpg key's user info when no profile exists
    • b3abb68d - swap user's name and the user's username
    • a8689373 - display gpg key in the popover with monospace font
    • 7a9e8796 - nicer email badges on the profile gpg page

    Compare with previous version

  • Alexis Reigel unmarked as a Work In Progress

    unmarked as a Work In Progress

  • Alexis Reigel changed the description

    changed the description

  • Alexis Reigel added 76 commits

    added 76 commits

    • b120c910 - Prototype key verification
    • c429fbbc - commit signature with spec
    • 470c2976 - add gpg key model
    • 9ad639c3 - add emails method to GgpKey
    • a273c443 - only validate gpg_key#fingerprint "internally"
    • 1e01a33f - add profile gpg key page to manage gpg keys
    • fde6e151 - extract gpg functionality to lib class
    • b53de8f6 - add / remove gpg keys to / from system keychain
    • 76111e03 - add second gpg key for specs
    • b78c3301 - feature spec for gpg signed commits
    • 17cd1207 - use example gpg key instead of my own
    • 400f25c7 - test with a gpg key with multiple emails
    • a3cfd648 - email handling for gpg keys
    • e344e4e3 - move current keychain methods to namespace
    • 51d47661 - gpg email verification
    • c3466179 - notification email on add new gpg key
    • fa516faa - remove gpg from keychain when user's email changes
    • 04f00817 - use more descriptive variable names
    • 2635b723 - don't sync to keychain file
    • 99ca7bff - add primary keyid attribute to gpg keys
    • 3bd87f04 - verify gpg commit using tmp keyring and db query
    • 43eadf37 - gpg signature model for gpg verification caching
    • b660ea69 - cache the gpg commit signature
    • c2d75fca - bail if the commit has no signature
    • 23bbb482 - gpg signature is only valid when key is verified
    • be02e6f7 - move signature cache read to Gpg::Commit
    • 24601c54 - store gpg_key_primary_keyid for unknown gpg keys
    • 72ca33f0 - memoize verified_signature call
    • 41f7a796 - allow updating of gpg signature through gpg commit
    • 09799a66 - update invalid gpg signatures when key is created
    • 9c9ce8ed - update invalid gpg signatures when email changes
    • c4789c2f - no need for passing parameter
    • f0ded2db - need to wrap the raw commit in a commit model
    • 1cb20ecc - update rugged
    • b4cc9292 - update features specs for gpg commits
    • a1f87562 - perform signature update in sidekiq worker
    • 53833405 - allow removal of gpg key by nullifying signatures
    • e6cf3910 - add gpg commit popover badges
    • a3aeb858 - we need to update the gpg_key as well
    • aebd58bf - linkify the whole user badge part, not only avatar
    • f1b93490 - extract variable
    • c5142a98 - use updated gitlab-test repo for signed commits
    • e02d475b - popover trigger needs to be defined in js init
    • 91aea38b - add changelog
    • be79f45c - remove the :gpg rspec tag
    • 31ab1cd5 - use sign_in instead of login_with
    • f881a3e2 - documentation for gpg signed commits
    • 686813eb - position gpg badge first on commit line
    • 1d369ca4 - add help links to gpg commits / gpg settings
    • 3f4e657a - no need for html_safe
    • 6ebaf3d0 - find_by_id -> find_by(:id, ...)
    • 4b04f0a6 - use hash instead of 2d array
    • d1475335 - validate presence of user on gpg_key
    • 66622d1c - use after_commit instead of AfterCommitQueue
    • dca8a6a9 - convert gpg badge helper methods to partials
    • 13713f52 - fix memoization
    • 199e0b31 - simplify fetching of commit
    • 8f73b392 - remove duplicate statement
    • 3091d196 - also update gpg_signatures when gpg_key is null
    • b972cc69 - extract common method
    • 03cb7d22 - use the correct flex classes on the commits list
    • d0acf8af - use existing status-box css class for gpg badge
    • 31a4896a - use ShaAttribute for commit_sha column
    • fea93b92 - improve spacing / alignments in gpg popup
    • 5964cb35 - generate gpg signature on push
    • 8e2893cb - don't use assignment in if condition
    • 3189e783 - add notfound icon (question mark)
    • b9d0f289 - use svg icons for gpg popovers
    • 4e761748 - use lighter gray for unverified gpg signature
    • 9c227920 - user may now revoke a gpg key
    • 57a2fc98 - unify commit signature colors with pipeline status
    • e12ada8d - store gpg user name and email on the signature
    • 54fdf8cd - show gpg key's user info when no profile exists
    • 69ddab48 - swap user's name and the user's username
    • 89feb5ff - display gpg key in the popover with monospace font
    • 2837b116 - nicer email badges on the profile gpg page

    Compare with previous version

  • Yorick Peterse mentioned in merge request !12869 (merged)

    mentioned in merge request !12869 (merged)

  • @koffeinfrei you probably already use this, but bootstrap (which our tooltips are also derived from) has a feature called static pop-overs: http://getbootstrap.com/javascript/#static-popover :)

    http://getbootstrap.com/javascript/#dismiss-on-next-click to be more precise

    Edited by Dimitrie Hoekstra
  • As we have an after push hook that processes all newly pushed commits the AJAX call doesn't make sense anymore. And for the existing commits the loading is slower only once. All consecutive calls don't require any re-verification. This AJAX-feature actually slows down the loading of the page in the long run.

    @koffeinfrei yes, but loading some info via AJAX rather than in same request won't hurt us much. However it will benefit for cases when I browse previously unvisited commits. For example I open file, click "history" and scroll through commits list fast. Here its 100% chance most of commits will be verified on go and slow down my experience. With AJAX we give user commits list without delay.

  • Author Contributor

    yes, but loading some info via AJAX rather than in same request won't hurt us much.

    @dzaporozhets It will add additional complexity to the codebase and additional points of failure to the UI. Also, we'll need to handle the case when more commits are loaded by scrolling down (which is already done via AJAX). This would make one AJAX call for the commits and then another one for the signatures.

    One last alternative idea (and then I'll give up :smiley_cat:):

    Can we find out how many existing GPG commits there are on GitLab.com? Could we potentially introduce a post-migration or something to pre-generate them?

  • Can we find out how many existing GPG commits there are on GitLab.com? Could we potentially introduce a post-migration or something to pre-generate them?

    @koffeinfrei imagine I import Linux repo with 1 million commits. We definitely don't want to verify signatures for each of them. Idea is to generate verification for next commits:

    1. commits a user look at
    2. new commits pushed to existing repo
  • @koffeinfrei honestly I don't see much of a problem with AJAX calls. And the benefit is clear to me - I see commits without waiting 1-2 seconds while signatures are generated for first time.

  • @DouweM what do you think about conversation about AJAX calls above ^ ?

  • Alexis Reigel added 15 commits

    added 15 commits

    • b4ab43ce - improve spacing / alignments in gpg popup
    • 85362c9c - generate gpg signature on push
    • 48653226 - don't use assignment in if condition
    • ea4eeac0 - add notfound icon (question mark)
    • 5b8f3e74 - use svg icons for gpg popovers
    • 475c9af5 - use lighter gray for unverified gpg signature
    • 29dc7d88 - user may now revoke a gpg key
    • 99f9efa7 - unify commit signature colors with pipeline status
    • 9b8083a7 - store gpg user name and email on the signature
    • 93b0cdb6 - show gpg key's user info when no profile exists
    • bff941af - swap user's name and the user's username
    • fb05b415 - display gpg key in the popover with monospace font
    • 4d74dc3e - nicer email badges on the profile gpg page
    • b9982abe - merge migrations to 1 single create per table
    • dd9f67d5 - use ShaAttribute for gpg table columns

    Compare with previous version

  • Author Contributor

    imagine I import Linux repo with 1 million commits. We definitely don't want to verify signatures for each of them.

    @dzaporozhets Does an import execute the post-push hook I implemented? If yes, could this be problematic due to the potential number of commits? If no, then this is an argument against my case, as we won't get generated signatures when importing projects from other repos.

    I guess I'll go ahead and implement some more AJAX... :smiley_cat:

    Edited by Alexis Reigel
  • Author Contributor

    @dzaporozhets I just thought about another issue concerning the generation of signatures. We should generate the signatures as soon as possible, as the key may be expired or removed later. I think we should make sure that the signature is created as soon as the commit lands in GitLab. This means that we should create the signatures when...

    • pushing new commits
    • importing a project from an existing repository
    • rolling out this feature
    • ...
  • Does an import execute the post-push hook I implemented? If yes, could this be problematic due to the potential number of commits? If no, then this is an argument against my case, as we won't get generated signatures when importing projects from other repos.

    no

    I guess I'll go ahead and implement some more AJAX... :smiley_cat:

    @koffeinfrei thanks, you are doing a great job in this MR!

    I just thought about another issue concerning the generation of signatures. We should generate the signatures as soon as possible, as the key may be expired or removed later. I think we should make sure that the signature is created as soon as the commit lands in GitLab. This means that we should create the signatures when...

    @koffeinfrei I think "pushing new commits" is enough. For people who migrate to git with legacy code there is little chance they will visit every commit ever. Also not sure about generating ASAP - we just explain users the limitation of the feature. If you visit commit 10 years old and by that time key already removed - its ok.

    Edited by Dmytro Zaporozhets (DZ)
  • But could we not just meet in the middle and provide an option to migrate the pgp signed commits of current projects? As for my personal activity, I always use PGP signing. So backward compatiblity would be nice to have.

    @dzaporozhets I agree with you that there is no need for every project to be migrated backwards. But providing an option to do so could be a good idea.

  • @phenomax I think we can make a rake task to run it for those who want it. But I believe this should be done separately from this MR. I propose once this merged (or now) - we make an issue to make such rake task.

    It will be something like

    task :verify_gpg_commits do 
      Project.each do |project|
        project.commits('master').each do |commit|
          verify(commit)
        end
      end
    end

    Maybe even accept group/project id as argument.

    Edited by Dmytro Zaporozhets (DZ)
  • @dzaporozhets Sounds like a good solution! Will you create the Issue & MR or shall I?

  • Alexis Reigel added 4 commits

    added 4 commits

    • 4c4e4c7d - use text instead of string for db columns
    • 303a596f - use short project path helpers
    • cfda9639 - use db's on_delete instead of has_many :dependent
    • 38266730 - length constrain on the index, not on the column

    Compare with previous version

  • Alexis Reigel added 1 commit

    added 1 commit

    • 78737c3e - fetch gpg signature badges by ajax

    Compare with previous version

  • Author Contributor

    @dzaporozhets / @DouweM The signature badges are now loaded with AJAX. It works like this:

    • The initial page load of the commits page (commits list) loads the badges by AJAX
    • When more paged commits are loaded by AJAX the badges are rendered in that same request
    • On the commit show page the badge is rendered statically, as it's only one badge which shouldn't be an isse

    Could you have another look at the code?

    In the meantime I'll try to fix the MySQL issue https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9546#note_35746099

  • Alexis Reigel added 1 commit

    added 1 commit

    • f60fdf35 - mysql hack: set length for binary indexes

    Compare with previous version

  • Alexis Reigel added 1 commit

    added 1 commit

    • f60fdf35 - mysql hack: set length for binary indexes

    Compare with previous version

  • Alexis Reigel added 2 commits

    added 2 commits

    • abcd9dbc - fetch gpg signature badges by ajax
    • 4a0badb1 - mysql hack: set length for binary indexes

    Compare with previous version

  • Alexis Reigel added 1 commit

    added 1 commit

    • 08005d47 - mysql hack: set length for binary indexes

    Compare with previous version

  • Alexis Reigel added 1 commit

    added 1 commit

    • edf983bb - mysql hack: set length for binary indexes

    Compare with previous version

  • Alexis Reigel added 1 commit

    added 1 commit

    • 0a3ab34e - mysql hack: set length for binary indexes

    Compare with previous version

  • Alexis Reigel added 1 commit

    added 1 commit

    • d5ef421c - mysql hack: set length for binary indexes

    Compare with previous version

  • Alexis Reigel added 1 commit

    added 1 commit

    • 281bf832 - mysql hack: set length for binary indexes

    Compare with previous version

  • Alexis Reigel added 1 commit

    added 1 commit

    • 03f46869 - mysql hack: set length for binary indexes

    Compare with previous version

  • @koffeinfrei I tried it locally and it works perfectly! AJAX loading does make a difference! I will take a look at the code once more. Hope we can merge it right after you've done with mysql issue

  • @koffeinfrei code looks good to me. Ping me once mysql is ready and @yorickpeterse comments are addressed

  • Author Contributor

    @dimitrieh I added a new commit where the sha columns are upcased (as they are stored as binary the type case value from the database is lower case). I added this as a getter method override in the models, e.g.

    def gpg_key_primary_keyid
      self[:gpg_key_primary_keyid]&.upcase
    end

    Is this fine? Or should I include this in the ShaAttribute, something like the following:

    sha_attribute :primary_keyid, upcase: true
    
    # OR
    
    upcased_sha_attribute :primary_keyid
  • Alexis Reigel added 5 commits

    added 5 commits

    • 1357a58a - mysql hack: set length for binary indexes
    • 453f773f - no more more :length due to mysql set_index hack
    • ab91d745 - upcase in the model instead of in the view
    • 19bafce4 - extract setter as before_action
    • 3553e774 - use Module#prepend instead of alias_method_chain

    Compare with previous version

  • @koffeinfrei I wouldn't adjust sha_attribute since it shouldn't care about casing. That is, it just stores bytes as-is and converts these to/from strings.

    Is there an actual need to enforce a specific case? Shouldn't Rugged/libgit return commit SHAs in one particular casing?

  • Seems casing doesn't actually matter:

    lower = ['fa5aa0dd6235bb10e3abc69d6e006bdfb151859b'].pack('H*') # this is what sha_attribute basically uses under the hoods
    upper = ['fa5aa0dd6235bb10e3abc69d6e006bdfb151859b'.upcase].pack('H*')
    
    upper == lower # => true
  • Author Contributor

    Is there an actual need to enforce a specific case?

    @yorickpeterse It's just relevant for the view, gpg keys / fingerprints are usually upcased. I didn't want to add .upcase everything scattered in the view, which is why I added it to the model.

  • Author Contributor

    Seems casing doesn't actually matter:

    Exactly, which means it doesn't preserve the original case.

  • @koffeinfrei If it's just for displaying purposes that would belong somewhere in the view, maybe wrapping the code in a presenter of sorts. If it's not a hard requirement I would just leave it as-is.

  • Yorick Peterse
  • Yorick Peterse
  • Yorick Peterse
  • Yorick Peterse
  • Yorick Peterse
  • Yorick Peterse
  • Yorick Peterse
  • Yorick Peterse
  • Alexis Reigel added 1 commit

    added 1 commit

    • 3a46d673 - use Module#prepend instead of alias_method_chain

    Compare with previous version

  • Alexis Reigel added 2 commits

    added 2 commits

    • cbd75e22 - fixup! no more more :length due to mysql set_index hack
    • b11a69fa - fixup! extract setter as before_action

    Compare with previous version

  • Alexis Reigel marked as a Work In Progress from siemens/gitlab-ce@cbd75e22

    marked as a Work In Progress from siemens/gitlab-ce@cbd75e22

  • Alexis Reigel added 5 commits

    added 5 commits

    • 816b4b61 - add unique indexes to gpg_keys
    • 71ae44c9 - simplify nil handling
    • fc9c1470 - fixup! upcase in the model instead of in the view
    • 8d9a8ff1 - update all records at once using update_all
    • 36a675e0 - validate the foreign_key instead of the relation

    Compare with previous version

  • Alexis Reigel added 2 commits

    added 2 commits

    • da2e3a9f - add unique index for gpg_signatures#commit_sha
    • 2f906bb5 - optimize query, only select relevant db columns

    Compare with previous version

  • Alexis Reigel added 1 commit

    added 1 commit

    • 1b7c19c8 - optimize query, only select relevant db columns

    Compare with previous version

  • Alexis Reigel marked the checklist item Conform by the merge request performance guides as completed

    marked the checklist item Conform by the merge request performance guides as completed

  • Alexis Reigel added 2 commits

    added 2 commits

    • 9e035c72 - improve gpg key validation
    • f72bfd49 - remove log statements from workers

    Compare with previous version

  • Author Contributor

    @yorickpeterse Does this look ok to you now?

  • Alexis Reigel added 1 commit

    added 1 commit

    Compare with previous version

  • @yorickpeterse please take a look once more and approve if ok.

  • Yorick Peterse approved this merge request

    approved this merge request

  • Alexis Reigel resolved all discussions

    resolved all discussions

  • Dmytro Zaporozhets (DZ) unmarked as a Work In Progress

    unmarked as a Work In Progress

  • Dmytro Zaporozhets (DZ) changed title from WIP: Gpg signed commits to GPG signed commits

    changed title from WIP: Gpg signed commits to GPG signed commits

  • Dmytro Zaporozhets (DZ) changed the description

    changed the description

  • @koffeinfrei awesome! please update with latest master so I can merge it

  • @koffeinfrei :heart: for this awesome feature!

  • Alexis Reigel added 822 commits

    added 822 commits

    • 7692109d...4eebd8e1 - 727 commits from branch gitlab-org:master
    • 817d9558 - Prototype key verification
    • 28bb5e3d - commit signature with spec
    • fbf1fd1a - add gpg key model
    • 7b7cd6f6 - add emails method to GgpKey
    • ab4120de - only validate gpg_key#fingerprint "internally"
    • 7b4d29f4 - add profile gpg key page to manage gpg keys
    • e34cef0c - extract gpg functionality to lib class
    • 87c0fd34 - add / remove gpg keys to / from system keychain
    • eb77e106 - add second gpg key for specs
    • 597ae6e2 - feature spec for gpg signed commits
    • 5ce61120 - use example gpg key instead of my own
    • 41c96c45 - test with a gpg key with multiple emails
    • 0e3d3d60 - email handling for gpg keys
    • 0668521b - move current keychain methods to namespace
    • f0fe1b9d - gpg email verification
    • c1281982 - notification email on add new gpg key
    • 8bd94a73 - remove gpg from keychain when user's email changes
    • d1101ec0 - use more descriptive variable names
    • 7e13d967 - don't sync to keychain file
    • 3c42d730 - add primary keyid attribute to gpg keys
    • 2f956fae - verify gpg commit using tmp keyring and db query
    • 8236b12d - gpg signature model for gpg verification caching
    • 69e511c4 - cache the gpg commit signature
    • 8c4b6a32 - bail if the commit has no signature
    • 7b616d39 - gpg signature is only valid when key is verified
    • 34810acd - move signature cache read to Gpg::Commit
    • 5d5fd4ba - store gpg_key_primary_keyid for unknown gpg keys
    • 502e31be - memoize verified_signature call
    • d48eb77a - allow updating of gpg signature through gpg commit
    • 24671cd6 - update invalid gpg signatures when key is created
    • e75ab064 - update invalid gpg signatures when email changes
    • d7f42643 - no need for passing parameter
    • 028ecb08 - need to wrap the raw commit in a commit model
    • a01eabc1 - update rugged
    • 9d30a80d - update features specs for gpg commits
    • 9816856d - perform signature update in sidekiq worker
    • 2ea95145 - allow removal of gpg key by nullifying signatures
    • 78b52645 - add gpg commit popover badges
    • ee7468e7 - we need to update the gpg_key as well
    • 7fc69a7e - linkify the whole user badge part, not only avatar
    • afd7582a - extract variable
    • 5013f3a8 - use updated gitlab-test repo for signed commits
    • 4648d001 - popover trigger needs to be defined in js init
    • 9a759c62 - add changelog
    • e9515dff - remove the :gpg rspec tag
    • bd476c1b - use sign_in instead of login_with
    • 28c75fc1 - documentation for gpg signed commits
    • 3b7ac360 - position gpg badge first on commit line
    • ade54803 - add help links to gpg commits / gpg settings
    • a06494bf - no need for html_safe
    • 075dae65 - find_by_id -> find_by(:id, ...)
    • d9fd3709 - use hash instead of 2d array
    • e79e2ae1 - validate presence of user on gpg_key
    • 084cc718 - use after_commit instead of AfterCommitQueue
    • 36c05b31 - convert gpg badge helper methods to partials
    • 4f7ba8f2 - fix memoization
    • a7d2ebe5 - simplify fetching of commit
    • 7f03282f - remove duplicate statement
    • b66e3726 - also update gpg_signatures when gpg_key is null
    • deb474b4 - extract common method
    • 3729c3a7 - use the correct flex classes on the commits list
    • 8ccce9d5 - use existing status-box css class for gpg badge
    • 4c5d4a69 - improve spacing / alignments in gpg popup
    • e63b693f - generate gpg signature on push
    • 71d884ad - don't use assignment in if condition
    • e5c9c714 - add notfound icon (question mark)
    • f1e6a9c8 - use svg icons for gpg popovers
    • 111edaa9 - use lighter gray for unverified gpg signature
    • 027309eb - user may now revoke a gpg key
    • 506836a6 - unify commit signature colors with pipeline status
    • cd01e828 - store gpg user name and email on the signature
    • c5271833 - show gpg key's user info when no profile exists
    • ccf3ed43 - swap user's name and the user's username
    • a03a6ff3 - display gpg key in the popover with monospace font
    • 312dc89a - nicer email badges on the profile gpg page
    • 8c8a9e6d - merge migrations to 1 single create per table
    • 8e0c33ed - use ShaAttribute for gpg table columns
    • 895efdfb - use text instead of string for db columns
    • 786b5a59 - use short project path helpers
    • 57ccff8e - use db's on_delete instead of has_many :dependent
    • c4c44c6a - length constrain on the index, not on the column
    • eda00156 - fetch gpg signature badges by ajax
    • ce4e0837 - mysql hack: set length for binary indexes
    • f86580c0 - no more more :length due to mysql set_index hack
    • 98531fc2 - upcase in the model instead of in the view
    • ecbc11a8 - extract setter as before_action
    • 07dbd564 - use Module#prepend instead of alias_method_chain
    • 14551424 - add unique indexes to gpg_keys
    • 843b1de0 - simplify nil handling
    • a5f04df8 - update all records at once using update_all
    • fef030c2 - validate the foreign_key instead of the relation
    • 4e53131f - add unique index for gpg_signatures#commit_sha
    • 9488b778 - optimize query, only select relevant db columns
    • f1ccecc9 - improve gpg key validation
    • 7f7e93a3 - remove log statements from workers

    Compare with previous version

  • Author Contributor

    @dzaporozhets Rebased.

    I just noticed that "GPG Keys" is missing from the new navigation. I added this in 5ebccab1.

  • Alexis Reigel added 1 commit

    added 1 commit

    • 5ebccab1 - add "GPG Keys" to new navigation

    Compare with previous version

  • Dmytro Zaporozhets (DZ) approved this merge request

    approved this merge request

  • Dmytro Zaporozhets (DZ) enabled an automatic merge when the pipeline for 5ebccab1 succeeds

    enabled an automatic merge when the pipeline for 5ebccab1 succeeds

  • Author Contributor

    @koffeinfrei :heart: for this awesome feature!

    @phenomax Thanks! :smiley_cat:

  • @koffeinfrei Thanks for responding to all the picky database comments! :thumbsup:

  • mentioned in commit ac0cbe69

  • Finally, we got it merged! It was a long journey. Thanks to everyone involved!

    And special thank to @koffeinfrei who had passion and patience to build it

    :rocket: :rocket: :rocket:

  • Thank you all for making this happen! I'm so happy to have you @koffeinfrei on board @siemens to contribute such awesome features to GitLab!

  • 1 export default class GpgBadges {
    2 static fetch() {
    3 const form = $('.commits-search-form');
    4
    5 $.get({
    6 url: form.data('signatures-path'),
    7 data: form.serialize(),
    8 }).done((response) => {
    9 const badges = $('.js-loading-gpg-badge');
    10 response.signatures.forEach((signature) => {
    11 badges.filter(`[data-commit-sha="${signature.commit_sha}"]`).replaceWith(signature.html);
  • This looks like a super cool feature. I am trying to figure out why there was not any FE reviews on this. There are a lot of Frontend changes here and a lot of FE decisions that were made.

  • Author Contributor

    What a ride! :sweat_smile:

    Thank you all for your support!

  • Author Contributor

    I'm so happy to have you @koffeinfrei on board @siemens to contribute such awesome features to GitLab!

    @bufferoverflow My pleasure! Thank you for the opportunity!

  • Dmytro Zaporozhets (DZ) marked the checklist item Added for this feature/bug as completed

    marked the checklist item Added for this feature/bug as completed

  • Dmytro Zaporozhets (DZ) marked the checklist item All builds are passing as completed

    marked the checklist item All builds are passing as completed

  • mentioned in issue #35668 (moved)

  • I just heard this news and came to here. @koffeinfrei you're rock :metal: l:

  • As per everyone else, just dropped in to say thanks to @koffeinfrei - this is certainly a great feature to have.

  • This MR seems to be the source of some transient build failures seen in master. @koffeinfrei any chance you could take a look into this?

    See: https://gitlab.com/gitlab-org/gitlab-ce/issues/35986

      1) Commits GPG signed commits changes from unverified to verified when the user adds the missing gpg key
         Failure/Error: expect(page).to have_content 'Verified'
           expected to find text "Verified" in "26 Jun, 2017 3 commits Merge branch 'add-gpg-commits' into 'signed-commits' ... Dmitriy Zaporozhets committed a month ago 5d4a1cba Browse Files signed commit by bette cartwright Alexis Reigel committed a month ago Unverified 8a852d50 Browse Files signed commit by nannie bernhard Alexis Reigel committed a month ago Unverified 0f44cd1d Browse Files 11 Apr, 2017 2 commits Merge branch 'gitlab-test-usage-dev-testing-docs' into 'master' ... Sean McGivern committed 3 months ago e63f41fe Browse Files Update README.md to include `Usage in testing and development` Luke \"Jared\" Bennett committed 3 months ago 4a24d82d Browse Files 27 Sep, 2016 1 commit Merge branch 'branch-merged' into 'master' ... Job van der Voort committed 10 months ago b83d6e39 Browse Files 21 Sep, 2016 1 commit adds bar folder and branch-test text file to check Repository merged_to_root_ref method tiagonbotelho committed 10 months ago 498214de Browse Files 18 Aug, 2016 2 commits Merge branch 'add-directory-with-space' into 'master' ... Stan Hu committed 11 months ago 1b12f15a Browse Files Add a directory containing a space in its name Winnie committed 11 months ago 38008cb1 Browse Files 30 Jun, 2016 3 commits Merge branch 'rename-file' into 'master' ... Douwe Maan committed a year ago 6907208d Browse Files Restore commit.js.coffee as commit.coffee Douwe Maan committed a year ago c347ca2e Browse Files Delete commit.js.coffee Douwe Maan committed a year ago d59c6002 Browse Files 11 Jun, 2016 1 commit Merge branch 'feature.custom-highlighting' into 'master' ... Stan Hu committed a year ago 281d3a76 Browse Files 10 Jun, 2016 1 commit add tests for .gitattributes custom highlighting http://jneen.net/ committed a year ago a5391128 Browse Files 20 Jan, 2016 1 commit Add plain text README Douglas Barbosa Alexandre committed a year ago 54fcc214 Browse Files 07 Dec, 2015 2 commits Merge branch 'master' into 'master' ... Marin Jankovski committed a year ago be936876 Browse Files LFS object pointer. Marin Jankovski committed a year ago 048721d9 Browse Files 13 Nov, 2015 9 commits GitLab currently doesn't support patches that involve a merge commit: add a commit here Stan Hu committed a year ago 5f923865 Browse Files Merge branch 'add-svg' into 'master' ... Stan Hu committed a year ago d2d43067 Browse Files Add GitLab SVG Stan Hu committed a year ago 2ea1f3de Browse Files Merge branch 'whitespace' into 'master' ... Stan Hu committed a year ago 59e29889 Browse Files add spaces in whitespace file 윤민식 committed a year ago 66eceea0 Browse Files remove emtpy file.(beacase git ignore empty file) ... 윤민식 committed a year ago 08f22f25 Browse Files Merge branch 'whitespace' into 'master' ... Stan Hu committed a year ago 19e2e9b4 Browse Files add whitespace in empty 윤민식 committed a year ago c642fe9b Browse Files add empty file 윤민식 committed a year ago 9a944d90 Browse Files 25 Aug, 2015 1 commit Add ISO-8859 test file Stan Hu committed a year ago c7fbe50c Browse Files 10 Jan, 2015 2 commits Merge branch 'tree_helper_spec' into 'master' ... Sytse Sijbrandij committed 2 years ago e56497bb Browse Files add directory structure for tree_helper spec marmis85 committed 2 years ago 4cd80cca Browse Files 27 Feb, 2014 11 commits Add submodule from gitlab.com ... Dmitriy Zaporozhets committed 3 years ago Unverified 5937ac0a Browse Files Change some files ... Dmitriy Zaporozhets committed 3 years ago Unverified 570e7b2a Browse Files More submodules ... Dmitriy Zaporozhets committed 3 years ago Unverified 6f6d7e7e Browse Files Remove ds_store files ... Dmitriy Zaporozhets committed 3 years ago Unverified d14d6c0a Browse Files Ignore DS files ... Dmitriy Zaporozhets committed 3 years ago Unverified c1acaa58 Browse Files Binary file added ... Dmitriy Zaporozhets committed 3 years ago Unverified ae73cb07 Browse Files Ruby files modified ... Dmitriy Zaporozhets committed 3 years ago Unverified 874797c3 Browse Files Modified image ... Dmitriy Zaporozhets committed 3 years ago Unverified 2f63565e Browse Files Image added ... Dmitriy Zaporozhets committed 3 years ago Unverified 33f3729a Browse Files Files, encoding and much more ... Dmitriy Zaporozhets committed 3 years ago Unverified 913c66a3 Browse Files Add submodule ... Dmitriy Zaporozhets committed 3 years ago Unverified cfe32cf6 Browse Files 26 Jun, 2017 3 commits Merge branch 'add-gpg-commits' into 'signed-commits' ... Dmitriy Zaporozhets committed a month ago 5d4a1cba Browse Files signed commit by bette cartwright Alexis Reigel committed a month ago Unverified 8a852d50 Browse Files signed commit by nannie bernhard Alexis Reigel committed a month ago Unverified 0f44cd1d Browse Files 11 Apr, 2017 2 commits Merge branch 'gitlab-test-usage-dev-testing-docs' into 'master' ... Sean McGivern committed 3 months ago e63f41fe Browse Files Update README.md to include `Usage in testing and development` Luke \"Jared\" Bennett committed 3 months ago 4a24d82d Browse Files 27 Sep, 2016 1 commit Merge branch 'branch-merged' into 'master' ... Job van der Voort committed 10 months ago b83d6e39 Browse Files 21 Sep, 2016 1 commit adds bar folder and branch-test text file to check Repository merged_to_root_ref method tiagonbotelho committed 10 months ago 498214de Browse Files 18 Aug, 2016 2 commits Merge branch 'add-directory-with-space' into 'master' ... Stan Hu committed 11 months ago 1b12f15a Browse Files Add a directory containing a space in its name Winnie committed 11 months ago 38008cb1 Browse Files 30 Jun, 2016 3 commits Merge branch 'rename-file' into 'master' ... Douwe Maan committed a year ago 6907208d Browse Files Restore commit.js.coffee as commit.coffee Douwe Maan committed a year ago c347ca2e Browse Files Delete commit.js.coffee Douwe Maan committed a year ago d59c6002 Browse Files 11 Jun, 2016 1 commit Merge branch 'feature.custom-highlighting' into 'master' ... Stan Hu committed a year ago 281d3a76 Browse Files 10 Jun, 2016 1 commit add tests for .gitattributes custom highlighting http://jneen.net/ committed a year ago a5391128 Browse Files 20 Jan, 2016 1 commit Add plain text README Douglas Barbosa Alexandre committed a year ago 54fcc214 Browse Files 07 Dec, 2015 2 commits Merge branch 'master' into 'master' ... Marin Jankovski committed a year ago be936876 Browse Files LFS object pointer. Marin Jankovski committed a year ago 048721d9 Browse Files 13 Nov, 2015 3 commits GitLab currently doesn't support patches that involve a merge commit: add a commit here Stan Hu committed a year ago 5f923865 Browse Files Merge branch 'add-svg' into 'master' ... Stan Hu committed a year ago d2d43067 Browse Files Add GitLab SVG Stan Hu committed a year ago 2ea1f3de Browse Files"
         # ./spec/features/commits_spec.rb:261:in `block (4 levels) in <top (required)>'
         # ./spec/features/commits_spec.rb:259:in `block (3 levels) in <top (required)>'
         # ./spec/spec_helper.rb:102:in `block (2 levels) in <top (required)>'
    
    Finished in 45 minutes 2 seconds (files took 49.13 seconds to load)
    991 examples, 1 failure
    
    Failed examples:
    
    rspec ./spec/features/commits_spec.rb:239 # Commits GPG signed commits changes from unverified to verified when the user adds the missing gpg key
    
  • mentioned in issue #35986 (closed)

  • mentioned in issue #36046 (closed)

  • mentioned in issue #36047 (closed)

  • @koffeinfrei Should a commit be considered verified if the commit author email doesn’t actually match any of the emails in the signature? The signature is valid, sure, but the commit itself is not verified to be by the author of the commit. I could sign with my own key and use your email and name, and people would believe the commit is yours…

    On GitHub it shows like this:

    Screen_Shot_2017-08-15_at_13.38.13

    Note that even if one of the emails in the signature matches the commit email, that email may not actually be verified: it may not be the main GitLab account address. I think we need to verify that commit author email == verified user email == one of the emails in the signature. If any of those 3 don't match up, we should show Unverified.

    Edited by Douwe Maan
  • @DouweM I think there is more value in the gpg key being correct, than the email. I think it needs a intermediate state, like GH does. Its not verified, but its also not unverified.. at least not in the extended popup

  • @dimitrieh I don't think there's any value in the GPG key being correct, by itself. It only means that the person who created the commit has a GitLab account (any GitLab account) which in it itself isn't very interesting.

    Signing a commit only becomes valuable when the signature email and the author email (and GitLab email) match, because then we have verified that the commit was actually created by the email/user that we attribute it to, i.e. it's not some random guy trying to get a commit into linux by claiming it was created by Linus.

    I'd say GPG key correctness is worth nothing without email correctness.

    An intermediate state makes sense, but only in the popup. In the badge, we should treat this situation as Unverified, like GH does.

    Edited by Douwe Maan
  • Author Contributor

    @DouweM /cc @dimitrieh

    Should a commit be considered verified if the commit author email doesn’t actually match any of the emails in the signature?

    I don't believe that the intention of the GPG verification is that the committer's email must match the GPG key's email.

    Without GitLab a GPG key just needs to be valid (i.e not expired, not revoked).

    In the context of GitLab, the only thing that needs to match is the key's email and the GitLab user account's email.

    I think we need to verify that commit author email == verified user email == one of the emails in the signature. If any of those 3 don't match up, we should show Unverified.

    We need to make sure that the person that signed the commit actually owns the GPG key. To verify this we don't need all 3 matches, but only verified user email == one of the emails in the signature.

    Signing a commit only becomes valuable when the signature email and the author email (and GitLab email) match, because then we have verified that the commit was actually created by the email/user that we attribute it to

    Which is how it's currently implemented.

    Edited by Alexis Reigel
  • In the context of GitLab, the only thing that needs to match is the key's email and the GitLab user account's email.

    We need to make sure that the person that signed the commit actually owns the GPG key. To verify this we don't need all 3 matches, but only verified user email == one of the emails in the signature.

    @koffeinfrei Is that true? What if I create a GitLab account with douwe@gitlab.com, create a signature with douwe@gitlab.com and alexis@example.com, and create a commit with author Alexis Reigel <alexis@example.com>? Following your logic I think that would show as Verified, but I'd argue that that's incorrect/misleading since Alexis didn't actually create that commit.

    Which is how it's currently implemented.

    Are you sure? I've tested this with email1 as my user email, a signature with email1 and email2, and a commit with email3. I see "Verified" on GitLab, where GitHub shows "Unverified—The email in this signature doesn't match the committer email"

    Edited by Douwe Maan
  • @koffeinfrei It looks like GitHub will only regard signed commits verified if the commit email is in the signature, that email is added to the user account as a secondary email, and that email is verified, what we currently don't allow for secondary emails.

    Screen_Shot_2017-08-15_at_16.03.58

  • Douwe Maan mentioned in merge request !13561 (merged)

    mentioned in merge request !13561 (merged)

  • I think we need to verify that commit author email == verified user email == one of the emails in the signature

    @koffeinfrei I tend to agree with @DouweM on that. Since we don't check every email in the signature is verified - we better add extra check with commit author. As a user when I see "Verified" status I want to be sure it is legit as much as possible.

  • Author Contributor

    @DouweM

    It looks like GitHub will only regard signed commits verified if the commit email is in the signature

    From https://help.github.com/articles/using-a-verified-email-address-in-your-gpg-key:

    When verifying a signature, GitHub checks that the committer or tagger email address matches an email address from the GPG key's identities and is a verified email address on the user's account.

    After some thinking and discussing I agree that it's misleading when the committer is a different user than the user that created the signature. Without a net of trust for GPG we should probably make sure that they match.

    This changes the semantics of the signature though. The signature on GitLab (and GitHub) doesn't mean that the commit is created by a trusted user, but that it is actually signed and committed by the same user.

    Edited by Alexis Reigel
  • What about showing the status as 'signed by username' if the committer's email address is not contained in the key?

  • This changes the semantics of the signature though. The signature on GitLab (and GitHub) doesn't mean that the commit is created by a trusted user, but that it is actually signed and committed by the same user.

    @koffeinfrei Not "trusted user" in the web of trust sense, but if you consider GitLab users trusted users (which may make more sense on a private instance than on a public one), verifying that the commit was actually authored by the GitLab user that's linked to the commit servers the same purpose. On a public instance like GitLab.com, it can be used to verify that a commit was actually authored by a maintainer of the (open source) project you're looking at.

  • @znixian Just like we show "Authored by X; Committed by Y"? I like that idea!

    Edited by Douwe Maan
  • @koffeinfrei Are the few tweaks discussed here something that you would like to work on, or would you prefer someone at GitLab take it on?

  • @DouweM I haven't seen how GitLab displays commits with seperate authers and committors, but that sounds about right.

  • https://gitlab.com/gitlab-org/gitlab-ce/issues/4232#note_37755360

    500 error when trying to add a GPG key to my profile.

  • @junderw The fix for the 500 error will be included in the next Release Candidate to be deployed to GitLab.com. Please see the Merge Request for the fix here:

  • Author Contributor

    @DouweM

    Are the few tweaks discussed here something that you would like to work on, or would you prefer someone at GitLab take it on?

    I can work on it.

    What do you think should be done exactly?

    1. Only change the "verified" logic to match the committer's email as well
    2. Add one popover variation for the unverified state.
    • Signature is invalid:
      • Badge: "Unverified"
      • Popover: "This commit was signed with an unverified signature."
    • Signature is valid:
      • Badge: "Verified"
      • Popover: "This commit was signed with a verified signature." + linked user
    • Verified signature, different user (commiter ≠ gpg key uid)
      • Badge: "Unverified"
      • Popover: "This commit was signed by a different user's verified signature." Maybe for this we could link to the user.
    1. Add one popover variation for the unverified state and add one badge variation
    • Verified signature, different user (commiter ≠ gpg key uid)
      • Badge: "Verified by someone else", possibly with a different color
      • Popover: "This commit was signed by a different user's verified signature." Maybe for this we could link to the user.

    I'd go for option 1 and just modify the "verified" logic for now. I'm not sure if it's helpful to the user to know that a signature is verified but by someone else. Ultimately the user wants to know whether a commit is legit or not.

  • @koffeinfrei I'd go with 1 and 2: modify the logic and add a new popover state. We can leave the badge "Unverified", so no option 3 (yet).

    Note that we have some sub options of "signature is verified, but...", that would likely need different phrasings. In pseudo code:

    if gpg_key && gpg_key.verified? && verified_signature.valid?
      if gpg_key.user.verified_email?(committer_email) # basically just `committer_email == gpg_key.user.email`
        [true, "This commit was signed with a verified signature and the committer email is verified to belong to the same user."]
      else
        committer_user = User.find_by_any_email(committer_email)  
    
        if committer_user && committer_user != gpg_key.user
          [false, "This commit was signed with a different user's verified signature."]
        else
          [false, "This commit was signed with a verified signature, but the committer email is not verified to belong to the same user."]
        end
      end
    else
      [false, "This commit was signed with an unverified signature."]
    end
  • @koffeinfrei just to confirm, verified tags are not supported yet, right?

  • mentioned in merge request !13660 (merged)

  • Author Contributor

    just to confirm, verified tags are not supported yet, right?

    @axil No they're not.

    also what algorithms are supported?

    I haven't tested this, but any algorithm supported by GPG should be supported by GitLab, i.e. any key that can be added to GPG can be added to GitLab.

  • Author Contributor

    @DouweM I'm not sure about the distinction between This commit was signed with a different user's verified signature. and This commit was signed with a verified signature, but the committer email is not verified to belong to the same user.. Is the distinction that the user exists on GitLab in the former case?

    I'd see it as follows:

    signature valid && user == key && user == committer -> valid
    signature valid && user == key && user != committer -> invalid, different user
    signature valid && user != key -> invalid, unknown signature
    signature invalid -> invalid, invalid signature

    EDIT

    signature valid && key && user == key && user == committer -> valid
    signature valid && key && user == key && user != committer -> invalid, different user
    signature valid && key && user != key -> invalid, unverified key
    signature valid && !key -> invalid, unknown key
    signature invalid -> invalid, invalid signature

    key present && user == key && user == committer  -> valid
    key present && user == key && user != committer  -> invalid, different user
    key present && user != key                       -> invalid, unverified key
    key not present                                  -> invalid, unknown key
    else                                             -> invalid, invalid signature
    Edited by Alexis Reigel
  • mentioned in issue #36231 (closed)

  • Please make sure that it's should ignore the lowercase and uppercase of the signed email, that's to say, they are the same one. Because I use the uppercase email to generate my GPG key, but GitLab just use the lowercase emails in profile settings.

  • @koffeinfrei The distinction is that in the This commit was signed with a verified signature, but the committer email is not verified to belong to the same user. case, the commit email could be one of the signing user's secondary emails, and the commit will have that user's avatar and will link to that user's profile. I think it's worth pointing out that in this case it's not a "different user's signature", but rather the same user's signature, but with an unverified email on the commit. It would be confusing if we say "different user", but still link to the same user in both the PGP popup and the commit author link.

    Edited by Douwe Maan
  • Author Contributor

    I think it's worth pointing out that in this case it's not a "different user's signature", but rather the same user's signature, but with an unverified email on the commit.

    @DouweM Ok. Should we leave this out for now because of https://gitlab.com/gitlab-org/gitlab-ce/issues/28621?

    EDIT

    Actually I think it's fine to add this, since it's not problematic from a security POV. The signature stays invalid, it's just an additional info.

    Edited by Alexis Reigel
  • mentioned in issue #36258 (closed)

  • mentioned in issue #42493 (closed)

  • mentioned in issue #36880 (moved)

  • mentioned in issue #43394 (closed)

  • mentioned in issue #53770 (moved)

  • Diego Louzán mentioned in merge request gitlab!17773 (merged)

    mentioned in merge request gitlab!17773 (merged)

  • Please register or sign in to reply
    Loading