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

Support authentication response are multiple frames #53

Merged
merged 15 commits into from
Mar 14, 2022

Conversation

liukun-msft
Copy link
Contributor

@liukun-msft liukun-msft commented Feb 17, 2022

Fix #5124

Issue

When using proxy configuration, it is possible that the HTTP response does not come in a single frame, but multiple frames.
This will cause the authorization with the proxy failure with execption.

Changes

  • Add a new interface ProxyResponse and implementation ProxyResponseImpl to save multiple frames into one response.
  • Add logic to read multiple frames in ProxyImpl and update validation logic by getting infomation from ProxyResponse header.
  • Remove class ProxyResponseResult since all information are already saved in ProxyResponse.
  • Update DigestProxyChallengeProcessorImpl authentication logic since now we get types from header rather than error message.
  • Reduce buffer size PROXY_HANDSHAKE_BUFFER_SIZE from 8 * 1024 to 4 * 1024.
  • Change related UT test.

Test scenarios

  • BASIC with response over 4K
  • Digest with response over 4K
  • BASIC with response less 4K
  • Digest with response less 4K

@liukun-msft liukun-msft marked this pull request as ready for review February 18, 2022 09:09
Copy link
Member

@conniey conniey left a comment

Choose a reason for hiding this comment

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

Thanks! Looks great. :D Just a few comments.

@liukun-msft
Copy link
Contributor Author

liukun-msft commented Feb 24, 2022

Reduced buffer size PROXY_HANDSHAKE_BUFFER_SIZE from 8 * 1024 to 4 * 1024.

@liukun-msft liukun-msft requested a review from conniey February 24, 2022 09:26
@conniey conniey self-assigned this Mar 1, 2022
Copy link
Member

@conniey conniey left a comment

Choose a reason for hiding this comment

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

Thanks! It looks good to me. I pinged @JamesBirdsall to see if he could take a quick look since they also use this library.

@@ -34,79 +37,63 @@ public void testCreateProxyRequest() {
Assert.assertEquals(expectedProxyRequest, actualProxyRequest);
}

@ParameterizedTest
@ValueSource(ints = {200, 201, 202, 203, 204, 205, 206})
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we removed all of these 2xx status code test cases?

Copy link
Contributor Author

@liukun-msft liukun-msft Mar 2, 2022

Choose a reason for hiding this comment

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

We changed the validation for proxy response. Now we only assume 200 Connection Established is the successful response.

 return status.getStatusCode() == 200
                && SUPPORTED_VERSIONS.contains(status.getProtocolVersion())
                && CONNECTION_ESTABLISHED.equalsIgnoreCase(status.getReason());

Previously, we use the pattern to do this validation. So the test verify all 2xx status code.

Pattern.compile("^http/1\\.(0|1) (?<statusCode>2[0-9]{2})", Pattern.CASE_INSENSITIVE);

Do you think we need add back and consider other 2xx status as successful response?

@JamesBirdsall
Copy link
Contributor

Looks reasonable to me!

@conniey conniey merged commit 73fb95f into Azure:master Mar 14, 2022
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.

[Event Hubs] qpid-proton-j-extensions does not wait for proxy auth sometimes
3 participants