Open ppacher opened 5 years ago
Should this be implemented as a plugin, or we can implement them in the main program and allow users to choose between these external authentication methods and the built-in credential manager? I'm doubting whether it is beneficial to implement it as a plugin or just make this feature built-in.
Since there are lots of different identity/authentication providers IMHO a plug-able approach might be easier to maintain because the actual implementation of these plugins can be done by the community. You might consider merging them into upstream gotify when they are stable but it will allow the gotify team to focus on different feature/problems instead of fighting with various authentication providers.
Regarding the built-in credential manager, I think we will need to keep it enabled all the time so there can be "internal" users as well. In case the external auth is broken it will still be possible to log into Gotify as an administration and reconfigure/fix authentication.
Since there are lots of different identity/authentication providers IMHO a plug-able approach might be easier to maintain because the actual implementation of these plugins can be done by the community. You might consider merging them into upstream gotify when they are stable but it will allow the gotify team to focus on different feature/problems instead of fighting with various authentication providers.
Good point.
Regarding the built-in credential manager, I think we will need to keep it enabled all the time so there can be "internal" users as well. In case the external auth is broken it will still be possible to log into Gotify as an administration and reconfigure/fix authentication.
So, all users will have a credential stored in the built-in authentication system for fallback, or only some of them would need to have one?
My first thoughts on possible different implementations:
All requests are first examined by the built-in credential manager. If a valid token is not present, the request is then passed on to external auth plugins one-by-one. A whitelist may be implemented to limit which group of users the plugin might be used to authenticate for so that some not-so-secure methods cannot be used to authenticate high-power users.
A header such as X-Gotify-Auth
might be present to indicate that the user wish to authenticate this request with a plugin. Authentication plugins will call a function like RegisterAuthenticationMethod(key string, authFunc func(req *http.Request) (userID uint64, err error))
so that when that header value matches the key
parameter, the corresponding authentication method is called to determine whether the request is authenticated.
Hi, sorry for my late reply. I have been really busy the last days.
So, all users will have a credential stored in the built-in authentication system for fallback, or only some of them would need to have one?
I'd only allow additional internal users (ones that have passwords stored in the built-in credential manager). This way there could be a single local administration account for rescue purposes while all the others are always authenticated using an external provider. In addition we don't need to care about password synchronization, expiry and related problems.
I really like the idea of the header value and starting with built-in first seems reasonable. This also means that the user should be able to configure the order in which external plugins are probed so they can sort "less-secure" ones down. However, I'd suggest to "lock" the user to the plugin that authenticated him first. So if the user foo
tries to authenticate the first time and the ldap
plugin successfully verified the identity, we should lock the user to the ldap
plugin and not try different ones for the next authentication requests.
@jmattheis I think this is doable, what do you think?
Yeah, seems like a reasonable approach. I'd go with the first proposed implementation Built-in first, then external
. The origin of the user should be saved in the database and further access is only granted from the same origin. Passwords from != local origin should never be stored.
The API could look like this
type Permission string
const (
PermAdmin Permission = "admin"
PermUser Permission = "user"
)
func Authenticate(user, pass string, ctx *gin.Context, required Permission) (ok bool) {
}
In case of errors the plugin should call ctx.AbortWithError
by itself, with 401 Unauthorized, 403 Forbidden or even 504 Gateway Timeout.
AuthenticationProviders should be configured via yaml/env vars adding an UI for it seems overkill.
Okay, I will come up with a more detailed (maybe API stub) so we can polish it further.
Just another idea: In case gotify notices an unauthorized request (i.e. no session-cookie/token available) should we check if a plugin is able to authenticate the request as well by checking request headers? This would allow "single-sign-on" solutions as well as "trusted-proxies" where the proxy already authenticated the user and forwards the user-id in a header-value?
We could have two different plugin interfaces:
// IdentityAuthenticator authenticates user by username/password and returns as much user-information as possible (ie. fullname, ....)
type IdentityAuthenticator interface {
Authenticate(user, pass string, ctx *gin.Context, required Permission) (*User, bool)
}
// Same but for the first request that is received
type RequestAuthenticator interface {
Authenticate(req *http.Request, ctx *gin.Context, required Permission) (*User, bool)
}
I don't know the gin
framework so I may already be possible to access request headers using the gin.Context
Yeah, the gin.Context
contains the http.Request
. But sure, we can omit the user & pass parameter.
My proposal on the API based on the one @ppacher proposed:
The plugin would export another symbol, Authenticator plugin.Authenticator
. I agree with @jmattheis on the idea that a user should be bound to the authentication method it is used.
plugin.Authenticator should contain:
type Authenticator struct {
Authenticate func (req *http.Request) (user *User, createNonExist bool) `required`
AuthPageHandleFunc gin.HandleFunc `optional`
}
Once a request pending authentication is received and the internal credential manager did not find a matching token, the request is passed along the available Authenticate
func provided by loaded plugins. The plugin would verify the authenticity of the request and,
nil, false
, a nil
value on user
return value means that this plugin don't think this request is validated.*User
will be returned with information to be passed on to gorm as a database query, an additional field SourcePlugin string
will be added to the user field and is filled in with plugin module path. This will make sure that the plugin will only be able to authenticate users that are created by itself. Additionally, a true createNonExist
value will lead to this returned *User
to be created if a matching user can not be find.Additionally, AuthPageHandleFunc, if provided, will be registered on /auth/{module_path}
so it can be used for SSO landing (f.ex. Set a session cookie here so that it can be used to authenticate future requests) and other purposes, plugins should not be allowed to tamper with gin context directly on other requests (in that case we are asking on whether the request is authenticated, not how should exceptions be handled).
Several detailed questions:
@eternal-flame-AD
Sorry for answering that late, must've overlooked the notification.
Should we enforce at least one admin to use the internal credential manager?
:+1:
Should we allow users to set a password even if they are created with an external authentication plugin, just like how many websites' SSO practices where they give the user the option to set a account password which can also be used to login as well?
Hmm, I'm not sure. I rather not do it because when a password is set for a != internal authenticator then changing the password inside the authenticator f.ex. ldap wouldn't change the password in gotify. Maybe we could add functionally like "migrate to internal user management" but this should be an other issue and not be done together with this plugin.
I'd say the Authenticator should get a Name string
property for error messages.
AuthPageHandleFunc
could return a normal http.HandlerFunc
, we could then wrap this internally with gin.WrapF()
-> no dependency to gin inside the authenticator api.
Side question, would it be possible to add the authenticator api to the current v1 plugin api by adding a new folder with the api inside, or would that still break compatibility?
Some authentications require redirects to third parties, therefore we probably need an extra button(s) on the login page like Sign in with Tech
. GitHub uses something similar to this: https://developer.github.com/apps/building-oauth-apps/authorizing-oauth-apps/
Also, the authentication plugins need some configurable settings, I'd say we include this inside the yaml
authenticators:
"ldap.so":
host: "ldap.example.com"
timeoutSeconds: 100
"github_oauth.so":
token: "abc"
# or
authenticators:
- path: "ldap.so"
host: "ldap.example.com"
timeoutSeconds: 100
- path: "github_oauth.so"
token: "abc"
Should we enforce at least one admin to use the internal credential manager? I also think it is a good idea to enforce at least one administrator that will always work
Should we allow users to set a password even if they are created with an external authentication plugin, just like how many websites' SSO practices where they give the user the option to set a account password which can also be used to login as well? I agree with @jmattheis that this will cause some stupid / weird situations with changing passwords. It might even be a problem with password expiry for example.
I'd say the Authenticator should get a Name string property for error messages.
I think it would be better to move the Name
property to the configuration of the plugin. This way a user can easily use the same plugin multiple times (for example a generic OAuth plugin for github, facebook, ...). In addition, the order of the authentication plugins might be important so the configuration should be an array instead of a map. I would suggest the following configuration structure:
authenticators:
- name: Active Directory
path: ldap.so
timeoutSeconds: 100
- name: Github OAuth
path: oauth.so
clientId: some-client-token
endpoint: https://auth.github.com
- name: Facebook OAuth
path: oauth.so
clientId: another-client-token
endpoint: https://api.facebook.com/auth
Some authentications require redirects to third parties, therefore we probably need an extra button(s) on the login page like Sign in with Tech. GitHub uses something similar to this: https://developer.github.com/apps/building-oauth-apps/authorizing-oauth-apps/
:+1:
I am considering removing the requirement that a plugin must have the symbol NewGotifyPluginInstance
exported, and instead check for either NewGotifyPluginInstance
and GotifyExternalAuthenticator
symbol, the presence of either indicate that it can handle a user plugin and/or an authentication plugin.
Another approach would be to build this entirely independent of the current plugin system and use its own API and loading mechanism, and users need to put authentication plugins in another folder.
I prefer the first solution, what do you think?
I think it needs to be a separate system for loading, because the current system has a plugin instance for every user, with authentication we do not want this. The loading system should be much simpler because it basically only has the Authenticator instance. No enable/disable per user, no displayer, storager.
Could you add your answer to:
Side question, would it be possible to add the authenticator api to the current v1 plugin api by adding a new folder with the api inside, or would that still break compatibility?
Side question, would it be possible to add the authenticator api to the current v1 plugin api by adding a new folder with the api inside, or would that still break compatibility?
Sorry I think I overlooked this. I don't think it breaks compatibility, as it won't conflict with any current implementation. I think we can add it to the current api under a new subfolder.
I think we now have a somewhat mature idea on what we want to do and it's time to do some hands-on work. I will open a PR to do this :)
If we split up the work into smaller pieces I'd be happy to assist in implementation :)
I'm working on refactoring the current auth
package to make it more suitable for our new needs. After that I think maybe we can split the work up. I will update after I finished this.
Just as a suggestion, the goth library seems to work really well (used in Gitea for example): https://github.com/markbates/goth/
I was just testing gotify. One thing that would be a great addition would be a option to connect it to a SSO (keycloak for example).
goth support openidconnect that would be great!
Any progress?
Hello!
First I want to thank you for this awesome project!
I'd like to propose an extension to the current plugin system that may also relate to #78 and #20. Both issues asked for a different authentication method and I really understand that implementing and maintaining different authentication mechanisms is a bit overkill. However, I think we could extend the current plugin system in a way that the community would be able to contribute authentication/identity-provider plugins. Gotify will still need to maintain a database for applications/clients and user information. The actual authentication process however may be "outsourced" to a plugin.
This would allow a lot of different use-cases, like:
Webhooker
we can already setup the OAuth endpoint)Extending the current plugin system to support authentication providers would require some kind of system-plugin that is not bound to a specific user.
If you find this idea interesting I will try to come up with a more detailed proposal on the implementation part.
Best regards Patrick