Add support for http signatures #553
Labels
No Label
has/backport
has/pull
in progress
invalid
kind/breaking
kind/bug
kind/build
kind/deployment
kind/docs
kind/enhancement
kind/feature
kind/lint
kind/proposal
kind/question
kind/refactor
kind/security
kind/testing
kind/translation
kind/ui
need/backport
priority/critical
priority/low
priority/maybe
priority/medium
reviewed/duplicate
reviewed/invalid
reviewed/wontfix
skip-changelog
status/blocked
status/needs-feedback
status/needs-reviews
status/wip
upstream/gitea
No Milestone
No Assignees
6 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: gitea/go-sdk#553
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "42wim/go-sdk:httpsign"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Supports https://github.com/go-gitea/gitea/pull/17565
Needs some more finetuning, but can be used to test the gitea PR above.
When building tea against this, use
yes
ondo you have an access token
and just type something random.This only works when running an ssh-agent.
42wim referenced this pull request from gitea/tea2021-11-06 19:46:30 +00:00
WIP: Add support for http signaturesto Add support for http signatures@42wim can you resolve conflicts - thanks!
9bfbac7f7c
toa36fcc57be
sure, done
Is it possible to add some tests?
Can someone restart the drone build? It seems to fail on network connection issues not code issues.
@lunny I've been trying to see what/how I can test.
I think it's useless to test the httpsig (https://github.com/go-fed/httpsig/) library itself as it already has enough tests upstream.
To actually test the request against gitea I would have to first:
And then
I'm not sure how to do this and if it's worth the effort.
@42wim well CI should work again - it just conflicts with your branch, sicne I dont have acces to it you have to resolve it ... :/
@ -0,0 +1,26 @@
// Copyright 2021 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.
we should use go build tags for os specific stuff
The filenames already are the build constraints, no need to specify extra tags
From https://pkg.go.dev/go/build#hdr-Build_Constraints
@ -0,0 +1 @@
agent_linux.go
Is this a bug of Gitea?
that's a sym link
@ -239,6 +248,13 @@ func (c *Client) doRequest(method, path string, header http.Header, body io.Read
req.Header[k] = v
}
if c.httpsigner != nil {
I think we need to check Gitea version here.
good point but I think UseSSHCert() should check that!
@ -113,2 +113,4 @@
}
// UseSSHCert is an option for NewClient to enable SSH certificate authentication via HTTPSign
func UseSSHCert() func(client *Client) {
Add support for http signaturesto WIP: Add support for http signaturesI'm making this back a WIP, because the PR in gitea was original for ssh certs only, but now also has normal public/private key support.
So I'm going to add this here too, trying to get it done this week.
WIP: Add support for http signaturesto Add support for http signaturesOk, ready for review, the tea part (gitea/tea#442) is not done yet though.
Out of topic: How about send contribution to git to let git client support that?
☝️ good iea but outside of this pull scope :D
@ -70,2 +70,4 @@
return nil, err
}
// we need version 1.17.0 or higher to support the new API
if err := client.checkServerVersionGreaterThanOrEqual(version1_17_0); err != nil {
this check should be moved into
UseSSHCert()
andUseSSHPubkey()
Ok, the tea part is now also done and no more changes should be happening here, so ready for review.
anyone can review this @lunny maybe? would be nice to have this available on gitea 1.17 release.
@ -115,0 +125,4 @@
if client.httpsigner.Signer == nil {
return fmt.Errorf("invalid fingerprint or ssh key")
}
client.mutex.Unlock()
Move it to before the if condition, otherwise deadlock can occur.
@ -115,0 +142,4 @@
if client.httpsigner.Signer == nil {
return fmt.Errorf("invalid fingerprint or ssh key")
}
client.mutex.Unlock()
Ditto
@ -0,0 +59,4 @@
var signer ssh.Signer
if config.sshKey != "" {
priv, err := ioutil.ReadFile(config.sshKey)
ioutil is deprecated, please use
os.ReadFile
@ -0,0 +70,4 @@
}
if config.cert {
certbytes, err := ioutil.ReadFile(config.sshKey + "-cert.pub")
ditto
@ -0,0 +90,4 @@
return nil
}
}
} else {
I think this should be commented/documented that it will use the host's SSH agent if no SSH is specified.
@ -0,0 +151,4 @@
return fmt.Errorf("getBody() failed: %s", err)
}
contents, err = ioutil.ReadAll(body)
io.ReadAll
Issues resolved @gusted
@ -0,0 +61,4 @@
if config.sshKey != "" {
priv, err := os.ReadFile(config.sshKey)
if err != nil {
Show we hide these err? I think we should return the error as the second result parameter.
Done
@ -0,0 +142,4 @@
r.Header.Add("x-ssh-certificate", certString)
headersToSign = append(headersToSign, "x-ssh-certificate")
}
Maybe we need at least a warning here.
I don't understand, a warning about what?
I mean to add a
else
here and have a warning log.It shouldn't be possible that we arrive at this situation because we already do the necessary checks beforehand to check that it's actually a certificate.
On line 84 (for file based certificate) and on line 189 for ssh-agent based certificates.
But if you want an else there and as it is a library, we shouldn't put warnings there I think ?
So I will return an error.
current CI failure is not related :)
Looks like it's successful.
I restarted the ci ...
Did you test the failure mode for encrypted ssh keys (i.e. when not provided by an agent)? This case should produce a useful error message, which it didn't without extra handling - if I remember correctly from ssh work on tea..
(sorry, can't try myself right now without a dev machine at hand)
otherwise lgtm :>
@noerw good catch, I didn't write this with encrypted ssh keys in mind (actually this was written with only ssh-agent in mind ;))
So, for now using an encrypted key will show
ssh: cannot decode encrypted private keys
.I think we should support them though, going to work on it now.
@noerw @6543 please review again with latest changes
new diff looks valid :)