Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add require signed commit for protected branch #9708

Merged
merged 28 commits into from Jan 15, 2020

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Jan 11, 2020

  • Adds new require signed commit option to protect branch.
  • Adds indicators for CRUD and Merge screens to show if commits will be signed
  • Prevents CRUD and merge if these would affect a protected branch with require signed commit.
  • API merge and CRUD actions are handled
  • Fix bug in delete file API action

Fix #4249

Screenshots

New setting on protected branch

protected

Merge Signed

merge-signed

Prevent Merge Unsigned

merge-unsigned

CRUD signed

CRUD-signed

Prevent CRUD unsigned

crud-unsigned

@zeripath
Copy link
Contributor Author

@zeripath zeripath commented Jan 11, 2020

It's annoying but you can't just use gogitRepository to get these as it appears that the quarantine incoming checks don't work correctly.

routers/private/hook.go Outdated Show resolved Hide resolved
routers/private/hook.go Outdated Show resolved Hide resolved
@6543
Copy link
Member

@6543 6543 commented Jan 11, 2020

also close #8584

@codecov-io
Copy link

@codecov-io codecov-io commented Jan 11, 2020

Codecov Report

Merging #9708 into master will decrease coverage by <.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9708      +/-   ##
==========================================
- Coverage   42.21%   42.21%   -0.01%     
==========================================
  Files         602      602              
  Lines       78669    78669              
==========================================
- Hits        33213    33209       -4     
- Misses      41392    41395       +3     
- Partials     4064     4065       +1
Impacted Files Coverage Δ
models/repo.go 46.74% <ø> (ø) ⬆️
services/repository/transfer.go 66.66% <66.66%> (ø) ⬆️
services/pull/check.go 53.37% <0%> (-2.03%) ⬇️
models/error.go 30.76% <0%> (-0.55%) ⬇️
modules/log/event.go 65.64% <0%> (+1.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d454cd...788ed2f. Read the comment docs.

@zeripath zeripath changed the title WIP: Add require signed commit for protected branch Add require signed commit for protected branch Jan 12, 2020
@lafriks lafriks added this to the 1.12.0 milestone Jan 12, 2020
options/locale/locale_en-US.ini Outdated Show resolved Hide resolved
routers/api/v1/repo/pull.go Show resolved Hide resolved
modules/git/commit_reader.go Show resolved Hide resolved
modules/git/commit_reader.go Show resolved Hide resolved
modules/git/commit_reader.go Show resolved Hide resolved
routers/private/hook.go Outdated Show resolved Hide resolved
routers/private/hook.go Outdated Show resolved Hide resolved
services/pull/merge.go Outdated Show resolved Hide resolved
Copy link
Member

@guillep2k guillep2k left a comment

Only a concern about very large commits. In a future PR we should make it so the signature is checked as the payload is being loaded (and discarded) in order to require less memory.

modules/git/commit_reader.go Show resolved Hide resolved
@6543

This comment has been minimized.

@zeripath
Copy link
Contributor Author

@zeripath zeripath commented Jan 14, 2020

The issue will be verification

Nope it's that I failed to get the payload regeneration correct in the last few changes.

Copy link
Member

@6543 6543 left a comment

not working

EDIT: looks like 7a04a47 break something (worked before - with this commit it dont)

6543
6543 approved these changes Jan 14, 2020
@sapk sapk merged commit 66ee9b8 into go-gitea:master Jan 15, 2020
2 checks passed
@sapk
Copy link
Member

@sapk sapk commented Jan 15, 2020

@zeripath remember to claim the bounty.

@zeripath zeripath deleted the protect-branch-signed-commits-only branch Jan 15, 2020
<i class="icon lock green"></i>
{{$.i18n.Tr "repo.signing.will_sign" .SigningKey}}
</div>
{{else}}
Copy link
Contributor

@davidsvantesson davidsvantesson Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this else case should be here. It should only be shown if RequireSigned and is already handled above.

Copy link
Contributor Author

@zeripath zeripath Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users should know if the commit will be signed or not.

Copy link
Contributor

@davidsvantesson davidsvantesson Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but the green text + icon if it will be signed would be enough? Many teams/companies don't use signing at all, so if you don't require signed commits it would be a bit annoying to have a yellow text "Commits are never signed" which many users will not even know what it means.

Copy link
Contributor

@davidsvantesson davidsvantesson Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub allow admin to override also signing. If signed commits are not required, GitHub doesn't show anything about signing.

Copy link
Contributor Author

@zeripath zeripath Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So on the editor screen I was able to add a simple unlock icon to precede the Create Commit which could be mouseovered but there's not really a place on the merge screen to do that hence I felt that just adding it as another status box on merge is reasonable.

You need to know if your commits are going to be signed and why - on GH it's impossible to know why and if a commit is going to be signed.

In reality it's a status box that is only visible if you can merge.

{{$notAllOk := or .IsBlockedByApprovals .IsBlockedByRejection (and .RequireSigned (not .WillSign)) (and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess))}}
{{if and (or $.IsRepoAdmin (not $notAllOk)) (or (not .RequireSigned) .WillSign)}}
Copy link
Contributor

@davidsvantesson davidsvantesson Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall repo admin be able to bypass "require signed" or not? Currently the term in $notAllOk has no effect.

Copy link
Contributor Author

@zeripath zeripath Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a question I put on the original issue. I would guess we need an additional suboption to allow the repo admin to bypass this. The trouble is signing is different from pr approval or whitelisting.

Copy link
Contributor

@davidsvantesson davidsvantesson Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you really want every commit to be signed I think it doesn't make sense for admin to override it?
So the condition to $notAllOk are more there for future, in case we allow admin to merge?

Copy link
Contributor Author

@zeripath zeripath Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't changed $notAllOK - those are the other checks that were there previously. As I've written it RequireSign overrides all other checks as it's not overridable.

Perhaps I should have changed the name of notAllOK to notAllOverridableStatusChecksOK but it felt a little over the top.

Copy link
Contributor

@davidsvantesson davidsvantesson Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you have added (and .RequireSigned (not .WillSign)), is that not intentional?

Copy link
Contributor Author

@zeripath zeripath Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that was future proofing.

Copy link
Contributor Author

@zeripath zeripath Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also meant that the UI would keep sane.

Copy link
Contributor

@davidsvantesson davidsvantesson Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic would need to be revised anyhow if allowing overriding signing. Otherwise you will get this:
image

Copy link
Contributor Author

@zeripath zeripath Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah exactly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

8 participants