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

Auth: support AWS ALB JWT #45191

Closed
wants to merge 1 commit into from
Closed

Auth: support AWS ALB JWT #45191

wants to merge 1 commit into from

Conversation

mtanda
Copy link
Collaborator

@mtanda mtanda commented Feb 10, 2022

What this PR does / why we need it:
Add support for AWS ALB JWT authentication.

AWS ALB is popular service, and it provides authentication feature.
If authentication is enabled, ALB pass header which includes JWT to backend instance.

AWS doesn't provides public key as JWK, and key URL contain key id.
Current Grafana implementation doesn't support it.

There is sample code how to verify JWT in official doc.
https://docs.aws.amazon.com/elasticloadbalancing/latest/application/listener-authenticate-users.html#user-claims-encoding

Which issue(s) this PR fixes:
related to #44261

Special notes for your reviewer:
There is a pitfall. If JWT contain extra padding, go-jose return error in cryptographic primitive error.

AWS JWT does not conform to standard.
auth0/node-jws#84 (comment)
golang-jwt/jwt#92

go-jose generate data to be signed from parsed JWT data.
https://github.com/square/go-jose/blob/v2.5.1/jws.go#L105-L138

The computed sign doesn't match to JWT embedded sign.

Currently, I don't have idea to fix this without forking go-jose or switching other library.

@grafanabot grafanabot added area/backend pr/external This PR is from external contributor labels Feb 10, 2022
@mtanda mtanda marked this pull request as ready for review February 14, 2022 01:46
@mtanda mtanda requested a review from a team as a code owner February 14, 2022 01:46
@mtanda mtanda requested review from sdboyer, zserge, dsotirakis and sakjur and removed request for a team February 14, 2022 01:46
@sakjur
Copy link
Contributor

sakjur commented Feb 16, 2022

Thank you, I'll review this!

Currently, I don't have idea to fix this without forking go-jose or switching other library.

Did you get a sense for how large the fix would be for go-jose when looking into it?

@mtanda
Copy link
Collaborator Author

mtanda commented Feb 17, 2022

Thanks for review!

When I pass raw AWS JWT (which contain extra =) without sanitize, go-jose return error like illegal base64 data at input byte 423.
I think it is hard to fix the issue by clean way.

The header of AWS JWT contain signer field, the value format like arn:aws:elasticloadbalancing:ap-northeast-1:012345678901:loadbalancer/app/foo/abcdef0123567890.
When validating JWT, checking the signer and match the pattern, if matched add extra =, is dirty but easiest fix...
https://github.com/square/go-jose/blob/v2.5.1/jws.go#L105-L138

@mtanda
Copy link
Collaborator Author

mtanda commented Feb 17, 2022

Another option is adding skip verification flag to Grafana.
If Grafana restrict public access, and allow only ALB, the JWT might be valid.

@sakjur sakjur self-assigned this Feb 22, 2022
@sakjur
Copy link
Contributor

sakjur commented Feb 22, 2022

Another option is adding skip verification flag to Grafana.
If Grafana restrict public access, and allow only ALB, the JWT might be valid.

I'm not very fond of that, it feels like that might be error prone with someone using it "to test" and it sticking around 🤔

@mtanda
Copy link
Collaborator Author

mtanda commented Feb 28, 2022

it feels like that might be error prone with someone using it "to test" and it sticking around

Basically, I agree with you. Skipping verification is recommendation of AWS Support.

auth0/node-jws#84 (comment)

Looking at precedent cases on this topic, the internal teams have recommended to adjust your back-end code to take the padding into account while it parses the token, and then send the token upstream unsigned. Because the load balancer does not encrypt the user claims, we recommend that you configure the target group to use HTTPS. If you configure your target group to use HTTP, be sure to restrict the traffic to your load balancer using security groups.

I think AWS never fix it. I need this feature. Skipping verification could be workaround...

@davidnewhall
Copy link

Skipping verification is recommendation of AWS Support.

That leaves your infrastructure wide open with one accidental change that exposes Grafana. If you're willing to risk that, you can setup proxy auth and use the x-amzn-oidc-identity header as your username. This doesn't get you email or name claims, but it gets your users in.

@diranged
Copy link

I definitely want to see this get finished and merged ... but I agree, it needs to do the full authentication end-to-end.

@grafanabot
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@grafanabot grafanabot added the stale Issue with no recent activity label Apr 16, 2022
@mtanda mtanda removed the stale Issue with no recent activity label Apr 16, 2022
@grafanabot
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@grafanabot grafanabot added the stale Issue with no recent activity label May 16, 2022
@mtanda mtanda removed the stale Issue with no recent activity label May 16, 2022
@grafanabot
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@grafanabot grafanabot added the stale Issue with no recent activity label Jun 16, 2022
@mtanda mtanda removed the stale Issue with no recent activity label Jun 16, 2022
@grafanabot
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@grafanabot grafanabot added the stale Issue with no recent activity label Jul 17, 2022
@grafanabot
Copy link
Contributor

This pull request has been automatically closed because it has not had activity in the last 2 weeks. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@grafanabot grafanabot closed this Jul 31, 2022
@mjpowersjr
Copy link

FWIW - AWS ALB support sure would be nice. :-)

@lewis262626
Copy link

+1 from me ALB support would be great

@backending
Copy link

Hey, was this implemented?

@davidnewhall
Copy link

This won't be implemented because it's an Enterprise feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend pr/external This PR is from external contributor stale Issue with no recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants