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.

Sign up for GitHub

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

Digest auth support #10

Merged
merged 10 commits into from
Mar 10, 2019
Merged

Digest auth support #10

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2e6b529
Digest-Auth-Support: hack the input buffer to support Digest Auth
mssfang Feb 21, 2019
792dae7
DigestSuppot: change inputBuffer size to max threshold, and now suppo…
mssfang Feb 25, 2019
2552471
Digest-Sup(PR-Updates): delete 4 dummy lines
mssfang Feb 27, 2019
68049bd
refactor: added ProxyChallengeProcessor and implementation
mssfang Mar 4, 2019
54d8820
ProxyChallengeProcessorImpl: add ProxySelector support
mssfang Mar 4, 2019
47fb627
ProxyChallengeProcessorImpl Test: add test cases
mssfang Mar 4, 2019
00ca1f3
buffer size: change back to smaller size for now
mssfang Mar 4, 2019
e67852b
refactor: seperate ProxyChallengeProcessorImpl into Basic and Digest …
mssfang Mar 7, 2019
4690853
Refactor-Test: make code with less lines
mssfang Mar 7, 2019
59cdd87
Merge branch 'master' into DigestAuthSupport
JamesBirdsall Mar 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

<groupId>com.microsoft.azure</groupId>
<artifactId>qpid-proton-j-extensions</artifactId>
<version>1.1.0</version>
<version>1.1.0-SNAPSHOT</version>

<url>https://github.com/Azure/qpid-proton-j-extensions</url>

Expand Down
136 changes: 134 additions & 2 deletions src/main/java/com/microsoft/azure/proton/transport/proxy/impl/ProxyImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,19 @@
import com.microsoft.azure.proton.transport.proxy.Proxy;
import com.microsoft.azure.proton.transport.proxy.ProxyHandler;

import java.io.UnsupportedEncodingException;
import java.net.Authenticator;
import java.net.PasswordAuthentication;

import java.nio.ByteBuffer;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.util.Base64;
import java.util.HashMap;
import java.util.Map;
import java.util.Scanner;
import java.util.concurrent.atomic.AtomicInteger;

import org.apache.qpid.proton.engine.Transport;
import org.apache.qpid.proton.engine.TransportException;
Expand All @@ -21,8 +32,10 @@
import org.apache.qpid.proton.engine.impl.TransportOutput;
import org.apache.qpid.proton.engine.impl.TransportWrapper;

import javax.xml.bind.DatatypeConverter;

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. 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.
  2. 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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

private final ByteBuffer inputBuffer;
private final ByteBuffer outputBuffer;

Expand All @@ -36,6 +49,9 @@ public class ProxyImpl implements Proxy, TransportLayer {

private ProxyHandler proxyHandler;

private final String PROXY_AUTH_DIGEST = "Proxy-Authenticate: Digest";
private final String PROXY_AUTH_BASIC = "Proxy-Authenticate: Basic";
private final AtomicInteger nonceCounter = new AtomicInteger(0);
/**
* Create proxy transport layer - which, after configuring using
* the {@link #configure(String, Map, ProxyHandler, Transport)} API
Expand Down Expand Up @@ -167,9 +183,26 @@ public void process() throws TransportException {
final ProxyHandler.ProxyResponseResult responseResult = proxyHandler
.validateProxyResponse(inputBuffer);
inputBuffer.compact();

inputBuffer.clear();
if (responseResult.getIsSuccess()) {
proxyState = ProxyState.PN_PROXY_CONNECTED;
} else if (responseResult.getError() != null &&
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

new states added

responseResult.getError().contains(PROXY_AUTH_DIGEST)) {
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()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

added ProxyChallengeProcessor and ProxyChallengeProcessorImpl classes

String line = responseScanner.nextLine();
if (line.contains(PROXY_AUTH_DIGEST)){
getChallengeQuestionHeaders(line, challengeQuestionValues);
break;
}
}
computeDigestAuthHeader(challengeQuestionValues);
} else if (responseResult.getError() != null &&
responseResult.getError().contains(PROXY_AUTH_BASIC)) {
proxyState = ProxyState.PN_PROXY_NOT_STARTED;
computeBasicAuthHeader();
} else {
tailClosed = true;
underlyingTransport.closed(
Expand Down Expand Up @@ -274,5 +307,104 @@ public void close_head() {
headClosed = true;
underlyingOutput.close_head();
}

private void getChallengeQuestionHeaders(String line, Map<String, String> challengeQuestionValues) {
String context = line.substring(PROXY_AUTH_DIGEST.length());
String[] headerValues = context.split(",");

for (String headerValue : headerValues) {
if (headerValue.contains("=")) {
String key = headerValue.substring(0, headerValue.indexOf("="));
String value = headerValue.substring(headerValue.indexOf("=") + 1);
challengeQuestionValues.put(key.trim(), value.replaceAll("\"", "").trim());
}
}
}

private void computeBasicAuthHeader(){
final PasswordAuthentication authentication = Authenticator.requestPasswordAuthentication(
"",
null,
0,
"https",
"Event Hubs client websocket proxy support",
"basic",
null,
Authenticator.RequestorType.PROXY);
if (authentication == null) return;

final String proxyUserName = authentication.getUserName();
JamesBirdsall marked this conversation as resolved.
Show resolved Hide resolved
final String proxyPassword = authentication.getPassword() != null
? new String(authentication.getPassword())
: null;
if (isNullOrEmpty(proxyUserName) || isNullOrEmpty(proxyPassword)) return;

final String usernamePasswordPair = proxyUserName + ":" + proxyPassword;
if (headers == null)
headers = new HashMap<String, String>();
headers.put(
"Proxy-Authorization",
"Basic " + Base64.getEncoder().encodeToString(usernamePasswordPair.getBytes()));
}

private void computeDigestAuthHeader(Map<String, String> challengeQuestionValues) {
String uri = host;
PasswordAuthentication passwordAuthentication = Authenticator.requestPasswordAuthentication(
"",
null,
0,
"https",
"Event Hubs client websocket proxy support",
"digest",
null,
Authenticator.RequestorType.PROXY);

String username = passwordAuthentication.getUserName();
String password = passwordAuthentication.getPassword() != null
? new String(passwordAuthentication.getPassword())
: null;

String digestValue;
try {
String nonce = challengeQuestionValues.get("nonce");
String realm = challengeQuestionValues.get("realm");
String qop = challengeQuestionValues.get("qop");

MessageDigest md5 = MessageDigest.getInstance("md5");
SecureRandom secureRandom = new SecureRandom();
String a1 = DatatypeConverter.printHexBinary(md5.digest(String.format("%s:%s:%s", username, realm, password).getBytes("UTF-8"))).toLowerCase();
String a2 = DatatypeConverter.printHexBinary(md5.digest(String.format("%s:%s", "CONNECT", uri).getBytes("UTF-8"))).toLowerCase();

byte[] cnonceBytes = new byte[16];
secureRandom.nextBytes(cnonceBytes);
String cnonce = DatatypeConverter.printHexBinary(cnonceBytes).toLowerCase();
String response;
if (qop == null || qop.isEmpty()) {
response = DatatypeConverter.printHexBinary(md5.digest(String.format("%s:%s:%s", a1, nonce, a2).getBytes("UTF-8"))).toLowerCase();
digestValue = String.format("Digest username=\"%s\",realm=\"%s\",nonce=\"%s\",uri=\"%s\",cnonce=\"%s\",response=\"%s\"",
username, realm, nonce, uri, cnonce, response);
} else {
int nc = nonceCounter.incrementAndGet();
response = DatatypeConverter.printHexBinary(md5.digest(String.format("%s:%s:%08X:%s:%s:%s", a1, nonce, nc, cnonce, qop, a2).getBytes("UTF-8"))).toLowerCase();
digestValue = String.format("Digest username=\"%s\",realm=\"%s\",nonce=\"%s\",uri=\"%s\",cnonce=\"%s\",nc=%08X,response=\"%s\",qop=\"%s\"",
username, realm, nonce, uri, cnonce, nc, response, qop);
}

if (headers == null) {
headers = new HashMap<>();
}
headers.put("Proxy-Authorization", digestValue);

} catch(NoSuchAlgorithmException ex) {
throw new RuntimeException(ex);
} catch (UnsupportedEncodingException ex) {
throw new RuntimeException(ex);
}
}


private boolean isNullOrEmpty(String string) {
return (string == null || string.isEmpty());
}
}
}
2 changes: 1 addition & 1 deletion src/test/java/com/microsoft/azure/proton/transport/proxy/impl/ProxyImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
public class ProxyImplTest {

private String hostName = "test.host.name";
private int bufferSize = 4 * 1024;
private int bufferSize = 2 * 1024 * 1024;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please consider adding unit tests for the scenario you added.

private Map<String, String> headers = new HashMap<>();
private int proxyConnectRequestLength = 132;

Expand Down