feat: support proxy forward headers authentication #1105
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1105 +/- ##
==========================================
+ Coverage 50.08% 50.73% +0.64%
==========================================
Files 74 75 +1
Lines 7389 7532 +143
==========================================
+ Hits 3701 3821 +120
- Misses 3366 3371 +5
- Partials 322 340 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
internal/http/middleware/auth.go
Outdated
| ip, _, err := net.SplitHostPort(remoteAddr) | ||
| if err != nil { | ||
| var addrErr *net.AddrError | ||
| if errors.As(err, &addrErr) && addrErr.Err == "missing port in address" { |
There was a problem hiding this comment.
While testing locally this always came as ip:port, as the documentation says https://pkg.go.dev/net/http#Request
But the last run on pipelines was a clear ip, i added this just in case 🤷🏻♂️
There was a problem hiding this comment.
Did you manage to reproduce this? (So we can add a test case)
There was a problem hiding this comment.
I couldnt reproduce it, but i added a test with both cases
| this.loadAccount(); | ||
| this.loadSetting(); |
There was a problem hiding this comment.
This swap and the following remove of check is because now we can have a valid session without a token or a previous login, and we need to have the account information already defined to prevent an exception
There was a problem hiding this comment.
Let's dogfood this a bit in an RC before moving it forward with it. It should work in all cases from the looks of it, but I'd like to be sure.
internal/config/config.go
Outdated
| @@ -65,6 +65,10 @@ type HttpConfig struct { | |||
| IDLETimeout time.Duration `env:"HTTP_IDLE_TIMEOUT,default=10s"` | |||
| DisableKeepAlive bool `env:"HTTP_DISABLE_KEEP_ALIVE,default=true"` | |||
| DisablePreParseMultipartForm bool `env:"HTTP_DISABLE_PARSE_MULTIPART_FORM,default=true"` | |||
|
|
|||
| SSOEnabled bool `env:"SSO_ENABLED,default=false"` | |||
There was a problem hiding this comment.
Should we rename this settings to SSOProxyAuth...? In case we ever support more than one SSO auth method in the future.
internal/config/config.go
Outdated
|
|
||
| SSOEnabled bool `env:"SSO_ENABLED,default=false"` | ||
| SSOHeaderName string `env:"SSO_HEADER_NAME,default=Remote-User"` | ||
| SSOTrustedProxy []string `env:"SSO_TRUSTED_PROXY,default=10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, fc00::/7"` |
There was a problem hiding this comment.
| SSOTrustedProxy []string `env:"SSO_TRUSTED_PROXY,default=10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, fc00::/7"` | |
| SSOTrustedProxies []string `env:"SSO_TRUSTED_PROXY,default=10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, fc00::/7"` |
internal/http/middleware/auth.go
Outdated
| } | ||
|
|
||
| func NewAuthMiddleware(deps model.Dependencies) *AuthMiddleware { | ||
| return &AuthMiddleware{deps: deps} | ||
| plainIPs := deps.Config().Http.SSOTrustedProxy |
There was a problem hiding this comment.
Can we move the SSO logic to a new middleware? That way we would only inject it if needed (SSOEnabled == true)
Having the logic split would prove useful and is better for long term maintenance. You could even put it in a different file (auth_sso_proxy.go?)
internal/http/middleware/auth.go
Outdated
| ip, _, err := net.SplitHostPort(remoteAddr) | ||
| if err != nil { | ||
| var addrErr *net.AddrError | ||
| if errors.As(err, &addrErr) && addrErr.Err == "missing port in address" { |
There was a problem hiding this comment.
Did you manage to reproduce this? (So we can add a test case)
internal/http/middleware/auth.go
Outdated
| @@ -46,6 +70,57 @@ func (m *AuthMiddleware) OnRequest(deps model.Dependencies, c model.WebContext) | |||
| return nil | |||
| } | |||
|
|
|||
| func (m *AuthMiddleware) ssoAccount(deps model.Dependencies, c model.WebContext) *model.AccountDTO { | |||
There was a problem hiding this comment.
I would use getSSOAccount(...) (*model.AccountDTO, error) instead of just logging internally, so we can log all the errors directly in the parent function. What do you think?
| this.loadAccount(); | ||
| this.loadSetting(); |
There was a problem hiding this comment.
Let's dogfood this a bit in an RC before moving it forward with it. It should work in all cases from the looks of it, but I'd like to be sure.
|
I think i've addressed all the comments. They were nice suggestions. Otherwise let me know and i would do what i can. |
Hi, i started using Authelia for my SSO and Shiori is one of the apps i use that currently doesnt support neither proxy forwarding headers or OIDC.
This is my attempt at the first as its easier to implement.
There is #860 but it seems dead and outdated.
The default settings are tailored to my use case and should be enough for almost everyone, but open to different ones as this is a new world to me.
I've been testing it locally for now and everything seems to be working