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

Feature/mask query 3 #506

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kasimtj
Copy link
Contributor

@kasimtj kasimtj commented Jan 16, 2025

Description

Mask secrets in logs #505

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Checklist

  • Linter passes correctly
  • Add tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary

Does this introduce a breaking change?

  • Yes
  • No

Further comments

@mga-chka
Copy link
Collaborator

Hi, I saw the issue and understand the pb. That being said, the current solution is not safe either:
You said in the issue that logs (that are written on disk) can leaks data like "s3 secret keys for s3() function, postgres or other passwords"
But your solution is to write the secrets in a conf file (written on disk), so it's not better.
A better implementation would be to look for the patterns that could contain a secret (for example the pattern of an s3 function containing secrets) and remove them.

@kasimtj
Copy link
Contributor Author

kasimtj commented Jan 16, 2025

Hi, I saw the issue and understand the pb. That being said, the current solution is not safe either: You said in the issue that logs (that are written on disk) can leaks data like "s3 secret keys for s3() function, postgres or other passwords" But your solution is to write the secrets in a conf file (written on disk), so it's not better. A better implementation would be to look for the patterns that could contain a secret (for example the pattern of an s3 function containing secrets) and remove them.

chexporter has feature that allow using env variables in config, we used this approach to hide our s3 secrets

clusters:
  - name: "yandex_cloud"
    scheme: "http"
    nodes: 
      - rc1a-86su01nci1umvb8j.mdb.yandexcloud.net:8123
      - rc1a-8l6i96v0amf7i28j.mdb.yandexcloud.net:8123
      - rc1a-t6s2fbn8svh1aq11.mdb.yandexcloud.net:8123
      - rc1b-9lvltuf7mkuiud58.mdb.yandexcloud.net:8123
      - rc1b-g3g5pu5qavrb67d9.mdb.yandexcloud.net:8123
      - rc1b-pu7kedgcdhk82e4m.mdb.yandexcloud.net:8123
    kill_query_user: statistdev

log_mask:
  - ${S3_SECRET_KEY_ANALYTICS_SERVICE_DEV}

like so

@kasimtj
Copy link
Contributor Author

kasimtj commented Jan 16, 2025

also i forgot to push commit with withoutSensitiveInfo change, to mask secrets while logging config

@kasimtj
Copy link
Contributor Author

kasimtj commented Jan 16, 2025

but maybe you're right and looking for patterns is better solution, clickhouse itself does so

@mga-chka
Copy link
Collaborator

mga-chka commented Jan 16, 2025

it's not only safer but also easier in terms of ops: the day the secrets change, there is only one place that needs to be modified. With the following implementation, it's the best way to forget to change it on chproxy (because it won't brake chproxy to use the previous secrets) then have new leaks. Ideally, all the infra part should be perfectly automated and a change of secrets should be automatically propagated to all subsystems, but I'm not sure many companies have reach this level.

@mga-chka
Copy link
Collaborator

mga-chka commented Feb 4, 2025

@kasimtj do you plan to do the pattern based solution?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants