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

Include user's privileges actions in IdP plugin _has_privileges request #104026

Merged
merged 5 commits into from Jan 11, 2024

Conversation

s-nel
Copy link
Contributor

@s-nel s-nel commented Jan 8, 2024

Currently the identity provider (IdP) plugin pulls and caches a given application's privileges' actions in ApplicationActionsResolver which are used to construct a _has_privileges request and each that matches the service provider's template will be returned as roles in the SAML response. The problem with this approach is that it misses actions defined in roles that may not be present in the predefined application privileges. For example given this role:

{
    "application": [
        {
            "application": "elastic-cloud",
            "privileges": ["sso:custom"],
            "resources": "*"
        }
    ]
}

and service provider that matches sso:(.*). Unless sso:custom is returned in GET /_security/privilege/elastic-cloud, custom role will not be present in the SAML response.

This PR pulls user privileges from the GET /_security/user/_privileges response and includes all actions with the matching application in the _has_privileges request alongside the cached actions.

@s-nel s-nel self-assigned this Jan 8, 2024
@elasticsearchmachine elasticsearchmachine added v8.13.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Jan 8, 2024
@s-nel s-nel marked this pull request as ready for review January 8, 2024 03:16
@elasticsearchmachine elasticsearchmachine added the needs:triage Requires assignment of a team area label label Jan 8, 2024
@s-nel s-nel requested a review from tvernum January 8, 2024 03:16
@s-nel s-nel added Team:Security Meta label for security team >enhancement labels Jan 8, 2024
@elasticsearchmachine elasticsearchmachine removed the Team:Security Meta label for security team label Jan 8, 2024
@s-nel s-nel added Team:Security Meta label for security team and removed needs:triage Requires assignment of a team area label labels Jan 8, 2024
@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label and removed Team:Security Meta label for security team labels Jan 8, 2024
@tvernum tvernum added the :Security/IdentityProvider Identity Provider (SSO) project in X-Pack label Jan 9, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Jan 9, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@elasticsearchmachine elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Jan 9, 2024
@elasticsearchmachine
Copy link
Collaborator

Hi @s-nel, I've created a changelog YAML for you.

@s-nel s-nel added the cloud-deploy Publish cloud docker image for Cloud-First-Testing label Jan 10, 2024
@s-nel s-nel closed this Jan 10, 2024
@s-nel s-nel reopened this Jan 10, 2024
@s-nel s-nel added v8.12.1 cloud-deploy Publish cloud docker image for Cloud-First-Testing and removed external-contributor Pull request authored by a developer outside the Elasticsearch team cloud-deploy Publish cloud docker image for Cloud-First-Testing labels Jan 10, 2024
Copy link
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

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

code itself LGTM

however, not sure I understand the functional side or why this change is needed.

Unless sso:custom is returned in GET /_security/privilege/elastic-cloud, custom role will not be present in the SAML response.

Why would sso:custom NOT be returned by GET /_security/privilege/elastic-cloud ?

that it misses actions defined in roles that may not be present in the predefined application privileges

Isn't that a good thing ? The input to get the application privs is the application name and why would we want to get all the of the them in the role ? (I see the code filters the role results by application name, so no worries about the code, just trying to understand why this change is needed)

@jakelandis
Copy link
Contributor

Also tangential to this change ... I noticed that we are doing all of this work on the transport thread. The new action call is all local, so it probably doesn't add much overhead but long term we probably want to fork earlier here to get off the transport thread to allow for more concurrent throughput. As-is we we are not getting much benefit from NIO or the overhead of the creating all of these listeners. (we could probably also do some of this work in parallel too)

@s-nel
Copy link
Contributor Author

s-nel commented Jan 10, 2024

Why would sso:custom NOT be returned by GET /_security/privilege/elastic-cloud ?

GET /_security/privilege/elastic-cloud only returns predefined named privilege -> actions mappings. Since sso:custom is an action that only exists in the role it's not returned.

The background is that we want to give users the ability to specify a role that they will get during SSO. This could be any custom role that they've configured and we don't know it in advance. Our approach to support this is to add the action to the user's role. For example

{
    "custom-sso-role": {
        "cluster": [],
        "indices": [],
        "applications": [
            {
                "application": "elastic-cloud",
                "privileges": [
                    "deployment-viewer",
                    "deployment:sso-custom" // <-- this privilege should allow the user to SSO with role `custom`
                ],
                "resources": [...]
            }
        ]
    }
}

Also tangential to this change ... I noticed that we are doing all of this work on the transport thread. The new action call is all local, so it probably doesn't add much overhead but long term we probably want to fork earlier here to get off the transport thread to allow for more concurrent throughput. As-is we we are not getting much benefit from NIO or the overhead of the creating all of these listeners. (we could probably also do some of this work in parallel too)

Are you suggesting that I make the change in this PR?

@jakelandis
Copy link
Contributor

Caught up with @s-nel in slack...thanks for the additional context !

Why would sso:custom NOT be returned by GET /_security/privilege/elastic-cloud ?

I see now that the idea is the privilege (sso:custom) will not be registered with the application (elastic-cloud). If it was registered it then this PR would not be needed but we could also introduce a possible issue with the cache and introduce a performance issue since we run all registered privileges through a has_privileges check. Since the number of custom privileges are essentially unbounded we need a way to ensure we don't blow up the cache or performance. The change here allows us not register the privilege with the application and instead read the privilege from the role directly. This mitigates the issue since we will not greatly expand the number of privileges for the application and we can grab this new information from the user's role (in the security cluster) directly. It does slightly muddy the water w.r.t. application level privs since for this specific workflow you can you use privs for an application without declaring those privs for the application ...but in the context of the end goal this makes sense. The end result is that we will have pre-defined privileges registered with the application and purely custom roles (represented as privileges that are then are reflected in the SAML assertion used for role mapping) that live only in the role (in the security cluster).

Are you suggesting that I make the change in this PR?

No .. sorry that was not clear. Just something I noticed while reviewing this PR... that change would be way outside the scope of this change.

Copy link
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

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

LGTM

@s-nel s-nel merged commit 1e1d151 into elastic:main Jan 11, 2024
19 of 20 checks passed
jedrazb pushed a commit to jedrazb/elasticsearch that referenced this pull request Jan 17, 2024
…uest (elastic#104026)

* Include user's privileges actions in IdP plugin has privileges request

* Update docs/changelog/104026.yaml

* Use `GroupedActionListener` instead of nested listeners

Co-authored-by: Tim Vernum <tim@adjective.org>

* Fixes after applying review suggestion

* Fix IT flakiness

---------

Co-authored-by: Tim Vernum <tim@adjective.org>
@s-nel s-nel removed the v8.12.1 label Feb 13, 2024
s-nel added a commit to s-nel/elasticsearch that referenced this pull request Feb 14, 2024
…uest (elastic#104026)

* Include user's privileges actions in IdP plugin has privileges request

* Update docs/changelog/104026.yaml

* Use `GroupedActionListener` instead of nested listeners

Co-authored-by: Tim Vernum <tim@adjective.org>

* Fixes after applying review suggestion

* Fix IT flakiness

---------

Co-authored-by: Tim Vernum <tim@adjective.org>
s-nel added a commit that referenced this pull request Feb 14, 2024
…uest (#104026) (#105522)

* Include user's privileges actions in IdP plugin has privileges request

* Update docs/changelog/104026.yaml

* Use `GroupedActionListener` instead of nested listeners



* Fixes after applying review suggestion

* Fix IT flakiness

---------

Co-authored-by: Tim Vernum <tim@adjective.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cloud-deploy Publish cloud docker image for Cloud-First-Testing >enhancement :Security/IdentityProvider Identity Provider (SSO) project in X-Pack Team:Security Meta label for security team v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants