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

[fix][broker] Only send AuthChallenge when client supports it #19626

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

michaeljmarshall
Copy link
Member

Motivation

While reviewing the ServerCnx, I noticed the broker and client compatibility could have a missing edge case. In this case, the concept of multi-stage authentication was introduced with version 14 of the protocol by #3677. However, we send an AuthChallenge to the client without verifying it has a sufficient protocol version to support such a challenge.

This change is unlikely to fix any real issues because the AuthChallenge was introduced a while ago. However, I think we should add this logic to further support our claim that any version of the client can connect to any version of the broker.

Another reason we might choose not to merge this PR is that it's possible #3677 introduced mutual authentication in the client and the proxy at the same time, so this case isn't possible. My main response is that the protocol is an open spec, so we should use our versioning system to ensure that there isn't even a third party client library that receives protocol messages it is unable to handle.

Modifications

  • When an old client tries to connect using an authentication provider that generates an auth challenge, send an AuthenticationError and close the connection for both broker and proxy.
  • Only send the AuthChallenge that is triggered by authentication refresh when the client knows how to handle the AuthChallenge protocol message. This is arguably covered by the FeatureFlag, but given what our protocol versioning means, I think we should cover this unlikely edge case.

Verifying this change

Includes a test for ServerCnx and I created an issue for #19624 the proxy.

Does this pull request potentially affect one of the following parts:

This enforces the existing protocol.

Documentation

  • doc-not-needed

This is part of the protocol spec.

Matching PR in forked repository

PR in forked repository: michaeljmarshall#32

@michaeljmarshall michaeljmarshall added area/broker area/proxy doc-not-needed Your PR changes do not impact docs labels Feb 24, 2023
@michaeljmarshall michaeljmarshall added this to the 3.0.0 milestone Feb 24, 2023
@michaeljmarshall michaeljmarshall self-assigned this Feb 24, 2023
@michaeljmarshall
Copy link
Member Author

I did not modify the ProxyClientCnx or the DirectProxyHandler logic because in both cases, the proxy only ever sends an AuthChallenge when the broker sends initiates one. Because we send the broker the minimum version of the proxy and the client, the proxy will not get this message unless running with an older version of the broker.

However, I think this is an edge case that this PR should cover, so I'll follow up with another commit tomorrow.

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Mar 27, 2023
@poorbarcode
Copy link
Contributor

Since we will start the RC version of 3.0.0 on 2023-04-11, I will change the label/milestone of PR who have not been merged.

  • The PR of type feature is deferred to 3.1.0
  • The PR of type fix is deferred to 3.0.1

So drag this PR to 3.0.1

@poorbarcode poorbarcode modified the milestones: 3.0.0, 3.1.0 Apr 10, 2023
@github-actions github-actions bot removed the Stale label Apr 11, 2023
@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label May 11, 2023
@Technoboy- Technoboy- modified the milestones: 3.1.0, 3.2.0 Jul 31, 2023
@Technoboy- Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023
@coderzc coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024
@lhotari lhotari modified the milestones: 4.0.0, 4.1.0 Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker area/proxy doc-not-needed Your PR changes do not impact docs Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants