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

Operator Console OpenID configuration #1949

Merged
merged 8 commits into from
Feb 12, 2024

Conversation

pjuarezd
Copy link
Member

Add example configuration by environment variables to enable OpenID in Operator Console.
Added also a README.md document explaining the different options.

@feorlen
Copy link
Contributor

feorlen commented Jan 24, 2024

I was unable to make a PR from Pedro's branch. I did a quick edit of the readme to better match the style of the main web docs. This makes it possible to include it into the docs if we decide to do that. Changes attached as a md file, let me know if it is accurate and how I can best apply this to the PR. (There's a lot of small changes throughout the file.)

readme-edits.md

@cesnietor
Copy link
Contributor

cesnietor commented Jan 24, 2024

@pjuarezd I see that the env variables start with CONSOLE and those were brought from the minio/console project when we split UI parts. So I'm thinking whether it should be CONSOLE or rather OPERATOR_... as the other ones. or could be called OPERATOR_UI.

@pjuarezd
Copy link
Member Author

I was unable to make a PR from Pedro's branch. I did a quick edit of the readme to better match the style of the main web docs. This makes it possible to include it into the docs if we decide to do that. Changes attached as a md file, let me know if it is accurate and how I can best apply this to the PR. (There's a lot of small changes throughout the file.)

readme-edits.md

suggestions applied, thank you @feorlen

@pjuarezd
Copy link
Member Author

@pjuarezd I see that the env variables start with CONSOLE and those were brought from the minio/console project when we split UI parts. So I'm thinking whether it should be CONSOLE or rather OPERATOR_... as the other ones. or could be called OPERATOR_UI.

That is an excellent point @cesnietor, and I agree.

We talked about normalize our env variabe names in Operator in the past, you can see how there is more non-matching env names in the env-variables md doc, that is a pending task to do, probably in another PR? I don't think it will be approved on this one.

@feorlen
Copy link
Contributor

feorlen commented Jan 24, 2024

Oops some typos in the new readme text 🙃, added suggestions to fix

@ravindk89
Copy link
Contributor

ravindk89 commented Jan 24, 2024

@pjuarezd , question - what permission are we expecting here? Or can any user on the OIDC server gain full access via this method?

@pjuarezd
Copy link
Member Author

pjuarezd commented Jan 24, 2024

@pjuarezd , question - what permission are we expecting here? Or can any user on the OIDC server gain full access via this method?

This question unfolds in some others:

Who can login with IDP?

I would not say that "any user on the OIDC", but instead any user exisitng in the OIDC realm that the client exists on.

What can a user do once loged in?

Full access, the way it is implemented right now does not have granular access control like minio does with the "policies" attached to an OIDC user.

How can be even further limited which users in the OIDP realm can log-in?

Well, there are a few more settings available in the Operator Console to set using environment variables, but is not that clear to me how those work right now, the scope of this task was to enlight the minimal requred to configure OIDP so maybe a deeper look on those settings could answer that?, Maybe @bexsoft is familiar with the IDP settings and can explain a bit more

CONSOLE_IDP_HMAC_PASSPHRASE ( passphrase to decrypt "access_token")
CONSOLE_IDP_HMAC_SALT (passphrase salt to decrypt "access_token")
CONSOLE_IDP_USERINFO (bool, "on" = true, somenthing else = false) whether the response should include "acces_token"

@ravindk89
Copy link
Contributor

Well, there are a few more settings available in the Operator Console to set using environment variables, but is not that clear to me how those work right now, the scope of this task was to enlight the minimal requred to configure OIDP so maybe a deeper look on those settings could answer that?,

OK - I think this is a minimum requirement for us to migrate this to our webdocs. Otherwise it feels to me a security risk.

I suppose the other route is to direct users to create a standalone realm just for Operator, not sure how much or little of a lift that is.

@pjuarezd
Copy link
Member Author

pjuarezd commented Jan 24, 2024

Well, there are a few more settings available in the Operator Console to set using environment variables, but is not that clear to me how those work right now, the scope of this task was to enlight the minimal requred to configure OIDP so maybe a deeper look on those settings could answer that?,

OK - I think this is a minimum requirement for us to migrate this to our webdocs. Otherwise it feels to me a security risk.

I suppose the other route is to direct users to create a standalone realm just for Operator, not sure how much or little of a lift that is.

I do see as a viable solution the ask to create a dedicated realm to authenticate in Operator Console in the short term, but yeah I see as a potential security risk: "allow anybody in the realm with valid credentials to login full access" if that is not possible.

A fine grain authorization could be implemented in Minio Console like this:

Adding "claims" evaluation, by adding a specific attribute to the user on the OIDP service, then we specify (to Operator Console) which claim evaluate the user must have on login, just like the MINIO_IDENTITY_OPENID_CLAIM_NAME env variable does in MinIO.

This is my 2 cents on how further authorization could be implemented, but rather I would ask to those who implemented this feature (OIDP) in Operator Console, maybe there is already a solution for this.

ravindk89
ravindk89 previously approved these changes Jan 31, 2024
shtripat
shtripat previously approved these changes Feb 7, 2024
Copy link
Contributor

@shtripat shtripat left a comment

Choose a reason for hiding this comment

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

couple of tiny ones

@pjuarezd pjuarezd dismissed stale reviews from shtripat and ravindk89 via 1a644f1 February 7, 2024 05:26
@pjuarezd pjuarezd force-pushed the example-operator-console-openid branch from 7656315 to 1a644f1 Compare February 7, 2024 05:26
Add example configuration by environment variables to enable OpenID in Operator Console.
Added also a README.md document explaining the different options.

Signed-off-by: pjuarezd <pjuarezd@users.noreply.github.com>
Signed-off-by: pjuarezd <pjuarezd@users.noreply.github.com>
Signed-off-by: pjuarezd <pjuarezd@users.noreply.github.com>
pjuarezd and others added 5 commits February 6, 2024 21:27
Co-authored-by: Andrea Longo <feorlen@users.noreply.github.com>
Co-authored-by: Andrea Longo <feorlen@users.noreply.github.com>
Co-authored-by: Andrea Longo <feorlen@users.noreply.github.com>
Co-authored-by: Ravind Kumar <ravindk89@gmail.com>
Signed-off-by: pjuarezd <pjuarezd@users.noreply.github.com>
@pjuarezd pjuarezd force-pushed the example-operator-console-openid branch from 1a644f1 to 608541a Compare February 7, 2024 05:27
Copy link
Contributor

@shtripat shtripat left a comment

Choose a reason for hiding this comment

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

LGTM

@pjuarezd pjuarezd merged commit 4569bdd into minio:master Feb 12, 2024
26 checks passed
@pjuarezd pjuarezd deleted the example-operator-console-openid branch February 12, 2024 19:55
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.

6 participants