Multiple retries with invalid password

[rburgst]

Multiple retries with invalid password

[glenharris]

Sorry, I meant that havePreviousDigestAuthorizationWithSameNonce() returns false!

[rburgst]

I am certainly interested, maybe @N4zroth can tell us what server he is using and how to replicate the stale nonce.

[glenharris]

Sorry, I am new to github, so I am sure there is a better way of sending a patch, but the issue web interface would only let me attach a .zip. I have tried to keep the same coding style, but feel free to improve the logic. Let me know what you think.

stale_nonce.diff.zip

[rburgst]

thanks a lot for the patch. In principle it looks good. Could you be so good to add/update the tests?

[rburgst]

I have tested the patch and IMHO it is not correct. It tries to read the stale parameter from the REQUEST rather than the RESPONSE.

 final String previousAuthorizationHeader = previousRequest.header("Authorization");

        if (previousAuthorizationHeader != null && previousAuthorizationHeader.startsWith("Digest")) {
            // check if the previous nonce is the same as the current nonce
            Map<String, String> previousParameters = new HashMap<String, String>();
            parseChallenge(previousAuthorizationHeader, 7, previousAuthorizationHeader.length() - 7, previousParameters);
            final String previousStale = previousParameters.get("stale");
            if (previousStale != null && previousStale.toLowerCase().equals("true")) {
                // we should have been given a new nonce to recompute with, in which case we should not fail
                // the authorization
                final String previousNonce = previousParameters.get("nonce");
                if (!nonce.equals(previousNonce)) {
                    // We have been given a new nonce, so try recomputing our Digest.
                    // Note that there is a potential infinite loop if the server keeps giving us a new nonce
                    // each time, however it would also have to give us the 'stale' flag so I think this is pretty
                    // unlikely
                    return false;
                }
            }

My original code was only trying to figure out whether the previous request had a nonce, however, you now want to check the previous client nonce (from the request) and the stale header from the server response.

FYI, I started with the following tests, currently one of them fails with your code:

    @Test
    public void testAuthenticate_withDifferentButNotStaleNonce_shouldNotRetry() throws IOException {
        // given
        Request dummyRequest = new Request.Builder()
                .url("http://www.google.com")
                .header("Authorization", "Digest username=\"user1\", realm=\"myrealm\", nonce=\"AAAAAAA\", uri=\"/\", response=\"[0-9a-f]+\", qop=auth, nc=00000001, cnonce=\"[0-9a-f]+\", algorithm=MD5")
                .get()
                .build();
        Response response = new Response.Builder()
                .request(dummyRequest)
                .protocol(Protocol.HTTP_1_1)
                .code(401)
                .addHeader("WWW-Authenticate",
                        "Digest realm=\"myrealm\", nonce=\"BBBBBB\", algorithm=MD5, qop=\"auth\"")
                .addHeader("WWW-Authenticate", "Basic realm=\"DVRNVRDVS\"")
                .build();

        // when
        final Request authenticated = authenticator.authenticate(null, response);

        // then
        assertThat(authenticated, is(nullValue()));
    }

    @Test
    public void testAuthenticate_withStaleNonce_shouldRetry() throws IOException {
        // given
        Request dummyRequest = new Request.Builder()
                .url("http://www.google.com")
                .header("Authorization", "Digest username=\"user1\", realm=\"myrealm\", nonce=\"AAAAAAA\", uri=\"/\", response=\"[0-9a-f]+\", qop=auth, nc=00000001, cnonce=\"[0-9a-f]+\", algorithm=MD5")
                .get()
                .build();
        Response response = new Response.Builder()
                .request(dummyRequest)
                .protocol(Protocol.HTTP_1_1)
                .code(401)
                .addHeader("WWW-Authenticate",
                        "Digest realm=\"myrealm\", nonce=\"BBBBBB\", stale=\"true\", algorithm=MD5, qop=\"auth\"")
                .addHeader("WWW-Authenticate", "Basic realm=\"DVRNVRDVS\"")
                .build();

        // when
        final Request authenticated = authenticator.authenticate(null, response);

        // then
        assertThat(authenticated.header("Authorization"),
                matchesPattern("Digest username=\"user1\", realm=\"myrealm\", nonce=\"BBBBBB\", uri=\"/\", response=\"[0-9a-f]+\", qop=auth, nc=00000001, cnonce=\"[0-9a-f]+\", algorithm=MD5"));
    }

[glenharris]

Sorry about that - the 'stale' code was pretty much just a guess that
passed the compile test and seemed plausible :-)

I am pretty sure you are correct - it should be looking at the response for
'Stale'. I am unable to run any of the tests because I have hacked the
project to run in a plain java (non-android) environment. Would you
consider a patch set that removed the android dependency (e.g. switching
out logging for slf4j/slf4j-android)?

Glen.

On 22 June 2016 at 18:42, rburgst [email protected] wrote:

I have tested the patch and IMHO it is not correct. It tries to read the
stale parameter from the REQUEST rather than the RESPONSE.

final String previousAuthorizationHeader = previousRequest.header("Authorization");

    if (previousAuthorizationHeader != null && previousAuthorizationHeader.startsWith("Digest")) {
        // check if the previous nonce is the same as the current nonce
        Map<String, String> previousParameters = new HashMap<String, String>();
        parseChallenge(previousAuthorizationHeader, 7, previousAuthorizationHeader.length() - 7, previousParameters);
        final String previousStale = previousParameters.get("stale");
        if (previousStale != null && previousStale.toLowerCase().equals("true")) {
            // we should have been given a new nonce to recompute with, in which case we should not fail
            // the authorization
            final String previousNonce = previousParameters.get("nonce");
            if (!nonce.equals(previousNonce)) {
                // We have been given a new nonce, so try recomputing our Digest.
                // Note that there is a potential infinite loop if the server keeps giving us a new nonce
                // each time, however it would also have to give us the 'stale' flag so I think this is pretty
                // unlikely
                return false;
            }
        }

My original code was only trying to figure out whether the previous
request had a nonce, however, you now want to check the previous client
nonce (from the request) and the stale header from the server response.

FYI, I started with the following tests, currently one of them fails with
your code:

@Test
public void testAuthenticate_withDifferentButNotStaleNonce_shouldNotRetry() throws IOException {
    // given
    Request dummyRequest = new Request.Builder()
            .url("http://www.google.com")
            .header("Authorization", "Digest username=\"user1\", realm=\"myrealm\", nonce=\"AAAAAAA\", uri=\"/\", response=\"[0-9a-f]+\", qop=auth, nc=00000001, cnonce=\"[0-9a-f]+\", algorithm=MD5")
            .get()
            .build();
    Response response = new Response.Builder()
            .request(dummyRequest)
            .protocol(Protocol.HTTP_1_1)
            .code(401)
            .addHeader("WWW-Authenticate",
                    "Digest realm=\"myrealm\", nonce=\"BBBBBB\", algorithm=MD5, qop=\"auth\"")
            .addHeader("WWW-Authenticate", "Basic realm=\"DVRNVRDVS\"")
            .build();

    // when
    final Request authenticated = authenticator.authenticate(null, response);

    // then
    assertThat(authenticated, is(nullValue()));
}

@Test
public void testAuthenticate_withStaleNonce_shouldRetry() throws IOException {
    // given
    Request dummyRequest = new Request.Builder()
            .url("http://www.google.com")
            .header("Authorization", "Digest username=\"user1\", realm=\"myrealm\", nonce=\"AAAAAAA\", uri=\"/\", response=\"[0-9a-f]+\", qop=auth, nc=00000001, cnonce=\"[0-9a-f]+\", algorithm=MD5")
            .get()
            .build();
    Response response = new Response.Builder()
            .request(dummyRequest)
            .protocol(Protocol.HTTP_1_1)
            .code(401)
            .addHeader("WWW-Authenticate",
                    "Digest realm=\"myrealm\", nonce=\"BBBBBB\", stale=\"true\", algorithm=MD5, qop=\"auth\"")
            .addHeader("WWW-Authenticate", "Basic realm=\"DVRNVRDVS\"")
            .build();

    // when
    final Request authenticated = authenticator.authenticate(null, response);

    // then
    assertThat(authenticated.header("Authorization"),
            matchesPattern("Digest username=\"user1\", realm=\"myrealm\", nonce=\"BBBBBB\", uri=\"/\", response=\"[0-9a-f]+\", qop=auth, nc=00000001, cnonce=\"[0-9a-f]+\", algorithm=MD5"));
}

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#12 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AS6VeAL_YAB3qs7lwifuZOT6KfK20JMBks5qONlLgaJpZM4IxmG5
.

[rburgst]

The next version will no longer rely on android logging. See also #13

[rburgst]

latest code doesn't rely on android logging anymore, can you give it a go?

[glenharris]

Will do, however it may take a couple of days before I can get to it.

Thanks,

Glen.

On 6 July 2016 at 18:11, rburgst [email protected] wrote:

latest code doesn't rely on android logging anymore, can you give it a go?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#12 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AS6VeGOo9RVjw0QSZwqruXUgUo5GGhSKks5qS0cHgaJpZM4IxmG5
.