-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Complain if the env set in SecretEnv cannot be found #3218
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing. If a warning is required, we need to add it to all os.Getenv calls, e.g., for IDEnv also.
c.StaticClients[i].Secret = os.Getenv(client.SecretEnv) | ||
secret := os.Getenv(client.SecretEnv) | ||
if secret == "" { | ||
return fmt.Errorf("invalid config: could not find SecretEnv %q", client.SecretEnv) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer a warning message instead of returning an error here because it may be a breaking change with the current implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it should be breaking because if the env is undefined then the config is broken and not working anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Client secret can be empty if this is a public client.
- If there is a single invalid client in the config, it will break all the working clients.
I insist on at least making this behavior opt-in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Client secret can be empty if this is a public client.
That is unchanged. Only if the env used in secretEnv is empty or does not exist the config would break. I consider this a hard configuration error.
For example the following config is invalid because envs cannot contain dashes:
- id: oauth2_proxy
name: example
redirectURIs:
- https://example.de/oauth2/callback
secretEnv: DEX_CLIENT_oauth2-proxy
also if secretEnv is set to a variable which wasn't provided, the config would fail. Before it just silently continued.
If there is a single invalid client in the config, it will break all the working clients.
It would probably require more refactoring of the client loading logic, to skip invalid clients.
I insist on at least making this behavior opt-in.
Should we allow end users to run with known broken configs? I am not sure what the security implications are when you run with a public client but intended to run with a secret. It probably breaks the security expectations and all clients being broken by this where broken anyway and didn't work correct.
Signed-off-by: Sandro <[email protected]>
f25f720
to
39ecb13
Compare
I've added it to all dynamic os.Getenv calls except for k8s because I wasn't sure if those are standard env variables. |
Overview
I generated a config which contained a secretEnv with a - which is not possible and possibly the clientSecret was set to empty string instead of the value I wanted.
What this PR does / why we need it
In such cases in the future the config check should just fail.
Special notes for your reviewer
Does this PR introduce a user-facing change?