-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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(cli): add version header + warning on client-server mismatch. Fixes #9212 #13635
Conversation
…ixes argoproj#9212 This adds a warning message to the CLI when it detects a mismatch between the client and server versions. There was another PR (argoproj#11909) for this implemented it by making a blocking API call to `/api/v1/version` in a `PersistentPreRun` hook. This PR takes a different approach: have the server send the version in a new header called `argo-version`, which the client will detect and extract. There's several advantages to this approach: 1. Negligible performance impact, since no additional requests are needed. 2. Warning is only shown when the command would normally send an API request. 3. Can be useful for bug triaging, since the header can be seen in `curl` output. Exposing the version information has security implictions, since it could be used by attackers to identify vulnerable Argo servers. To mitigate that, the header is not sent on 401 errors. Of course, if a user is exposing their Argo server to the internet without authentication, then an attacker could see this header, but then they've got bigger problems (and an attacker could just call `/api/v1/version`). This is implemented on the client and server side using [grpc-go interceptors](https://github.com/grpc/grpc-go/blob/master/examples/features/interceptor/README.md). On the server side, there's interceptors to set the version header in the response. On the client side, there's an interceptor to check the response for the header and stash it in a global variable (which is obviously not ideal, but I couldn't think of a better way to handle that). Testing process: 1. Manually changed the version to `v9.99`: https://github.com/argoproj/argo-workflows/blob/ce7f9bfb9b45f009b3e85fabe5e6410de23c7c5f/Makefile#L95 2. Ran `make cli && cp dist/argo argo2` 3. Ran `make start API=true` 4. Ran `ARGO_SECURE=false ARGO_TOKEN="Bearer $(kubectl get secret argo-server.service-account-token -o=jsonpath='{.data.token}' | base64 --decode)" ARGO_SERVER=localhost:2746 ./dist/argo2 list` Output: ``` No workflows found WARN[2024-09-20T18:03:26.116Z] CLI version (v9.99+303bcce.dirty) does not match server version (latest+303bcce.dirty). This can lead to unexpected behavior. ``` Signed-off-by: Mason Malone <[email protected]>
The one test failure is pretty clearly unrelated, and seems to be due to resource issues:
@agilgur5 do you know how to selectively re-run that failed test? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not necessarily opposed to this given that I mentioned using a header in #11909 (comment), but modifying gRPC a decent bit here seems suboptimal given #13542
In CI, you can't. You can re-run an individual CI job with a Speaking of, would you be interested in becoming a Member? You have enough contributions to qualify now and enough would merge by the next quarterly Membership Meeting (November) as well |
Signed-off-by: Mason Malone <[email protected]>
@agilgur5 Thanks! Yes, I'm interested in becoming a member, but I need to get sign off from my employer first. Are you willing to sponsor me? |
Yes I mentioned it, so ofc I can be one sponsor 🙂 You mentioned you were contributing in your personal capacity, so you don't have to list your employer as your affiliation, but it is usually good to cross-check with them anyway. Pretty sure Adobe would be happy to have more contributors affiliated too 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work with all the grpc-go
interceptor nuances here, I learned some things from your code 🙂
The detailed PR and tests are great too ❤️
I just have two non-blocking comments that we may or may not want to act further on
Signed-off-by: Mason Malone <[email protected]>
Test failures are unrelated and should go away when #13660 is merged |
Signed-off-by: Mason Malone <[email protected]>
Signed-off-by: Mason Malone <[email protected]>
Signed-off-by: Mason Malone <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for iterating on this and being careful of the possible security ramifications!
Fixes #9212
Motivation
This adds a warning message to the CLI when it detects a mismatch between the client and server versions.
Modifications
There was another PR (#11909) for this issue that implemented it by making a blocking API call to
/api/v1/version
in aPersistentPreRun
hook. This PR takes a different approach: have the server send the version in a new header calledargo-version
, which the client will extract. There's several advantages to this approach:curl
output:Exposing the version information has security implications, since it could be used by attackers to identify vulnerable Argo servers. To mitigate that, the header is not sent on 401 errors. Of course, if a user is exposing their Argo server to the internet without authentication, then an attacker could see this header, but then they've got bigger problems (and an attacker could just call
/api/v1/version
).This is implemented on the client and server side using grpc-go interceptors. On the server side, there's interceptors to set the version header in the response. On the client side, there's an interceptor to check the response for the header and stash it in a global variable (which is obviously not ideal, but I couldn't think of a better way to handle that).
Verification
Testing process:
v9.99
here:argo-workflows/Makefile
Line 95 in ce7f9bf
make cli && cp dist/argo argo2
git checkout Makefile && make start API=true
ARGO_SECURE=false ARGO_TOKEN="Bearer $(kubectl get secret argo-server.service-account-token -o=jsonpath='{.data.token}' | base64 --decode)" ARGO_SERVER=localhost:2746 ./dist/argo2 list
Output: