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

Add methods to support client request digest authentication #1931

Merged
merged 1 commit into from Feb 24, 2018

Conversation

Projects
None yet
3 participants
@tchaloupka
Contributor

tchaloupka commented Sep 17, 2017

Added the method to generate digest authentication request header from the response digest parameters.

Also added the example to test it.
It works for the start but I would like to solve also the problem with cnonce and nc parameters.

If I understand it correctly, cnonce should be the same if the server uses the previous nonce and nc just incremented in following client requests.
For that it is needed to store the values somewhere on the client.
I think that maybe if we add Variant context storage to the HTTPClient and make it accessible from the HTTPClientRequest and HTTPClientResponse it can be used for this as HTTPClient is reused from the pool. Also it should be cleared when the connection is closed.

Is this the correct way of thinking to provide facilities to implement digest authentication for the client calls properly?

@s-ludwig

Thanks, this looks good overall. It would be nice though to reuse DigestAuthParams in the existing checkDigest to avoid duplicating the parsing code.

this(string auth) {
import std.algorithm : splitter;
assert(auth.startsWith("Digest "), "Correct Digest authentication request not provided");

This comment has been minimized.

@s-ludwig

s-ludwig Oct 3, 2017

Member

This should probably be an enforcement, because the string comes from external sources.

nc = the count of requests sent by the client (required only when some qop is requested)
entityBody = request entity body required only if qop==auth-int
*/
auto createDigestAuthHeader(U)(HTTPMethod method, U url, string username, string password, DigestAuthParams auth,

This comment has been minimized.

@s-ludwig

s-ludwig Oct 3, 2017

Member

I'd split this up into two methods:

void addDigestAuth(scope HTTPRequest req, ...);  // analogous to addBasicAuth()
void writeDigestAuthHeader(R)(ref R dst, ...); // dst is an output range that receives the contents

The latter can probably be left private for now (to keep the API small and avoid the need for deprecation processes if something ever needs to change).

Qop qop;
/// Parses WWW-Authenticate header value with the digest parameters
this(string auth) {

This comment has been minimized.

@s-ludwig

s-ludwig Oct 3, 2017

Member

A static method static DigestAuthParams parse(in char[] auth) would arguably be better here to make it clearer that actual parsing work is going on, and it's not just a field that is initialized.

@tchaloupka

This comment has been minimized.

Contributor

tchaloupka commented Dec 16, 2017

Sorry for the delay on this, I've made the required changes.
Let me know if more is needed, or if I should quash the PR before merge.

@s-ludwig

This comment has been minimized.

Member

s-ludwig commented Dec 16, 2017

Thanks again! Looks good to merge now.

@s-ludwig s-ludwig added the auto-merge label Dec 16, 2017

@tchaloupka

This comment has been minimized.

Contributor

tchaloupka commented Feb 19, 2018

Failed due to timeout of the test :(

@s-ludwig s-ludwig merged commit e259ab0 into vibe-d:master Feb 24, 2018

2 of 3 checks passed

codecov/patch 0% of diff hit (target 35.339%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment