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

Signer authorized keys #610

Merged
merged 7 commits into from
Nov 6, 2023
Merged

Signer authorized keys #610

merged 7 commits into from
Nov 6, 2023

Conversation

nicolasochem
Copy link
Collaborator

@nicolasochem nicolasochem commented Oct 23, 2023

Authorized key is the tezos native method to authenticate signing
requests, one that we use in the new tezos-kms-signer-lambda.

This adds the required support on tezos-k8s to sign with such a signer.

The way it works in octez is:

  • when the baker/client connects to the signer for the first time,
    signer answers with a list of "authorized_keys" that the signature
    request must be signed with. These authorized keys are just tezos
    accounts
  • if the baker/client has the secret key for one of these authorized
    keys, they will just sign every request with it. otherwise, there will
    be an error
  • this can't be nested. the authorized_key can't be remote

We add support in tezos-k8s by assuming the authorized_keys are just
standard "accounts". Then, you may configure a baker as follows:

nodes:
  mybaker:
    bake_using_accounts:
    - mybakeraddy
    authorized_keys:
    - my_authorized_key

config-generator then ensures that the private authorized key is
accessible to the baker.

We also add support on octez-signer end:

octezSigners:
  mysigner:
    sign_for_accounts:
    - mybakeraddy
    authorized_keys:
    - my_authorized_key

When set, the signer mandates requests to be authenticated. Otherwise,
it signs anything.

This way, you can test end-to-end in a private chain.

We modify mkchain to do this by default: mkchain now generates an
authorized key and uses it to sign by default.

Also, mkchain was previously defaulting to using one remote signer, but
this broke when adding support for tacoInfra signer. I fixed it.

I have tested it with 3 bakers and 2 signers, one authorized and one
not. It's all working. I haven't tried zerotier and public chains.

Other changes:

  • switch default version to 17.3
  • no magic byte restriction from signer - prevents activation

Authorized key is the tezos native method to authenticate signing
requests, one that we use in the new tezos-kms-signer-lambda.

This adds the required support on tezos-k8s to sign with such a signer.

The way it works in octez is:

* when the baker/client connects to the signer for the first time,
  signer answers with a list of "authorized_keys" that the signature
  request must be signed with. These authorized keys are just tezos
  accounts
* if the baker/client has the secret key for one of these authorized
  keys, they will just sign every request with it. otherwise, there will
  be an error
* this can't be nested. the authorized_key can't be remote

We add support in tezos-k8s by assuming the authorized_keys are just
standard "accounts". Then, you may configure a baker as follows:

```
nodes:
  mybaker:
    bake_using_accounts:
    - mybakeraddy
    authorized_keys:
    - my_authorized_key
```

config-generator then ensures that the private authorized key is
accessible to the baker.

We also add support on octez-signer end:

```
octezSigners:
  mysigner:
    sign_for_accounts:
    - mybakeraddy
    authorized_keys:
    - my_authorized_key
```

When set, the signer mandates requests to be authenticated. Otherwise,
it signs anything.

This way, you can test end-to-end in a private chain.

We modify mkchain to do this by default: mkchain now generates an
authorized key and uses it to sign by default.

Also, mkchain was previously defaulting to using one remote signer, but
this broke when adding support for tacoInfra signer. I fixed it.

I have tested it with 3 bakers and 2 signers, one authorized and one
not. It's all working. I haven't tried zerotier and public chains.

Other changes:

* switch default version to 17.3
* no magic byte restriction from signer - prevents activation
mkchain/tqchain/mkchain.py Outdated Show resolved Hide resolved
charts/tezos/values.yaml Outdated Show resolved Hide resolved
charts/tezos/values.yaml Outdated Show resolved Hide resolved
charts/tezos/values.yaml Outdated Show resolved Hide resolved
charts/tezos/scripts/remote-signer.sh Show resolved Hide resolved
Comment on lines 335 to 339
all_authorized_keys = [key for node in NODES.values() for instance in node['instances'] for key in instance.get('authorized_keys', [])]
if account_name in all_authorized_keys:
# populate all known authorized keys in the activation account.
# This avoids annoying edge cases for activating private chains, when security is not critical.
return True
Copy link
Collaborator

Choose a reason for hiding this comment

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

what are the edge cases? Does it mean activation itself is the edge case?

Also can this code be formatted on multi lines?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

reformatting done. Yes, the activation itself is the edge case. I rephrased.

utils/config-generator.py Show resolved Hide resolved
nicolasochem and others added 3 commits October 24, 2023 21:14
Co-authored-by: Aryeh Harris <harryttd@users.noreply.github.com>
Co-authored-by: Aryeh Harris <harryttd@users.noreply.github.com>
@nicolasochem nicolasochem marked this pull request as ready for review October 25, 2023 17:52
@nicolasochem
Copy link
Collaborator Author

I have tested this end-to-end with the lambda on ghostnet so marking this ready:

https://ghost.tzstats.com/tz1Qf1pSbJzMN4VtGFfVJRgbXhBksRv36TxW#baking

charts/tezos/scripts/remote-signer.sh Show resolved Hide resolved
charts/tezos/values.yaml Outdated Show resolved Hide resolved
charts/tezos/values.yaml Outdated Show resolved Hide resolved
@harryttd
Copy link
Collaborator

harryttd commented Oct 27, 2023

Perhaps Helm should do validation on the auth accounts. It could fail fast if for example, a signer/baker specifies an account that doesn't exist.

EDIT
Currently:

  • we check for dupe signer account:
    {{- define "tezos.checkDupeSignerAccounts" }}
    {{- $accountNames := dict }}
    {{- range $signer := concat list
    (values (.Values.octezSigners | default dict ))
    (values (.Values.tacoinfraSigners | default dict ))
    }}
    {{- range $account := $signer.accounts }}
    {{- if hasKey $accountNames $account }}
    {{- fail (printf "Account '%s' is specified by more than one remote signer" $account) }}
    {{- else }}
    {{- $_ := set $accountNames $account "" }}
    {{- end }}
    {{- end }}
    {{- end }}
    {{- end }}
  • Here's how we get a list of all baking accounts:
    {{- define "tezos.getAccountsBaking" }}
    {{- $allAccounts := list }}
    {{- range $node := .Values.nodes }}
    {{- range $instance := $node.instances }}
    {{- if and .bake_using_accounts (kindIs "slice" .bake_using_accounts) }}
    {{- $allAccounts = concat $allAccounts .bake_using_accounts }}
    {{- else if and .bake_using_account (kindIs "string" .bake_using_account) }}
    {{- $allAccounts = append $allAccounts .bake_using_account }}
    {{- end }}
    {{- end }}
    {{- end }}
    {{- dict "data" (uniq $allAccounts) | toJson }}
    {{- end }}
  • We check if accounts are not defined:
    {{- $accountsBaking := include "tezos.getAccountsBaking" . | fromJson | values | first }}
    {{- range $accountName := $accountsBaking }}
    {{- $account := get $.Values.accounts $accountName | default dict }}
    {{- if and
    (not $.Values.should_generate_unsafe_deterministic_data)
    (not $account)
    }}
    {{- fail (printf "Baking account '%s' is not defined." $account) }}
    {{- end }}

@harryttd
Copy link
Collaborator

My thinking is if an auth key is not defined, and generate unsafe data is true, that they can be auto generated similar to baking accounts.

def fill_in_missing_accounts():

@nicolasochem
Copy link
Collaborator Author

Perhaps Helm should do validation on the auth accounts. It could fail fast if for example, a signer/baker specifies an account that doesn't exist.

Done.

My thinking is if an auth key is not defined, and generate unsafe data is true, that they can be auto generated similar to baking accounts.

Can I skip? I actually want to deprecate account autogeneration. I think generation of accounts should happen when you create the yaml file (with mkchain or otherwise).

Comment on lines +342 to +344
# Populate authorized keys known by all bakers in the activation account.
# This ensures that activation will succeed with a remote signer that requires auth,
# regardless of which baker does it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically the activation pod is is doing the operation, not a baker, right? And security isn't such an issue, because once the chain is activated, activation params can be commented out in values.yaml and the activation pod won't be created anymore to have keys mounted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, right, it probably does not have to be a baker, since at block 0, the list of bakers is not known... But what's the alternative? create a dedicated list of authorized_keys for activation? I've never activated a chain with an account that's not a baker, so I think it's fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah i think it's fine as is.

What could be done is look up the signer that is signing for the baker baking with the activation key, get this signer's auth keys, and then only import those keys. If no signer is found, then no auth key would be required for activation.

Co-authored-by: Aryeh Harris <harryttd@users.noreply.github.com>
@nicolasochem nicolasochem merged commit 9d1750c into master Nov 6, 2023
20 checks passed
@nicolasochem nicolasochem deleted the signer_authorized_keys branch December 16, 2023 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants