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.

Sign up for GitHub

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

Jump to bottom

[Security] Add OIDC Discovery #20579

Open
wants to merge 1 commit into
base: 7.3
Choose a base branch
from
Open

[Security] Add OIDC Discovery #20579

wants to merge 1 commit into from
+145 −0

Conversation

vincentchalamon
Copy link
Contributor

@carsonbot carsonbot added this to the 7.3 milestone Jan 17, 2025
@vincentchalamon vincentchalamon mentioned this pull request Jan 17, 2025
Merged
7 tasks
@javiereguiluz javiereguiluz added the Waiting Code Merge Docs for features pending to be merged label Jan 17, 2025
@carsonbot carsonbot modified the milestones: 7.3, next Jan 17, 2025
fabpot added a commit to symfony/symfony that referenced this pull request Mar 1, 2025
@fabpot
feature #54932 [Security][SecurityBundle] OIDC discovery (vincentchal…
8b9ed36 
…amon)

This PR was merged into the 7.3 branch.

Discussion
----------

[Security][SecurityBundle] OIDC discovery

This PR introduces [OIDC discovery](https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfigurationResponse) on `oidc` and `oidc_user_info` token handlers.

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Issues        | Fix #50433 Fix #50434
| License       | MIT
| Doc PR | symfony/symfony-docs#20579 |

### TODO

- [x] use JWSLoader in OidcTokenHandler
- [x] introduce OidcUserInfoDiscoveryTokenHandler
- [x] introduce OidcDiscoveryTokenHandler
- [x] update src/**/CHANGELOG.md files
- [x] update UPGRADE-*.md files
- [x] add tests on AccessTokenFactoryTest with discovery
- [x] create documentation PR

### What is OIDC Discovery?

[OIDC discovery](https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfigurationResponse) is a generic endpoint on the OIDC server, which gives any public information such as signature public keys and endpoints URIs (userinfo, token, etc.). An example is available on the API Platform Demo:
https://demo.api-platform.com/oidc/realms/demo/.well-known/openid-configuration.

Using the OIDC discovery simplifies the `oidc` security configuration, allowing to just configure the discovery and let Symfony store the configuration and the keyset in cache. For instance, if the _userinfo_endpoint_ or _signature keyset_ change on the OIDC server, no need to update the environment variables in the Symfony application, just clear the corresponding cache and it'll retrieve the configuration and the keyset accordingly on the next request.

In the `oidc_user_info` security configuration, it does the same logic but only about _userinfo_endpoint_ as this token handler doesn't need the _keyset_.

### How Do I Use This New Feature in Symfony?

The current `oidc` token handler configuration requires a `keyset` option which may change on the OIDC server. It is configured as following:
```yaml
# config/packages/security.yaml
security:
    firewalls:
        main:
            access_token:
                oidc:
                    claim: 'email'
                    audience: 'symfony'
                    issuers: ['https://example.com/']
                    algorithms: ['RS256']
                    keyset: '{"keys":[{"kty":"EC",...}]}'
```

> Note: those parameters should be configured with environment variables.

With the `discovery` option, Symfony will retrieve the `keyset` directly from the OIDC discovery URI and store it in a cache:
```yaml
# config/packages/security.yaml
security:
    firewalls:
        main:
            access_token:
                oidc:
                    # 'keyset' option is not necessary anymore as it's retrieved from OIDC discovery and stored in cache
                    claim: 'email'
                    audience: 'symfony'
                    issuers: ['https://example.com/']
                    algorithms: ['RS256']
                    discovery:
                        base_uri: 'https://example.com/oidc/realms/master/'
                        cache:
                            id: cache.app # require to create this cache in framework.yaml
```

> Note: some other parameters might be retrieven from the OIDC discovery, maybe 'algorithm' or 'issuers'. To discuss.

The current `oidc_user_info` token handler required a `base_uri` corresponding to the _userinfo_endpoint_ URI on the OIDC server. This URI may change if it's changed on the OIDC server. Introducing the discovery helps to configure it dynamically.

The current configuration looks like the following:
```yaml
# config/packages/security.yaml
security:
    firewalls:
        main:
            access_token:
                oidc_user_info:
                    # 'base_uri' is the userinfo_endpoint URI
                    base_uri: 'https://example.com/oidc/realms/master/protocol/openid-connect/userinfo'
                    claim: 'email'
```

With the `discovery`, it will look like this:
```yaml
# config/packages/security.yaml
security:
    firewalls:
        main:
            access_token:
                oidc_user_info:
                    # 'base_uri' can be the userinfo_endpoint for backward compatibility
                    # and can be the OIDC server url in addition of 'discovery' option
                    base_uri: 'https://example.com/oidc/realms/master/'
                    claim: 'email'
                    discovery:
                        cache:
                            id: cache.app # require to create this cache in framework.yaml
```

Commits
-------

93f369a feat(security): OIDC discovery
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Security Status: Needs Review Waiting Code Merge Docs for features pending to be merged
Projects
None yet
Milestone
next
Development

Successfully merging this pull request may close these issues.
3 participants
@vincentchalamon @javiereguiluz @carsonbot