Replies: 2 comments 6 replies
-
|
Thank you for opening this discussion. While I won't comment on SAML so much as I am no expert on this (this would be @mposolda), I comment on the general contribution process a bit more. It would be good to understand the business value of this change, and the scenarios it enables. Also if this is a common or a niche feature. What I find in the referenced repo is still quite technical, and could possibly be extended. Also explaining why eHerkenning or DigiD are important for NL (?) users. A link to the SAML standard might be helpful to understand the functionality that is provided here. Citing the text I found in the repo below:
Feel free to update the description at the top. When looking into PRs, it is usually the question if this can be broken down to functionally independent PRs that incrementally add the functionality. We also ask for integration tests (no mocks) included in the PRs. While we currently move to the new test suite, AFAIK all SAML tests are still in the old test suite. Please let me know your thoughts about this. |
Beta Was this translation helpful? Give feedback.
-
|
Hi Alexander, Thank you for your clear response. I have taken the liberty to embed your questions and/or concerns in the initial post of this discussion, as I assumed that is what you meant with "update the description at the top."
Would you like any additional clarifications and/or alterations? |
Beta Was this translation helpful? Give feedback.
-
|
@at-first8 - thank you for updating the initial post, this is what I had in mind. It explains the business value very well. I also assume (but not stated here) that it is all part of a SAML standard or extension, so it will be useful outside of this use case as well. I'm happy to support you on this journey, still the team that owns the SAML parts and has the most insights into SAML is lead by @mposolda is working on a tight schedule and they will need to make time to review the contribution. Maybe @rmartinc can add a comment here assessing the above. Once there is a feedback from one of them, I'd say you are good to start. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for the proposal @at-first8! Here are a few comments/questions.
The screenshots you provided are hard to compare as there are many options. Are you suggesting adding some new options? Or you suggest restructuring the old page to improve UX? If the second, I think it makes sense as at the moment the configuration page is quite messy. If not, could you please point out where to look at and what you added?
What is the primary reason the extension is needed? Is it some missing part of specification we do not implement? Or is it about the UI improvement?
Do you mean Artifact binding by the
Could you be more specific on where you suggest to decrypt encrypted elements? I can see we already support receiving encrypted elements when acting as SP as we have tests for it here.
I looked into the source code of the extension and it seems you are changing a lot of files. Big pull requests are very hard to review and overall time to get it merged can grow a lot. If you struggle to split the change you can send a draft PR and we can discuss how it can be split into multiple reasonable sized improvements. |
Beta Was this translation helpful? Give feedback.
-
|
@at-first8 - I know that Keycloak is AFAIK not very good in listing its own capabilities, so we're asking for more than we currently provide. The current code of the extension seems duplicate a lot of the existing codes, and then adds distinct bits to it to add the new functionality. Still it is a bit hard to anticipate the size of the PR, and with that if it should be broken down. Maybe a draft pull request wich shows the changes to the existing class (in its initial form without additional tests) could show us the amount of code changes planned. |
Beta Was this translation helpful? Give feedback.
-
|
Dear @ahus1 thank you for inspecting the code w.r.t. an effective PR approach. We will provide a draft PR in its initial form without additional tests to facilitate assessing whether it should be broken down or not. |
Beta Was this translation helpful? Give feedback.
-
Dear @ahus1 we took the liberty to provide a draft pull request over here. Would you be willing to decide whether this PR can be accepted as a single PR, or whether we should split it up, and if yes, whether you have a preference on how we should split up the pull request. If so, we would be happy to process your feedback and work on splitting up the pull request or developing the (integration) tests. |
Beta Was this translation helpful? Give feedback.
-
|
Thank you for the draft PR. It turned out to be larger than I anticipated and hoped. Given its size, it might need scheduling and prioritization in the core-clients team first. Given the already existing roadmap, this might take months, and I feel sorry for raising false hopes. I'll leave it to @mposolda to comment when/how this can be added to the roadmap. |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
0. Context
After this pull request was merged to list this extended SAML identity provider plugin, we would be happy to include the extended SAML identity provider code into future Keycloak distributions.
1. Proposed Changes (high-level)
1.1 Keycloak 26.0.0 (default) Admin Console
Below is an example of the unmodified Keycloak 26.0.0 admin console (with most settings on
Trueto display all options).1.2 Keycloak 26.0.0 Admin Console as generated with the extended SAML identity provider
2. Business Value
The business value of the proposed pull request is split up into primary and secondary values.
2.1 Primary Business value/Scenario
The proposed pull requests helps companies that use Keycloak to natively provide support to use the 2 default ways of logins provided by the Dutch government [1]:
Currently, if those companies want to allow their users to login with either DigiD/eHerkenning, they need a plugin that modifies, amongst others, the Keycloak interface (in essence the plugin in question). So the primary business value is that all these companies can just use the default Keycloak version for this, which also makes upgrading easier.
Regarding the why eHerkenning or DigiD are important for NL(?) users question:
To make it simple, if you are Dutch and want to do anything government related, your login is DigiD or eHerkenning for companies.
Regarding the common or niche question:
[1] This pertains to roughly [16.8](https://www.logius.nl/onze-dienstverlening/domeinen/toegang/digid/digid-door-de-jaren-heen million DigiD users, which did a combined 550 million logins in 2024 (though not all, or perhaps mostly not, via Keycloak) (apologies for the Dutch references).
Regarding the helpful to understand the functionality that is provided here question:
The functionality that is provided is adding and retrieving the backchannel token. Additionally, we add decryption of encrypted SAML elements.
2.2 Secondary Business Value/Scenario
Since the plugin/proposed pull request provides a functional extension on the SAML protocol, it can also be used by Keycloak users/operators directly, or as jump-off point, to facilitate different login protocols that extend the default SAML functionality.
Furthermore, the provided plugin supports eIDAS. I would not be surprised if this becomes the European standard for government-related logins for civilians, however that is speculative.
3. Proposed Approach
We propose to implement the changes listed above, by modifying the already existing extended SAML identity provider extension, such that it is embedded in Keycloak 26.0.0. whilst adhering to the Keycloak Contribution Guide, and to generate a pull request that may be reviewed.
3.1 Separate Smaller PRs vs 1 Larger PR
We understand the added value of smaller modular PRs. Nevertheless, our preference goes out to a single PR that contains the extended (direct synchronous) SAML requests to external providers like eHerkenning or Digid.
3.2 Testing
We propose to include integration tests with the PR using the old test suite.
If you would prefer us to specify the changes in a higher level of detail before implementing them, or would suggest an alternative approach and/or changes, please let us know. Otherwise we would like to kindly ask: _do you agree on this approach (where of course the PR still needs to be reviewed and/or approved)?
Beta Was this translation helpful? Give feedback.
All reactions