infisical icon indicating copy to clipboard operation
infisical copied to clipboard

🩹 — Don’t try to mask unmaskable Gitlab variables

Open ldidry opened this issue 1 year ago • 4 comments

Description 📣

Fix issue #2035

I choose to not mask unmaskable secrets.

If #2036 is implemented, the regex should be adapted (see here).

The frontend should have a warning saying that unmaskable secrets will not be masked, but I’m not a frontend guy, I can’t do that myself, sorry.

Type ✨

  • [X] Bug fix
  • [ ] New feature
  • [ ] Breaking change
  • [ ] Documentation

Tests 🛠️

Tested in production: maskable secrets are masked, the other are not.


ldidry avatar Sep 16 '24 14:09 ldidry

Good catch @ldidry! What happens currently when Infisical tries to mask a secret that's not maskable? Does it throw an error? If so, can you please include the error that this PR will resolve?

varonix0 avatar Sep 16 '24 14:09 varonix0

What happens currently when Infisical tries to mask a secret that's not maskable?

As said: "I choose to not mask unmaskable secrets.", so secrets non maskable are still added to Gitlab CI/CD variables, but not masked.

Previous behaviour: the synchronisation was marked as Failed.

The error was (I think, as I fixed it on my instance, I had to look at old logs):

{
    "level":50,
    "time":1726152317975,
    "pid":1318178,
    "hostname":"infisical",
    "severity":"ERROR",
    "err":{
        "message":"Request failed with status code 400",
        "name":"AxiosError",
        "stack":"AxiosError: Request failed with status code 400
    at settle (file:///var/www/infisical/backend/node_modules/axios/lib/core/settle.js:19:12)
    at IncomingMessage.handleStreamEnd (file:///var/www/infisical/backend/node_modules/axios/lib/adapters/http.js:599:11)
    at IncomingMessage.emit (node:events:531:35)
    at IncomingMessage.emit (node:domain:488:12)
    at endReadableNT (node:internal/streams/readable:1696:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
    at Axios.request (file:///var/www/infisical/backend/node_modules/axios/lib/core/Axios.js:45:41)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async syncSecretsGitLab (file:///var/www/infisical/backend/dist/services/integration-auth/integration-sync-secret.mjs:1457:7)
    at async syncIntegrationSecrets (file:///var/www/infisical/backend/dist/services/integration-auth/integration-sync-secret.mjs:2362:7)
    at async Worker.processFn (file:///var/www/infisical/backend/dist/services/secret/secret-queue.mjs:480:28)
    at async Worker.processJob (/var/www/infisical/backend/node_modules/bullmq/dist/cjs/classes/worker.js:408:28)
    at async Worker.retryIfFailed (/var/www/infisical/backend/node_modules/bullmq/dist/cjs/classes/worker.js:597:24)",
        "config":"[Redacted]",
        "code":"ERR_BAD_REQUEST",
        "status":400
    },
    "msg":"Secret integration sync error [projectId=[Redacted]] [environment=prod]  [secretPath=[Redacted]]"
}

ldidry avatar Sep 17 '24 07:09 ldidry

I see. I think we need to inform the user about this behavior instead of syncing without masking. If we sync without masking, and the user is expecting masking to be applied across all their secrets, it may lead to unexpected and potentially dangerous behavior.

Instead, I think we should catch the 400 error that you included above, and format it so we tell the user "Some secrets aren't able to be masked. Please update your secrets name in accordance with GitLab's requirements for secret masking", and a link to their documentation where this behavior is mentioned.

Additionally, you have a lint error as seen here: https://github.com/Infisical/infisical/actions/runs/10885555569/job/30248102789?pr=2438

varonix0 avatar Sep 17 '24 09:09 varonix0

I think we need to inform the user about this behavior

Indeed. As I said: "The frontend should have a warning saying that unmaskable secrets will not be masked, but I’m not a frontend guy, I can’t do that myself, sorry." 🙂

Please update your secrets name

That’s the secret value that needs to be updated.

you have a lint error

Fixed, thx.

ldidry avatar Sep 17 '24 12:09 ldidry