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

Can't login with user without email (Keycloak provider) #769

Closed
urbanpawel90 opened this issue Sep 8, 2020 · 17 comments
Closed

Can't login with user without email (Keycloak provider) #769

urbanpawel90 opened this issue Sep 8, 2020 · 17 comments

Comments

@urbanpawel90
Copy link

urbanpawel90 commented Sep 8, 2020

We have the use case where we want to allow users defined in KeyCloack to log in to some website after successful login and we use OAuth2 Proxy for that. The issue is that we have those user defined without email addresses in KeyCloak and OAuth2 proxy reports an error because of that.

Expected Behavior

I expect that it is possible to login for users that do not have email defined in KeyCloak.

Current Behavior

As for now OAuth2 Proxy reports 403 error:

403 Permission Denied
Invalid Account

With following line in logs:
172.17.0.1:40206 - - [2020/09/08 10:19:38] [AuthFailure] Invalid authentication via OAuth2: unauthorized

Possible Solution

Introduce some kind of option/flag to allow to login email-less users and modify email validator code (

if email == "" {
) to treat empty emails as valid.

Steps to Reproduce (for bugs)

  1. Setup some example realm and client in KeyCloak following OAuth2 Proxy guide/docs.
  2. Create explicit user in KeyCloak and DO NOT fill the email field for him.
  3. Configure OAuth2 Proxy with KeyCloak provider as mentioned in guide.
  4. Try to login as a user created in step 2
  5. You'll see 403 Permission Denied error page.

Your Environment

  • Version used: v6.1.1

Code references

Error log source:

logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: unauthorized")

Validator login for empty email:
if email == "" {

@urbanpawel90
Copy link
Author

I'll post the Pull Request today or tomorrow which fixes the issue.

@urbanpawel90
Copy link
Author

I also wonder if it's not a bug in case we set email-domain to *. Shouldn't we allow empty emails then?

@JoelSpeed
Copy link
Member

I think I would have expected a user to be allowed if they have no email and the wildcard was set. Not sure if this would constitute a breaking change or not 🤔

@JoelSpeed JoelSpeed added the bug label Sep 8, 2020
@urbanpawel90
Copy link
Author

Good question @JoelSpeed. I would say that it's not breaking and it wasn't explicitly described in docs before. I think a lot of people could have expected that ALL emails will be validated as docs say Use * to authenticate any email.

@NickMeves
Copy link
Member

I think I would have expected a user to be allowed if they have no email and the wildcard was set. Not sure if this would constitute a breaking change or not 🤔

I've thought many times this line

if allowAll {
should be at the top before the blank returning false check.

I think when I last looked into it, this behavior is why so many token -> session converter parts of the code set the session.Email to claim.Subject if there's no claim.Email.

@urbanpawel90
Copy link
Author

OK, guys. Regarding your comments here and in PR - I see that is going to be released with the next minor or major release. When do you plan such a release? When could we expect it to land in some official Docker image?

BTW. Are there any more actions required from my side for now?

@urbanpawel90
Copy link
Author

urbanpawel90 commented Sep 9, 2020

One more thing which might be also the solution for security concerns, tool stability and probable fastest release date. Maybe we could introduce some extra flag like --allow-empty-emails to allow/disallow empty email (regardless of email-domain parameter)?
By default it would disallow empty email as it does right now, so it won't be a breaking change, but some users (me for example ;) ) will be able to turn on the empty email users.

What do you think?

@avanier
Copy link

avanier commented Sep 17, 2020

I get a sense that with what @macrame reported in #766 this might actually be due to the emails not being available in the checked claims. Either because they're not set properly by the mappers, or because the claims are not read properly.

I'm digging a bit more right now. I'm experiencing the same problem.

@avanier
Copy link

avanier commented Sep 17, 2020

Nevermind, I have no idea what the problem is, and it's not the mappers.

@avanier
Copy link

avanier commented Sep 17, 2020

Nevernevermind. It was the mappers.

The default mapper for groups in Keycloak does something, and it's not the right thing.

Creating a custom mapper for groups with the Mapper Type set to Group Membership does the trick. 🎉

@github-actions
Copy link
Contributor

This issue has been inactive for 60 days. If the issue is still relevant please comment to re-activate the issue. If no action is taken within 7 days, the issue will be marked closed.

@gustabart
Copy link

Hi,
I have the same problem. I think it is not okay for the proxy to fail if the email claim is empty.
The openid specification says nothing about this restriction.
I think the --allow-empty-emails flag proposed by @urbanpawel90 is a good option.

I would like you to reconsider this issue.
Thanks!

@NickMeves
Copy link
Member

@gustabart If you are using Keycloak I recommend you connect to it via the OIDC Provider instead of the Keycloak provider until this PR is merged: #1107

The keycloak provider is horribly out of date and missing functionality. We are looking to deprecate it once the OIDC-based Keycloak provider is setup instead.

@NickMeves
Copy link
Member

Otherwise set --email-domains or whatever the flag is to *. That allows SessionStates that are missing the Email field by the email domain authorization check.

@gustabart
Copy link

Thanks for the reply Nick.
I am using the 'oidc' provider. I also tried --email-domains = * but unfortunately it doesn't work for empty email claims. Inspecting the validator.go (v7.1.0) code I can see:

func newValidatorImpl(domains []string, usersFile string,
	done <-chan bool, onUpdate func()) func(string) bool {
	validUsers := NewUserMap(usersFile, done, onUpdate)

	var allowAll bool
	for i, domain := range domains {
		if domain == "*" {
			allowAll = true
			continue
		}
		domains[i] = fmt.Sprintf("@%s", strings.ToLower(domain))
	}

	validator := func(email string) (valid bool) {
		if email == "" {
			return
		}
		email = strings.ToLower(email)
		for _, domain := range domains {
			valid = valid || strings.HasSuffix(email, domain)
		}
		if !valid {
			valid = validUsers.IsValid(email)
		}
		if allowAll {
			valid = true
		}
		return valid
	}
	return validator
}

The first if is the problem, when email == "", the function returns regardless of whether allowAll is true.

@NickMeves
Copy link
Member

If you are using this for Bearer auth, this is the fallback code for no email claim:

// Allow empty Email in Bearer case since we can't hit the ProfileURL

If you are interactive, and you still get nothing from the userinfo endpoint during the EnrichSession stage, I recommend you have in --oidc-email-claim and set it to sub. Then --email-domains=* will work on it.

@gustabart
Copy link

Ok, with --oidc-email-Claim set to sub, it works!
Anyway, this is a workaround.
I think a real solution is necessary. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants