coop-cloud/organising
coop-cloud
/
organising
7
0
Fork 0
Code Issues 157 Pull Requests Projects 2 Releases Activity

Secret name generation only reads env vars #464

New Issue
Closed
opened 2023-07-19 12:44:19 +00:00 by moritz · 14 comments
moritz commented 2023-07-19 12:44:19 +00:00
Member
Copy Link

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
decentral1se commented 2023-07-19 22:16:17 +00:00
Owner
Copy Link

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
decentral1se commented 2023-08-26 08:00:54 +00:00
Owner
Copy Link

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
moritz commented 2023-10-19 13:25:39 +00:00
Author
Member
Copy Link

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"

d49bf7735d/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} ?

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
decentral1se commented 2023-10-19 13:42:18 +00:00
Owner
Copy Link

@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.
decentral1se commented 2023-11-23 12:23:44 +00:00
Owner
Copy Link

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 😬
decentral1se commented 2023-11-28 11:19:05 +00:00
Owner
Copy Link

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)
decentral1se commented 2023-11-28 20:28:33 +00:00
Owner
Copy Link

Keep running into a brick wall on this one sadly, see #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.
decentral1se commented 2023-12-01 11:04:24 +00:00
Owner
Copy Link

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.
decentral1se commented 2023-12-02 12:31:05 +00:00
Owner
Copy Link

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

In cb49cf06d1/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. 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?
p4u1 commented 2023-12-04 08:32:38 +00:00
Member
Copy Link

@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.
🎉 1
3wordchant commented 2024-01-16 02:30:27 +00:00
Owner
Copy Link

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`
decentral1se commented 2024-01-16 10:19:49 +00:00
Owner
Copy Link

@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
moritz commented 2024-01-16 12:13:36 +00:00
Author
Member
Copy Link

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.
3wordchant commented 2024-01-16 13:22:27 +00:00
Owner
Copy Link

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 Branch/Tag Specified
Branches Tags
master
Labels
Clear labels   
abra

Everything to do with abra

  
abra-gandi

Abra Gandi plugin

  
awaiting-feedback

Ping/pong on comms

  
backups

   
bug

Something is not working

  
build

Go build related issues

  
ci/cd

Getting the robots into the mix

  
community organising

Opening this thing up

  
contributing

Contributors stuff

  
coopcloud.tech

Our main website

  
democracy

Democratic decision making

  
design

Design thinking required

  
documentation

Let's write things together

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
finance

Money things

  
funding

Anything related to grant funding

  
good first issue

Easy start with development

  
help wanted

Need some help

  
installer

Installation related issues

  
kadabra

   
performance

Performance related

  
proposal

Large change which requires feedback & decisin making

  
question

More information is needed

  
recipes.coopcloud.tech

Our recipes catalogue

  
security

Securing our shit

  
test

Unit or integration test suite

  
wontfix

This won't be fixed

No Label
abra
abra-gandi
awaiting-feedback
backups
bug
build
ci/cd
community organising
contributing
coopcloud.tech
democracy
design
documentation
duplicate
enhancement
finance
funding
good first issue
help wanted
installer
kadabra
performance
proposal
question
recipes.coopcloud.tech
security
test
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Critical fixes
Assignees
Clear assignees
No Assignees
decentral1se
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: coop-cloud/organising#464
No description provided.
Delete Branch "%!s(<nil>)"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?