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.

Sign up for GitHub

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
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cmd/dex/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,9 @@ func (p *password) UnmarshalJSON(b []byte) error {
})
if len(data.Hash) == 0 && len(data.HashFromEnv) > 0 {
data.Hash = os.Getenv(data.HashFromEnv)
if data.Hash == "" {
return fmt.Errorf("invalid config: could not find HashFromEnv %q", data.HashFromEnv)
}
}
if len(data.Hash) == 0 {
return fmt.Errorf("no password hash provided")
Expand Down
12 changes: 10 additions & 2 deletions cmd/dex/serve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,11 @@ func runServe(options serveOptions) error {
if client.ID != "" {
return fmt.Errorf("invalid config: ID and IDEnv fields are exclusive for client %q", client.ID)
}
c.StaticClients[i].ID = os.Getenv(client.IDEnv)
id := os.Getenv(client.IDEnv)
if id == "" {
return fmt.Errorf("invalid config: could not find IDEnv %q", id)
}
c.StaticClients[i].ID = id
}
if client.Secret == "" && client.SecretEnv == "" && !client.Public {
return fmt.Errorf("invalid config: Secret or SecretEnv field is required for client %q", client.ID)
Expand All @@ -197,7 +201,11 @@ func runServe(options serveOptions) error {
if client.Secret != "" {
return fmt.Errorf("invalid config: Secret and SecretEnv fields are exclusive for client %q", client.ID)
}
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.

}
c.StaticClients[i].Secret = secret
}
logger.Infof("config static client: %s", client.Name)
}
Expand Down
Loading