-
Notifications
You must be signed in to change notification settings - Fork 28
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
Changes from 26 commits
bd87a68
41a1bba
70e9b23
2fcaf0c
bae5946
203bdb7
43141ee
ae70584
ca19837
e48fa44
683e0a3
b1c4c48
d9d7364
fc52931
96f7bd5
67f4416
1324f55
4786391
448a95d
2c9d166
346df6c
00507fb
0328180
7935fe5
c28b5d6
37926a5
5815fe9
5bf066b
f24894c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,8 @@ | |
package redis | ||
|
||
import ( | ||
"encoding/base64" | ||
"errors" | ||
"fmt" | ||
"time" | ||
) | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. for the other values in the server? id argue we should base64 anything that isnt in a format that is easy to handle.
from docs 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -35,10 +36,28 @@ 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, out []byte | ||
|
||
// 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 | ||
} | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it