Digest auth support #10

mssfang · 2019-02-25T08:30:55Z

Current proxy implementation has a limitation: we donot negotiate the Auth mechanisms with the Proxy and assume that it is going to be No-Auth or Basic-Auth. With java 1.8's recent update - Basic Auth is no longer part of the defualt settings for jre applications.

This PR adds the below 2 functionalities and brings us relevant to the current security standards:

adds proxy authentication negotiation
implements digest auth.

cc: @timtay-microsoft @apraovjr @binzywu @ShubhaVijayasarathy @tameraw @prmathur-microsoft @jasmineymlo @sjkwak @JonathanGiles @mayurid

…rt basic and digest both

JamesBirdsall · 2019-02-26T22:29:42Z

src/main/java/com/microsoft/azure/proton/transport/proxy/impl/ProxyImpl.java

+            try {
+                SecureRandom random = SecureRandom.getInstance("SHA1PRNG");
+                random.setSeed(System.currentTimeMillis());
+                byte[] nonceBytes = new byte[16];


I cannot find that nonceBytes is ever used. It looks like these four lines can be removed.

That's cnonceBytes, which is a separate array from nonceBytes. There's a similar duplication with random/secureRandom.

Yeah, thanks. Just delete these 4 lines with a new commit.

SreeramGarlapati

Thanks a lot for your contribution @mssfang. Adding digest Auth to our library makes us relevant to the current security standards. Truly appreciate your help!
Please make the necessary changes & lets ship it.

SreeramGarlapati · 2019-02-27T19:12:15Z

src/main/java/com/microsoft/azure/proton/transport/proxy/impl/ProxyImpl.java

 public class ProxyImpl implements Proxy, TransportLayer {
-    private final int proxyHandshakeBufferSize = 4 * 1024; // buffers used only for proxy-handshake
+    private final int proxyHandshakeBufferSize = 2 * 1024 * 1024; // buffers used only for proxy-handshake


Why is the buffer size updated to 2MB? This is supposed to be used only for Proxy Handshake. Are the Digest Auth headers large?

Digest Auth header is larger than 4096 bytes, in my case, it is about 4150 bytes. The reason to change to 2 * 1024 * 1024 is that I saw the proton impl class TransportImpl has defined BUFFER_RELEASE_THRESHOLD as 2 * 1024 * 1024.

Current implementation holds on to the 4k buffers - as long as the Connection is live. With this change, when customers create large no. of Connections - these 2MB buffers are going to show up in the memory profile - which is essentially unused but allocated memory.

Can you please evaluate these 2 options:

Find the maximum size of the buffer needed for Digest Auth. If this is below say 10K then we are good. Just update that constant.

If we have to go with 2MB buffers - please explore how you can clear the buffers, once the proxy negotiation is done.

In reply to: 261047300 [](ancestors = 261047300)

Thank you for the explanation. I have tried find a way to flush the input socket buffer but seems it is controlled by proton-j lib. So I change to use the 8 * 1024 in our use case now.

SreeramGarlapati · 2019-02-27T19:22:54Z

src/main/java/com/microsoft/azure/proton/transport/proxy/impl/ProxyImpl.java

                        if (responseResult.getIsSuccess()) {
                            proxyState = ProxyState.PN_PROXY_CONNECTED;
+                        } else if (responseResult.getError() != null &&


Previous implementation assumes that - we dont negotiate the AuthMechanism with the Proxy and always assumes Digest Auth. In your current design, you have a new state where you are reading proxy challenge and responding with the appropriate response. Can you indicate this by adding a new state for this?

In the case of Proxies with No Auth:

PN_PROXY_NOT_STARTED, PN_PROXY_CONNECTING, PN_PROXY_CONNECTED

In case of Proxies with Auth:

PN_PROXY_NOT_STARTED, PN_PROXY_CONNECTING, PN_PROXY_CHALLENGE, PN_PROXY_CHALLENGE_RESPONDED, PN_PROXY_CONNECTED

new states added

SreeramGarlapati · 2019-02-27T19:42:18Z

src/main/java/com/microsoft/azure/proton/transport/proxy/impl/ProxyImpl.java

+                            proxyState = ProxyState.PN_PROXY_NOT_STARTED;
+                            final Scanner responseScanner = new Scanner(responseResult.getError());
+                            final Map<String, String> challengeQuestionValues = new HashMap<String, String>();
+                            while (responseScanner.hasNextLine()) {


Think about the scope of this class as -a place holder for the state transistions and buffers. Please refactor processing the challenge headers into a different files.

I would Create an interface something like:

interface ProxyChallengeProcessor { string getHeader(); }

implement a basic auth processor and digest auth processor. Select processor based on the Challege response.

added ProxyChallengeProcessor and ProxyChallengeProcessorImpl classes

src/main/java/com/microsoft/azure/proton/transport/proxy/impl/ProxyImpl.java

SreeramGarlapati · 2019-02-27T19:48:02Z

src/test/java/com/microsoft/azure/proton/transport/proxy/impl/ProxyImplTest.java

@@ -31,7 +31,7 @@
 public class ProxyImplTest {

    private String hostName = "test.host.name";
-    private int bufferSize = 4 * 1024;
+    private int bufferSize = 2 * 1024 * 1024;


Please consider adding unit tests for the scenario you added.

JamesBirdsall · 2019-03-07T03:32:46Z

src/main/java/com/microsoft/azure/proton/transport/proxy/impl/ProxyChallengeProcessorImpl.java

+import java.util.*;
+import java.util.concurrent.atomic.AtomicInteger;
+
+public class ProxyChallengeProcessorImpl implements ProxyChallengeProcessor {


There should be multiple implementations of the ProxyChallengeProcessor interface, one for each auth scheme we support: DigestProxyChallengeProcessorImpl, BasicProxyChallengeProcessorImpl. process() should look at the challenge, select the preferred scheme, and instantiate the class which supports that scheme. This design allows us to cleanly add new schemes later.

Thank you, James. Current PR has updated with the new suggest changes. Please let me know if there is missing.

Any updates of reviewing, we are trying to get this PR approved and merged ASAP. Please let me know if there is anything I can do to make it happen. Free feel to ping me. Thank you.
@JamesBirdsall @SreeramGarlapati

It looks good to me. I'll merge it tomorrow morning unless Sreeram has something more to say. I think we're at the point where all the major pieces are in place and any tweaks can be handled as separate PRs. Thanks Shawn!

Thank you, James.

…two files with a new ProxyAuthenticator

Related to Azure/qpid-proton-j-extensions#10

* Update Apache Proton-J dependency (0.29.0 --> 0.31.0) (#407) * PartitionReceiver - add a method that provides an EventPosition which corresponds to an EventData returned last by the receiver (#408) * Support IsPartitionEmpty property for PartitionRuntimeInformation (#399) * Move setPrefetchCount API to the ReceiverOptions class from the PartitionReceiver and update the settings of Default & Max Prefetch count (#410) This pull request includes two major changes related to Prefetch API. 1) Move setPrefetchCount API to the ReceiverOptions class so that prefetch value specified by a user can be used instead of using default value when communicating to the service during link open and initializing a receiver. This change also addresses the receiver stuck issue caused by setPrefetchAPI in a race condition. 2) Change the default value and set the upper bound of the prefetch count. Note that prefetch count should be greater than or equal to maxEventCount which can be set when either a) calling receive() API or b) implementing the getMaxEventCount API of the SessionReceiverHandler interface. * Fixes several issues in the reactor related components (#411) This pull request contains the following changes. 1) Finish pending tasks when recreating the reactor and make sure pending calls scheduled on the old reactor get complete. 2) Fix the session open timeout issue which can result in NPE in proton-J engine. 3) Make session open timeout configurable and use the value of OperationTimeout. 4) Update the message of exceptions and include an entity name in the exception message. 5) API change - use ScheduledExecutorService. 6) Improve tracing. * Implement comparable on EventData (#395) * Update receive/send link creation logic and improve tracing (#414) * Prep for releasing client 2.0.0 and EPH 2.2.0 (#415) * Ensure that links are closed when transport error occurrs (#417) * ensure links are recreated on transport/connection failure * update API document for EventProcessorOptions class * add traces for link create/close case * Prep for releasing client 2.1.0 and EPH 2.3.0 (#418) * Update prefetch sendflow logic and increment version for new release (#420) * Fix args for proxy auth call to Authenticator (#421) * Prepare EPH 2.3.4 release (#423) * Prepare EPH 2.4.0 release (#423) (#424) * Handle proton:io errors with meaningful error msg (#427) * Handle proton:io errors with meaningful error msg * Use Proton-supplied message if present * Minor changes to lease scanner (#428) * Add logging if the scanner threw an exception. * Change logging level to warn when scanner shuts down for any reason. * Scanner can call EventProcessorOptions.notifyOfException, which calls user code. Change notifyOfException to defensively catch any exceptions coming out of user code. * Make EventData.SystemProperties completely public (#435) Porting testability changes from .NET Core to Java: provide full access to EventData's SystemProperties so that a complete EventData can be fabricated in tests. * Digest Support: init first connection with null headers (#431) Related to Azure/qpid-proton-j-extensions#10 * Fix lease scanner issues when Storage unreachable (#434) This fix is for issue #432. There are two parts: AzureStorageCheckpointLeaseManager performs certain Storage actions within a forEach. If those actions fail, the StorageException gets wrapped in a NoSuchElementException. Catch those and strip off the NoSuchElementException, then handle the StorageException in the existing way. The unexpected NoSuchElementExceptions were not being caught anywhere and the scanner thread was dying without rescheduling itself. Added code in PartitionMananger.scan to catch any exceptions that leak out of PartitionScanner and reschedule the scanner unless the host instance is shutting down. * message receiver - fix null pointer error and ensure that receive link is recreated upon a failure (#439) * message receiver/sender - fix null pointer error and ensure that receive/send link is recreated on a failure. * Update version numbers for release (#440) * Update prefetch count for a receiver (#441) * Fix an issue of creating multiple sessions for $management & $cbs channel for a single connection and improve logging (#443) * Fix an issue of creating multiple sessions for $management & $cbs for a connection and improve logging * Running through java files and double checking changes * Fix casing on test names * Ignore testcases that hang. * Fix NullPointerException when there is no inner exception * Move parent node to the top of the file. * Update version numbers in spotbugs-reporting * Increasing wait time until event hub scheduler is completed.

* Update Apache Proton-J dependency (0.29.0 --> 0.31.0) (#407) * PartitionReceiver - add a method that provides an EventPosition which corresponds to an EventData returned last by the receiver (#408) * Support IsPartitionEmpty property for PartitionRuntimeInformation (#399) * Move setPrefetchCount API to the ReceiverOptions class from the PartitionReceiver and update the settings of Default & Max Prefetch count (#410) This pull request includes two major changes related to Prefetch API. 1) Move setPrefetchCount API to the ReceiverOptions class so that prefetch value specified by a user can be used instead of using default value when communicating to the service during link open and initializing a receiver. This change also addresses the receiver stuck issue caused by setPrefetchAPI in a race condition. 2) Change the default value and set the upper bound of the prefetch count. Note that prefetch count should be greater than or equal to maxEventCount which can be set when either a) calling receive() API or b) implementing the getMaxEventCount API of the SessionReceiverHandler interface. * Fixes several issues in the reactor related components (#411) This pull request contains the following changes. 1) Finish pending tasks when recreating the reactor and make sure pending calls scheduled on the old reactor get complete. 2) Fix the session open timeout issue which can result in NPE in proton-J engine. 3) Make session open timeout configurable and use the value of OperationTimeout. 4) Update the message of exceptions and include an entity name in the exception message. 5) API change - use ScheduledExecutorService. 6) Improve tracing. * Implement comparable on EventData (#395) * Update receive/send link creation logic and improve tracing (#414) * Prep for releasing client 2.0.0 and EPH 2.2.0 (#415) * Ensure that links are closed when transport error occurrs (#417) * ensure links are recreated on transport/connection failure * update API document for EventProcessorOptions class * add traces for link create/close case * Prep for releasing client 2.1.0 and EPH 2.3.0 (#418) * Update prefetch sendflow logic and increment version for new release (#420) * Fix args for proxy auth call to Authenticator (#421) * Prepare EPH 2.3.4 release (#423) * Prepare EPH 2.4.0 release (#423) (#424) * Handle proton:io errors with meaningful error msg (#427) * Handle proton:io errors with meaningful error msg * Use Proton-supplied message if present * Minor changes to lease scanner (#428) * Add logging if the scanner threw an exception. * Change logging level to warn when scanner shuts down for any reason. * Scanner can call EventProcessorOptions.notifyOfException, which calls user code. Change notifyOfException to defensively catch any exceptions coming out of user code. * Make EventData.SystemProperties completely public (#435) Porting testability changes from .NET Core to Java: provide full access to EventData's SystemProperties so that a complete EventData can be fabricated in tests. * Digest Support: init first connection with null headers (#431) Related to Azure/qpid-proton-j-extensions#10 * Fix lease scanner issues when Storage unreachable (#434) This fix is for issue #432. There are two parts: AzureStorageCheckpointLeaseManager performs certain Storage actions within a forEach. If those actions fail, the StorageException gets wrapped in a NoSuchElementException. Catch those and strip off the NoSuchElementException, then handle the StorageException in the existing way. The unexpected NoSuchElementExceptions were not being caught anywhere and the scanner thread was dying without rescheduling itself. Added code in PartitionMananger.scan to catch any exceptions that leak out of PartitionScanner and reschedule the scanner unless the host instance is shutting down. * message receiver - fix null pointer error and ensure that receive link is recreated upon a failure (#439) * message receiver/sender - fix null pointer error and ensure that receive/send link is recreated on a failure. * Update version numbers for release (#440) * Update prefetch count for a receiver (#441) * Fix an issue of creating multiple sessions for $management & $cbs channel for a single connection and improve logging (#443) * Fix an issue of creating multiple sessions for $management & $cbs for a connection and improve logging * Update version numbers for new release (#444) * Update spotbugs.xml report versions

Related to Azure/qpid-proton-j-extensions#10

mssfang added 2 commits February 21, 2019 12:54

Digest-Auth-Support: hack the input buffer to support Digest Auth

2e6b529

DigestSuppot: change inputBuffer size to max threshold, and now suppo…

792dae7

…rt basic and digest both

JamesBirdsall suggested changes Feb 26, 2019

View reviewed changes

Digest-Sup(PR-Updates): delete 4 dummy lines

2552471

JamesBirdsall approved these changes Feb 27, 2019

View reviewed changes

SreeramGarlapati suggested changes Feb 27, 2019

View reviewed changes

mssfang added 4 commits March 3, 2019 16:20

refactor: added ProxyChallengeProcessor and implementation

68049bd

ProxyChallengeProcessorImpl: add ProxySelector support

54d8820

ProxyChallengeProcessorImpl Test: add test cases

47fb627

buffer size: change back to smaller size for now

00ca1f3

JamesBirdsall reviewed Mar 7, 2019

View reviewed changes

mssfang added 2 commits March 6, 2019 23:35

refactor: seperate ProxyChallengeProcessorImpl into Basic and Digest …

e67852b

…two files with a new ProxyAuthenticator

Refactor-Test: make code with less lines

4690853

SreeramGarlapati approved these changes Mar 8, 2019

View reviewed changes

Merge branch 'master' into DigestAuthSupport

59cdd87

JamesBirdsall merged commit 3c28728 into Azure:master Mar 10, 2019

mssfang mentioned this pull request Mar 11, 2019

Digest Support: init first connection with null headers Azure/azure-event-hubs-java#431

Merged

7 tasks

JamesBirdsall pushed a commit to Azure/azure-event-hubs-java that referenced this pull request Mar 11, 2019

Digest Support: init first connection with null headers (#431)

07b4072

Related to Azure/qpid-proton-j-extensions#10

Jgomez13 pushed a commit to Azure/azure-event-hubs-java that referenced this pull request May 17, 2024

Digest Support: init first connection with null headers (#431)

a2013b8

Related to Azure/qpid-proton-j-extensions#10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Digest auth support #10

Digest auth support #10

mssfang commented Feb 25, 2019 •

edited

Loading

JamesBirdsall Feb 26, 2019

JamesBirdsall Feb 27, 2019

mssfang Feb 27, 2019

SreeramGarlapati left a comment

SreeramGarlapati Feb 27, 2019

mssfang Feb 28, 2019

SreeramGarlapati Feb 28, 2019

mssfang Mar 4, 2019

SreeramGarlapati Feb 27, 2019

mssfang Mar 4, 2019

SreeramGarlapati Feb 27, 2019

mssfang Mar 4, 2019

SreeramGarlapati Feb 27, 2019

JamesBirdsall Mar 7, 2019

mssfang Mar 7, 2019 •

edited

Loading

mssfang Mar 8, 2019 •

edited

Loading

JamesBirdsall Mar 8, 2019

mssfang Mar 8, 2019

Digest auth support #10

Digest auth support #10

Conversation

mssfang commented Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SreeramGarlapati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mssfang Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

mssfang Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mssfang commented Feb 25, 2019 •

edited

Loading

mssfang Mar 7, 2019 •

edited

Loading

mssfang Mar 8, 2019 •

edited

Loading