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

feat(queue)!: add priv/pub key signing #843

Merged
merged 29 commits into from
Aug 25, 2023
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
bd87a68
wip: queue item signing via asymm keys
plyr4 Oct 21, 2022
41a1bba
revert: docker-compose local changes
plyr4 Oct 21, 2022
70e9b23
wip: queue item signing via asymm keys
plyr4 Oct 21, 2022
2fcaf0c
wip: queue signing cleanup
plyr4 Oct 21, 2022
bae5946
fix: generate example keys
plyr4 May 11, 2023
203bdb7
tweak: env var naming
plyr4 May 11, 2023
43141ee
enhance: key validations
plyr4 May 11, 2023
ae70584
chore: comment wording
plyr4 May 11, 2023
ca19837
chore: merge with main
plyr4 May 11, 2023
e48fa44
chore: go mod tidy
plyr4 May 11, 2023
683e0a3
wip: nil return to allow read-only setup
plyr4 May 22, 2023
b1c4c48
chore: merge with main
plyr4 Jul 20, 2023
d9d7364
chore: add base64 encoded comment
plyr4 Jul 20, 2023
fc52931
chore: merge with main
plyr4 Aug 8, 2023
96f7bd5
fix: adding keys to redis test mocks
plyr4 Aug 9, 2023
67f4416
Merge branch 'main' into feat/queue-signing
plyr4 Aug 9, 2023
1324f55
Merge branch 'main' of github.com:go-vela/server into feat/queue-signing
plyr4 Aug 16, 2023
4786391
fix: do not allow empty signing keys
plyr4 Aug 17, 2023
448a95d
Merge branch 'main' into feat/queue-signing
plyr4 Aug 17, 2023
2c9d166
fix: lint
plyr4 Aug 17, 2023
346df6c
fix: lint
plyr4 Aug 17, 2023
00507fb
chore: add middleware tests
plyr4 Aug 17, 2023
0328180
Merge branch 'main' into feat/queue-signing
plyr4 Aug 17, 2023
7935fe5
Merge branch 'main' into feat/queue-signing
plyr4 Aug 23, 2023
c28b5d6
Merge branch 'main' into feat/queue-signing
ecrupper Aug 23, 2023
37926a5
tweak: queue/redis/pop.go suggestion
plyr4 Aug 23, 2023
5815fe9
tweak: variable naming for queue keys
plyr4 Aug 23, 2023
5bf066b
tweak: cli variable naming for queue keys
plyr4 Aug 23, 2023
f24894c
Merge branch 'main' into feat/queue-signing
plyr4 Aug 23, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions cmd/vela-server/queue.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@ func setupQueue(c *cli.Context) (queue.Service, error) {

// queue configuration
_setup := &queue.Setup{
Driver: c.String("queue.driver"),
Address: c.String("queue.addr"),
Cluster: c.Bool("queue.cluster"),
Routes: c.StringSlice("queue.routes"),
Timeout: c.Duration("queue.pop.timeout"),
Driver: c.String("queue.driver"),
Address: c.String("queue.addr"),
Cluster: c.Bool("queue.cluster"),
Routes: c.StringSlice("queue.routes"),
Timeout: c.Duration("queue.pop.timeout"),
EncodedSigningPrivateKey: c.String("queue.signing.private-key"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Matter of preference, but I'd prefer brevity with this struct field name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, im bad at naming, any suggestions on replacement? maybe since we're already in the queue package...

Suggested change
EncodedSigningPrivateKey: c.String("queue.signing.private-key"),
PrivateKey: c.String("queue.private-key"),

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it

}

// setup the queue
Expand Down
1 change: 1 addition & 0 deletions cmd/vela-server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ func server(c *cli.Context) error {
middleware.Secret(c.String("vela-secret")),
middleware.Secrets(secrets),
middleware.Scm(scm),
middleware.QueueSigningPrivateKey(c.String("queue.signing.private-key")),
middleware.Allowlist(c.StringSlice("vela-repo-allowlist")),
middleware.DefaultBuildLimit(c.Int64("default-build-limit")),
middleware.DefaultTimeout(c.Int64("default-build-timeout")),
Expand Down
2 changes: 2 additions & 0 deletions docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ services:
VELA_USER_ACCESS_TOKEN_DURATION: 60m
VELA_DISABLE_WEBHOOK_VALIDATION: 'true'
VELA_ENABLE_SECURE_COOKIE: 'false'
VELA_QUEUE_SIGNING_PRIVATE_KEY: 'tCIevHOBq6DdN5SSBtteXUusjjd0fOqzk2eyi0DMq04NewmShNKQeUbbp3vkvIckb4pCxc+vxUo+mYf/vzOaSg=='
VELA_REPO_ALLOWLIST: '*'
VELA_SCHEDULE_ALLOWLIST: '*'
env_file:
Expand Down Expand Up @@ -78,6 +79,7 @@ services:
VELA_SERVER_SECRET: 'zB7mrKDTZqNeNTD8z47yG4DHywspAh'
WORKER_ADDR: 'http://worker:8080'
WORKER_CHECK_IN: 5m
VELA_QUEUE_SIGNING_PUBLIC_KEY: 'DXsJkoTSkHlG26d75LyHJG+KQsXPr8VKPpmH/78zmko='
restart: always
ports:
- '8081:8080'
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ require (
github.com/spf13/afero v1.9.5
github.com/urfave/cli/v2 v2.25.7
go.starlark.net v0.0.0-20230725161458-0d7263928a74
golang.org/x/crypto v0.11.0
golang.org/x/oauth2 v0.9.0
golang.org/x/sync v0.3.0
gopkg.in/square/go-jose.v2 v2.6.0
Expand Down Expand Up @@ -116,7 +117,6 @@ require (
github.com/xrash/smetrics v0.0.0-20201216005158-039620a65673 // indirect
github.com/yuin/gopher-lua v1.1.0 // indirect
golang.org/x/arch v0.3.0 // indirect
golang.org/x/crypto v0.11.0 // indirect
golang.org/x/net v0.12.0 // indirect
golang.org/x/sys v0.10.0 // indirect
golang.org/x/text v0.11.0 // indirect
Expand Down
12 changes: 12 additions & 0 deletions queue/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,16 @@ var Flags = []cli.Flag{
Usage: "timeout for requests that pop items off the queue",
Value: 60 * time.Second,
},
&cli.StringFlag{
EnvVars: []string{"VELA_QUEUE_SIGNING_PRIVATE_KEY"},
FilePath: "/vela/signing.key",
Name: "queue.signing.private-key",
Usage: "set value of base64 encoded queue signing private key",
},
&cli.StringFlag{
EnvVars: []string{"VELA_QUEUE_SIGNING_PUBLIC_KEY"},
FilePath: "/vela/signing.pub",
Name: "queue.signing.public-key",
Usage: "set value of base64 encoded queue signing public key",
},
}
2 changes: 1 addition & 1 deletion queue/redis/length_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ func TestRedis_Length(t *testing.T) {
}

// setup redis mock
_redis, err := NewTest("vela", "vela:second", "vela:third")
_redis, err := NewTest(_signingPrivateKey, _signingPublicKey, "vela", "vela:second", "vela:third")
if err != nil {
t.Errorf("unable to create queue service: %v", err)
}
Expand Down
74 changes: 74 additions & 0 deletions queue/redis/opts.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
package redis

import (
"encoding/base64"
"errors"
"fmt"
"time"
)
Expand Down Expand Up @@ -69,3 +71,75 @@ func WithTimeout(timeout time.Duration) ClientOpt {
return nil
}
}

// WithPrivateKey sets the private key in the queue client for Redis.
//
//nolint:dupl // ignore similar code
func WithPrivateKey(privateKeyEncoded string) ClientOpt {
plyr4 marked this conversation as resolved.
Show resolved Hide resolved
return func(c *client) error {
c.Logger.Trace("configuring private key in redis queue client")

if len(privateKeyEncoded) == 0 {
c.Logger.Warn("unable to base64 decode private key, provided key is empty. queue service will be unable to sign items")
return nil
}

privateKeyDecoded, err := base64.StdEncoding.DecodeString(privateKeyEncoded)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't expect base64 encoding in any of our other private keys (token manager, DB encryption). Is there a reason why we're going with this pattern here? Should this be standardized across all private keys?

Copy link
Contributor Author

@plyr4 plyr4 Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question, i used base64 here because the private/public keys i generated arent in unicode.
(using this process to make it easier: https://go.dev/play/p/-go_7SnJbnP)
base64 encoding them made it easy to copy+paste, store elsewhere, put into transit, easier handling.

for the other values in the server? id argue we should base64 anything that isnt in a format that is easy to handle.
though i think the other keys are fine because we recommend generating them using -hex from a unicode-safe characterset.

openssl rand -hex 16

from docs
https://go-vela.github.io/docs/installation/server/docker/#step-4-create-the-private-key

so... honestly yeah we should base64 more, but for queue signing since hex isnt a valid way to generate the keypair it becomes more important to encode the pair.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks for the explanation

if err != nil {
return err
}

if len(privateKeyDecoded) == 0 {
return errors.New("unable to base64 decode private key, decoded key is empty")
}

c.config.SigningPrivateKey = new([64]byte)
copy(c.config.SigningPrivateKey[:], privateKeyDecoded)

if c.config.SigningPrivateKey == nil {
return errors.New("unable to copy decoded queue signing private key, copied key is nil")
}

if len(c.config.SigningPrivateKey) == 0 {
return errors.New("unable to copy decoded queue signing private key, copied key is empty")
}

return nil
}
}

// WithPublicKey sets the public key in the queue client for Redis.
//
//nolint:dupl // ignore similar code
func WithPublicKey(publicKeyEncoded string) ClientOpt {
plyr4 marked this conversation as resolved.
Show resolved Hide resolved
return func(c *client) error {
c.Logger.Tracef("configuring public key in redis queue client")

if len(publicKeyEncoded) == 0 {
c.Logger.Warn("unable to base64 decode public key, provided key is empty. queue service will be unable to open items")
return nil
}

publicKeyDecoded, err := base64.StdEncoding.DecodeString(publicKeyEncoded)
if err != nil {
return err
}

if len(publicKeyDecoded) == 0 {
return errors.New("unable to base64 decode public key, decoded key is empty")
}

c.config.SigningPublicKey = new([32]byte)
copy(c.config.SigningPublicKey[:], publicKeyDecoded)

if c.config.SigningPublicKey == nil {
return errors.New("unable to copy decoded queue signing public key, copied key is nil")
}

if len(c.config.SigningPublicKey) == 0 {
return errors.New("unable to copy decoded queue signing public key, copied key is empty")
}

return nil
}
}
137 changes: 137 additions & 0 deletions queue/redis/opts_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package redis

import (
"encoding/base64"
"fmt"
"reflect"
"testing"
Expand Down Expand Up @@ -180,3 +181,139 @@ func TestRedis_ClientOpt_WithCluster(t *testing.T) {
}
}
}

func TestRedis_ClientOpt_WithSigningPrivateKey(t *testing.T) {
// setup tests
// create a local fake redis instance
//
// https://pkg.go.dev/github.com/alicebob/miniredis/v2#Run
_redis, err := miniredis.Run()
if err != nil {
t.Errorf("unable to create miniredis instance: %v", err)
}
defer _redis.Close()

tests := []struct {
failure bool
privKey string
want string
}{
{ //valid key input
failure: false,
privKey: "tCIevHOBq6DdN5SSBtteXUusjjd0fOqzk2eyi0DMq04NewmShNKQeUbbp3vkvIckb4pCxc+vxUo+mYf/vzOaSg==",
want: "tCIevHOBq6DdN5SSBtteXUusjjd0fOqzk2eyi0DMq04NewmShNKQeUbbp3vkvIckb4pCxc+vxUo+mYf/vzOaSg==",
},
{ //empty key input
failure: false,
privKey: "",
want: "",
},
{ //invalid base64 encoded input
failure: true,
privKey: "abc123",
want: "",
},
}

// run tests
for _, test := range tests {
_service, err := New(
WithAddress(fmt.Sprintf("redis://%s", _redis.Addr())),
WithPrivateKey(test.privKey),
)

if test.failure {
if err == nil {
t.Errorf("WithPrivateKey should have returned err")
}

continue
}

if err != nil {
t.Errorf("WithPrivateKey returned err: %v", err)
}

got := ""
if _service.config.SigningPrivateKey != nil {
got = fmt.Sprintf("%s", *_service.config.SigningPrivateKey)
} else {
got = ""
}

w, _ := base64.StdEncoding.DecodeString(test.want)

want := string(w)
if !reflect.DeepEqual(got, want) {
t.Errorf("WithPrivateKey is %v, want %v", got, want)
}
}
}

func TestRedis_ClientOpt_WithSigningPublicKey(t *testing.T) {
// setup tests
// create a local fake redis instance
//
// https://pkg.go.dev/github.com/alicebob/miniredis/v2#Run
_redis, err := miniredis.Run()
if err != nil {
t.Errorf("unable to create miniredis instance: %v", err)
}
defer _redis.Close()

tests := []struct {
failure bool
pubKey string
want string
}{
{ //valid key input
failure: false,
pubKey: "DXsJkoTSkHlG26d75LyHJG+KQsXPr8VKPpmH/78zmko=",
want: "DXsJkoTSkHlG26d75LyHJG+KQsXPr8VKPpmH/78zmko=",
},
{ //empty key input
failure: false,
pubKey: "",
want: "",
},
{ //invalid base64 encoded input
failure: true,
pubKey: "abc123",
want: "",
},
}

// run tests
for _, test := range tests {
_service, err := New(
WithAddress(fmt.Sprintf("redis://%s", _redis.Addr())),
WithPublicKey(test.pubKey),
)

if test.failure {
if err == nil {
t.Errorf("WithPublicKey should have returned err")
}

continue
}

if err != nil {
t.Errorf("WithPublicKey returned err: %v", err)
}

got := ""
if _service.config.SigningPublicKey != nil {
got = fmt.Sprintf("%s", *_service.config.SigningPublicKey)
} else {
got = ""
}

w, _ := base64.StdEncoding.DecodeString(test.want)

want := string(w)
if !reflect.DeepEqual(got, want) {
t.Errorf("SigningPublicKey is %v, want %v", got, want)
}
}
}
25 changes: 23 additions & 2 deletions queue/redis/pop.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

"github.com/go-vela/types"
"github.com/redis/go-redis/v9"
"golang.org/x/crypto/nacl/sign"
)

// Pop grabs an item from the specified channel off the queue.
Expand All @@ -35,10 +36,30 @@ func (c *client) Pop(ctx context.Context) (*types.Item, error) {
return nil, err
}

item := new(types.Item)
// this should already be validated on startup
if c.config.SigningPublicKey == nil || len(*c.config.SigningPublicKey) != 32 {
return nil, errors.New("no valid signing public key provided")
}

// extract signed item from pop results
signed := []byte(result[1])

var opened []byte

var out []byte
plyr4 marked this conversation as resolved.
Show resolved Hide resolved
plyr4 marked this conversation as resolved.
Show resolved Hide resolved

// open the item using the public key generated using sign
//
// https://pkg.go.dev/golang.org/x/[email protected]/nacl/sign
opened, ok := sign.Open(out, signed, c.config.SigningPublicKey)
if !ok {
return nil, errors.New("unable to open signed item")
}

// unmarshal result into queue item
err = json.Unmarshal([]byte(result[1]), item)
item := new(types.Item)

err = json.Unmarshal(opened, item)
if err != nil {
plyr4 marked this conversation as resolved.
Show resolved Hide resolved
return nil, err
}
Expand Down
Loading