Add support for http signatures #553

Merged
6543 merged 23 commits from 42wim/go-sdk:httpsign into master 2022-07-12 16:45:08 +00:00
Member

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 on do you have an access token and just type something random.

This only works when running an ssh-agent.

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` on `do you have an access token` and just type something random. This only works when running an ssh-agent.
42wim changed title from WIP: Add support for http signatures to Add support for http signatures 2021-11-06 22:08:53 +00:00
Owner

@42wim can you resolve conflicts - thanks!

@42wim can you resolve conflicts - thanks!
42wim force-pushed httpsign from 9bfbac7f7c to a36fcc57be 2022-01-15 18:04:22 +00:00 Compare
Author
Member

sure, done

sure, done
Owner

Is it possible to add some tests?

Is it possible to add some tests?
6543 added the
kind/feature
label 2022-05-14 22:22:48 +00:00
6543 added this to the v0.16.0 milestone 2022-05-14 22:22:51 +00:00
42wim added 1 commit 2022-05-29 16:27:50 +00:00
Merge remote-tracking branch 'origin/master' into httpsign
Some checks failed
continuous-integration/drone/pr Build is failing
b47d2636fb
42wim added 1 commit 2022-05-29 18:17:35 +00:00
Fix missing package for windows
Some checks failed
continuous-integration/drone/pr Build is failing
6fc72b394c
Author
Member

Can someone restart the drone build? It seems to fail on network connection issues not code issues.

Can someone restart the drone build? It seems to fail on network connection issues not code issues.
Author
Member

@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:

  • create a gitea instance
  • set up a ssh CA
  • set TrustedUserCAKeys from ssh CA
  • create a user in gitea
  • add a principal to the user
  • have a priv/publickey signed by CA
  • run an ssh-agent with the key loaded

And then

  • create a request against gitea and verify it

I'm not sure how to do this and if it's worth the effort.

@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: - create a gitea instance - set up a ssh CA - set TrustedUserCAKeys from ssh CA - create a user in gitea - add a principal to the user - have a priv/publickey signed by CA - run an ssh-agent with the key loaded And then - create a request against gitea and verify it I'm not sure how to do this and if it's worth the effort.
Owner

@42wim well CI should work again - it just conflicts with your branch, sicne I dont have acces to it you have to resolve it ... :/

@42wim well CI should work again - it just conflicts with your branch, sicne I dont have acces to it you have to resolve it ... :/
6543 reviewed 2022-05-30 22:24:19 +00:00
@ -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.
Owner

we should use go build tags for os specific stuff

we should use go build tags for os specific stuff
Author
Member

The filenames already are the build constraints, no need to specify extra tags

From https://pkg.go.dev/go/build#hdr-Build_Constraints

Build constraints may also be part of a file's name (for example, source_windows.go will only be included if the target operating system is windows).

The filenames already are the build constraints, no need to specify extra tags From https://pkg.go.dev/go/build#hdr-Build_Constraints > Build constraints may also be part of a file's name (for example, source_windows.go will only be included if the target operating system is windows).
lunny marked this conversation as resolved
42wim added 1 commit 2022-06-12 22:00:18 +00:00
Merge branch 'master' into httpsign
Some checks failed
continuous-integration/drone/pr Build is failing
aae7189589
42wim added 1 commit 2022-06-12 22:15:14 +00:00
Adjust date as we are in 2022 now
All checks were successful
continuous-integration/drone/pr Build is passing
4b4431a59b
6543 approved these changes 2022-06-14 10:28:21 +00:00
lunny reviewed 2022-06-14 11:20:52 +00:00
@ -0,0 +1 @@
agent_linux.go
Owner

Is this a bug of Gitea?

Is this a bug of Gitea?
Owner

that's a sym link

that's a sym link
42wim marked this conversation as resolved
lunny reviewed 2022-06-14 11:23:57 +00:00
gitea/client.go Outdated
@ -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 {
Owner

I think we need to check Gitea version here.

I think we need to check Gitea version here.
Owner

good point but I think UseSSHCert() should check that!

good point but I think UseSSHCert() should check that!
42wim marked this conversation as resolved
6543 reviewed 2022-06-14 11:25:58 +00:00
gitea/client.go Outdated
@ -113,2 +113,4 @@
}
// UseSSHCert is an option for NewClient to enable SSH certificate authentication via HTTPSign
func UseSSHCert() func(client *Client) {
Owner
-func UseSSHCert() func(client *Client) {
+func UseSSHCert() ClientOption {
```diff -func UseSSHCert() func(client *Client) { +func UseSSHCert() ClientOption { ```
42wim marked this conversation as resolved
6543 requested review from 6543 2022-06-14 11:28:07 +00:00
42wim changed title from Add support for http signatures to WIP: Add support for http signatures 2022-06-14 12:02:46 +00:00
Author
Member

I'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.

I'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.
42wim added 1 commit 2022-06-14 21:07:20 +00:00
Add support for normal public/private key and check supported version
Some checks failed
continuous-integration/drone/pr Build is failing
ec8b1c65bc
42wim added 1 commit 2022-06-14 21:40:54 +00:00
Fix linting issue
Some checks failed
continuous-integration/drone/pr Build is failing
0b631e6445
42wim added 1 commit 2022-06-14 23:21:00 +00:00
Move versioncheck to NewClient instead off dorequest
All checks were successful
continuous-integration/drone/pr Build is passing
5509645218
42wim added 1 commit 2022-06-14 23:50:44 +00:00
Support pubkey as well as fingerprint
All checks were successful
continuous-integration/drone/pr Build is passing
9d24596d92
42wim changed title from WIP: Add support for http signatures to Add support for http signatures 2022-06-15 00:01:44 +00:00
Author
Member

Ok, ready for review, the tea part (gitea/tea#442) is not done yet though.

Ok, ready for review, the tea part (https://gitea.com/gitea/tea/pulls/442) is not done yet though.
Owner

Out of topic: How about send contribution to git to let git client support that?

Out of topic: How about send contribution to git to let git client support that?
Owner

☝️ good iea but outside of this pull scope :D

☝️ good iea but outside of this pull scope :D
6543 reviewed 2022-06-15 07:22:22 +00:00
gitea/client.go Outdated
@ -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 {
Owner

this check should be moved into UseSSHCert() and UseSSHPubkey()

this check should be moved into `UseSSHCert()` and `UseSSHPubkey()`
42wim marked this conversation as resolved
42wim added 1 commit 2022-06-15 19:21:16 +00:00
Move versioncheck to useSSHCert/Pubkey
All checks were successful
continuous-integration/drone/pr Build is passing
8c821f6c34
42wim added 1 commit 2022-06-15 23:00:39 +00:00
Expose GetAgent for reuse
Some checks failed
continuous-integration/drone/pr Build is failing
d98b3f731b
42wim added 1 commit 2022-06-16 00:08:08 +00:00
Add support for keys not in ssh-agent
Some checks failed
continuous-integration/drone/pr Build is failing
57eaee10e1
42wim added 1 commit 2022-06-16 21:02:21 +00:00
Fix linter
All checks were successful
continuous-integration/drone/pr Build is passing
4b22a93c88
42wim added 1 commit 2022-06-17 00:06:17 +00:00
Add support for ssh certificates on disk
All checks were successful
continuous-integration/drone/pr Build is passing
9af3e24dc1
Author
Member

Ok, the tea part is now also done and no more changes should be happening here, so ready for review.

Ok, the tea part is now also done and no more changes should be happening here, so ready for review.
6543 approved these changes 2022-06-17 00:33:05 +00:00
6543 requested review from Maintainers 2022-06-21 14:48:50 +00:00
Author
Member

anyone can review this @lunny maybe? would be nice to have this available on gitea 1.17 release.

anyone can review this @lunny maybe? would be nice to have this available on gitea 1.17 release.
Gusted reviewed 2022-06-21 17:20:18 +00:00
gitea/client.go Outdated
@ -115,0 +125,4 @@
if client.httpsigner.Signer == nil {
return fmt.Errorf("invalid fingerprint or ssh key")
}
client.mutex.Unlock()
Contributor

Move it to before the if condition, otherwise deadlock can occur.

Move it to before the if condition, otherwise deadlock can occur.
42wim marked this conversation as resolved
gitea/client.go Outdated
@ -115,0 +142,4 @@
if client.httpsigner.Signer == nil {
return fmt.Errorf("invalid fingerprint or ssh key")
}
client.mutex.Unlock()
Contributor

Ditto

Ditto
42wim marked this conversation as resolved
@ -0,0 +59,4 @@
var signer ssh.Signer
if config.sshKey != "" {
priv, err := ioutil.ReadFile(config.sshKey)
Contributor

ioutil is deprecated, please use os.ReadFile

ioutil is deprecated, please use `os.ReadFile`
42wim marked this conversation as resolved
@ -0,0 +70,4 @@
}
if config.cert {
certbytes, err := ioutil.ReadFile(config.sshKey + "-cert.pub")
Contributor

ditto

ditto
42wim marked this conversation as resolved
@ -0,0 +90,4 @@
return nil
}
}
} else {
Contributor

I think this should be commented/documented that it will use the host's SSH agent if no SSH is specified.

I think this should be commented/documented that it will use the host's SSH agent if no SSH is specified.
42wim marked this conversation as resolved
@ -0,0 +151,4 @@
return fmt.Errorf("getBody() failed: %s", err)
}
contents, err = ioutil.ReadAll(body)
Contributor

io.ReadAll

`io.ReadAll`
42wim marked this conversation as resolved
42wim added 1 commit 2022-06-21 18:15:26 +00:00
Apply code review remarks
All checks were successful
continuous-integration/drone/pr Build is passing
65c697827a
Author
Member

Issues resolved @gusted

Issues resolved @gusted
lunny reviewed 2022-06-22 01:41:18 +00:00
@ -0,0 +61,4 @@
if config.sshKey != "" {
priv, err := os.ReadFile(config.sshKey)
if err != nil {
Owner

Show we hide these err? I think we should return the error as the second result parameter.

Show we hide these err? I think we should return the error as the second result parameter.
Author
Member

Done

Done
42wim marked this conversation as resolved
lunny reviewed 2022-06-22 01:42:15 +00:00
@ -0,0 +142,4 @@
r.Header.Add("x-ssh-certificate", certString)
headersToSign = append(headersToSign, "x-ssh-certificate")
}
Owner

Maybe we need at least a warning here.

Maybe we need at least a warning here.
Author
Member

I don't understand, a warning about what?

I don't understand, a warning about what?
Owner

I mean to add a else here and have a warning log.

I mean to add a `else` here and have a warning log.
Author
Member

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.

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.
42wim marked this conversation as resolved
42wim added 1 commit 2022-06-22 08:38:00 +00:00
Return errors everywhere
All checks were successful
continuous-integration/drone/pr Build is passing
9aa2ea331e
42wim added 1 commit 2022-06-23 12:57:42 +00:00
Add extra error checking for invalid certificate
All checks were successful
continuous-integration/drone/pr Build is passing
c843d3a5e7
Author
Member

current CI failure is not related :)

current CI failure is not related :)
6543 approved these changes 2022-06-23 16:31:35 +00:00
baspepilta reviewed 2022-06-23 21:45:38 +00:00
Owner

current CI failure is not related :)

Looks like it's successful.

> current CI failure is not related :) Looks like it's successful.
Owner

I restarted the ci ...

I restarted the ci ...
noerw approved these changes 2022-06-24 08:48:55 +00:00
noerw left a comment
Member

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 :>

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 :>
Author
Member

@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 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.
42wim added 1 commit 2022-06-24 18:57:32 +00:00
Add support for sshkeys with passphrases
All checks were successful
continuous-integration/drone/pr Build is passing
8f2b7601e5
Author
Member

@noerw @6543 please review again with latest changes

@noerw @6543 please review again with latest changes
42wim added 1 commit 2022-06-24 19:01:12 +00:00
Fix typo
All checks were successful
continuous-integration/drone/pr Build is passing
135a1e6a29
42wim added 1 commit 2022-06-24 19:02:09 +00:00
Fix missed password to passphrase rename
All checks were successful
continuous-integration/drone/pr Build is passing
04147197ae
6543 approved these changes 2022-06-26 22:35:20 +00:00
6543 left a comment
Owner

new diff looks valid :)

new diff looks valid :)
6543 merged commit e5f0c189f2 into master 2022-07-12 16:45:08 +00:00
Sign in to join this conversation.
No description provided.