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

feat: Imprement jwt-auth header auth method #2961

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Nerahikada
Copy link
Contributor

Description
This change is based on #2474. See original PR for detailed description.

To configure the filebrowser for Cloudflare Zero Trust, run the following command:

./filebrowser config set \
  --auth.method=jwt-header \
  --auth.jwt-header.header=Cf-Access-Jwt-Assertion \
  --auth.jwt-header.aud=<your application AUD> \
  --auth.jwt-header.iss=https://<your team name>.cloudflareaccess.com \
  --auth.jwt-header.certsurl=https://<your team name>.cloudflareaccess.com/cdn-cgi/access/certs \
  --auth.jwt-header.usernameClaim=email

🚨 Before submitting your PR, please read community, and indicate which issues (in any of the repos) are either fixed or closed by this PR. See GitHub Help: Closing issues using keywords.

  • DO make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your master!
  • DO make sure you are making a pull request against the master branch (left side). Also you should start your branch off our master.
  • DO make sure that File Browser can be successfully built. See builds and development.
  • DO make sure that related issues are opened in other repositories. I.e., the frontend, caddy plugins or the web page need to be updated accordingly.
  • AVOID breaking the continuous integration build.

Further comments

Nerahikada and others added 2 commits January 23, 2024 20:35
Comment on lines +33 to +35
a.init.Do(func() {
a.remoteKeySet = oidc.NewRemoteKeySet(context.Background(), a.CertsURL)
})
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to do it in the constructor func

func NewJWTAuth(...){
...
}

Comment on lines +42 to +47
// The Application Audience (AUD) tag for your application
config := &oidc.Config{
ClientID: a.Aud,
}

verifier := oidc.NewVerifier(a.Iss, a.remoteKeySet, config)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a good candidate to be moved into the constructor


accessJWT := r.Header.Get(a.Header)
if accessJWT == "" {
return nil, os.ErrPermission
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to log an error here


token, err := verifier.Verify(r.Context(), accessJWT)
if err != nil {
return nil, os.ErrPermission
Copy link
Member

Choose a reason for hiding this comment

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

Please log the original error

payload := map[string]any{}
err = token.Claims(&payload)
if err != nil {
return nil, os.ErrPermission
Copy link
Member

Choose a reason for hiding this comment

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

Please log the original error


user, err := usr.Get(srv.Root, payload[a.UsernameClaim])
if nerrors.Is(err, errors.ErrNotExist) {
return nil, os.ErrPermission
Copy link
Member

Choose a reason for hiding this comment

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

Please log the original error

auth/jwt.go Outdated Show resolved Hide resolved
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Apr 29, 2024
@Nerahikada Nerahikada marked this pull request as draft April 29, 2024 09:30
@github-actions github-actions bot removed the Stale label Apr 30, 2024
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.

None yet

2 participants