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

chore(@console|@profile): remove database credentials from logs #1160

Closed

Conversation

pedroyremolo
Copy link
Contributor

@pedroyremolo pedroyremolo commented Dec 18, 2024

Why

Currently, in the console app and profiles service database credentials were being exposed in logs when a new database client connection was established. This can lead to a secret leak which is undesirable.

What

This change removes the connection URL from logs since having this information on the connection step does not add any value and in case of failure, the error already shows its cause.

it was leading to credentials exposure since they were not being obfuscated. Since having this information on the connection step does not add any value, we've opted to remove it.
it was leading to credentials exposure since they were not being obfuscated. Since having this information on the connection step does not add any value, we've opted to remove it.
@pedroyremolo pedroyremolo marked this pull request as ready for review December 18, 2024 21:39
@vklimontovich
Copy link
Contributor

vklimontovich commented Dec 18, 2024

Let's mask credentials, instead of removing it. E.g. print

postgresql://username:******@host/newjitsu?sslmode=no-verify&schema=newjitsu

instead

postgresql://username:SECRET_PASSWORD@host/newjitsu?sslmode=no-verify&schema=newjitsu

We might already have a function for it somewhere. @absorbb did the masking in events logs

@pedroyremolo
Copy link
Contributor Author

Let's mask credentials, instead of removing it. E.g. print

postgresql://username:******@host/newjitsu?sslmode=no-verify&schema=newjitsu)

instead

okey dokey, gonna adjust here =)

@pedroyremolo
Copy link
Contributor Author

We might already have a function for it somewhere. @absorbb did the masking in events logs

During this research on jitsu repo I've found some options, I guess this one is suitable @vklimontovich :
https://github.com/jitsucom/jitsu/blob/newjitsu/webapps/ee-api/lib/store.ts#L85

May I replicate this function on these apps?

@absorbb
Copy link
Contributor

absorbb commented Dec 28, 2024

@pedroyremolo
It is better to move this function into libs/juava subproject. it is common lib for all of them

@pedroyremolo
Copy link
Contributor Author

@pedroyremolo It is better to move this function into libs/juava subproject. it is common lib for all of them

perfect. Gonna do it here 🙏🏾

@rodfsoares
Copy link
Contributor

hey @vklimontovich and @absorbb 👋

I've opened another version of this PR - @pedroyremolo is on PTO, and I don't have permission to commit to his personal branch 😅

I've applied your suggestions in #1164 - can we continue the discussion there, please?

@absorbb absorbb closed this Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants