GPG commit validation #1150

sapk · 2017-03-09T00:25:38Z

Display signed commit and validation against gpg key in database. I have started with a very little UI with the a simple title attr indicating the reason.

Examples:

For comparison : https://github.com/sapk/testing-git/commits/master

Need and follow : #710 ✅ go-gitea/git#36 ✅

TODO List :

UI of commits list
UI of commit page
UI file history (same as commits list)
UI search commit (same as commits list)
UI diff commit list (same as commits list)
Add support for translation of new strings

lunny · 2017-03-09T00:46:56Z

build failed. and it seems the image is not transparent ?

appleboy · 2017-03-09T01:11:44Z

models/gpg_key.go

+
+	"code.gitea.io/gitea/modules/log"
+	"github.com/go-xorm/xorm"
+)


please following the style guide. https://github.com/go-gitea/gitea/blob/master/CONTRIBUTING.md#styleguide

Thx for the catch i will fix it in the PR of the API from where I start this one : #710

sapk · 2017-03-09T10:05:10Z

I didn't precise but this is a really early WIP.
The build failed is cause by the need of go-gitea/git#36 change.
In this go-gitea/git#36 we need to discuss a litlle about https://github.com/sapk-fork/git-module/blob/1395a17c1a5a05a465032519932794c486e43559/commit.go#L35 and test in this PR if we could do without or if is to much burden and keep the raw payload.

I open this PR mainly to be able to discuss UI design.

sapk · 2017-03-11T00:42:34Z

Validation of commit is ok and display a simple message on hover (title attr) with user/key that succeed or why it failed. (need go-gitea/git#36 to compile)

I need to add more detail in commit page.

For the little API part, I have a problem with json auto-encoding replacing "<" and ">" don't know the good way to tackle this prob in gitea.
https://golang.org/pkg/encoding/json/#Marshal
Example :

"verification": {
            "verified": true,
            "reason": "Antoine GIRARD \u003csapk@sapk.fr\u003e / AE8255B8A0D828E4",
            "signature": "-----BEGIN PGP SIGNATURE-----\n\niQEzBAABCAAdFiEEEIOwJg/1vpF1itJ4roJVuKDYKOQFAljC2uIACgkQroJVuKDY\nKOSkQgf+KFT2smNxRlL8S3zptsJAr4fQVV1Lmia5TdepJIUGDvlZMTNuNk2gHqeQ\nnuIJtKGzTD8s5vJIHUQSH2R82oizRPJNSaFKbohTwmMNphQ5uq4gt5DEELkfcNpo\n75N8m0ck+L2+iyPUqmvcofaUqRYjqrvMz/BJ7fB8kEzEJukvjkepfKAQ59T+dzYD\nTd5TY48L18lJv5EdBlNdzpP62/731LEEktUWb6Nttl4MCRXR5CBRPlpBv8rQekjX\nycUAnVM1Yk1BxUQ6zBXJj7xIrc5RChbsDnvAVZmzWtEzME+9w9JiTHg51EJ/fT36\nX6TUL4VQhEhdoQZqJDbzUmIYwPbUoQ==\n=lqpn\n-----END PGP SIGNATURE-----",
            "payload": "tree 7377d79e8a22b4ba522ea204fcdd175490706dd2\nparent 74cde12677bc548487ed1caf192eb19b0b7cf99c\nauthor Antoine GIRARD \u003csapk@sapk.fr\u003e 1488936721 +0100\ncommitter Antoine GIRARD \u003csapk@sapk.fr\u003e 1489165026 +0100\n\nImplement GPG API\n"
        },

ghost · 2017-03-12T18:55:46Z

Great work! Like to see it in Gitea. 👍
But in my opinion your display solution looks more like a CI indicator. Maybe you want to use something like a lock which is green, open or gray. What do you think?

(And "Message" should be on the same line than all commit messages.)

sapk · 2017-03-12T22:42:26Z

Visual updated in description.

ghost · 2017-03-12T22:48:31Z

@sapk Great! If you only see the question mark, you do not know what is missing. I would use a gray lock which indicates that this commit is signed but not trusted.

sapk · 2017-03-12T23:03:44Z

I think that if we use a grey lock it could be understood as trusted (with less security) also by the user and that is not the case. Actually if you hover the question mark it display "No good signature found in keystore" but i plan to find a better describing string like "No known key found for this signature" ?
Furthermore, I will add more detail in the commit page about the signature and is validity against key in db.

ghost · 2017-03-12T23:05:07Z

@sapk I think it will also be much cleaner if you use no border radius between the hash and the symbol.

One might use border-top-left-radius and its variations. :)

ghost · 2017-03-12T23:06:15Z

@sapk I see your point. What about an open gray lock?

sapk · 2017-03-12T23:26:18Z

Changes made
I also tested changing the icon background a little darker but I don't really know if it is usefull.

ghost · 2017-03-12T23:30:29Z

Nice! 💯

Background is ok. Do you think some kind of really small green border around the hash and icon looks good? (Border)

sapk · 2017-03-12T23:42:36Z

Like that ?

ghost · 2017-03-12T23:45:21Z

Yes, looks good. :) Green border: "my hash is trusted" ✔️

sapk · 2017-03-12T23:51:36Z

Better ?

ghost · 2017-03-12T23:54:38Z

This is really good! But then we should add a gray border around untrusted signatures and commits without. :)

sapk · 2017-03-13T00:02:46Z

sapk · 2017-03-13T00:57:16Z

Some test for commit page.

ghost · 2017-03-13T06:51:27Z

Looks good. I would use a grey lock on commit page as you have used in the commit list - not a black one.

ghost · 2017-03-13T06:53:07Z

I am not sure if one should also use the border system of the commit list for the whole commit box on these single pages - so a green border around everything, might be too colorful.

lunny · 2017-03-13T07:03:02Z

Yes. green border and background with black text maybe better.

lunny · 2017-03-17T11:40:35Z

models/gpg_key.go

 	"github.com/go-xorm/xorm"
+	"github.com/ngaut/log"


why use this log?

That my editor that auto-import :-(

+ some little fix

sapk · 2017-03-17T23:25:58Z

So after testing it seems that .IssuerKeyId in sig is not the same the the KeyID of key so remove testing commits.

lunny · 2017-03-18T03:03:32Z

Another thing, maybe all Id should be ID.

sapk · 2017-03-19T17:24:45Z

@lunny I didn't found any "Id" string in my change where did you see it ?

strk · 2017-03-19T17:37:02Z

@sapk make lint should find any of those, but given drone passes I guess there are none ?

strk · 2017-03-19T17:43:42Z

LGTM, great to also see tests in here !

strk · 2017-03-19T17:45:37Z

@sapk I guess migrations are needed, right ?

sapk · 2017-03-19T17:57:18Z

@strk I have try to separate api and db from validation and presentation.
So there is no change in DB in this PR. All change needed were made in the API PR (with a bug for mariadb #1303). All the new types are just for display (CommitVerification, SignCommit). This PR is just validation of commit and display it. API handle change in DB and manipulation via API (Add, remove, list, ...)
A other PR for managing user gpg keys via UI is in WIP. #1293.

I think it will be best to merge it after fix #1303.

lunny · 2017-03-21T00:57:25Z

@sapk #1303 is merged, maybe this need rebase?

sapk · 2017-03-21T09:45:06Z

@lunny I could rebase to be sure that everything works and be more clear but it's not needed. The goal of API #710 was to put the foundation with db format and base object. This PR don't change DB and base object so this would merge seamlessly. All the new code is only call for rendering and don't even write to DB.

lunny · 2017-03-21T10:05:25Z

@sapk this PR is ready to review and merge?

sapk · 2017-03-21T10:17:19Z

Yes and allready have a LGTM ^^

strk · 2017-03-22T09:10:57Z

LGTM , but I think we should deal with vendoring in a different way (as I know another git vendor upgrade is needed for the --follow support :/)

strk · 2017-03-22T09:11:39Z

Make L-G-T-M work !

lunny · 2017-03-22T10:42:57Z

LGTM

lunny added this to the 1.2.0 milestone Mar 9, 2017

lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Mar 9, 2017

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 9, 2017

appleboy suggested changes Mar 9, 2017

View reviewed changes

sapk force-pushed the gpg-commit-validation branch 4 times, most recently from 4765471 to 3748414 Compare March 11, 2017 00:32

sapk force-pushed the gpg-commit-validation branch from 3748414 to ac80c69 Compare March 11, 2017 00:54

sapk force-pushed the gpg-commit-validation branch from ac80c69 to a9664b3 Compare March 12, 2017 19:23

lunny reviewed Mar 17, 2017

View reviewed changes

sapk added 2 commits March 17, 2017 18:15

GPG commit validation

3afb5b0

Add translation

ff77237

+ some little fix

sapk force-pushed the gpg-commit-validation branch 2 times, most recently from 3be5753 to ff77237 Compare March 17, 2017 23:24

sapk changed the title ~~[WIP] GPG commit validation~~ GPG commit validation Mar 17, 2017

Move hash calc after retrieving of potential key + missing translation

4ced54d

sapk force-pushed the gpg-commit-validation branch from 9fe1278 to 4ced54d Compare March 17, 2017 23:39

Add some little test

1721cf4

tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 19, 2017

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 22, 2017

lunny merged commit 14fe901 into go-gitea:master Mar 22, 2017

sapk deleted the gpg-commit-validation branch April 28, 2017 10:04

go-gitea locked and limited conversation to collaborators Nov 23, 2020

GPG commit validation #1150

GPG commit validation #1150

Conversation

sapk commented Mar 9, 2017 • edited Loading

lunny commented Mar 9, 2017

appleboy Mar 9, 2017

Choose a reason for hiding this comment

sapk Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

sapk commented Mar 9, 2017 • edited Loading

sapk commented Mar 11, 2017 • edited Loading

ghost commented Mar 12, 2017

sapk commented Mar 12, 2017

ghost commented Mar 12, 2017

sapk commented Mar 12, 2017

ghost commented Mar 12, 2017

ghost commented Mar 12, 2017

sapk commented Mar 12, 2017

ghost commented Mar 12, 2017

sapk commented Mar 12, 2017

ghost commented Mar 12, 2017 • edited by ghost Loading

sapk commented Mar 12, 2017

ghost commented Mar 12, 2017

sapk commented Mar 13, 2017

sapk commented Mar 13, 2017

ghost commented Mar 13, 2017

ghost commented Mar 13, 2017

lunny commented Mar 13, 2017

lunny Mar 17, 2017

Choose a reason for hiding this comment

sapk Mar 17, 2017

Choose a reason for hiding this comment

sapk commented Mar 17, 2017

lunny commented Mar 18, 2017

sapk commented Mar 19, 2017

strk commented Mar 19, 2017

strk commented Mar 19, 2017

strk commented Mar 19, 2017

sapk commented Mar 19, 2017

lunny commented Mar 21, 2017

sapk commented Mar 21, 2017 • edited Loading

lunny commented Mar 21, 2017

sapk commented Mar 21, 2017

strk commented Mar 22, 2017 • edited Loading

strk commented Mar 22, 2017

lunny commented Mar 22, 2017

sapk commented Mar 9, 2017 •

edited

Loading

sapk Mar 9, 2017 •

edited

Loading

sapk commented Mar 9, 2017 •

edited

Loading

sapk commented Mar 11, 2017 •

edited

Loading

ghost commented Mar 12, 2017 •

edited by ghost

Loading

sapk commented Mar 21, 2017 •

edited

Loading

strk commented Mar 22, 2017 •

edited

Loading