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

Implement http signatures support for the API #17565

Merged
merged 27 commits into from
Jun 5, 2022
Merged

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
services/auth/httpsign.go Outdated Show resolved Hide resolved
@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.

services/auth/httpsign.go Outdated Show resolved Hide resolved
services/auth/httpsign.go Outdated Show resolved Hide resolved
services/auth/httpsign.go Outdated Show resolved Hide resolved
@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
services/auth/httpsign.go Outdated Show resolved Hide resolved
services/auth/httpsign.go Outdated Show resolved Hide resolved
@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 <[email protected]>
Co-authored-by: zeripath <[email protected]>
Signed-off-by: Andrew Thornton <[email protected]>
@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