Skip to content

Conversation

@CrimsonFez
Copy link
Contributor

This is a simple change that enables us to extract the JWT from the WebSocket protocols as defined here kubernetes/kubernetes#47740. This satisfies #499.

Let me know if any changes are needed.

Thank you!

@CrimsonFez CrimsonFez changed the title add support for bearer tokens in websocket protocols feat: add support for bearer tokens in websocket protocols Sep 30, 2024
@CrimsonFez CrimsonFez force-pushed the websocket-fix branch 2 times, most recently from bc8b63a to 117380b Compare September 30, 2024 03:56
@oliverbaehler oliverbaehler self-assigned this Oct 2, 2024
@oliverbaehler
Copy link
Collaborator

HI! You need to sign off your commits and gpg sign them as well :)

@CrimsonFez
Copy link
Contributor Author

Should now be correct

Copy link
Member

@prometherion prometherion left a comment

Choose a reason for hiding this comment

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

Maybe the suggested code is incorrect, but we cannot return an empty token, otherwise the following calls would inherit the capsule-proxy one leading to a potential privilege escalation.

@CrimsonFez
Copy link
Contributor Author

You have a good point.
However, I believe that later parts of the code check if the token string is empty and return an error stating that we cannot authenticate anonymous users.

@prometherion
Copy link
Member

@CrimsonFez you're right, touché!

Given the increased complexity it would be good if we could return an error and make it visible in the logs: we can achieve that with panic which is not ideal per se, or rather, by returning a second return such as the error.

Copy link
Member

@prometherion prometherion left a comment

Choose a reason for hiding this comment

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

LGTM, happy to get this merged, just ensure the changes comply with the linters:

make golint

@prometherion prometherion linked an issue Nov 11, 2024 that may be closed by this pull request
Copy link
Member

@prometherion prometherion left a comment

Choose a reason for hiding this comment

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

Super happy getting this merged, thanks a lot! 🚀

@prometherion
Copy link
Member

Ah, golangci-lint complains about minor style stuff: this should be the latest round, please @CrimsonFez, may I ask you to amend it?

You can test with golangci-lint by issuing make golint

@CrimsonFez
Copy link
Contributor Author

Sure thing. I thought I got it all ;(

CrimsonFez and others added 4 commits November 14, 2024 14:36
@prometherion prometherion merged commit 4ae014c into projectcapsule:main Nov 14, 2024
7 of 8 checks passed
@CrimsonFez
Copy link
Contributor Author

Thanks for cleaning that up! I don't always have a lot of time to do stuff. Very much appreciated!

@prometherion
Copy link
Member

prometherion commented Nov 14, 2024 via email

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.

add support for Websockets

3 participants