Skip to content

Conversation

@ravindk89
Copy link
Collaborator

@ravindk89 ravindk89 commented Feb 17, 2023

@ravindk89 ravindk89 added the WIP Work In Progress (Review Only If Requested) label Feb 17, 2023
@ravindk89 ravindk89 self-assigned this Feb 17, 2023
@ravindk89 ravindk89 requested review from djwfyi and donatello March 1, 2023 22:58
@ravindk89 ravindk89 added Review Assigned Reviewers Must LGTM To Close and removed WIP Work In Progress (Review Only If Requested) labels Mar 1, 2023
Copy link
Collaborator

@djwfyi djwfyi left a comment

Choose a reason for hiding this comment

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

This is an incredible amount of work. Well done!
Some of the lists really don't work the way they currently render, so maybe try list tables or the field lists instead?

Otherwise, just a bunch of nits.

I'll take a look again when you think it needs it.

- Configuring Keycloak for use with MinIO authentication and authorization
- Configuring a new or existing MinIO cluster to use Keycloak as the OIDC provider
- Create policies to control access of Keycloak-authenticated users
- Log into the MinIO Tenant Console using SSO and a Keycloak-managed identity
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this just be "MinIO Console"?


.. cond:: k8s

- Configuring Keycloak for use with MinIO authentication and authorization
Copy link
Collaborator

Choose a reason for hiding this comment

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

Configure instead of Configuring to match the tense of the other points.
Need to change through the lists.


This procedure was written and tested against Keycloak ``21.0.0``.
The provided instructions may work against other Keycloak versions.
This procedure assumes you have prior experience with Keycloak and have reviewed their documentation for guidance and best practices in deploying, configuring, and managing the service.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably worth linking out to their documentation here.
https://www.keycloak.org/documentation

- Returns the JWT ID claim from the client authentication information.

* - ``jwt:upn``
- Returns the JWT ID claim from the client authentication information.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just confirming, this returns the same thing as the one above it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Quick search on my part suggests upn is User Principal Name. In Azure it would be the email address the user logs in with.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wonder if upn is an Azure-specific extension. I couldn't find much on it in the OIDC spec. Fun stuff.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Follow the instructions for running `Keycloak in a container <https://www.keycloak.org/server/containers>`__.
The `Try Keycloak in development mode <https://www.keycloak.org/server/containers#_trying_keycloak_in_development_mode>`__ steps are sufficient for this procedure.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Extra space after Try Keycloack

Set the following :ref:`environment variables <minio-server-envvar-external-identity-management-openid>` in the appropriate configuration location, such as ``/etc/default/minio``.

The following example code sets the minimum required environment variables related to enabling the Keycloak Admin API for an existing Keycloak configuration.
Replace the suffix ``_KEYCLOAK`` with the unique identifier for the target Keycloak configuration.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not clear what this sentence means.
So MINIO_IDENTITY_OPENID_VENDOR_KEYCLOAK="keycloak" might become
MINIO_IDENTITY_OPENID_VENDOR_MYKEYTARGET="keycloak"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Basically yes. Not sure how to approach this to be honest. Open to suggestions!

Copy link
Collaborator

Choose a reason for hiding this comment

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

So I think my suggestion would be something like

         MINIO_IDENTITY_OPENID_VENDOR_{SUFFIX}="keycloak"
         MINIO_IDENTITY_OPENID_KEYCLOAK_ADMIN_URL_{SUFFIX}="https://keycloak-url:port/admin"

Then say,

Replace {SUFFIX} with a unique identifier for your keycloak IDP. For example, MINIO_IDENTITY_OPENID_VENDOR_KEYCLOAK.
If you do not expect to have more than one OpenID vendor, you can leave the suffix off entirely and use MINIO_IDENTITY_OPENID_VENDOR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using your suggestion of PRIMARY_IAM


.. tab-item:: MinIO Operator Console

You can use the MinIO Operator Console to configure Keycloak as the External Identity Provider for the MinIO Tenant:
Copy link
Collaborator

Choose a reason for hiding this comment

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

The way this ends with the : just looks odd when rendered.
Swap to a period maybe?

With the tabs included for the port-forward/node ports include, it looks like the whole section is left undone.


You can use the MinIO Operator Console to configure Keycloak as the External Identity Provider for the MinIO Tenant:

.. include:: /includes/common/common-k8s-connect-operator-console.rst
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm also not a fan of how you have tabs nested under the MinIO Operator Console tab. It's sort of visually confusing. Not sure what to do here, since it's an include.
My first thought was to rework this section as dropdowns instead of tabs, but not sure that impacts all the other places you use this content.

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Redone with dropdowns.


Navigate to the :guilabel:`Client scopes` view and create a new scope:

- Set the :guilabel:`Name` to a recognizable name for the scope (``minio-admin-API-access``)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This section renders as a mess of gray boxes.
Maybe field lists will improve it? Or a table?

image


3) Configure or Create a Client for Accessing Keycloak
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Copy link
Collaborator

Choose a reason for hiding this comment

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

The guilabel and monotype doesn't work for these lists. It's really hard to parse.
It's just a sea of gray boxes.

Field lists or tables instead?

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tables! Field lists seem to render really weirdly. Got to be some CSS issue I can't quite pin down for this project.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The tables are great!

Copy link
Collaborator Author

@ravindk89 ravindk89 left a comment

Choose a reason for hiding this comment

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

@djwfyi @feorlen OK so included Daryl's last slate of changes.

Am still working on pluggable IDP + other fixups related to ongoing work, but if this core stuff looks OK (I am going to drop dex before merge) then this procedure at least is solid.

@donatello LMK if anything looks off.

- Returns the JWT ID claim from the client authentication information.

* - ``jwt:upn``
- Returns the JWT ID claim from the client authentication information.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wonder if upn is an Azure-specific extension. I couldn't find much on it in the OIDC spec. Fun stuff.

-e KEYCLOAK_ADMIN_PASSWORD=keycloakadmin123 \
quay.io/keycloak/keycloak:latest start-dev
You can access the Keycloak container over the ``http://localhost:8080`` in your browser.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As a fun exercise I looked at the Microsoft style guide on this.

Will adopt their very simple "Go to URL to Do the thing" format.


3) Configure or Create a Client for Accessing Keycloak
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tables! Field lists seem to render really weirdly. Got to be some CSS issue I can't quite pin down for this project.

Select :guilabel:`Create client` and follow the instructions to create a new Keycloak client for MinIO.

- Set :guilabel:`Client ID` to a unique identifier for MinIO (``minio``)
- Set :guilabel:`Client type` is ``OpenID Connect``
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rewrote a number of these around a table format, which turned these all to "Set to" or "toggle to"

4) Create Client Scope for MinIO Client
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Client scopes allow Keycloak to map user attributes as part of the Java Web Token (JWT) returned in authentication requests.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

uhhhh yes yes very true


You can otherwise leave this field blank and allow MinIO to automatically determine the appropriate redirect URI to send based on the hostname from which the Console login attempt originated.

Select :guilabel:`Save` to save the configuration.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tweaked - I'd like to keep Select <UI element> to do <thing> as a standard approach.


You must restart the MinIO deployment for the changes to apply.

Check the MinIO logs and verify that startup succeeded with no errors related to the OIDC configuration.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really. The server logs page needs some work before we can really make good use of linking to it.

8) Next Steps
~~~~~~~~~~~~~

Applications should implement the STS flow using their SDK of choice.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed and done for both.


You can use the MinIO Operator Console to configure Keycloak as the External Identity Provider for the MinIO Tenant:

.. include:: /includes/common/common-k8s-connect-operator-console.rst
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Redone with dropdowns.

Set the following :ref:`environment variables <minio-server-envvar-external-identity-management-openid>` in the appropriate configuration location, such as ``/etc/default/minio``.

The following example code sets the minimum required environment variables related to enabling the Keycloak Admin API for an existing Keycloak configuration.
Replace the suffix ``_KEYCLOAK`` with the unique identifier for the target Keycloak configuration.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Basically yes. Not sure how to approach this to be honest. Open to suggestions!

@ravindk89 ravindk89 requested a review from feorlen March 13, 2023 22:12
Copy link
Collaborator

@djwfyi djwfyi left a comment

Choose a reason for hiding this comment

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

A few more issues to take care of.
The policy variables section is missing from a couple of pages.
And some of the previously added comments were not yet addressed.

* - ``jwt:client_id``
- Returns the ``client_id`` claim for the user.

See the `OpenID Connect Core 1.0 <https://openid.net/specs/openid-connect-core-1_0.html>` document for more information on these scopes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing the trailing underscore for the link.

Replace the ``TOKEN`` with the ``access_token`` value returned by Keycloak.

The API should return an XML document on success containing the following keys:
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about

On success, the API returns an XML document containing the following keys:

I'm not a fan of the should here.


* - ``_KEYCLOAK``
- Replace the suffix ``_KEYCLOAK`` with a unique identifier for this Keycloak configuration.
For example, ``MINIO_IDENTITY_OPENID_CONFIG_URL_KEYCLOAK_PRIMARY``.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using _KEYCLOAK_PRIMARY as the replacement for the _KEYCLOAK placeholder looks like you're just extending it rather than replacing it.

Maybe URL_PRIMARY_IAM instead?

This occurs elsewhere to. So if you change it here, change it everywhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed - made the change here, at the least. We can make this the de facto approach moving forward and clean things up as we go.

- Specify the Keycloak client ID and secret configured in Step 1

* - :envvar:`DISPLAY_NAME <MINIO_IDENTITY_OPENID_DISPLAY_NAME>`
- Specify the user-facing name the MinIO Console should displays as part of the Single-Sign On (SSO) workflow for the configured Keycloak service
Copy link
Collaborator

Choose a reason for hiding this comment

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

should display or displays but it can't be should displays.
I still don't like the word should. 😂

Copy link
Collaborator

Choose a reason for hiding this comment

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

You use should a lot, but I didn't mark them all.
It just gives the vibe that we aren't sure what will happen, but maybe this will? Fingers crossed? And I prefer tech docs to be confident.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed throughout, wherever I could find it at least

.. note::

Some Kubernetes deployments may experience issues with timeouts during port-forwarding operations with the Operator Console.
Use one of the methods in the other tabs on this section as a work-around.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd drop this, since there aren't tabs anymore.

Or maybe move it to above line one so that's it's over both drop downs and mention Port Forwarding and Node Ports as the options. I assume Port Forwarding is preferred, since that's the one you marked as open on load.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm. The note is specific to the port forwarding content.
So maybe instead:

Use the Node Ports configuration option instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done


You can deploy a new :ref:`MinIO Tenant <minio-k8s-deploy-minio-tenant>` from
the Operator Dashboard.
.. dropdown:: Node Ports
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there shouldn't be a space.
Should be NodePorts


3) Configure or Create a Client for Accessing Keycloak
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Copy link
Collaborator

Choose a reason for hiding this comment

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

The tables are great!

~~~~~~~~~~~~~~~~

This procedure assumes an existing MinIO cluster running the :minio-git:`latest stable MinIO version <minio/releases/latest>`.
Defer to the :ref:`minio-installation` for more complete documentation on new MinIO deployments.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Refer instead of Defer here.

Set the following :ref:`environment variables <minio-server-envvar-external-identity-management-openid>` in the appropriate configuration location, such as ``/etc/default/minio``.

The following example code sets the minimum required environment variables related to enabling the Keycloak Admin API for an existing Keycloak configuration.
Replace the suffix ``_KEYCLOAK`` with the unique identifier for the target Keycloak configuration.
Copy link
Collaborator

Choose a reason for hiding this comment

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

So I think my suggestion would be something like

         MINIO_IDENTITY_OPENID_VENDOR_{SUFFIX}="keycloak"
         MINIO_IDENTITY_OPENID_KEYCLOAK_ADMIN_URL_{SUFFIX}="https://keycloak-url:port/admin"

Then say,

Replace {SUFFIX} with a unique identifier for your keycloak IDP. For example, MINIO_IDENTITY_OPENID_VENDOR_KEYCLOAK.
If you do not expect to have more than one OpenID vendor, you can leave the suffix off entirely and use MINIO_IDENTITY_OPENID_VENDOR.

]
}
The ``${jwt:PreferredUsername}`` substitutes in the username to that portion of the policy.
Copy link
Collaborator

@feorlen feorlen Mar 16, 2023

Choose a reason for hiding this comment

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

substitutes in the username to that portion of the policy.

It took me a few tries to read this as intended. (There's also a similar line below, in the LDAP example.)

My understanding is these are template variables, replaced by a real value when known. Perhaps something like this?

${jwt:PreferredUsername} in the Resource field is replaced with the authenticated user's name.

I feel like there could be "when the policy is applied" at the end too, although I'm not sure that is correct terminology.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made some tweaks to your suggestion, mostly for active voice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also added the bit about policy evaluation in .

need to specify this field to ensure the OIDC provider returns the
authentication response to the correct URL.
Ensure you start the MinIO Server with the :mc-cmd:`~minio server --console-address` option to set a static Console listen port.
The default behavior with that option omitted is to select a random port number at startup.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to be called out more? Seems a randomly assigned port might be a nasty surprise if you forget.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is a nasty surprise.

Our deploy tutorials always set the console port explicitly, as does K8s by default - so this should be something sufficiently covered.

oc minio proxy
oc minio proxy
The command output includes a JWT token you must use to log into the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps replace you must use with needed or required here?

~~~~~~~~~~~~~~~~

This procedure assumes an existing MinIO cluster running the :minio-git:`latest stable MinIO version <minio/releases/latest>`.
Defer to the :ref:`minio-installation` for more complete documentation on new MinIO deployments.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Defer to feels a little fancy to me, what about See?

}
}
MinIO would revoke access for an authenticated user if the returned value has ``enabled: false``, or if the Admin API returned no matching user.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not entirely following this paragraph. The first part makes sense: if the returned value has enabled: false, access is revoked even if the user would otherwise be correctly authenticated.

Does the Admin API returned no matching user mean the user doesn't exist, or the user does exist but lacks some needed permission? Is there anything to revoke in that case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe it means the user specifically does not exist.

This deals with the following timeline:

T=0: User foo authenticates successfully to keycloak
T=1: STS Token generated with 1 hour validity
T=2: User foo deleted from KeyCloak
T=10: MinIO revokes STS token based on Admin API

Either way, simplified a bit.

MinIO would revoke access for an authenticated user if the returned value has ``enabled: false``, or if the Admin API returned no matching user.

If you do not enable this functionality, the earliest point in time that MinIO could disable access for a disabled or removed user is when the last retrieved authentication token expires.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does enable here mean enable it within Keycloak? Maybe there is a specific Keycloak feature name we could mention. (I don't know anything about Keycloak, so dunno.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Referencing MinIO's support for the Keycloak admin API, will clean this up.

- Replace ``KEYCLOAK_IDENTIFIER`` with the name of the configured Keycloak IDP.
You can use :mc-cmd:`mc admin idp openid list` to view all configured IDP configurations on the MinIO deployment

- Specify the Keycloak admin URL to :mc-conf:`keycloak_admin_url <identity_openid.keycloak_admin_url>` configuration setting
Copy link
Collaborator

Choose a reason for hiding this comment

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

Specify... to reads a bit oddly to me, although I'm not sure what might be better given I'm not familiar with the context.
(Here and also next item.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Idea's @djwfyi ? I use "Specify" a lot.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To clarify my comment, it's more about the to bit. "Specify thing to URL." I think I'd be more likely to write it as "specify thing for URL."

Maybe that better describes what I was thinking? I'm not entirely certain what keycloak_admin_url is, so perhaps I'm the one misunderstanding.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe

Specify the Keycload admin URL in the ...

I agree that the to phrasing catches me by surprise.

Copy link
Collaborator Author

@ravindk89 ravindk89 left a comment

Choose a reason for hiding this comment

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

@djwfyi @feorlen

OK back to both of you. I think that should be everything, but a bit more:

  • Added the AssumeRoleWithCustomToken STS API + a new addition to the Administration -> Identity Management - > MinIO Identity Management Plugin

Hopefully we're close to done here. @donatello I figure you might be busy, but if you can find 30 minutes this week to look over this that would be appreciated. Otherwise we will push this up and refine later.

]
}
The ``${jwt:PreferredUsername}`` substitutes in the username to that portion of the policy.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made some tweaks to your suggestion, mostly for active voice.

]
}
The ``${jwt:PreferredUsername}`` substitutes in the username to that portion of the policy.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also added the bit about policy evaluation in .

need to specify this field to ensure the OIDC provider returns the
authentication response to the correct URL.
Ensure you start the MinIO Server with the :mc-cmd:`~minio server --console-address` option to set a static Console listen port.
The default behavior with that option omitted is to select a random port number at startup.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is a nasty surprise.

Our deploy tutorials always set the console port explicitly, as does K8s by default - so this should be something sufficiently covered.


* - ``_KEYCLOAK``
- Replace the suffix ``_KEYCLOAK`` with a unique identifier for this Keycloak configuration.
For example, ``MINIO_IDENTITY_OPENID_CONFIG_URL_KEYCLOAK_PRIMARY``.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed - made the change here, at the least. We can make this the de facto approach moving forward and clean things up as we go.

- Specify the Keycloak client ID and secret configured in Step 1

* - :envvar:`DISPLAY_NAME <MINIO_IDENTITY_OPENID_DISPLAY_NAME>`
- Specify the user-facing name the MinIO Console should displays as part of the Single-Sign On (SSO) workflow for the configured Keycloak service
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed throughout, wherever I could find it at least

.. note::

Some Kubernetes deployments may experience issues with timeouts during port-forwarding operations with the Operator Console.
Use one of the methods in the other tabs on this section as a work-around.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

}
}
MinIO would revoke access for an authenticated user if the returned value has ``enabled: false``, or if the Admin API returned no matching user.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe it means the user specifically does not exist.

This deals with the following timeline:

T=0: User foo authenticates successfully to keycloak
T=1: STS Token generated with 1 hour validity
T=2: User foo deleted from KeyCloak
T=10: MinIO revokes STS token based on Admin API

Either way, simplified a bit.

MinIO would revoke access for an authenticated user if the returned value has ``enabled: false``, or if the Admin API returned no matching user.

If you do not enable this functionality, the earliest point in time that MinIO could disable access for a disabled or removed user is when the last retrieved authentication token expires.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Referencing MinIO's support for the Keycloak admin API, will clean this up.

- Replace ``KEYCLOAK_IDENTIFIER`` with the name of the configured Keycloak IDP.
You can use :mc-cmd:`mc admin idp openid list` to view all configured IDP configurations on the MinIO deployment

- Specify the Keycloak admin URL to :mc-conf:`keycloak_admin_url <identity_openid.keycloak_admin_url>` configuration setting
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Idea's @djwfyi ? I use "Specify" a lot.

Set the following :ref:`environment variables <minio-server-envvar-external-identity-management-openid>` in the appropriate configuration location, such as ``/etc/default/minio``.

The following example code sets the minimum required environment variables related to enabling the Keycloak Admin API for an existing Keycloak configuration.
Replace the suffix ``_KEYCLOAK`` with the unique identifier for the target Keycloak configuration.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using your suggestion of PRIMARY_IAM

Copy link
Collaborator

@djwfyi djwfyi left a comment

Choose a reason for hiding this comment

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

Nibbles and bits throughout, but nothing really substantive that I would need to see.
Approving it, with the expectation you have the noted fixes in place before you merge it. 😄

Stunning amount of work in this one. 🛌


The login flow for an application is as follows:

1. Make a POST request :ref:`minio-sts-assumerolewithcustomtoken` API.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need either a "using" or "with" here.

  • Make a POST request using the ...
  • Make a POST request with the ...

Creating Policies to Match Claims
---------------------------------

Use either the MinIO Console *or* the :mc:`mc admin policy` command to create policies that match one or more claim values.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could link MinIO Console to :ref:minio-console-admin-policies in the Console > Security and Access page.

]
}
MinIO replaces the ``${ldap:username}`` variable in the ``Resource`` field with the value of the authenticatd user's ``name``.
Copy link
Collaborator

Choose a reason for hiding this comment

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

authenticatd needs its missing e.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should link to this from the table on the parent STS page.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I checked and it is linked. I think. Maybe it wasn't in staging?


The MinIo Security Token Service (STS) ``AssumeRoleWithCustomToken`` API endpoint generates a token for use with the :ref:`minio-external-identity-management-plugin`.

This page documents the ``AssumeRoleWithCustomToken`` API endpoint.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd cut this. It doesn't add anything that isn't already obvious, and the previous graf is enough of an intro.

:start-after: start-configure-keycloak-minio-envvar
:end-before: end-configure-keycloak-minio-envvar

You must restart the MinIO deployment for the changes to apply.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe

Restart the MinIO deployment to apply the changes.

:depth: 2

MinIO supports offloading identity management onto one of the following supported IDentity Providers (IDP):
MinIO supports multiple external identity managers through the following supported IDentity Providers (IDP):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd drop supported at the end of the line. You already said "MinIO supports" at the beginning.

- Replace ``KEYCLOAK_IDENTIFIER`` with the name of the configured Keycloak IDP.
You can use :mc-cmd:`mc admin idp openid list` to view all configured IDP configurations on the MinIO deployment

- Specify the Keycloak admin URL to :mc-conf:`keycloak_admin_url <identity_openid.keycloak_admin_url>` configuration setting
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe

Specify the Keycload admin URL in the ...

I agree that the to phrasing catches me by surprise.

*Optional*
.. include:: /includes/common-minio-external-auth.rst
] .. include:: /includes/common-minio-external-auth.rst
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like this open bracket is out of place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is a Ctrl + ] whoopsie

curl -H "Authentication: Bearer ACCESS_TOKEN_VALUE" \
http://keycloak-url:port/admin/realms/REALM/users/UUID
Replace the UUID with the unique ID for the user which you want to retrieve.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe

Replace UUID with the unique ID ...

djwfyi added a commit that referenced this pull request Mar 24, 2023
- updates list of metrics to add new IDP plugin metrics
- Adds info about expiring sts tokens

Depends on two topics added in PR #735 for internal links.

Closes #746
djwfyi added a commit that referenced this pull request Mar 24, 2023
- updates list of metrics to add new IDP plugin metrics
- Adds info about expiring sts tokens

Depends on two topics added in PR #735  for internal links.

Closes #746
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review Assigned Reviewers Must LGTM To Close

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants