Signer authorized keys #610

nicolasochem · 2023-10-23T20:05:33Z

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

charts/tezos/values.yaml

charts/tezos/scripts/remote-signer.sh

harryttd · 2023-10-25T02:52:47Z

utils/config-generator.py

+        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


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

Also can this code be formatted on multi lines?

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

utils/config-generator.py

Co-authored-by: Aryeh Harris <harryttd@users.noreply.github.com>

nicolasochem · 2023-10-25T17:53:11Z

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

charts/tezos/values.yaml

harryttd · 2023-10-27T15:33:36Z

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:

tezos-k8s/charts/tezos/templates/_helpers.tpl

Lines 166 to 182 in cc77815

    
           {{- 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:

tezos-k8s/charts/tezos/templates/_helpers.tpl

Lines 124 to 136 in cc77815

    
           {{- 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:

tezos-k8s/charts/tezos/templates/configs.yaml

Lines 94 to 102 in cc77815

    
           {{- $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 · 2023-10-27T15:45:03Z

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.

tezos-k8s/utils/config-generator.py

Line 221 in cc77815

def fill_in_missing_accounts():

nicolasochem · 2023-11-06T05:21:04Z

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).

charts/tezos/templates/_helpers.tpl

harryttd · 2023-11-06T15:24:17Z

utils/config-generator.py

+            # 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.


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.

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.

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 force-pushed the signer_authorized_keys branch from a305ac5 to 5afa03f Compare October 24, 2023 20:24

nicolasochem requested a review from harryttd October 24, 2023 20:25

harryttd requested changes Oct 25, 2023

View reviewed changes

nicolasochem and others added 3 commits October 24, 2023 21:14

Update mkchain/tqchain/mkchain.py

804b2c8

Co-authored-by: Aryeh Harris <harryttd@users.noreply.github.com>

Update charts/tezos/values.yaml

aa9233a

Co-authored-by: Aryeh Harris <harryttd@users.noreply.github.com>

comment phrasing, per review

e1c5304

nicolasochem requested a review from harryttd October 25, 2023 04:31

nicolasochem marked this pull request as ready for review October 25, 2023 17:52

harryttd requested changes Oct 27, 2023

View reviewed changes

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

nicolasochem added 2 commits November 5, 2023 20:16

fix comments per review

2ec0c45

validate in helm that authroized keys exist

fd58c59

nicolasochem requested a review from harryttd November 6, 2023 05:21

harryttd reviewed Nov 6, 2023

View reviewed changes

charts/tezos/templates/_helpers.tpl Outdated Show resolved Hide resolved

harryttd reviewed Nov 6, 2023

View reviewed changes

Update charts/tezos/templates/_helpers.tpl

be05447

Co-authored-by: Aryeh Harris <harryttd@users.noreply.github.com>

harryttd approved these changes Nov 6, 2023

View reviewed changes

nicolasochem merged commit 9d1750c into master Nov 6, 2023
20 checks passed

nicolasochem deleted the signer_authorized_keys branch December 16, 2023 05:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Signer authorized keys #610

Signer authorized keys #610

nicolasochem commented Oct 23, 2023 •

edited

Loading

harryttd Oct 25, 2023

nicolasochem Oct 25, 2023

nicolasochem commented Oct 25, 2023

harryttd commented Oct 27, 2023 •

edited

Loading

harryttd commented Oct 27, 2023

nicolasochem commented Nov 6, 2023

harryttd Nov 6, 2023

nicolasochem Nov 6, 2023

harryttd Nov 6, 2023

Signer authorized keys #610

Signer authorized keys #610

Conversation

nicolasochem commented Oct 23, 2023 • edited Loading

harryttd Oct 25, 2023

Choose a reason for hiding this comment

nicolasochem Oct 25, 2023

Choose a reason for hiding this comment

nicolasochem commented Oct 25, 2023

harryttd commented Oct 27, 2023 • edited Loading

harryttd commented Oct 27, 2023

nicolasochem commented Nov 6, 2023

harryttd Nov 6, 2023

Choose a reason for hiding this comment

nicolasochem Nov 6, 2023

Choose a reason for hiding this comment

harryttd Nov 6, 2023

Choose a reason for hiding this comment

nicolasochem commented Oct 23, 2023 •

edited

Loading

harryttd commented Oct 27, 2023 •

edited

Loading