Improvements to SAML2 authentication. #2593
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
drgrice1
force-pushed
the
add-saml2-auth-fixes-no-plugin
branch
from
aa6c082 to
476fa01
Compare
drgrice1
force-pushed
the
add-saml2-auth-fixes-no-plugin
branch
3 times, most recently
from
5c5f96b to
ce7f36d
Compare
drgrice1
force-pushed
the
add-saml2-auth-fixes-no-plugin
branch
4 times, most recently
from
ec532a4 to
34990c0
Compare
somiaj
approved these changes
Nov 15, 2024
There was a problem hiding this comment.
I suggest we merge this so it can be used.
@ionparticle can you still confirm this meets your needs and you haven't noticed any other issues with it?
|
@somiaj regarding issues, the code has been rewritten, it's very different from what we run in prod, so our experience isn't really applicable. However, this does still meet our needs, as this implementation should work for us and we would prefer upstream maintenance of the auth system. |
drgrice1
force-pushed
the
add-saml2-auth-fixes-no-plugin
branch
from
34990c0 to
df61423
Compare
This has two components, a regular Webwork Authen module and a Mojolicious plugin for SAML2. The plugin implements a SAML2 SP using the Net::SAML2 library. Net::SAML2 claims to be compatible with many SAML2 implementations, including Shibboleth. The intent for this plugin is to replace the old Shibboleth auth that depended on the shibd service which requires Apache's mod_shib. The WeBWorK::Authen::Saml2 authen module's main purpose is to call the Saml2 plugin helper sendLoginRequest() that properly sends the user to the IdP to authenticate. There is a bypass option that allows skipping the Saml2 authen in favour of the authen module after it. So you can, for example, put Basic_TheLastOption after it allow access to the internal username/password auth. It seems to be standard for Mojolicous plugins to be located in lib/Mojolicious/Plugin, so I've put the Saml2 plugin there. Additional detail and configuration instructions can be found in lib/Mojolicious/Plugin/Saml2/README.md The Saml2 plugin will only be loaded if the corresponding conf file exists. Copy conf/authen_saml2.dist.yml to conf/authen_saml2.yml to enable it. 3 settings in the conf are crucial, the idp.metadata_url must be set to the IdP's xml metadata endpoint and a unique sp.cert & sp.signing_key pair should be generated. The example cert and signing_key must not be used for prod under any circumstances. I initially put the config in conf/mojolicious.webwork.yml under a saml2 section but seeing as how other authen modules have their own conf files, I figure it was better to follow that convention. And as there seems to be more work around the yaml config, I've made the saml2 authen conf also a yaml file. The NotYAMLConfig plugin for reading yaml conf files gave me some trouble though, as I didn't want to step on the main conf and had to read the code to figure out how to use just the load conf file functionality. The Saml2 plugin will generate its own xml metadata that can be used by the IdP for configuration. This is available at the /saml2/metadata URL under the default config. Note that endpoints are configurable as I wanted to be able to change them to match shibd endpoints. The Saml2 plugin has its own set of controllers and a router for them. The AcsPostController is the most important one as it handles the SAMLResponse that sends the user back to Webwork from the IdP. The errors aren't the most friendly unfortunately, should probably add a proper 401 error template so users don't see the more scary stacktrace one. Note that unlike shibd, attribute maps are not supported. So you probably have to replace user friendly attribute names like "studentNumber" with URN style names like "urn:mace:dir:attribute-def:studentNumber". You can check your IdP's attribute map xml for the official URN names. Some discussion about alternatives that I tried before settling on the Mojolicious plugin approach: The Saml2 as a plugin idea started after trying out Mojolicious::Plugin::SAML. Mojolicious::Plugin::SAML didn't work out in the end due to two downsides. The major one being a lack of RelayState support. RelayState is the defacto standard way for SPs to store where the user wants to go after returning from a successful auth from the IdP. This is a necessary feature for Webwork as auth to each course is handled separately and we need to know exactly what course the user wants to get into. The minor issue is that it doesn't parse the SAMLResponse attributes for you and I really didn't want to have to muck with xml parsing. Apache mod_proxy using mod_shib and shibd was another possibility. This requires passing shib params through HTTP headers via the use of the ShibUseHeaders setting. Shibboleth documentation says the ShibUseHeaders option should be avoided as they're not confident in the protections against spoofed HTTP header attacks. Running Webwork under mod_perl again so we don't need to use mod_proxy. This resulted in hard to debug issues with a few sections of async code. While I was able to 'unasync' those sections to get the thing running, it feels unmaintainable in the long run, especially if Webwork increases usage of async features.
WEBWORK_ROOT_URL definition in Dockerfiles used two colons before the port when it's supposed to be only 1. Also deleted version line from docker-compose.dist.yml, as I get a warning message that `version` is obsolete.
Mojo::Base's -strict option removed where -signature is present as it makes strict redundant. Use Mojo::JSON's functions instead of JSON. When using WeBWorK::Debug, specify import of debug() function. Remove import of WeBWorK::Debug and Data::Dumper where they're not actually being used. Fix README.md to pass markdownlint inspection.
Reformatted to follow .editorconfig rules.
This fixes some issues with openwebwork#2511. This pull request is built on that one. That pull request is a nice start, but has some issues. Thanks to @ionparticle for starting this work. One of the most important things introduced in that pull request is the development identity provider via SimpleSAMLphp. That gave me a way to work with this. Although I liked the idea of using a Mojolicious plugin, that approach does not fit into the webwork2 course environment system. So, unfortunately, that had to be changed. So this rewrites the SAML2 authentication module to use the usual approach that all of the other authentication modules use. There is a `authen_saml2.conf.dist` file that should be copied to `authen_saml2.conf` to use SAML2 authentication. All settings for the authentication module are in that file. The primary limitation of the plugin approach was that it is not possible for different courses to use different identity providers. Or for one course to use SAML2 authentication and another only use the basic password authentication (without the need to add a URL parameter). This is something that is expected of the authentication modules, and is desirable for multi-institutional servers. Another problem with the implementation is that is does not work with `$session_management_via = "key"` set. The reason that doesn't work is because the implementation broke the rule of creating and accessing a session before authentication is completed. So this reimplementation uses the database via the "nonce" key hack much like LTI 1.1 authentication does. The previous implentation also used the `WEBWORK_ROOT_URL` environment variable in the code. That is not an environment variable that is defined for webwork2. It is only defined for the docker build, and can not be used in the code. The previous implementation also does not work with two factor authentication. If an identity provider does not provide two factor authentication, then webwork2's two factor authentication should be used. Of course, if the identity provider does provide two factor authentication, then the two factor authentication can be disabled in `localOverrides.conf`. An option to enable service provider initiated logout has also been added. It is off by default, but if enabled, when the user clicks the "Log Out" button, a request is sent to the identity provider to also end its session. The `README.md` file that was with the plugin code has been moved to the `docker-config/idp` directory, and is now purely for instructions on how to use the simpleSAMLphp development instance. The readme also includes instructions on how to set up a simpleSAMLphp development instance without docker. The documentation on how to configure and use the SAML2 authentication module is now all in the `authen_saml2.conf.dist` file. Note that the webwork2 service provider certificate files are no longer directly in the configuration file. Instead file locations are to be provided. It didn't really make sense to have the actual certificates in the configuration file, then write them to a file, which the Net::SAML2 module would read. Furthermore, it would be very easy to add the certificates incorrectly to the configuration file. With the YAML file approach if indentation were not correct, then the configuration file would fail to load.
Any route can now specify the methods that are allowed by adding a `methods` key to the route parameters. The value of the key should be a reference to an array containing the allowed methods. The ACS route is the only route that uses this at this point to restrict to the POST method only.
This option is for the case that the identity provider offers multi
factor authentication, and yet the $saml2{bypass_query} is also
allowed. In this case you would not want webwork2's two factor
authentication to be used when signing in via the identity provider.
However, two factor authentication should be used if the bypass query is
used. Setting $saml2{twoFAOnlyWithBypass} to 1 makes it so that
webwork2's two factor authentication is skipped for users signing in via
the identity provider, but still required for users signing in with a
username/password. If this is set to 0, then webwork2's two factor
authentication will always be required.
drgrice1
force-pushed
the
add-saml2-auth-fixes-no-plugin
branch
from
df61423 to
188055f
Compare
Alex-Jordan
approved these changes
Nov 24, 2024
|
I am going to merge this, if SAML users have issues they can report them. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes some issues with #2511. This pull request is built on that one. That pull request is a nice start, but has some issues. Thanks to @ionparticle for starting this work. One of the most important things introduced in that pull request is the development identity provider via SimpleSAMLphp. That gave me a way to work with this.
Although I liked the idea of using a Mojolicious plugin, that approach does not fit into the webwork2 course environment system. So, unfortunately, that had to be changed. So this rewrites the SAML2 authentication module to use the usual approach that all of the other authentication modules use. There is a
authen_saml2.conf.distfile that should be copied toauthen_saml2.confto use SAML2 authentication. All settings for the authentication module are in that file.The primary limitation of the plugin approach was that it is not possible for different courses to use different identity providers. Or for one course to use SAML2 authentication and another only use the basic password authentication (without the need to add a URL parameter). This is something that is expected of the authentication modules, and is desirable for multi-institutional servers.
Another problem with the implementation is that is does not work with
$session_management_via = "key"set. The reason that doesn't work is because the implementation broke the rule of creating and accessing a session before authentication is completed. So this reimplementation uses the database via the "nonce" key hack much like LTI 1.1 authentication does.The previous implentation also used the
WEBWORK_ROOT_URLenvironment variable in the code. That is not an environment variable that is defined for webwork2. It is only defined for the docker build, and can not be used in the code.The previous implementation also does not work with two factor authentication. If an identity provider does not provide two factor authentication, then webwork2's two factor authentication should be used. Of course, if the identity provider does provide two factor authentication, then the two factor authentication can be disabled in
localOverrides.conf.An option to enable service provider initiated logout has also been added. It is off by default, but if enabled, when the user clicks the "Log Out" button, a request is sent to the identity provider to also end its session.
The
README.mdfile that was with the plugin code has been moved to thedocker-config/idpdirectory, and is now purely for instructions on how to use the simpleSAMLphp development instance. The readme also includes instructions on how to set up a simpleSAMLphp development instance without docker. The documentation on how to configure and use the SAML2 authentication module is now all in theauthen_saml2.conf.distfile.Note that the webwork2 service provider certificate files are no longer directly in the configuration file. Instead file locations are to be provided. It didn't really make sense to have the actual certificates in the configuration file, then write them to a file, which the Net::SAML2 module would read. Furthermore, it would be very easy to add the certificates incorrectly to the configuration file. With the YAML file approach if indentation were not correct, then the configuration file would fail to load.
This is #2547 again. That branch was accidentally deleted.