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

Inconsistent validation logic between init and refresh #306

Open
griffinmyers opened this issue May 28, 2021 · 2 comments
Open

Inconsistent validation logic between init and refresh #306

griffinmyers opened this issue May 28, 2021 · 2 comments
Assignees

Comments

@griffinmyers
Copy link

Describe the bug

This proxy supports a variety of mechanisms to authorize access to a host:

validators := []options.Validator{}
if len(upstreamConfig.AllowedEmailAddresses) != 0 {
validators = append(validators, options.NewEmailAddressValidator(upstreamConfig.AllowedEmailAddresses))
}
if len(upstreamConfig.AllowedEmailDomains) != 0 {
validators = append(validators, options.NewEmailDomainValidator(upstreamConfig.AllowedEmailDomains))
}
if len(upstreamConfig.AllowedGroups) != 0 {
validators = append(validators, options.NewEmailGroupValidator(provider, upstreamConfig.AllowedGroups))
}

When responding to the Oauth callback (that is, when a user first authenticates and initiates a session), the proxy requires any of its validators be satisfied before proceeding:

errors := options.RunValidators(p.Validators, session)
if len(errors) == len(p.Validators) {

However, when the proxy is refreshing a session sometime later, it requires that a user satisfy "groups" validation, exclusively:

} else if session.RefreshPeriodExpired() {
// Refresh period is the period in which the access token is valid. This is ultimately
// controlled by the upstream provider and tends to be around 1 hour.
ok, err := p.provider.RefreshSession(session, allowedGroups)

inGroups, validGroup, err := p.ValidateGroup(s.Email, allowedGroups, newToken)
if err != nil {
// When we detect that the auth provider is not explicitly denying
// authentication, and is merely unavailable, we refresh and continue
// as normal during the "grace period"
if err == ErrAuthProviderUnavailable && p.withinGracePeriod(s) {
tags := []string{"action:refresh_session", "error:user_groups_failed"}
p.StatsdClient.Incr("provider_error_fallback", tags, 1.0)
s.RefreshDeadline = extendDeadline(p.SessionValidTTL)
return true, nil
}
return false, err
}
if !validGroup {
return false, errors.New("Group membership revoked")
}

userGroups, err := p.UserGroups(email, allowedGroups, accessToken)
if err != nil {
return nil, false, err
}
allowed := false
for _, userGroup := range userGroups {
for _, allowedGroup := range allowedGroups {
if userGroup == allowedGroup {
inGroups = append(inGroups, userGroup)
allowed = true
}
}
}

I might be reading things wrong, but I have produced this with a live instance, and it seems like strange behavior to me.

The consequence of this is a user who satisfies email domain validation but fails group validation will be able to authenticate and briefly see the service, until the refresh period expires (or validation period, for that matter), where they'll be logged out.

To Reproduce

upstream_configs.yml

Create a group named [email protected] with 0 people (or as close to it as the provider allows). Register a backend that uses this group in the allowed_groups field:

- service: my-service
  default:
    from: my-service.int.corp.com
    to: http://my-service.corp-internal.com
    options:
      allowed_groups:
        # an empty google group
        - [email protected]

run the service, ensuring that DEFAULT_ALLOWED_EMAIL_DOMAINS=corp.com. Then, as a user not in [email protected], attempt to access my-service.int.corp.com. You will succeed, but be logged out as soon as the session refreshes.

Expected behavior

The user should not be logged out when the session is refreshed.

@Jusshersmith
Copy link
Contributor

Hey @griffinmyers,

Thanks for sending in this issue, and with the level of detail you've included.

Just wanted to acknowledge it -- I'm going to try to find some time (realistically) next week to look into this.

@Jusshersmith
Copy link
Contributor

Just to check in here -- I'm still working through this. Finding time has been difficult, but I'm getting there!

@Jusshersmith Jusshersmith self-assigned this Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants