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

Implement OpenID token expressions evaluation #63

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ErmakovDmitriy
Copy link
Contributor

This PR implements an OpenID token expression evaluation.

The idea and the use-case is to be able to define different behavior in HAProxy based on OpenID token claims.

An example:

  • An OpenID token contains a field roles which is an array of admin, viewer, editor;
  • We want to allow access to /admin URL path only for people with admin role;

With this PR, it is possible to define in HAProxy:

acl host1.example.com hdr(host) -i host1.example.com
acl host1.example.com_admin_path path_beg -i /admin
acl host1.example.com-admin-allowed var(sess.auth.token_expression_in_roles_admin) -m bool

http-request send-spoe-group spoe-auth try-auth-all if host1.example.com
http-request set-var(req.oidc_token_expressions) str("in;roles;admin") if host1.example.com
http-request send-spoe-group spoe-auth try-auth-all if host1.example.com

use_backend haproxy-spoe-auth-error if host1.example.com oauth2_error
use_backend haproxy-spoe-auth-redirect if host1.example.com !oauth2_authenticated

http-request deny deny_status 403 if host1.example.com !host1.example.com-admin-allowed host1.example.com_admin_path oauth2_authenticated
use_backend host1.example.com-backend if host1.example.com host1.example.com-admin-allowed

Copy link
Contributor

@mougams mougams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR, a bit complex since this adds lots of things in the same PR - would have been nice if this could be split, but that's ok.

I've put few remarks, and the patch seems breaking.
Also, you might need to rebase, since I've merged some lib updates.

func main() {
var configFile string
flag.StringVar(&configFile, "config", "", "The path to the configuration file")
dynamicClientInfo := flag.Bool("dynamic-client-info", false, "Dynamically read client information")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems breaking. If I read correctly, you're replacing option -dynamic-client-info by option -dynamic/-d which clearly breaks the bakward compatibility. If so please mark it as breaking

@@ -41,15 +49,15 @@ func (ocf *StaticOIDCClientsStore) GetClient(domain string) (*OIDCClientConfig,
return nil, ErrOIDCClientConfigNotFound
}

func (ocf *StaticOIDCClientsStore) AddClient(domain string, clientid string, clientsecret string, redirecturl string) {
func (ocf *StaticOIDCClientsStore) AddClient(domain, clientid, clientsecret, redirecturl string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please keep the parameters type ?

@@ -14,7 +11,7 @@ type OIDCClientConfig struct {
type OIDCClientsStore interface {
// Retrieve the client_id and client_secret based on the domain
GetClient(domain string) (*OIDCClientConfig, error)
AddClient(domain string, clientid string, clientsecret string, redirecturl string)
AddClient(domain, clientid, clientsecret, redirecturl string)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please keep the parameters type ?


# If set, the LDAP authenticator is enabled
ldap:
# SPOE message name as it is defined in HAProxy spoe-message {{ NAME }} value.
spoe_message: "try-auth-ldap"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one too is breaking

}
}

return &StaticOIDCClientsStore{clients: clients}
}

func NewEmptyStaticOIDCClientStore() *StaticOIDCClientsStore {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could probably be removed since never used

return ""
}

const (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To simplify, you could probably merge this with the constants

	operationIn           string = "in"
	operationNotIn        string = "notin"
	operationExists       string = "exists"
	operationDoesNotExist string = "doesnotexist"
)```

so that during the assignement it becomes as simple as

		expr.Operation = val[0].value

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

Successfully merging this pull request may close these issues.

2 participants