Skip to content

Conversation

42wim
Copy link
Member

@42wim 42wim commented Nov 6, 2021

Fixes #12338

This allows use to talk to the API with our ssh certificate (and/or ssh-agent) without needing to fetch an API key or tokens.
It will just automatically work when users have added their ssh principal in gitea.

This needs client code in tea
Update: also support normal pubkeys

ref: https://tools.ietf.org/html/draft-cavage-http-signatures

@42wim 42wim force-pushed the httpsign branch 3 times, most recently from 716659b to 90596a2 Compare November 6, 2021 00:40
@lunny lunny added the modifies/api This PR adds API routes or modifies them label Nov 6, 2021
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 6, 2021
@42wim
Copy link
Member Author

42wim commented Nov 6, 2021

I've made PRs against go-sdk and tea:

@lunny
Copy link
Member

lunny commented Nov 7, 2021

Did Git supports the standard?

@42wim
Copy link
Member Author

42wim commented Nov 7, 2021

Did Git supports the standard?

no

@42wim
Copy link
Member Author

42wim commented Nov 11, 2021

Is this something that could still get in 1.16 ?

@lunny
Copy link
Member

lunny commented Nov 16, 2021

Is this something that could still get in 1.16 ?

I think we have feature freezed for v1.16 before this PR sent. I would like to put it into v1.17 .

@lunny lunny added this to the 1.17.0 milestone Dec 2, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 4, 2021

Codecov Report

Merging #17565 (2870bc0) into main (6171ea7) will increase coverage by 0.05%.
The diff coverage is 46.28%.

@@            Coverage Diff             @@
##             main   #17565      +/-   ##
==========================================
+ Coverage   47.28%   47.33%   +0.05%     
==========================================
  Files         957      959       +2     
  Lines      133374   133628     +254     
==========================================
+ Hits        63067    63257     +190     
- Misses      62633    62673      +40     
- Partials     7674     7698      +24     
Impacted Files Coverage Δ
cmd/serv.go 2.32% <0.00%> (-0.02%) ⬇️
modules/context/repo.go 51.42% <0.00%> (ø)
modules/markup/external/external.go 1.31% <0.00%> (-0.04%) ⬇️
modules/setting/repository.go 55.71% <ø> (ø)
routers/api/v1/repo/release_attachment.go 0.00% <0.00%> (ø)
routers/install/install.go 1.77% <0.00%> (-0.01%) ⬇️
services/mailer/mailer.go 29.79% <0.00%> (-0.25%) ⬇️
services/mirror/mirror_pull.go 27.65% <0.00%> (ø)
services/repository/push.go 51.83% <0.00%> (ø)
routers/api/v1/repo/file.go 69.76% <36.36%> (-6.87%) ⬇️
... and 40 more

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 df9612b...2870bc0. Read the comment docs.

@lunny
Copy link
Member

lunny commented Feb 13, 2022

I'm afraid this may result in a performance problem like we encountered in token ?

@42wim
Copy link
Member Author

42wim commented Feb 13, 2022

I'm afraid this may result in a performance problem like we encountered in token ?

Can you give some more context ?
If the signature header doesn't exist there is no performance impact on the API, no further checks or verification is done.

@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 Jun 1, 2022
@lunny
Copy link
Member

lunny commented Jun 3, 2022

@42wim Good work for that. But just like @6543 said, I think tests are needed. I think it's not difficult to add some tests in integrations test.

@42wim
Copy link
Member Author

42wim commented Jun 3, 2022

@lunny remarks fixed and integration tests added

@lunny
Copy link
Member

lunny commented Jun 4, 2022

CI failure is related.

@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 Jun 4, 2022
@42wim
Copy link
Member Author

42wim commented Jun 4, 2022

latest CI failure seems to be a CI issue

@zeripath
Copy link
Contributor

zeripath commented Jun 4, 2022

I'm gonna ignore this CI failure pass but if we see it a few times in other PRs and this one again I think we need to go hunt if there is a potential deadlock...

Hmm... it also happened on

https://drone.gitea.io/go-gitea/gitea/55674/2/15 - here was in code.gitea.io/gitea/modules/markup.render
https://drone.gitea.io/go-gitea/gitea/55672/2/15 - here was in github.com/go-testfixtures/testfixtures/v3.(*mySQL).resetSequences

I guess I can still ignore...

@lunny
Copy link
Member

lunny commented Jun 4, 2022

But yesterday, it's right https://drone.gitea.io/go-gitea/gitea/55639 for the same PR

@42wim
Copy link
Member Author

42wim commented Jun 4, 2022

Failed again, but seems to happen in other recent drone builds too.

@lunny
Copy link
Member

lunny commented Jun 5, 2022

Failed again, but seems to happen in other recent drone builds too.

#19887

@zeripath
Copy link
Contributor

zeripath commented Jun 5, 2022

Make lgtm work

@zeripath zeripath merged commit e528e2b into go-gitea:main Jun 5, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 5, 2022
* giteaofficial/main:
  Add alt text to logo (go-gitea#19892)
  Limit max-height of CodeMirror editors for issue comment and wiki (go-gitea#18271)
  Implement http signatures support for the API (go-gitea#17565)
  Increment tests time out from 40m to 50m because sometimes the machine is slow (go-gitea#19887)
  fix(CI/CD): correct CI variable. (go-gitea#19886)
  Fix typo (go-gitea#19889)
  Fixing wrong paging when filtering on the issue dashboard (go-gitea#19801)
  Move `/info` outside authorization (go-gitea#19888)
  Fix order by parameter (go-gitea#19849)
  Exclude Archived repos from Dashboard Milestones (go-gitea#19882)
  use exact search instead of fuzzy search for branch filter dropdown (go-gitea#19885)
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
Fixes go-gitea#12338

This allows use to talk to the API with our ssh certificate (and/or ssh-agent) without needing to fetch an API key or tokens.
It will just automatically work when users have added their ssh principal in gitea.

This needs client code in tea
Update: also support normal pubkeys

ref: https://tools.ietf.org/html/draft-cavage-http-signatures

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: zeripath <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Add http signatures support for the API
7 participants