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.

Sign up for GitHub

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

Jump to bottom

feat: Implement JWTAuthenticator.Authenticate #18052

Merged

jujubot merged 1 commit into juju:3.5 from SimoneDutto:implement-jwt-authenticate

Sep 23, 2024

Contributor

SimoneDutto commented Sep 9, 2024

Hi, I am Simone from the JaaS Team.

This pr implements the JWTAuthenticator.Authenticate() method.
It is needed for JaaS to authenticate to HTTP endpoints via JWT similarly as it is already doing it for websockets endpoints.

For example, we are currently authenticated via basic-auth for the http endpoint handling the debug-logs (PR), and we would like to switch to JWT auth.

Implementation

The Authenticate method extracts the Bearer token from the Authorization header and validates the jwt.
Since the HTTPStrategicAuthenticator is looping around multiple http authenticators (apiserver/stateauthenticator/auth.go:21), and going to the next if the error is "not found" or "not implemented", to allow both basic-auth and jwt authentication to work I needed to change apiserver/stateauthenticator/auth.go:265 to return a NotFound error in case of missing basic-auth header.

Checklist

Code style: imports ordered, good names, simple structure, etc
Comments saying why design decisions were made
Go unit tests, with comments saying what you're testing
Integration tests, with comments saying what you're testing
doc.go added or updated in changed packages

QA steps

After bootstrapping a controller with jwt authenticator properly configured.
Issuing this command

# bootstrap a juju controller with a properly jwt authenticator configured.
juju bootstrap lxd qa-jwt --config login-token-refresh-url=<url>
# curl a http endpoint and see it correctly parse the jwt and authenticate the user.
curl -H "Authorization: Bearer <jwt_token>" -k https://<controller_endpoint>:17070/charms

Documentation changes

Collaborator

jujubot commented Sep 9, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

9 similar comments

Collaborator

jujubot commented Sep 9, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

Collaborator

jujubot commented Sep 9, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

Collaborator

jujubot commented Sep 9, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

Collaborator

jujubot commented Sep 9, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

Collaborator

jujubot commented Sep 9, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

Collaborator

jujubot commented Sep 9, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

Collaborator

jujubot commented Sep 9, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

Collaborator

jujubot commented Sep 9, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

Collaborator

jujubot commented Sep 9, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

juju deleted a comment from SimoneDutto

Contributor Author

SimoneDutto commented Sep 9, 2024

@SimonRichardson for the two workflows failing:

the first one mentions some vulns in the std-lib, since I didn't add any new deps I don't know how to fix it.
the second one from the log seems it's trying to curl an ipv6 address, which I suppose it's the reason it is failing. I tried the same commands on my locally built juju and it seems to be working with ipv4 but not with ipv6

Do you have suggestions to fix the issues?

Member

SimonRichardson commented Sep 9, 2024

/build

Member

SimonRichardson commented Sep 9, 2024

the first one mentions some vulns in the std-lib, since I didn't add any new deps I don't know how to fix it.

The new go release caused this. I'll need to workout what the fix is.

Member

SimonRichardson commented Sep 9, 2024

We need to bump the go version in the go.mod file.

@hpidcock have you already fixed this, or do we want to do this here?

hpidcock added the 3.5 label

Contributor Author

SimoneDutto commented Sep 11, 2024

@SimonRichardson do you have any update on this? If there is something I can do to help let me know :)

hpidcock assigned SimonRichardson

Member

hpidcock commented Sep 12, 2024

We need to bump the go version in the go.mod file.

@hpidcock have you already fixed this, or do we want to do this here?

Handled here #18070

SimoneDutto force-pushed the implement-jwt-authenticate branch from 6fc8960 to 25f94bf Compare

September 12, 2024 06:45

Contributor Author

SimoneDutto commented Sep 12, 2024

We need to bump the go version in the go.mod file.
@hpidcock have you already fixed this, or do we want to do this here?

Handled here #18070

I've rebased on 3.5 to apply the changes mentioned

SimonRichardson reviewed

View reviewed changes

Member

SimonRichardson left a comment

I think I'd want tooling to be added that can craft me a JWT token I can use for curl. I've had that in previous companies I've worked for, and it can be invaluable.

Could be a simple as:

go run ./scripts/auth/jwt.go ....

apiserver/authentication/jwt/jwt.go

Comment on lines +84 to +86

+              	if authHeader == "" {
+              		return authentication.AuthInfo{}, errors.NotFoundf("authorization header missing")
+              	}

Member

SimonRichardson Sep 12, 2024

This should be a bad request. We might have to do some additional plumbing to get this to work.

Contributor Author

SimoneDutto Sep 12, 2024

Yes, as described in the description, this change is to comply with the for loop apiserver/stateauthenticator/auth.go:21. If there is any error besides not-found or not-implemented the authentication fails and it doesn't try the next authenticator.
I can change and add the not-valid to the for logic, what do you think?

Member

SimonRichardson Sep 12, 2024

Sorry, I shot the original comment too early.

I'm not sure where your link leads to?

juju/apiserver/stateauthenticator/auth.go

Line 21 in bc3a4ce

"github.com/juju/juju/apiserver/apiserverhttp"

Do you mean?

juju/apiserver/authentication/http.go

Lines 20 to 31 in bc3a4ce

    
           func (s HTTPStrategicAuthenticator) Authenticate(req *http.Request) (AuthInfo, error) { 
        
           	for _, authenticator := range s { 
        
           		authInfo, err := authenticator.Authenticate(req) 
        
           		if errors.Is(err, errors.NotImplemented) { 
        
           			continue 
        
           		} else if errors.Is(err, errors.NotFound) { 
        
           			continue 
        
           		} else if err != nil { 
        
           			return AuthInfo{}, err 
        
           		} 
        
           		return authInfo, nil 
        
           	}

If we return errors.BadRequest then we need to understand what to do there. I believe if we get a errors.BadRequest then we should return that directly to the client.

Member

SimonRichardson Sep 12, 2024

I just want to clarify, that we should ensure that we don't leak information about the user, just that JWT is malformed if the JWT header was sent. Otherwise returning NotFound is valid.

Contributor Author

SimoneDutto Sep 12, 2024 •

edited

Loading

yes, it is the code I was referring to.

In this current implementation we return:

not found if the Authorization header is missing -> go to next authenticator
not found if the Authorization header is there, but it is not formatted as "Bearer <>" -> go to next authenticator
another error if the jwt cannot be parsed -> return the error to the user

Member

SimonRichardson Sep 12, 2024

Ok, that's fine. I'm happy with that logic. Leave as not found error, given the scenarios you've described. Can you document the expectations of that in the HTTPStrategicAuthenticator.Authenticate method, and validate the tests work as expected according to those expectations?

Thanks for this @SimoneDutto

Contributor Author

SimoneDutto Sep 12, 2024

perfect.

the behavior is described at the interface level, a few lines above the authenticate method

juju/apiserver/authentication/http.go

Lines 13 to 16 in bc3a4ce

    
           // HTTPStrategicAuthenticator is responsible for trying multiple Authenticators 
        
           // until one succeeds or an error is returned that is not equal to NotFound or 
        
           // NotImplemented. 
        
           type HTTPStrategicAuthenticator []HTTPAuthenticator

I've added some tests for the HTTPStrategicAuthenticator

Member

SimonRichardson Sep 12, 2024

Amazing thank-you.

apiserver/authentication/jwt/jwt.go Show resolved Hide resolved

apiserver/stateauthenticator/auth.go Show resolved Hide resolved

Contributor Author

SimoneDutto commented Sep 12, 2024

I think I'd want tooling to be added that can craft me a JWT token I can use for curl. I've had that in previous companies I've worked for, and it can be invaluable.

Could be a simple as:
go run ./scripts/auth/jwt.go ....

Regarding the jwt tooling, I'll try to work out something.


          feat: implement JWTAuthenticator.Authenticate for http

SimoneDutto force-pushed the implement-jwt-authenticate branch from f4a4bdc to 7036128 Compare

September 12, 2024 14:39

Contributor Author

SimoneDutto commented Sep 13, 2024

as discussed with @SimonRichardson, the JWT tooling is not needed for this PR.
However, he suggested creating another PR to add an integration test between Juju and a proper JaaS.

Member

hpidcock commented Sep 15, 2024

/build

SimonRichardson approved these changes

View reviewed changes

hpidcock approved these changes

View reviewed changes

Member

hpidcock commented Sep 23, 2024

/merge

jujubot merged commit 1f9651c into juju:3.5

25 of 27 checks passed

ycliuhw mentioned this pull request

chore: merge 3.5 to 3.6 #18155

Merged

jujubot added a commit that referenced this pull request


          Merge pull request #18155 from ycliuhw/merge-3.5

2af7bd8

#18155

Forward merge:

- #18154
- #18022
- #17699
- #18095
- #18052
- #18073
- #18091
- #18083

Conflicts:
worker/uniter/runner/context/context_test.go
version/version.go
scripts/win-installer/setup.iss
snap/snapcraft.yaml

Note: this PR also has a small change for the #18154 to include the checksum for the idempotent secret feature.

ycliuhw mentioned this pull request

chore: merge 3.6 #18162

Merged

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

Labels

3.5