SAML2 Service Provider Pull Request

[cmrockwell]

The intent of this PR is to get it reviewed and provide me with valuable feedback about making it better, and ideally getting direct support from Sling Developers.

Work left to do

1. Sync attributes released by the IDP
2. Create a metadata provider servlet
3. Consider whether use of SAML2ConfigService and SAML2ConfigServiceImpl is a good design or not.

• Pros: Having configs as a service allows Saml2UserMgtServiceImpl and
  AuthenticationHandlerSAML2 to reference the same configuration.
• Cons: Other bundles installed could access the JKS information configured.
  @reference
  private SAML2ConfigService saml2ConfigService;

4. If o.a.s.auth.form exported TokenStore and SessionStorage then these could be removed. But as it stands for this Whiteboard PR that is not currently the case.

My assumption about the purpose of a Sling Whiteboard is it is similar to a whiteboard in an office. An area to sketch ideas. Stuff gets merged into the Whiteboard that dies there, and maybe a few things get promoted. If this gets promoted, then I assume the questions listed above and a lot more would need answers.

https://issues.apache.org/jira/browse/SLING-9397

[rombert]

I've been thinking some more about how to make sure the reviews are productive and reduce the time needed to get this into the whiteboard.

The idea I came up with takes two complementary approaches:

1. Simplify testing
2. Reduce the amount of submitted code

For item 1 I suggest that you provide a docker script or docker-compose setup that launches a SAML-enable identity provider. One idea would be Keycloak with Docker, but I admit I'm not at all familiar with identity providers to offer an informed suggestion.

This would allow you to drop ~400 LOC in the idp package and maybe some other parts that are only used from there.

For item 2 item 1 already helps :-) I think you can start with submitting the minimal functionality that works - and I get that is the AuthenticationHandler.

I see code for the ExternalIdentityProvider and LoginModule as well. If that is not needed for the basic login flow, I would suggest submitting them as follow-up PRs once we merge the initial one.

I also see a potential of dropping some code with the TokenStore class. You mention it's derived from the class in the o.a.s.auth.form bundle. Maybe that's something we can export, or you can inline the class in this bundle. The code looks quite complex and is large, I think it's a good idea to keep the duplication out of the bundle.

Would that work for you?

[cmrockwell]

Having a built-in IDP during development was really useful and simplified the setup and testing. I get the point is that there should be another way to test the SP code without the demo IDP. I'm curious whether you tried to test it with the internal IDP. This would be the simplest way.

I thought the mock IDP might potentially help in troubleshooting issues, which is why I considered an enable/disable switch for it. Nevertheless, the internal IDP and Saml2ExternalIdentityProvider can be removed from the PR. It wont work without Saml2LoginModule as it stands today, but let me know what code changes would allow it to work without a login module.

I will keep the internal IDP on a dev branch, because it is so handy. But otherwise could remove some code from the PR. At the same time I still need to add code for a few features.

1. User attribute synchronization: allows setting of user properties like email, given and family names, address, etc based on the data contained in the Assertion.

2. SP metadata provider servlet: this should simply IDP configuration since most IDP's allow
   SP's to be registered by inputing the metadata. Without this admins might need to craft the data by hand

There is going to be some code to review because there's a lot involved with implementing a SAML2 Service Provider authentication handler. Do you have a target number for LOC?

[rombert]

@cmrockwell - if you think the IDP is useful, let's keep it in for now. I am trying to make my review simpler, but if that makes you PR submission harder it does not make sense.

And no, there is no target LOC.

[cmrockwell]

Thanks, that's kind Robert.  The demo IDP doesn't offer much if any value to end users. If it creates confusion, uncertainty or concern; then there is no reason to push it. As a developer, I can push it as a separate bundle to my localhost if I want.  The demo IDP has been removed, so please take a look if things look a little cleaner now. Cris On Tue, Apr 14, 2020 at 3:20 AM, Robert Munteanu<notifications@github.com> wrote: @cmrockwell - if you think the IDP is useful, let's keep it in for now. I am trying to make my review simpler, but if that makes you PR submission harder it does not make sense. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[klcodanr]

@cmrockwell @rombert I wonder if we want to merge this PR and then further refine with additional commits / PRs?

[rombert]

@klcodanr - we would need at least a "legal" review in terms of code attribution and ownership. But I agree, we should not polish code indefinitely, but instead be more agile with reviewing.

[cmrockwell]

I've added attribution and the original license from webprofile-ref-project-v3. I've also reached out to Stefan Rasmusson to see whether this is sufficient.

https://blog.samlsecurity.com/p/opensaml.html?showComment=1587589973632

[rombert]

Thanks a lot @cmrockwell . I see that the LICENSE file is there, so that is good. I am not sure if we need a NOTICE file though. I am trying to go through https://www.apache.org/dev/licensing-howto.html#alv2-dep , especially the section titled Bundling an Apache-2.0-licensed dependency.

From an ASF point of view it does not seem that we need to change the NOTICE file, since the third party repository does not have a NOTICE file itself. A notice file should be generated automatically with the build ( but the build fails out-of-the-box for me). BTW, the possible names for the notice are NOTICE (preferred) and NOTICE.txt.

The remaining question is how and if we need to add attribution to the source code files. Maybe @bdelacretaz or @cziegeler have an idea?

Bertrand, Carsten - @cmrockwell has based some of his submission on code from https://bitbucket.org/srasmusson/webprofile-ref-project-v3, which is Apache-2.0 licensed. The changes are currently listed in https://github.com/apache/sling-whiteboard/blob/8386886dbe241020e11ae8c75ca487a204fe0bbf/saml-handler/NOTICE.md ( although I consider we don't need anything in the NOTICE file, see above ). How would you recommend that we record this attribution?

---

@cmrockwell - once clarified I think we can merge this and then iterate. I suggest that we can keep a record of open items. This can be a simple TODO file in the repository root, a Jira task titled 'SAML authentication handler initial contribution', or anything else really. What would you prefer?

Finally, I have not found an ICLA on file for you, I would encourage you to file on ( see https://www.apache.org/licenses/contributor-agreements.html ) as you seem to be on track for continued contributions.

[cziegeler]

As the code is ASL2 and does not require a notice or anything else, we don't need to mention in. But I think its usually good style to do so and have a single sentence in our NOTICE that we include (modified) code from ... which has ASL2 as the license

[cmrockwell]

@rombert I updated the NOTICE based on my understanding of Carsten's comment.
Created SLING-9397 to track the TODO items related to this initial PR.
Also submitted my ICLA.

Please let me know what the build issue is, and what if any git operations were preformed.
Does your ~/.m2/settings.xml have any mirrors? If so you might add an exception for shibboleth libraries.

!shibboleth,*

[cmrockwell]

@rombert

but the build fails out-of-the-box for me

Please post more about that

[rombert]

@cmrockwell - what I get is

[WARNING] SystemTray is not supported, skipping notifications...
[INFO] Scanning for projects...
[INFO] 
[INFO] ------------< org.apache.sling:org.apache.sling.auth.saml2 >------------
[INFO] Building SAML2 Service Provider 0.1.0-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
Downloading from shibboleth: https://build.shibboleth.net/nexus/content/groups/public/org/relaxng/com.springsource.org.relaxng.datatype/1.0.0/com.springsource.org.relaxng.datatype-1.0.0.pom
Downloading from central: https://repo.maven.apache.org/maven2/org/relaxng/com.springsource.org.relaxng.datatype/1.0.0/com.springsource.org.relaxng.datatype-1.0.0.pom
[WARNING] The POM for org.relaxng:com.springsource.org.relaxng.datatype:jar:1.0.0 is missing, no dependency information available
Downloading from shibboleth: https://build.shibboleth.net/nexus/content/groups/public/org/relaxng/com.springsource.org.relaxng.datatype/1.0.0/com.springsource.org.relaxng.datatype-1.0.0.jar
Downloading from central: https://repo.maven.apache.org/maven2/org/relaxng/com.springsource.org.relaxng.datatype/1.0.0/com.springsource.org.relaxng.datatype-1.0.0.jar
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  1.351 s
[INFO] Finished at: 2020-05-04T15:52:37+02:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal on project org.apache.sling.auth.saml2: Could not resolve dependencies for project org.apache.sling:org.apache.sling.auth.saml2:jar:0.1.0-SNAPSHOT: Could not find artifact org.relaxng:com.springsource.org.relaxng.datatype:jar:1.0.0 in shibboleth (https://build.shibboleth.net/nexus/content/groups/public) -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/DependencyResolutionException

I have no settings.xml. I am running with

Apache Maven 3.6.3 (SUSE 3.6.3-2.1)
Maven home: /usr/share/maven
Java version: 11.0.7, vendor: Oracle Corporation, runtime: /usr/lib64/jvm/java-11-openjdk-11
Default locale: en_US, platform encoding: UTF-8
OS name: "linux", version: "5.6.6-1-default", arch: "amd64", family: "unix"

At any rate, we'll have to solve the issue of external artifacts at some points, we should not have any custom repositories set.

I've merged this in now, let's focus on getting the build passing so we can start validating the functionality.

[cmrockwell]

Thanks for posting. Here is what the OpenSAML (Shibboleth) developers say about Maven Central.
https://wiki.shibboleth.net/confluence/display/DEV/Use+of+Maven+Central

Maven itself has no support for validating signatures of artifacts (be they signed jars or jars with a detached PGP signature).... If other people aren't worried about the veracity of the artifacts they use that's on them

[rombert]

Ack, I read that. I understand the reasons. Note that at the ASF the primary source of disitribution are cryptographically signed source releases, not binaries on Maven Central.

That does not change the fact that Maven Central discourages using repositories and pluginRepository elements - see https://central.sonatype.org/pages/requirements.html . If we do this we expose consumers to instability.

There are ways around it, for now we just need to keep in mind that we (probably) should not add extra Maven repositories and fix it before a first release.

[cmrockwell]

Sound good. Thank you for merging this. I'll spend some time to analyze the build issue you identified and report back once I'm able to reproduce it.

Update:
I was able to reproduce the build issue. org.relaxng.datatype is not in Maven Cenral. It comes from Spring. It seems to be an optional dependency from dom4J, so I don't think it is is needed. I removed it, and the bundle activates and works without it.

cmrockwell@81b1068

Assuming it builds for you after this, getting it installed and active in Sling will require your instance to provide org.apache.jackrabbit.oak-auth-external

[rombert]

The build works for me with that change. Can you submit a PR with it?