Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v2 breaks support for configuration lists on environment variables #48

Closed
tardfree opened this issue Jun 11, 2019 · 6 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@tardfree
Copy link

I've used environment variables exclusively for configuration rather than files.
This included multiple domains in the DOMAIN environment variable.
The recent push of v2 to the latest tag on docker hub broke my auth (as I had watchtower updating for me) with a log message:
time="2019-06-11T00:40:13Z" level=error msg="Invalid email" email=<redacted valid email> source_ip=<redacted IP>

I think this is because auth.go:ValidateEmail fails as config.Domains has treated multiple domains in a comma separated list as a single domain, so the domain supplied doesn't match. Looking at changes to config.go I wasn't able to see where this change came from - it looks like config.go is new for v2. I'm away from my developer pc so not able to dig further easily.
This change from comma separate lists to repeating single values could impact any setting which had multiple values set via environment variables - as duplicate environment variables aren't possible.

There is no deprecation warning logged on startup for this change of behaviour (I wasn't using DOMAINS as the environment variable which would show this). Maybe I had screwed up my config and been lucky it was working.

I'll need to update my config to an ini file which I didn't notice/realize was an option before, that looks better suited for how I've configured as .

Thanks for your time.

@thomseddon
Copy link
Owner

thomseddon commented Jun 11, 2019

Thank you for the bug report! I tried to do as much testing as I could in #26 but inevitably some bugs may slip through, the goal of v2 was to be fully backwards compatible so I'll look to push a fix ASAP.

I've just added some extra tests in 72fc88a and everything seems to work as expected.

Please could you provide an example config and let me know the behavior before/after, I'll look into it right away!

@thomseddon thomseddon added bug Something isn't working unverified labels Jun 11, 2019
@thomseddon thomseddon self-assigned this Jun 11, 2019
@tardfree
Copy link
Author

No problem.

The config I was using which I saw the bug on had the following environment variables set (viewed from docker inspect, this is all of them except PATH, names changed too)

"DOMAIN=mydomainname.net,anothername.com,athirdname.com",
"AUTH_HOST=login.mydomainname.net",
"COOKIE_DOMAINS=services.mydomainname.net",
"SECRET=secure string here",
"COOKIE_SECURE=false",
"CLIENT_ID=clientidhere",
"CLIENT_SECRET=secrethere",

If it helps, the domain I was trying to authenticate with was the middle one of the 3.
To work around the issue I've redeployed with a single domain in the list (until I'm back in my home country) and this is working again for me (for that single domain though).

@tardfree
Copy link
Author

Oops didn't answer the rest of your questions.
Before behaviour was users in the middle domain were able to authenticate as expected and get redirected to the intended page.
After the update, I received the "Not authorized" string in the browser, and the log message in the first post.
I'm not sure if any of my users in the other domains (first and third) have ever logged in, so that might not have been working.

@thomseddon
Copy link
Owner

Brilliant, I think I found the bug, I had incorrectly thought the v0 config option was called domains, however I can now see it was called domain!

I have fixed CSV support for domain and added tests for both the flag and the env var. I've also removed the domains config as this was never needed!

@thomseddon
Copy link
Owner

*I've pushed the fix in 3e92400

Thank you for your bug report and sorry for causing you an issue whilst you were away!

@tardfree
Copy link
Author

Thank you for the very quick fix and this simple tool. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants