feat(queue)!: add priv/pub key signing #843
Conversation
|
should i make this optional? |
|
Could the worker retrieve the public key as part of the registration flow? |
absolutely, and it makes me think there would be value in also allowing stored keys to be updated on the fly for a currently-running worker, so we dont depend on a service reboot/restart if, say, the server keys require rotation. |
|
opening this for review for v0.21. |
|
one question for the reviewers, should we accept empty signing keys and skip signing? or just embed a default private/public key so that if you dont provide one, it uses the insecure default? |
Codecov Report
@@ Coverage Diff @@
## main #843 +/- ##
==========================================
- Coverage 70.89% 70.84% -0.05%
==========================================
Files 310 311 +1
Lines 12743 12833 +90
==========================================
+ Hits 9034 9092 +58
- Misses 3255 3278 +23
- Partials 454 463 +9
|
i think i lean towards requiring signing as long as we keep it easy for local dev and either supply keys or create on the fly. that wouldn't be much different than slapping secret in docker-compose as we do. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
plyr4
changed the title
cmd/vela-server/queue.go
Outdated
| Cluster: c.Bool("queue.cluster"), | ||
| Routes: c.StringSlice("queue.routes"), | ||
| Timeout: c.Duration("queue.pop.timeout"), | ||
| EncodedSigningPrivateKey: c.String("queue.signing.private-key"), |
There was a problem hiding this comment.
Matter of preference, but I'd prefer brevity with this struct field name
There was a problem hiding this comment.
agreed, im bad at naming, any suggestions on replacement? maybe since we're already in the queue package...
| EncodedSigningPrivateKey: c.String("queue.signing.private-key"), | |
| PrivateKey: c.String("queue.private-key"), |
queue/redis/opts.go
Outdated
| return nil | ||
| } | ||
|
|
||
| privateKeyDecoded, err := base64.StdEncoding.DecodeString(privateKeyEncoded) |
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
Cool, thanks for the explanation
queue/redis/push.go
Outdated
| // sign the item using the private key generated using sign | ||
| // | ||
| // https://pkg.go.dev/golang.org/x/[email protected]/nacl/sign | ||
| signed = sign.Sign(out, item, c.config.SigningPrivateKey) |
There was a problem hiding this comment.
Interesting that the library here calls out
Sign uses Ed25519 to sign messages. The length of messages is not hidden. Messages should be small because: 1. The whole message needs to be held in memory to be processed. 2. Using large messages pressures implementations on small machines to process plaintext without verifying the signature.
Prior to #927, I would have been concerned about how long pipeline.Build can be. Something to keep in mind in case we wanted to add anything else to the queue... I think build, repo, and user are small enough to not cause issues. Tried digging in to how long a message can be before considered to be "too long", but didn't find much. I imagine it's a pretty big number.
There was a problem hiding this comment.
good call out, i'll try out some massive payloads and see what happens.
im not sure if the library maybe duplicates the in-memory object while processing it, but, we already store a couple pipelines in memory so im imaging it will be fine. ill test
Co-authored-by: Easton Crupper <[email protected]>
closes go-vela/community#807
this adds environment variables (
VELA_QUEUE_SIGNING_PRIVATE_KEYandVELA_QUEUE_SIGNING_PUBLIC_KEY) that represent a private/public key pair used for signing/opening items that land on the redis queue.VELA_QUEUE_SIGNING_PRIVATE_KEYwould be required in the server.VELA_QUEUE_SIGNING_PUBLIC_KEYwould be required in the worker.queue.Pushto "sign" items pre-flight.queue.Popto "open" items post-retrieval.its a sorta-hack to implement read-only access to the queue that is agnostic of redis installation.
it would require that admins generate and provide a keypair on startup that is encoded in base64.
merging this requires the followup PR for the worker: go-vela/worker#498
it also still needs admin documentation