Skip to content

Conversation

lyricwulf
Copy link
Contributor

Instance signing rule pubkey is described as "Only sign if the user has a public key", however if the user only has SSH public keys, this check will fail, as it only checks for GPG keys.

Changed the pubkey checks to call a helper userHasPubkeys which sequentially checks for GPG, then SSH keys.

Related #34341

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 25, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Aug 25, 2025
@GiteaBot GiteaBot 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 Aug 26, 2025
@lunny lunny added type/bug skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Aug 26, 2025
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 26, 2025

Is it right? (Update: see below) Any user can have a public key for SSH access (but not signing), so all user passes the check?

@lyricwulf
Copy link
Contributor Author

lyricwulf commented Aug 26, 2025

Is it right? Any user can have a public key for SSH access (but not signing), so all user passes the check?

Isn't it right? You use it in combination with "commitssigned" or "headsigned" to check if the user did sign it.

It is entirely possible I don't understand the intent behind the configuration, but I assumed from the code (being "pubkey" and not "gpgkey") and the documentation page (users with a public key) that users who sign with SSH should be able to use instance SSH signing. 😅

@wxiaoguang
Copy link
Contributor

Hmm, you are right, the configuration option looks strange, anyone who added a GPG pubkey or SSH pubkey (without verification) can pass the check, so no idea whether it really verifies anything useful (that's why I don't know whether it is right in firs time)


Since the "pubkey" is designed to work that way, so it's fine to add the SSH check.

Is it possible to add a test to cover userHasPubkeys logic?

@wxiaoguang wxiaoguang added this to the 1.25.0 milestone Aug 26, 2025
@lyricwulf
Copy link
Contributor Author

Hmm, you are right, the configuration option looks strange, anyone who added a GPG pubkey or SSH pubkey (without verification) can pass the check, so no idea whether it really verifies anything useful (that's why I don't know whether it is right in firs time)

Yes I'm not sure of the effect. It does seem extremely lenient, but at least this way it is consistently extremely lenient 😆

Is it possible to add a test to cover userHasPubkeys logic?

Added a unit test 🙂

@GiteaBot GiteaBot 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 Aug 26, 2025
@wxiaoguang wxiaoguang force-pushed the signing-pubkey-allow-ssh branch from 9025ea7 to a101161 Compare August 26, 2025 06:19
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 26, 2025
@lunny lunny merged commit da5ce5c into go-gitea:main Aug 26, 2025
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 26, 2025
@lyricwulf lyricwulf deleted the signing-pubkey-allow-ssh branch August 27, 2025 00:48
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 28, 2025
* giteaofficial/main:
  Remove incorrect "db.DefaultContext" usages (go-gitea#35366)
  Refactor to use reflect.TypeFor (go-gitea#35370)
  [skip ci] Updated translations via Crowdin
  Remove wrong "git.DefaultContext" (go-gitea#35364)
  Fix context usages (go-gitea#35348)
  Instance signing rule `pubkey` should allow all public keys, not just GPG (go-gitea#35357)
  Allow deleting comment with content via API like web did (go-gitea#35346)
hiifong added a commit to hiifong/gitea that referenced this pull request Aug 31, 2025
* main: (34 commits)
  when sorting issues by nearest due date, issues without due date should be sorted ascending (go-gitea#35267)
  [skip ci] Updated translations via Crowdin
  Upgrade xz to v0.5.15 (go-gitea#35377)
  Refactor db package (go-gitea#35380)
  Remove the duplicated function GetTags (go-gitea#35375)
  [skip ci] Updated translations via Crowdin
  Allow foreachref parser to grow its buffer (go-gitea#35365)
  Remove global context from db package (go-gitea#35371)
  Use gitrepo.SetDefaultBranch when set default branch of wiki repository (go-gitea#33911)
  Deleting branch could delete broken branch which has database record but git branch is missing (go-gitea#35360)
  Remove incorrect "db.DefaultContext" usages (go-gitea#35366)
  Refactor to use reflect.TypeFor (go-gitea#35370)
  [skip ci] Updated translations via Crowdin
  Remove wrong "git.DefaultContext" (go-gitea#35364)
  Fix context usages (go-gitea#35348)
  Instance signing rule `pubkey` should allow all public keys, not just GPG (go-gitea#35357)
  Allow deleting comment with content via API like web did (go-gitea#35346)
  Prevent duplicate actions email (go-gitea#35215)
  rename nightly docker tag for main branch (go-gitea#35344)
  Fix atom/rss mixed error (go-gitea#35345)
  ...

# Conflicts:
#	tests/integration/pull_merge_test.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants