Bug 1748368

Those fields are not optional in 4.x clusters as that would mean any user from the whole github would be allowed to access your cluster. To get your Github IdP working, you need to follow the steps in the validation error "The OAuth "cluster" is invalid: spec.identityProviders[0].github: Invalid value: "null": one of organizations or teams must be specified unless hostname is set or lookup is used".

Is there any reason you would like to have all github users be allowed to access your cluster?

I can see that this was opened solely because a migration tool was not able to migrate an insecure config, closing as NOTABUG.

I'm re-opening this BZ because the documentation [1] references organizations and teams fields as optional.

So either the documentation or the API service is wrong but this needs to be addressed.

[1] https://docs.openshift.com/container-platform/4.1/authentication/identity_providers/configuring-github-identity-provider.html#identity-provider-github-CR_configuring-github-identity-provider.


Also because this issue initially came from a migration tool is completely irrelevant and even discriminatory towards IMS teams.
But beyond that we have customers currently have started migration programs and engaged with our professional services.

And beyond please explain why this looks insecure as the description test set is a simple example to reproduce the issue.

Agree, we should update the documentation. This is not 4.2 blocking bugs. Standa can you open a pull against docs?

Gilles,

the explanation of this being insecure is contained in the first sentence of Comment 1, namely "any user from the whole github would be allowed to access your cluster".

I did not mean to diminish the importance of the migration effort, my point was that trying to have a migration path of such an insecure config which would require no user interaction is by itself not a good enough reason to have a bug about this behavior - I want a clear reason about why anyone would want this, until then it's not a bug for me.

I do agree that the documentation should probably be made clearer on when the organizations/teams fields are optional, the mention that these are actually required in some cases at a description of a different field might be confusing for people who don't read the documentation fully.

Andrea, could you please see to it?

Standa,

For the insecure part, I can't see why "any user form the whole github would be allowed to access your cluster".
Because each OAuth GitHub provider entry is bound to an uniquely defined github user name through its unique ID, see gidhubidp number (1) in [1],
and it goes along with a client ID which requires to register with GitHub OAuth application.
That allows a given GitHub user to authenticate independently of any restriction added (or not) by using organizations and teams as those fields only allow more filtering for a given user.

Regarding the organizations and teams fields, the documentation definitely needs to be revisited.
But there are currently issues as the behaviour is not as expected, independently of what the documentation means.

If those 2 fields are mandatory for non GitHub Enterprise then yes the documentation needs to be revisited.
Because description (7) and (8) says they're optionals no matter the mode (Enterprise or not). The only clearly stated restriction is that they are exclusive of each others.

What is, I think, wrong is the fact that behaviour from OCP 3.x [2] to OCP 4 is not supported, which doesn't make sense as I've explained above, it doesn't make anything more secure, as both fields are just allowing to filter out a given GitHub user to make them authenticated only if they belong to some organization(s) or team(s).

So in short, besides the documentation clarification, I believe the API server should make those fields optional in a non GitHub Enterprise case.


[1] https://docs.openshift.com/container-platform/4.1/authentication/identity_providers/configuring-github-identity-provider.html#identity-provider-github-CR_configuring-github-identity-provider
[2] https://docs.openshift.com/container-platform/3.11/install_config/configuring_authentication.html#config-github-auth-on-master

Gilles,

I believe you're looking at it from the perspective of a user, not the administrator of the cluster.

It's not very typical that you would like your organization's cluster be accessed by a random person from the internet who could then start creating namespaces and run workloads on it. Quite contrary - you usually want to limit the users of your cluster so that it's only people from your organizations using it. This is achieved by specifying teams/organizations or by having a GitHub Enterprise, which allows you to have your very own GitHub instance.

Believe me that we were aware of the changes that we were doing, and you will see that we've done some more security tightening in other identity provider configurations as well, beside some convenience improvements for the OIDC IdP. These are features from security perspective, not bugs.

Standa,

I perfectly understand changes are needed whether security driven or not.

Tests I did with OCP 3.x showed that the OAuth binding between OCP GitHub provider and GitHub app of a given user didn't let *a random* github user to authenticate bu I'm no security expert and maybe there was loopholes.
Just that this specific change still doesn't make sense and you have not convinced me.

But if that's the way it's from now on with OCP 4.2 then that's fine, we just need the documentation to be aligned and clearer.

Thanks

PR to update the docs: https://github.com/openshift/openshift-docs/pull/16512
Preview (internal): http://file.rdu.redhat.com/~ahoffer/2020/BZ-1748368/authentication/identity_providers/configuring-github-identity-provider.html#identity-provider-github-CR_configuring-github-identity-provider

@Pravin Mali, can you please review?

Preview (internal): http://file.rdu.redhat.com/~ahoffer/2020/BZ-1748368/authentication/identity_providers/configuring-github-identity-provider.html#identity-provider-github-CR_configuring-github-identity-provider

PRs have been merged.

Updates are live: https://docs.openshift.com/container-platform/4.6/authentication/identity_providers/configuring-github-identity-provider.html#identity-provider-github-CR_configuring-github-identity-provider