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

secrets: name validation seems broken #1563

Open
edsantiago opened this issue Jul 13, 2023 · 8 comments
Open

secrets: name validation seems broken #1563

edsantiago opened this issue Jul 13, 2023 · 8 comments

Comments

@edsantiago
Copy link
Collaborator

This changed in #1541 without any explanation:

// Allowed: 253 [a-zA-Z0-9-_.] characters, and the start and end character must be [a-zA-Z0-9]
-var secretNameRegexp = regexp.Delayed(`^[a-zA-Z0-9][a-zA-Z0-9_.-]*$`)
+var secretNameRegexp = regexp.Delayed("^[^/=\000]+$")

(then slightly tweaked in #1562).

I believe this is really badly broken:

  • it disagrees with the comment immediately above it
  • it allows almost all ASCII characters: backspace, newline, CR, bell, you get the idea

Maybe the idea was to allow Unicode? If so, this is not the way to do it.

@vrothberg
Copy link
Member

@rhatdan PTAL

@rhatdan
Copy link
Member

rhatdan commented Jul 17, 2023

I wanted to allow any character that an be placed on a file or environment variable.

For example an Email address I am willing to add more constraints, but not as strict as it was before.

@rhatdan
Copy link
Member

rhatdan commented Jul 17, 2023

Perhaps eliminate

\p{C} or \p{Other}: invisible control characters and unused code points. 

@vrothberg
Copy link
Member

Can we revert the other commit? We're blocked on vendoring c/common into Podman.

@rhatdan
Copy link
Member

rhatdan commented Jul 19, 2023

Why? And no.

@vrothberg
Copy link
Member

Why? And no.

As mentioned above, it's blocking c/common from being vendored into c/podman (see containers/podman#19178) for over a week.

@Luap99
Copy link
Member

Luap99 commented Jul 19, 2023

Why? And no.

As mentioned above, it's blocking c/common from being vendored into c/podman (see containers/podman#19178) for over a week.

vendoring was fixed in containers/podman#19187, we have to click rebase on the renovate PRs!

The original motivation seems to be #1494 which links to https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names
So I don't see why podman should allow more than that as well.

Container, pod, image, volume names they all are limited in a similar way so why should secret be special?

@vrothberg
Copy link
Member

vendoring was fixed in containers/podman#19187, we have to click rebase on the renovate PRs!

Thanks! I didn't notice this PR.

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

No branches or pull requests

4 participants