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

MySQL Scaler: Secrets openly shown in metric name #2165

Closed
tomkerkhove opened this issue Oct 7, 2021 Discussed in #2163 · 4 comments · Fixed by #2171
Closed

MySQL Scaler: Secrets openly shown in metric name #2165

tomkerkhove opened this issue Oct 7, 2021 Discussed in #2163 · 4 comments · Fixed by #2171
Assignees
Labels
bug Something isn't working security All issues related to security

Comments

@tomkerkhove
Copy link
Member

Discussed in #2163

Originally posted by TimShilov October 6, 2021
I just noticed that the metric name is created based on the MySQL scaler connection string (which is a secret and is stored in a Secret) which I think is not great. Is there any reason why it's not hashed?

Secrets are secrets and they should not be lying around. And Keda should not take something that is Secret and turn it into a plain text.
Basically, anyone with access to HPA can see the secret which is unexpected to me.

Снимок экрана 2021-10-06 в 17 41 18

The screenshot is taken from GKE UI. "Deployment" -> "Details".

@tomkerkhove tomkerkhove added bug Something isn't working security All issues related to security labels Oct 7, 2021
@JorTurFer
Copy link
Member

JorTurFer commented Oct 7, 2021

https://github.com/kedacore/keda/blob/main/pkg/scalers/mysql_scaler.go#L186-L190

	if s.metadata.connectionString != "" {
		metricName = kedautil.NormalizeString(fmt.Sprintf("%s-%s", metricName, s.metadata.connectionString))
	} else {
		metricName = kedautil.NormalizeString(fmt.Sprintf("%s-%s", metricName, s.metadata.dbName))
	}

IMHO, we have to mask it. We are already doing it in [mssql scaler] (https://github.com/kedacore/keda/blob/main/pkg/scalers/mssql_scaler.go#L149-L153) and others, we could use the same approach, or we can use PostgreSQL approach (which basically uses the function MaskPartOfURL from utils).
WDYT? (Maybe we can hash the connection string in all scalers and unify the behavior instead maintain different methods)

I can do it today

@zroubalik
Copy link
Member

Yeah we should reuse the MaskPartOfURL() function. Anyway if we implement the indexing for scalers, we can then revisit metricNames exposed by scalers and simplify them.

@JorTurFer
Copy link
Member

Yeah we should reuse the MaskPartOfURL() function. Anyway if we implement the indexing for scalers, we can then revisit metricNames exposed by scalers and simplify them.

I'm working on it, I hope that the next will the PR is created (I don't have so much time right now and I update one or two by day).
Until that, I will create another PR with this fix today

@zroubalik
Copy link
Member

@JorTurFer no need to rush :) I can tackle this particular issue. Have some rest!

@zroubalik zroubalik self-assigned this Oct 7, 2021
@zroubalik zroubalik changed the title Secrets openly shown in metric name MySQL Scaler: Secrets openly shown in metric name Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working security All issues related to security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants