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

[Proposal] Clarify authentication method interfaces #2582

Open
SamLR opened this issue Aug 10, 2022 · 1 comment
Open

[Proposal] Clarify authentication method interfaces #2582

SamLR opened this issue Aug 10, 2022 · 1 comment

Comments

@SamLR
Copy link
Contributor

SamLR commented Aug 10, 2022

User Story

As a developer I want clean interfaces to work to for adding authentication methods, so if/when we're asked to add more it can be done easily and securely without breaking core assumptions of our security model

Any solution to this should be documented as part of the ADR system and will likely supersede some of ADR-0015

The Problem

At the moment new Authentication methods (e.g. OIDC, User accounts, Token Passthrough) are added by implementing the PrincipalGetter interface:

type PrincipalGetter interface {
  Principal(r *http.Request) (*UserPrincipal, error)
}

pkg/auth/jwt.go

PrincipalGetters for the enabled authentication methods are then added to to a MultiAuthPrincipal and iterated over, calling Principal(request), to find one that succeeds. From this pattern it is unclear that Principal is also expected to authenticate the request (rather than just return a UserPrincipal).

There is also no uniform method for configuring authentication methods. This is currently carried out via an ad-hoc collection of CLI flags and by reading kubernetes secrets.

A potential solution

(This is just my thoughts on one way to improve this, feel free to disregard etc)

The PrincipalGetter interface is replaced with an AuthenticationMethod interface, something like:

type PrincipalGetter interface {
	Initialise(l logr.logger, k ctrlclient.Client) error
	IsAuthenticated(r *http.Request) (bool, error)
	Principal(r *http.Request) (*UserPrincipal, error)
}

IsAuthenticated explicitly checks that the user is authenticated whereas Principal returns user information. Often times these will overlap in what they do (e.g. parsing cookies etc) but by separating them we can make explicit the two tasks we currently expect Principal to carry out.

The Initialise method is expected to set up any configuration that the authentication method needs (e.g. calling the OIDC /.well-known endpoints). To reduce the number of CLI arguments the Initialise function would be expected to read configuration from environment variables or from Kubernetes.

@sympatheticmoose
Copy link
Contributor

@davidstauffer given this relates to Auth, could you take a look with Pesto and determine if anything further is required?

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

2 participants