Secret name generation only reads env vars #464

Closed
opened 2023-07-19 12:44:19 +00:00 by moritz · 14 comments
Member

I was to change the secret names in the matrix recipe and stumbled across this:

secrets:
  db_password:
    external: true
    name: ${STACK_NAME}_db_password_${SECRET_DB_PASSWORD_VERSION}
  registration_shared_secret:
    external: true
    name: ${STACK_NAME}_db_password_${SECRET_REGISTRATION_SHARED_SECRET_VERSION}
  macaroon_secret_key:
    external: true
    name: ${STACK_NAME}_db_password_${SECRET_MACAROON_SECRET_KEY_VERSION}
  form_secret:
    external: true
    name: ${STACK_NAME}_db_password_${SECRET_FORM_SECRET_VERSION}

This should not work. According this configuration every secret should have the same name in the end.

So I discovered that the secret names are note generated from name: ${STACK_NAME}_db_password_${SECRET_DB_PASSWORD_VERSION} which should be the way, but only from the secret version env. So changing SECRET_REGISTRATION_SHARED_SECRET_VERSION to SECRET_REGISTRATION_VERSION will change the generated secret name.
This is very confusing and should be fixed.

Note: even the secret names differ, they contain all the same secret...

I was to change the secret names in the matrix recipe and stumbled across this: ``` secrets: db_password: external: true name: ${STACK_NAME}_db_password_${SECRET_DB_PASSWORD_VERSION} registration_shared_secret: external: true name: ${STACK_NAME}_db_password_${SECRET_REGISTRATION_SHARED_SECRET_VERSION} macaroon_secret_key: external: true name: ${STACK_NAME}_db_password_${SECRET_MACAROON_SECRET_KEY_VERSION} form_secret: external: true name: ${STACK_NAME}_db_password_${SECRET_FORM_SECRET_VERSION} ``` This should not work. According this configuration every secret should have the same name in the end. So I discovered that the secret names are note generated from `name: ${STACK_NAME}_db_password_${SECRET_DB_PASSWORD_VERSION}` which should be the way, but only from the secret version env. So changing `SECRET_REGISTRATION_SHARED_SECRET_VERSION` to `SECRET_REGISTRATION_VERSION` will change the generated secret name. This is very confusing and should be fixed. Note: even the secret names differ, they contain all the same secret...
moritz added the
bug
label 2023-07-19 12:44:19 +00:00
Owner

Wow, what a bug 🤯 Good you caught this... adding to critical board.

Wow, what a bug 🤯 Good you caught this... adding to critical board.
decentral1se added this to the Critical fixes project 2023-07-19 22:16:25 +00:00
decentral1se added the
abra
label 2023-07-19 22:16:33 +00:00
decentral1se changed title from Secret name generation from version env variable. to Secret name generation only reads env vars 2023-07-26 07:32:46 +00:00
Owner

I think abra app secret ls is also affected by this and would need to be tested when changes are made.

I think `abra app secret ls` is also affected by this and would need to be tested when changes are made.
decentral1se self-assigned this 2023-09-22 16:28:42 +00:00
Author
Member

I reopen this, because there is still an inconstancy that produces bugs.
I tried to deploy matrix version 5.0.0+v1.93.0 and this compose produces problems:

secrets:
...
  registration_shared_secret:
    external: true
    name: ${STACK_NAME}_registration_${SECRET_REGISTRATION_VERSION}
  macaroon_secret_key:
    external: true
    name: ${STACK_NAME}_macaroon_${SECRET_MACAROON_VERSION}

When I run abra app secret ls the secrets are named as follows:

  • matrix_example_com_registration_shared_secret_v1
  • matrix_example_com_macaroon_secret_key_v1

But when I try to deploy the apps I got the following error:

  • "service synapse: secret not found: matrix_example_com_macaroon_v1"
  • "service synapse: secret not found: matrix_example_com__registration_v1"

secretRemoteName := fmt.Sprintf("%s_%s_%s", app.StackName(), secretName, val.Version)

secretRemoteName := fmt.Sprintf("%s_%s_%s", app.StackName(), secretName, val.Version)
Why not generate the secretRemoteName from the compose name variable:
name: ${STACK_NAME}_registration_${SECRET_REGISTRATION_VERSION} ?

I reopen this, because there is still an inconstancy that produces bugs. I tried to deploy matrix version `5.0.0+v1.93.0` and this compose produces problems: ``` secrets: ... registration_shared_secret: external: true name: ${STACK_NAME}_registration_${SECRET_REGISTRATION_VERSION} macaroon_secret_key: external: true name: ${STACK_NAME}_macaroon_${SECRET_MACAROON_VERSION} ``` When I run `abra app secret ls` the secrets are named as follows: - `matrix_example_com_registration_shared_secret_v1` - `matrix_example_com_macaroon_secret_key_v1` But when I try to deploy the apps I got the following error: - `"service synapse: secret not found: matrix_example_com_macaroon_v1"` - `"service synapse: secret not found: matrix_example_com__registration_v1"` https://git.coopcloud.tech/coop-cloud/abra/src/commit/d49bf7735dcfbe72d50a2b7aa6a0cf30429fed21/cli/app/secret.go#L275 `secretRemoteName := fmt.Sprintf("%s_%s_%s", app.StackName(), secretName, val.Version)` Why not generate the `secretRemoteName` from the compose `name` variable: `name: ${STACK_NAME}_registration_${SECRET_REGISTRATION_VERSION}` ?
moritz reopened this issue 2023-10-19 13:25:44 +00:00
Owner

@moritz oof yeh, that looks like another edition of this bug alright 😬 Don't have time to tackle this right now but please if anyone feels free to resolve it, go ahead! There are some unit tests & integration tests which cover that code AFAIR.

@moritz oof yeh, that looks like another edition of this bug alright 😬 Don't have time to tackle this right now but please if anyone feels free to resolve it, go ahead! There are some unit tests & integration tests which cover that code AFAIR.
Owner

Bug = we're now generating the secret names by the key (e.g. registration_shared_secret) and not the name: ${STACK_NAME}_registration_${SECRET_REGISTRATION_VERSION} which is actually a horrific bug. Critical fix candidate 😬

Bug = we're now generating the secret names by the key (e.g. `registration_shared_secret`) and not the `name: ${STACK_NAME}_registration_${SECRET_REGISTRATION_VERSION}` which is actually a horrific bug. Critical fix candidate 😬
Owner

Working on this yesterday & today, getting closer to wrapping my head around it again...

I implemented a kind of centralised function ReadSecretsConfig in coop-cloud/abra#356 which I now realise is doing too much and covering two cases (1) you have the app 2) you don't have the app). So, I just need to break that into two functions (I think!). In case 1, we just need to read the name: ... as we have the ${STACK_NAME} available. In 2) we still need to construct the proper name via the sanitisedAppName passed in from the user. WIP...

Will write docs about intended functionality so as to avoid getting lost again in the middle of an implementation...

Case 1)

./cli/app/secret.go: secretsConfig, err := secret.ReadSecretsConfig(app.Path, composeFiles, app.Recipe)
./cli/app/secret.go: secretsConfig, err := secret.ReadSecretsConfig(app.Path, composeFiles, app.Recipe)
./pkg/secret/secret.go: secretsConfig, err := ReadSecretsConfig(app.Path, composeFiles, app.Recipe)

Case 2)

./cli/app/new.go: secretsConfig, err := secret.ReadSecretsConfig(envSamplePath, composeFiles, recipe.Name)

Working on this yesterday & today, getting closer to wrapping my head around it again... I implemented a kind of centralised function `ReadSecretsConfig` in https://git.coopcloud.tech/coop-cloud/abra/pulls/356 which I now realise is doing too much and covering two cases (1) you have the app 2) you don't have the app). So, I just need to break that into two functions (I think!). In case 1, we just need to read the `name: ...` as we have the `${STACK_NAME}` available. In 2) we still need to construct the proper name via the `sanitisedAppName` passed in from the user. WIP... Will write docs about intended functionality so as to avoid getting lost again in the middle of an implementation... Case 1) > ./cli/app/secret.go: secretsConfig, err := secret.ReadSecretsConfig(app.Path, composeFiles, app.Recipe) > ./cli/app/secret.go: secretsConfig, err := secret.ReadSecretsConfig(app.Path, composeFiles, app.Recipe) > ./pkg/secret/secret.go: secretsConfig, err := ReadSecretsConfig(app.Path, composeFiles, app.Recipe) Case 2) > ./cli/app/new.go: secretsConfig, err := secret.ReadSecretsConfig(envSamplePath, composeFiles, recipe.Name)
Owner

Keep running into a brick wall on this one sadly, see coop-cloud/organising#535 (comment). Giving up for now.

Keep running into a brick wall on this one sadly, see https://git.coopcloud.tech/coop-cloud/organising/issues/535#issuecomment-19031. Giving up for now.
Owner

coop-cloud/abra#391 landed, gonna take another swing at this today hopefully.

https://git.coopcloud.tech/coop-cloud/abra/pulls/391 landed, gonna take another swing at this today hopefully.
Owner

@p4u1 I've run into a potential issue with the implementation in coop-cloud/abra#391

In

pkg/secret/secret.go Lines 116 to 118 in cb49cf06d1
if !strings.Contains(configWithoutEnv.Secrets[secretId].Name, k) {
continue
}
, IIRC, we're saying that the secret key must always substring match a part of the secret name?

I think this can lead to confusion and sorrow with recipe maintainers. E.g. 7b1b5c37ed when originally the secret key was called registration_shared_secret and the name was name: ${STACK_NAME}_registration_${SECRET_REGISTRATION_VERSION}.

Is there a way we can not create a dependency between the secret key and name?

@p4u1 I've run into a potential issue with the implementation in https://git.coopcloud.tech/coop-cloud/abra/pulls/391 In https://git.coopcloud.tech/coop-cloud/abra/src/commit/cb49cf06d1e14385ed2225c2389c9774d6ed43c2/pkg/secret/secret.go#L116-L118, IIRC, we're saying that the secret key must always substring match a part of the secret name? I think this can lead to confusion and sorrow with recipe maintainers. E.g. https://git.coopcloud.tech/coop-cloud/matrix-synapse/commit/7b1b5c37ed00241b95c14b361247af278dea99cd when originally the secret key was called `registration_shared_secret` and the name was `name: ${STACK_NAME}_registration_${SECRET_REGISTRATION_VERSION}`. Is there a way we can not create a dependency between the secret key and name?

@decentral1se I added more comments to the code in coop-cloud/abra#392 to better explain, how the length modifier gets added to the secret. This should explain, why it should not be a potential problem.

@decentral1se I added more comments to the code in https://git.coopcloud.tech/coop-cloud/abra/pulls/392 to better explain, how the length modifier gets added to the secret. This should explain, why it should not be a potential problem.
Owner

Woe. I am still seeing this on latest abra main:

➜ abra-dev app deploy backup-bot-two.foo.bar.com 
FATA[0011] unable to deploy, secrets not generated (restic_repo)? 

and yet, on the server:

user@foo:~$ docker secret ls | grep backup-bot-two_foo_bar_com_restic_repo_v1
dfa22yr9smehm4knib2z06llr   backup-bot-two_foo_bar_com_restic_repo_v1                 10 hours ago    10 hours ago

Where backup-bot-two_foo_bar_com_restic_repo_v1 is the output from abra app secret ls backup-bot-two.foo.bar.com

Woe. I am still seeing this on latest abra `main`: ``` ➜ abra-dev app deploy backup-bot-two.foo.bar.com FATA[0011] unable to deploy, secrets not generated (restic_repo)? ``` and yet, on the server: ``` user@foo:~$ docker secret ls | grep backup-bot-two_foo_bar_com_restic_repo_v1 dfa22yr9smehm4knib2z06llr backup-bot-two_foo_bar_com_restic_repo_v1 10 hours ago 10 hours ago ``` Where ` backup-bot-two_foo_bar_com_restic_repo_v1` is the output from `abra app secret ls backup-bot-two.foo.bar.com`
Owner

@3wordchant ouch. Just took another gander, can you confirm you're including compose.secret.yml? I assume latest backup-bot-two commit? Bitta churn in the repo, so maybe stuff got moved around in between the deployment altho unsure if this is a new depoy or not. Could you get us some --debugoutput? Thanks. /cc @p4u1

@3wordchant ouch. Just took another gander, can you confirm you're including `compose.secret.yml`? I assume latest `backup-bot-two` commit? Bitta churn in the repo, so maybe stuff got moved around in between the deployment altho unsure if this is a new depoy or not. Could you get us some `--debug`output? Thanks. /cc @p4u1
Author
Member

I can't reproduce it. Creating new backup-bot-two app from the latest commit, uncommenting

SECRET_RESTIC_REPO_VERSION=v1
COMPOSE_FILE="$COMPOSE_FILE:compose.secret.yml"

and inserting the secrets. Deploy with the latest commit of abra works for me.

I can't reproduce it. Creating new backup-bot-two app from the latest commit, uncommenting ``` SECRET_RESTIC_REPO_VERSION=v1 COMPOSE_FILE="$COMPOSE_FILE:compose.secret.yml" ``` and inserting the secrets. Deploy with the latest commit of abra works for me.
Owner

Tearing down the deployment and redeploying worked, thanks for help @decentral1se @moritz, unsure exact cause of what I was seeing but pushing past it for now.

Tearing down the deployment and redeploying worked, thanks for help @decentral1se @moritz, unsure exact cause of what I was seeing but pushing past it for now.
Sign in to join this conversation.
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: toolshed/organising#464
No description provided.