Hi, At the World Health Organization our South-East Asian regional office uses Koha for their regional library, with access by all WHO staff in the region and selected staff worldwide. Since local user databases can present a security risk we require that all applications for staff be integrated with our identity provider which at the moment is Microsoft's AzureAD. AzureAD uses OpenID-Connect and is mostly drop-in compatible with Google's implementation, with the exception that in order to construct the URLs both for redirection and for validation you need to know the organization's Azure tenant ID. Because our system integrator is not interested in pursuing this integration I've taken it on myself, and so I've set up a development environment and have started working on the code. My plan is to set up the configuration parameters and to then use them in a very slightly altered copy of the current googleopenidconnect file. I think in the long run it would be better to have a generic OpenID-Connect configuration to allow integrations with other providers like Okta and OneLogin, but we don't need for the moment that so for now I'll just add the AzureAD provider. I'll read up on the rest of the contribution procedure, and I'll be back with a patch/PR in a few days. Best, Mark
It's great that you're taking this on, Mark! I wrote a generic OpenID Connect client for Koha in 2014, but I only run and maintain it locally. (Finding time and energy for upstreaming can be tricky.) There are some old/outdated patches at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21586, but I've actually improved it a fair bit since then. I have a strong interest in authentication and authorization, so I'll follow along here. I really don't like Koha's Google OpenID Connect integration, so I'm tempted to look at upstreaming my work again. It would be nice to give the WHO a hand...
That's great David! I was about to post a quick-and-dirty implementation that I basically copied over from the Google version, but yes, I agree that it's far from perfect. It isn't really even an OpenID-Connect workflow, but something more like the code-grant flow with an openid scope tossed in. That said, it works, but it isn't quite right. I'd be very happy indeed to collaborate on this to get it right.
Created attachment 121315 [details] [review] Quick AzureAD OAuth2 implementation based on the existing implementation for Google Here's my current working patch. It does what we absolutely need in that it makes integration with AzureAD possible. I agree with David Cook, however, that this isn't ideal, since OpenID-Connect support should work with any identity provider and since it should actually implement OpenID-Connect whereas this patch and the code that it's based on actually implement the code-grant flow, with an additional openid scope. So I'm going to have a look at his implementation and try to get something off the ground with it. Also: I'm not sure if I've used the version number system correctly. If somebody could look at that I'd really appreciate it.
Created attachment 121320 [details] [review] Quick AzureAD OAuth2 implementation based on the existing implementation for Google In the second patch I've consolidated the db update code, and have (I think) done the version numbering more correctly.
Created attachment 121324 [details] [review] AzureAD OAuth2 implementation based on the existing implementation for Google Consolidated the db update code and possibly fixed my previous misunderstanding of the version numbering system.
(In reply to mark.jaroski from comment #3) > I agree with David Cook, however, that this isn't ideal, since > OpenID-Connect support should work with any identity provider and since it > should actually implement OpenID-Connect whereas this patch and the code > that it's based on actually implement the code-grant flow, with an > additional openid scope. Actually even in my case I haven't fully implemented the spec as there's just so much to it. I've only implemented the Authorization Code flow, although I find that's typically what's used for server-side auth anyway. I think Implicit Flow is usually only used by JS apps? And I'm not familiar with Hybrid Flow.
I think as long as we're doing pure authentication the only part of OAuth2 we'd want to implement is the straight OpenID-Connect flow which goes like this: 1. Fetch the IDP metadata 2. Send the user to the IDP to fetch their OpenID JWT 3. Validate the JWT against the prefetched metadata There's no reason AFAIK to continually fetch the metadata, so that can be done asynchronously with some kind of scheduled task like a cron job. It really is just about the simplest of OAuth2 flows, and simplicity is our friend in security, right? I guess the only reason to implement any other OAuth2 flow would be to work with third-party APIs. I guess there might be some of that these days, but I have to admit not knowing enough about Koha to know what they would be. Maybe some kind of third-party e-book provider?
(In reply to mark.jaroski from comment #7) > I think as long as we're doing pure authentication the only part of OAuth2 > we'd want to implement is the straight OpenID-Connect flow which goes like > this: > > 1. Fetch the IDP metadata > 2. Send the user to the IDP to fetch their OpenID JWT > 3. Validate the JWT against the prefetched metadata > > There's no reason AFAIK to continually fetch the metadata, so that can be > done asynchronously with some kind of scheduled task like a cron job. Personally, I hate cronjobs. They're a nightmare to manage if you have a lot of instances to manage or even try to scale up one instance. In this case, the metadata is available as JSON, so I'd say fetch it once, and then stick it in a cache. > I guess the only reason to implement any other OAuth2 flow would be to work > with third-party APIs. I guess there might be some of that these days, but I > have to admit not knowing enough about Koha to know what they would be. > Maybe some kind of third-party e-book provider? I'm not sure about Overdrive, but with EBSCO we've done SAML integrations for OpenAthens and shared client ADFS. I imagine we'd still use the Authorization Code Flow if we used OIDC instead though. It's such a common flow.
Cronjobs are also a nightmare in containers and development environments in general imho. But I like what you're saying about not fetching the metadata every time. In the past, I've cheated by manually setting up the endpoints, but that wasn't a great solution. I have to say Mark... you've got me excited about OpenID Connect again heh. I'm tempted to do some work on this tonight.
That's great! I think it's reasonable to cache the metadata somehow and then instead of grabbing it every time you can check the age of the cached data. I wouldn't want to keep it indefinitely because the IDP might need to switch signing certificates. Traditionally these are self-signed because they work more like pre-shared keys than TLS certificates which need to be signed by an authority. At least that's how we did it with SAML. We just used x509 for shipping the keys because it was handy.
Created attachment 121513 [details] [review] AzureAD OAuth2 implementation based on the existing implementation for Google
Created attachment 121514 [details] [review] AzureAD OAuth2 implementation based on the existing implementation for Google Corrected links in opac-auth.tt at the suggestion of Sunil Kumar.
Thanks for posting this, Mark. I have a few comments at a glance. As a contributor, you don't make changes to updatedatabase.pl or $Koha::VERSION directly. The release manager takes care of those. You need to create your database update as per https://wiki.koha-community.org/wiki/Database_updates. I'll put in some comments against opac/svc/auth/azureadopenidconnect using Bugzilla's tools.
Comment on attachment 121514 [details] [review] AzureAD OAuth2 implementation based on the existing implementation for Google Review of attachment 121514 [details] [review]: ----------------------------------------------------------------- Technically, a lot of this code should be put into a module rather than a script, but I know that you were basing your work on Nicholas's and his made it into Koha, so I don't think we can require you to put it into a module with unit tests. ::: opac/svc/auth/azureadopenidconnect @@ +48,5 @@ > + > +my $tenantID = C4::Context->preference('AzureADOAuth2TenantID'); > +my $authority= "https://login.microsoftonline.com/${tenantID}/v2.0"; > +my $discoveryDocURL = "${authority}/.well-known/openid-configuration"; > +warn $discoveryDocURL; This debugging code needs to be removed. @@ +253,5 @@ > + my $prompt = $query->param('reauthenticate') // q{}; > + if ( $authendpoint eq q{} ) { > + loginfailed( $query, 'Unable to discover authorisation endpoint.' ); > + } > + my $authorisationurl = This isn't a blocker, but I'd suggest using the URI Perl module for creating this URL. I find it a lot nicer to work with than trying to concatenate together a big URL string. https://metacpan.org/pod/URI
When you're ready for others to test your code, provide a test plan that they can use for testing it, and change the status to "Needs Signoff".
Actually, after reviewing the code again, I'm not sure that this has to be Azure-specific. The only thing I notice at a glance that is Azure specific is the $tenantID and $authority variables. I'd like to see this made more generic, so that users can provide their own $discoveryDocURL via a system preference. With the system preference, you could provide a hint like "For AzureAD, use https://login.microsoftonline.com/<tenantID>/v2.0/.well-known/openid-configuration". If you make this more generic, I'd be happy to test it against Keycloak.
Hi David, I think a lot of the code that you see in the new AzureAD "oidc" code is just left over from the old Google "oidc" code. Remember: my goal was to quickly get something that works and is reasonably secure without making any major changes to existing code. So the patch is purely additive. Basically, I made a copy of the googleopenidauth file and changed a few things to make it work with AzureAD. This also gave me better than even odds of adhering to the coding style, etc. However, I have also brought along whatever problems were there in the old code. I'll post a diff here so you can see which parts are from me and which parts were already there. In any case, this is probably not the *right* way to do it. But it's fast, and probably does no real harm since whatever problems it has are the same as the ones with the google version. Make sense? -mark
(In reply to mark.jaroski from comment #17) > In any case, this is probably not the *right* way to do it. But it's fast, > and probably does no real harm since whatever problems it has are the same > as the ones with the google version. > > Make sense? Oh yes I understand that. Personally, I was always against the Google version ;). But I hadn't prioritized upstreaming my own OpenID Connect client (which itself isn't perfect either of course), so I couldn't complain too much. Personally, I'm not really keen to have a Google version and an Azure version. I rather just have 1 generic version. But I won't stand in the way of someone else signing off and QAing this enhancement. You might want to sign up to the koha-devel listserv if you haven't already https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel. That can be a good place to try to generate interest for patches. In the meantime, if you update your patch to do your database update as per https://wiki.koha-community.org/wiki/Database_updates your patch will have a better chance of making it through.
I'll keep working on my different auth patches as well. At this point, I think that I could probably do a generic OpenID Connect client just using a Koha Plugin, but I'd like to make some Koha changes too.
Hi Mark! I tried your patch for WPRO, there version is Koha 20.05. I was getting an Internal Server error when going to the system preference for Azure and when I checked the log, I got "Unrecognized source of preference values: patron-categories at /usr/share/koha/intranet/cgi-bin/admin/preferences.pl line 97. ". Checked the perl file and the system preference yaml file as well as the GoogleOpenIdConnect settings, there seems to be an extra line "choices: patron-categories". I didn't see that in the GoofleOpenIDConnect so I removed that line. I am now able to see the system preference for AzureAD and have the library staff test it out. Thanks and keep safe!
Thanks Eugene. I'll have a look at that, and update the patch.
Hi Mark! For the masthead.inc in 21.05, it seem is not clickable, the code in your patch below: <a class="nav-link login-link" href="/cgi-bin/koha/svc/auth/azureadopenidconnect" role="button" data-toggle="modal"><i class="fa fa-user fa-icon-black fa-fw" aria-hidden="true"></i> <span class="userlabel">Log in to your account</span></a> I checked on the googleopenidconnect a link, we have the following below: <a class="nav-link login-link" href="/cgi-bin/koha/svc/auth/googleopenidconnect"><i class="fa fa-user fa-icon-black fa-fw" aria-hidden="true"></i> <span class="userlabel">Log in to your account</span></a> So, I adjusted that for azuread, now it's clickable, below is the code: <a class="nav-link login-link" href="/cgi-bin/koha/svc/auth/azureadopenidconnect"><i class="fa fa-user fa-icon-black fa-fw" aria-hidden="true"></i> <span class="userlabel">Log in to your account</span></a> Can the code be adjusted for 21.05? Best, -eugene
I think this should probably be marked as a duplicate of bug 30988 but I'll leave that decision up to you, Mark.
*** This bug has been marked as a duplicate of bug 30988 ***
Changing this to be a duplicate of bug 31378 since it's a generic implementation now available in master and presumably soon in 22.11 *** This bug has been marked as a duplicate of bug 31378 ***