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

Idea: Fail-fast if clientID/clientSecret missing #244

Open
esnible opened this issue Mar 1, 2022 · 2 comments
Open

Idea: Fail-fast if clientID/clientSecret missing #244

esnible opened this issue Mar 1, 2022 · 2 comments

Comments

@esnible
Copy link
Contributor

esnible commented Mar 1, 2022

Observatorium-API refuses to start if IssuerURL is empty:

https://github.com/observatorium/api/blob/main/authentication/oidc.go#L78

It would also be helpful if Observatorium-API failed to start without clientID or clientSecret. I spent about 20 minutes struggling because I had left clientID blank.

With a bogus tenant.yaml containing

  oidc:
    clientID: ""
    "clientSecret": ""
    issuerURL: "https:/myoidc.apps.observability-d.mycluster.p1.openshiftapps.com/auth/realms/myrealm"

Observatorium came up fine, but GRPC clients who sent valid headers saw:

ERROR:
  Code: InvalidArgument
  Message: failed to authenticate: oidc: invalid configuration, clientID must be provided or SkipClientIDCheck must be set

That message makes no sense to clients, only to Observatorium implementors. I assume HTTP clients delivering logs would see something similar.

@matej-g
Copy link
Contributor

matej-g commented Jun 22, 2022

I think we cannot simply fail to start Observatorium API, since we OIDC config is per tenant and we do not want one misconfigured tenant to bring the whole API down at the start-up. But there is a loop which tries to initiate the OIDC authenticator (with backoff), so if this keeps failing it should be logged + there's also a metric for that.

On second thought, this sounds like the OIDC provider initiation actually succeeded, which should not have been the case without client ID / secret.

@squat
Copy link
Member

squat commented Jun 22, 2022

It would also be helpful if Observatorium-API failed to start without clientID or clientSecret.

I don't fully agree with this. This will mean one tenant's borked configuration breaks the API for everyone else. Ideally, if one tenant rolls out invalid configuration, other tenants can continue to use their API and even make changes of their own.
In fact, we've had issues with this in the past and implemented something that tries to isolate the auth configurations of various tenants so that one tenant's misconfigured OIDC issuer (or correctly configured issuer but maybe not responding for any reason) doesn't block startup of the API for other tenants:
https://github.com/observatorium/api/blob/main/main.go#L514-L525

Maybe it would be useful instead to have a utility to statically validate a tenant's configuration and be able to assert if they are valid before being applied to the server? This could be nice feature of the CLI.

Observatorium-API refuses to start if IssuerURL is empty

I'm having a hard time imaging how the Observatorium fails to start up entirely if one tenant's OIDC config is missing an issuerURL. AFAICT:

  1. the empty issuer check only happens here: https://github.com/observatorium/api/blob/main/authentication/oidc.go#L84
  2. the bounding function, newOIDCAuthenticator, is only called here:
    authenticator, err := ProviderFactory(config, tenant, registrationRetryCount, logger)
    ;
  3. and this bounding function, InitializeProvider, is called asynchronously for every tenant to avoid one tenant breaking the API for other tenants: https://github.com/observatorium/api/blob/main/main.go#L515

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

3 participants