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

Complain if the env set in SecretEnv cannot be found #3218

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SuperSandro2000
Copy link

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?


Copy link
Member

@nabokihms nabokihms left a 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)
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Client secret can be empty if this is a public client.
  2. 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.

Copy link
Author

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 <sandro.jaeckel@gmail.com>
@SuperSandro2000
Copy link
Author

If a warning is required, we need to add it to all os.Getenv calls, e.g., for IDEnv also.

I've added it to all dynamic os.Getenv calls except for k8s because I wasn't sure if those are standard env variables.

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

Successfully merging this pull request may close these issues.

None yet

2 participants