GPG signed commits
What does this MR do?
- Shows gpg signed commits (excl. tags)
- 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
- If the gpg key is not verified or does not exist on GitLab an "Unverified" batch is displayed
- If the commit is not signed, the behaviour is unchanged
- 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:
Commit details:
Badge popovers:
User settings > GPG key:
Does this MR meet the acceptance criteria?
-
Changelog entry added -
Documentation created/updated -
API support added - Tests
-
Added for this feature/bug -
All builds are passing
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Branch has no merge conflicts with master
(if it does - rebase it please) -
Squashed related commits together
What are the relevant issue numbers?
Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/20268.
Merge request reports
Activity
marked the task Conform by the style guides as completed
Could someone UX please provide me with some suggestions / hints on how to style the badges in a nice way?
added 444 commits
-
cdc1c2ae...90040620 - 428 commits from branch
gitlab-org:master
- a6423a46 - Prototype key verification
- 56cff54b - remove commit actions width constraint
- 87fbd7c3 - commit signature with spec
- 6c16eccc - add gpg key model
- 73723fb0 - add emails method to GgpKey
- 0e8914c7 - only validate gpg_key#fingerprint "internally"
- a003d210 - add profile gpg key page to manage gpg keys
- 12a8e8f7 - extract gpg functionality to lib class
- ee271ed1 - add / remove gpg keys to / from system keychain
- b6ca048e - add second gpg key for specs
- 16067684 - feature spec for gpg signed commits
- 24224930 - use example gpg key instead of my own
- 1248a6c5 - test with a gpg key with multiple emails
- 59e4503e - email handling for gpg keys
- 5bb73924 - move current keychain methods to namespace
- 97da3d87 - gpg email verification
Toggle commit list-
cdc1c2ae...90040620 - 428 commits from branch
marked the task Squashed related commits together as completed
added 6 commits
Toggle commit listThanks @koffeinfrei! I'm trying to find the right person to review this. One question:
Currently the gpg keychain is stored in the rails process user's home directory. This means that the gpg verification works with all added keys within the same gitlab instance.
What happens if you have multiple servers running the Rails app, like we do on GitLab.com?
What happens if you have multiple servers running the Rails app, like we do on GitLab.com?
That's currently not working properly yet. I guess that
SystemHooksService
is the place for such behaviour isn't it? At least that's what theKey
(i.e. ssh key) interacts with when adding and removing keys. Could you give me any pointers on how this works exactly?added 14 commits
- 871d66b9 - add gpg key model
- 066d163d - add emails method to GgpKey
- 3f8ddf54 - only validate gpg_key#fingerprint "internally"
- 7911eb05 - add profile gpg key page to manage gpg keys
- ff6c72d9 - extract gpg functionality to lib class
- 271764dc - add / remove gpg keys to / from system keychain
- 98544b9a - add second gpg key for specs
- debaa96f - feature spec for gpg signed commits
- 9cfaea06 - use example gpg key instead of my own
- 7c47251c - test with a gpg key with multiple emails
- 3f1396ed - email handling for gpg keys
- c3af7b91 - move current keychain methods to namespace
- 79a0e67d - gpg email verification
- 63b5b654 - notification email on add new gpg key
Toggle commit list@koffeinfrei sorry about the slow progress on this one.
The file itself can go in shared storage: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/shared_files.md
My understanding (after getting a far more knowledgeable colleague to explain things to me) is that this keychain won't be written to often, once it has the most common intermediaries. Is that right?
At least that's what the
Key
(i.e. ssh key) interacts with when adding and removing keys. Could you give me any pointers on how this works exactly?This system hook is used for integrations and also for GitLab Geo. It would be nice to add this hook, but I don't think it's required for Geo in this case.
This looks very nice, but I think that we should not use KeyChain.
We should parse signature in order to figure out the key fingerprint that we would look in the database.
The key verification to some extent should be cached, as this is a time-consuming operation.
We should be careful about date validity.
- Resolved by Alexis Reigel
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
Toggle commit list- 806d3385...348dff0a - 437 commits from branch
[...] 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.
[...] 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
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.
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
@koffeinfrei I added some lower-level CR comments. I'll let @ayufan advise on the overall direction
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..?
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:
- Read signature of the commit,
- Decode signature and figure out the fingerprint or another identifier of the key,
- Get public part of the key from DB by doing fingerprint lookup,
- 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ńskiI'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 needramfs
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?
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 needramfs
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:- Read all the relevant keys from the database and store them to a temporary keychain in
ramfs
OR
- 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?
- Read all the relevant keys from the database and store them to a temporary keychain in
@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 MazettoInstead 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)assigned to @dzaporozhets
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.
mentioned in issue #20268 (closed)
@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:
- The user adds public key -> we save fingerprint to db.
- We read signed commit and extract signature and signed text using rugged.
- 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 use a fingerprint to find the public key in the database (single sql query).
- We import public key to the keychain and do a verification (I believe we can't just trust fingerprint match?).
- 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
- 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?
@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
.\1. The user adds public key -> we save fingerprint to db.
we do that already.\2. We read signed commit and extract signature and signed text using rugged.
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?
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@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.
mentioned in issue gitlab-com/www-gitlab-com#1458 (closed)
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
@koffeinfrei what do you think about ^ ?
@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 => ""
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?
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
Toggle commit list-
c4ed04c1...9cb3510a - 6923 commits from branch
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:
- Add caching to this merge request so commits page is performant.
- 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?
\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
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
Toggle commit list-
25e02c50...9cc126ea - 83 commits from branch
@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)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).
@koffeinfrei awesome, please tell me if you need help or when code is ready for review.
changed milestone to %9.4
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
Toggle commit list@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-
cache key (gpg signature) invalidation:
- Resolved by Alexis Reigel
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
Toggle commit listadded 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
Toggle commit listadded 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
Toggle commit listadded 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
Toggle commit listIs 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
- when a commit is verified and the key is expired, the signature will be invalid.
- 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.
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 :)
- Resolved by Dmytro Zaporozhets (DZ)
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
mentioned in merge request !9646 (merged)
@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.
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 MazettoHow 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.
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 MazettoOk 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.
added 1 commit
- 83e93e38 - linkify the whole user badge part, not only avatar
mentioned in merge request gitlab-test!23 (merged)
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
Toggle commit list-
6134f14d...ad3843ae - 790 commits from branch
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 Changelog entry added as completed
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 incomplete
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
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
Toggle commit list-
d83acb1d...a2e13104 - 140 commits from branch
@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.
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
Toggle commit listCould 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:
As for the commit view titlebar, can we position the label like this:
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:
Is "warning" color in place here? or just the white/grey enough? cc: @dzaporozhets
@dimitrieh gray is enough. Reasons are https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9546#note_33861295
@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?
@dzaporozhets Screenshots updated (/cc @dimitrieh)
marked the checklist item Documentation created/updated as completed
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
Toggle commit list-
d5496ac7...b071317c - 238 commits from branch
@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?
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
Toggle commit list- Resolved by Dmytro Zaporozhets (DZ)
@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 inGitPushService
. This way signature verified in async job before user opens the pageEdited by Dmytro Zaporozhets (DZ)@koffeinfrei btw great work done here. Can't wait to see this feature shipped
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 inGitPushService
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.
- Resolved by Alexis Reigel
@dzaporozhets @koffeinfrei @DouweM can we make these changes still:
- Make the badge, if verified, also have a green border:
- 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
-
- 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
(@lbennett correct?
)
-
- 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:
Edited by Dimitrie Hoekstra- Make the badge, if verified, also have a green border:
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- app/helpers/badges_helper.rb 0 → 100644
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'}" The reason for this is that multiple emails in a key may have different verification statuses.
@dimitrieh Suggestions on how to make this look nicer?
@koffeinfrei what function does this section have exactly? it should show:
- gpg key
- email connected to gpg key
- creation date
Why does it show a green verified badge and white verified badge at the same time?, including some of the info, but not all?
Can we make this different from the ssh section. So:
- Without the key icon
- all info on 1 line
- no badge markup
- just a real simple table like:
Can we make this different from the ssh section.
How does this behave responsively? This won't work that great on smaller devices. Is there something similar somewhere already?
Why does it show a green verified badge and white verified badge at the same time?
A GPG key may have multiple email addresses (multiple UUIDs). The verification status shows that the email address is linked to an email address that the user owns (currently only the primary email).
In order for a signature to be valid, at least one of the GPG key's UUIDs needs to be verified.
ah!! yes in pipeline schedules https://gitlab.com/gitlab-org/gitlab-ce/pipeline_schedules
As for mobile support, we can do something similar to https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12171 wdyt?
in that case you can list the emails like so:
user@example.com(verified), user2@example.com(unverified)
@dimitrieh / @DouweM / @dzaporozhets I'd like to keep the current design, as it matches the one used for the ssh keys section. I don't think we should introduce a new design in this MR as we'll end up with inconsistent UIs (ssh keys vs. gpg keys). IMO it would be more appropriate to create a separate MR to update all the UIs with the newly suggested design.
What do you think about either one of the following changes regarding the emails?
@koffeinfrei @dimitrieh honestly I think both SSH and GPG keys should be one page. But I believe it's a question for separate merge request. Let's keep this one as small as possible. It looks like SSH keys page - fine to me. Its community contribution in the end.
What do you think about either one of the following changes regarding the emails?
Looks much better to me
Edited by Dmytro Zaporozhets (DZ)@koffeinfrei agreed here with @dzaporozhets
i like the lower one, as it has a clearer distinction between gpg key and emails
changed this line in version 47 of the diff
@dimitrieh / @dzaporozhets / @DouweM Updated the design according to the second screenshot above.
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Douwe Maan
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
@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.
- Resolved by Alexis Reigel
mentioned in issue #30495 (closed)
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.@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 Reigeladded 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
Toggle commit list@koffeinfrei oops sorry! updating in a sec with improved mockups for thos
@koffeinfrei @dzaporozhets updated mockups compared to previous one, now including avatar and name/username references
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
assigned to @dimitrieh
@koffeinfrei I'll wait for the adjustments presented at https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9546#note_34186656
Let me know if you need any help!
assigned to @koffeinfrei
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
Toggle commit listthe following icons should be available already in the codebase
@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 ReigelMake 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.
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
Toggle commit listadded 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
Toggle commit list@DouweM / @dzaporozhets I created a worker that generates the signatures on push (added to
GitPushService
).- Resolved by Alexis Reigel
@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?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?
added 6 commits
Toggle commit list@koffeinfrei nice!
small detail: why is there a white border on the grey area?:
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.
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
Toggle commit listadded 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
Toggle commit listadded 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
Toggle commit list@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?
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...
- signed commits that were pushed before this feature gets shipped and...
- 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.
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
Toggle commit listsmall 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.
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
Toggle commit list-
f7383551...6658c706 - 934 commits from branch
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
Toggle commit list- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
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 HoekstraAs 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.
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
):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:
- commits a user look at
- 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 ^ ?
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
Toggle commit listimagine 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...
Edited by Alexis Reigel@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...
@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?
mentioned in issue #35378 (closed)
@phenomax cool, I made an issue - https://gitlab.com/gitlab-org/gitlab-ce/issues/35378
@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
added 2 commits
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
@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
@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
added 5 commits
Toggle commit list@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?
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.@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.
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Yorick Peterse
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Yorick Peterse
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
- Resolved by Alexis Reigel
added 1 commit
- 3a46d673 - use Module#prepend instead of alias_method_chain
marked as a Work In Progress from siemens/gitlab-ce@cbd75e22
added 1 commit
- 1b7c19c8 - optimize query, only select relevant db columns
marked the checklist item Conform by the merge request performance guides as completed
added 2 commits
@yorickpeterse Does this look ok to you now?
@yorickpeterse please take a look once more and approve if ok.
assigned to @yorickpeterse
assigned to @dzaporozhets
@koffeinfrei awesome! please update with latest master so I can merge it
@koffeinfrei
for this awesome feature!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
Toggle commit list-
7692109d...4eebd8e1 - 727 commits from branch
@dzaporozhets Rebased.
I just noticed that "GPG Keys" is missing from the new navigation. I added this in 5ebccab1.
enabled an automatic merge when the pipeline for 5ebccab1 succeeds
@koffeinfrei
for this awesome feature!@phenomax Thanks!
Awesome work @koffeinfrei @yorickpeterse @dzaporozhets!
@koffeinfrei Thanks for responding to all the picky database comments!
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
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!
- app/assets/javascripts/gpg_badges.js 0 → 100644
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); @jschatz1 What do you mean exactly? The returned JSON contains the html for each signature.
render json: { signatures: @commits.select(&:has_signature?).map do |commit| { commit_sha: commit.sha, html: view_to_html_string('projects/commit/_signature', signature: commit.signature) } end }
Hey @koffeinfrei! I mean inserting HTML into our DOM via an AJAX call. It makes it hard to maintain. We need to go into rails code to change HTML. I will talk to @dzaporozhets about this.
Yes, we should have asked someone from FE team to review this.
@jschatz1 will create issues for further FE improvements. I believe those can be done by FE team.
Thanks @dzaporozhets
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!
mentioned in issue #35668 (moved)
I just heard this news and came to here. @koffeinfrei you're rock
l:As per everyone else, just dropped in to say thanks to @koffeinfrei - this is certainly a great feature to have.
@koffeinfrei thank you!
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:
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 showUnverified
.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@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 showUnverified
.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 ReigelIn 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 withdouwe@gitlab.com
andalexis@example.com
, and create a commit with authorAlexis 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 withemail1
andemail2
, and a commit withemail3
. 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.
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.
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 ReigelThis 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:
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?
- Only change the "verified" logic to match the committer's email as well
- 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.
- 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)
@koffeinfrei also what algorithms are supported? GitHub has those documented https://help.github.com/articles/generating-a-new-gpg-key/#supported-gpg-key-algorithms.
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.
@koffeinfrei Let me know if you need any help on https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9546#note_37803138!
@DouweM I'm not sure about the distinction between
This commit was signed with a different user's verified signature.
andThis 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 signatureEDIT
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 signaturekey 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 Reigelmentioned in issue #36231 (closed)
@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 MaanI 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 ReigelFYI This MR takes 15 seconds to load http://207.154.197.115/gl/sitespeed-result/gitlab.com/2017-10-17-09-19-41/pages.html This is being worked on in https://gitlab.com/gitlab-org/gitlab-ce/issues/36258
mentioned in issue #36258 (closed)
mentioned in issue gitlab-com/blog-posts#368 (closed)
mentioned in issue #42493 (closed)
mentioned in issue #36880 (moved)
mentioned in issue #43394 (closed)
mentioned in issue #53770 (moved)
mentioned in merge request gitlab-com/www-gitlab-com!16726 (merged)
mentioned in merge request gitlab!17773 (merged)
mentioned in issue gitlab-com/www-gitlab-com#7665 (closed)
mentioned in issue gitlab-com/www-gitlab-com#9384 (closed)
mentioned in issue gitlab-com/www-gitlab-com#9525 (closed)
mentioned in issue gitlab-com/www-gitlab-com#9629 (closed)
mentioned in issue gitlab-com/www-gitlab-com#9664 (closed)
mentioned in issue gitlab#327128 (closed)
mentioned in issue gitlab-com/www-gitlab-com#11448 (closed)
mentioned in issue ux-research#1473 (closed)
mentioned in issue gitlab#290745 (closed)
mentioned in epic gitlab-com&1426 (closed)