-
Notifications
You must be signed in to change notification settings - Fork 26
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
Tezos signer forwarder #498
Conversation
1f68554
to
ab9443f
Compare
0fb05e7
to
6fffa75
Compare
b65bda5
to
da8dbef
Compare
The last remaining piece of https://github.com/midl-dev/tezos-on-gke/ to move into tezos-k8s, tezos-signer-forwarder is a terminating pod for ssh tunnels exposing a tezos signing endpoint from an on-prem location.
da8dbef
to
2bbf0a3
Compare
enable cold standby
I didn't know about `targetLabels` but it seems more natural to do it this way.
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 have to finish reviewing a few things but submitting my current comments. Went through most of the PR but still need to understand better the services and statefulset "endpoints". I don't recall if we spoke about sts vs deployment for this PR or another one, but that is one thing i'm thinking about. As there is no state for the forwarders, and they don't appear to be scalable on the sts pods level, then why not just use a deployment.
metadata: | ||
name: tezos-signer-forwarder-secret-{{ .Values.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.
I believe namespace should be used, otherwise resources will not be deployed in the correct ns. There are other resources as well missing the namespace field.
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.
no, I think helm always installs stuff in a specific namespace, and so does pulumi.
I've actually removed it wherever it was still set on this chart. I know it's different from all our other charts in this repo, so I'm ok to re-add them because of consistency, but by default I wouldn't because these lines don't do anything.
app.kubernetes.io/name: tezos-signer-forwarder | ||
midl_baker_name: {{ $name }} | ||
midl_endpoint_name: {{ $endpoint.alias }} | ||
spec: |
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.
podSecurityContext should be set, and more k8s fields in general should be configurable, such as serviceAccountName. See kms signer example
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 don't see the point. security context can't be modified since we need to run as root, and the service account is not needed. This is unlike kms where you need a service account to bind to IAM permissions to access the KMS.
secret: | ||
secretName: tezos-signer-forwarder-secret-{{ $.Values.name }} | ||
defaultMode: 0400 | ||
containers: |
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.
Containers should not be allowed privilege escalation, and all linux capabilities should be dropped unless they happen to be needed. See kms signer example
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 tried and I got
chroot("/var/empty"): Operation not permitted [preauth]
- name: config-volume | ||
mountPath: /home/signer/.ssh/authorized_keys | ||
subPath: authorized_keys | ||
- name: secret-volume | ||
mountPath: /etc/ssh/ssh_host_ecdsa_key | ||
subPath: ssh_host_ecdsa_key |
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 think we should be able to add readOnly: true
to these mounts. Although cm and secrets in aws at least are read-only, i don't believe the backing storage is like that across all k8s providers
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.
done
does not do anything since we use entrypoint in chart
Co-authored-by: Aryeh Harris <harryttd@users.noreply.github.com>
Co-authored-by: Aryeh Harris <harryttd@users.noreply.github.com>
This reverts commit 49cf3a9.
@harryttd long story short, I was struggling with this a lot in the past, I've now rediscovered that sshd must run as root. It's not really meant to be used in containers, but it's possible to do so as long as you don't change the user. There is still an user (signer) but sshd assumes this identity when it starts. see https://superuser.com/a/1548482 I've addressed your other comments, thanks for them. Please re-review. |
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.
left a couple more comments. but otherwise approved. thanks
The last remaining piece of https://github.com/midl-dev/tezos-on-gke/ to move into tezos-k8s, tezos-signer-forwarder is a terminating pod for ssh tunnels exposing a tezos signing endpoint from an on-prem location.
This PR introduces:
The chart allows to define several "bakers" which correspond to one baking public key. A "baker" can have several "endpoints" corresponding to an individual hardware signer. Every "baker" deploys a service that the tezos chart can target as remote signer; this is indistinguishable from a cloud-hosted signer.
This supports HA deployments with 2 remote signers.
It works well in combination with https://github.com/midl-dev/tezos-remote-signer-os but all that's really needed is to connect to the ssh tunnel endpoint and forward the signer port.
This will be better documented later.