Adds digest authentication example. #4112

1995parham · 2018-01-07T16:32:10Z

No description provided.

igrr · 2018-01-08T05:14:39Z

libraries/ESP8266HTTPClient/examples/DigestAuthorization/DigestAuthorization.ino

+
+#define USE_SERIAL Serial
+
+ESP8266WiFiMulti WiFiMulti;


Do we really need to use ESP8266WiFiMulti in this file? I know that some of the existing examples do that, but it would be great if the examples could be made orthogonal.

igrr · 2018-01-08T05:14:55Z

libraries/ESP8266HTTPClient/examples/DigestAuthorization/DigestAuthorization.ino

+
+#include <ESP8266HTTPClient.h>
+
+#define USE_SERIAL Serial


Suggest using Serial directly

igrr · 2018-01-08T05:15:55Z

libraries/ESP8266HTTPClient/examples/DigestAuthorization/DigestAuthorization.ino

+ESP8266WiFiMulti WiFiMulti;
+
+String _exractParam(String& authReq, const String& param, const char delimit){
+	int _begin = authReq.indexOf(param);


Please load this sketch into Arduino IDE and use Tools > Auto Format feature to fix the indentation.

Also, it is not a common practice to prefix function and field names with an underscore, when not implementing class members, at least in Arduino.

igrr · 2018-01-08T05:17:49Z

libraries/ESP8266HTTPClient/examples/DigestAuthorization/DigestAuthorization.ino

+	}
+
+	WiFi.mode(WIFI_STA);
+	WiFiMulti.addAP("Niligo-Prism-yyr9uma", "");


You might want to replace this with something generic, e.g. "wifi-ssid", "wifi-password", and move these settings to the top of the sketch, after includes.

igrr · 2018-01-08T05:20:33Z

libraries/ESP8266HTTPClient/examples/DigestAuthorization/DigestAuthorization.ino

+
+
+		// http.begin("http://admin:[email protected]/api/state?power=1");
+		http.begin("http://192.168.100.1/api/state?power=0");


Perhaps a URL of an existing service can be used?

For example, http://httpbin.org/ has a test endpoint which accept Digest Auth (/digest-auth/:qop/:user/:passwd/:algorithm).

That's awesome thanks, I will use it. 🙌

igrr · 2018-01-08T05:25:07Z

libraries/ESP8266HTTPClient/examples/DigestAuthorization/DigestAuthorization.ino

+
+			http.begin("http://192.168.100.1/api/state?power=0");
+			USE_SERIAL.println(_nonce);
+			String Author = "Digest username=\"admin\", realm=\"Protected\", nonce=\"" + _nonce + "\", uri=\"/api/state?power=0\", algorithm=\"MD5\", qop=auth, nc=00000001, cnonce=\"gDBuFY4s\", response=\"" + _response + "\"\r\n";


Suggest wrapping this part of the code (starting from extractParam calls) into a function which users can copy into their own sketch.

username and URI are duplicated here again, along with nonces...

igrr · 2018-01-08T05:25:30Z

libraries/ESP8266HTTPClient/examples/DigestAuthorization/DigestAuthorization.ino

+			String _H2 = md5.toString();
+
+			md5.begin();
+			md5.add(_H1 + ":" + _nonce + ":" + "00000001" + ":" + "gDBuFY4s" + ":" + "auth" + ":" + _H2);


What are the 00000001 and gDBuFY4s magic values?

In digest authentication client must generate nonce count and client nonce we set them here manually but they can set by creating a random string.

igrr · 2018-01-08T05:26:24Z

libraries/ESP8266HTTPClient/examples/DigestAuthorization/DigestAuthorization.ino

+			String _H1 = md5.toString();
+
+			md5.begin();
+			md5.add(String("GET:") + String("/api/state?power=0"));


It might be worth storing the URI part in a separate variable and using it both when making HTTP call and here, instead of duplicating.

Yes I created separate variables for them

igrr · 2018-01-08T05:26:45Z

libraries/ESP8266HTTPClient/examples/DigestAuthorization/DigestAuthorization.ino

+			md5.calculate();
+			String _response = md5.toString();
+
+			http.begin("http://192.168.100.1/api/state?power=0");


Likewise, use a variable for URI to avoid duplication.

igrr · 2018-01-08T05:28:27Z

libraries/ESP8266HTTPClient/examples/DigestAuthorization/DigestAuthorization.ino

+			USE_SERIAL.println(_nonce);
+			String Author = "Digest username=\"admin\", realm=\"Protected\", nonce=\"" + _nonce + "\", uri=\"/api/state?power=0\", algorithm=\"MD5\", qop=auth, nc=00000001, cnonce=\"gDBuFY4s\", response=\"" + _response + "\"\r\n";
+			USE_SERIAL.println(Author);
+			http.addHeader("Authorization", Author);


Please use lowercase names for local variables. Also, the name author is a bit confusing, should it be authorization?

igrr · 2018-01-08T10:38:36Z

libraries/ESP8266HTTPClient/examples/DigestAuthorization/DigestAuthorization.ino

+		http.end();
+		http.begin(String(server) + String(uri));
+
+		String authorization = "Digest username=\"admin\", realm=\"" + _realm + "\", nonce=\"" + _nonce + "\", uri=\"" + uri + "\", algorithm=\"MD5\", qop=auth, nc=00000001, cnonce=\"gDBuFY4s\", response=\"" + _response + "\"";


let's move the part starting from // extracting required parameters for RFC 2069 simpler Diges to calculation of authorization into a separate function, e.g. getDigestAuth.

Please break this line, Arduino IDE window is just ~60 characters wide, most of the line will not be readable.

igrr · 2018-01-08T10:39:26Z

libraries/ESP8266HTTPClient/examples/DigestAuthorization/DigestAuthorization.ino

+		// extracting required parameters for RFC 2069 simpler Digest
+		String _realm = exractParam(authReq, "realm=\"", '"');
+		String _nonce = exractParam(authReq, "nonce=\"", '"');
+		String _opaque = exractParam(authReq, "opaque=\"", '"');


As mentioned previously, it is not a common practice in Arduino sketches to start local variable names with underscores.

Yes, I am sorry about them.

igrr · 2018-01-08T10:41:37Z

libraries/ESP8266HTTPClient/examples/DigestAuthorization/DigestAuthorization.ino

+		String _H2 = md5.toString();
+
+		md5.begin();
+		md5.add(_H1 + ":" + _nonce + ":" + "00000001" + ":" + "gDBuFY4s" + ":" + "auth" + ":" + _H2);


Still some hard-coded strings here.
Nonce gDBuFY4s should be randomly generated.
And as far as i understand the RFC, 00000001 should be incremented on each request, which is not the case here.

igrr · 2018-01-08T10:43:52Z

libraries/ESP8266HTTPClient/examples/DigestAuthorization/DigestAuthorization.ino

+const char *uri = "/digest-auth/auth/admin/admin/MD5";
+
+String exractParam(String& authReq, const String& param, const char delimit){
+	int _begin = authReq.indexOf(param);


The comment about reformatting the sketch using Arduino IDE is still valid, by the way. Arduino sketches use 2 spaces for indentation, this file uses tabs.

1995parham · 2018-01-08T16:37:34Z

@igrr What do you think about adding digest authentication into ESP8266HTTPClient?

igrr · 2018-01-09T07:14:51Z

Looks pretty good @1995parham! I think moving this feature into ESP8266HTTPClient class could be useful. It's up to you whether to do it or not, adding it in the form of an example is also okay with me.

I have only one remaining comment about the request counter field, "00000001". I think you should pass the counter as an argument into getDigestAuth function, and increment it each time the loop runs. (Perhaps the web servers also allow sending the same counter, that's just my reading of the RFC).

1995parham · 2018-01-09T07:39:14Z

@igrr Yes, I add this feature to the getDigestAuth function.

igrr · 2018-01-10T03:02:34Z

libraries/ESP8266HTTPClient/examples/DigestAuthorization/DigestAuthorization.ino

+  String cNonce = getCNonce(8);
+
+  char nc[8];
+  sprintf(nc, "%08d", counter);


If that's "%08d", the size of nc should be 9 bytes to accommodate the zero terminator.

Counter is an int, so nc should have size 12. Then, the format specifier should be %11d.
Counter should actually be an unsigned int imo, it doesn't make sense for negative values. In that case, nc should have size 11.

Ok, let's read the RFC:

The nc-value is the hexadecimal count of the number of requests (including the current request) that the client has sent with the nonce value in this request. For example, in the first request sent in response to a given nonce value, the client sends "nc=00000001".

and

nc-value = 8LHEX

and

LHEX = "0" | "1" | "2" | "3" | "4" | "5" | "6" | "7" | "8" | "9" | "a" | "b" | "c" | "d" | "e" | "f"

So it must be "%08x".

yes, you are right, it must be in hex format so I convert it into %08x.

Agreed. But what stresses that counter should be unsigned :P

@devyte I corrects that too :)

igrr · 2018-01-10T04:27:36Z

libraries/ESP8266HTTPClient/examples/DigestAuthorization/DigestAuthorization.ino

+  String nonce = exractParam(authReq, "nonce=\"", '"');
+  String cNonce = getCNonce(8);
+
+  char nc[8];


Sorry, nc is still an 8 byte buffer, so the zero terminator will be written out of array bounds. Could you please change the array size to 9, and use snprintf(nc, sizeof(nc), "%08x", counter);?

Yes sorry for these mistakes.

1995parham · 2018-01-10T07:30:37Z

@igrr Thanks a lot

Adds digest authentication example.

f48a5e6

igrr requested changes Jan 8, 2018

View reviewed changes

1995parham added 4 commits January 8, 2018 10:24

Reviews digest authentication example.

4b69085

Corrects compilation errors.

8a35dce

Corrects compilation errors.

f00654d

Uses httpbin service.

f6ae2b6

igrr requested changes Jan 8, 2018

View reviewed changes

1995parham added 5 commits January 8, 2018 19:18

Corrects variable names in digest authorization.

13ea6e1

Create getDigestAuth function.

2d0b6eb

Uses 2 space instead of tab.

b154985

Breaks long lines.

34a3e86

Adds cNonce generator.

f6e8cf7

Adds some description into header.

1d0eadf

Adds counter into getDigestAuth.

857f992

igrr reviewed Jan 10, 2018

View reviewed changes

Using hex notation for nonce counter.

ec398bf

igrr reviewed Jan 10, 2018

View reviewed changes

Corrects sprintf issues.

40904f4

igrr approved these changes Jan 10, 2018

View reviewed changes

Merge branch 'master' into master

cffefde

1995parham closed this Jan 10, 2018

1995parham reopened this Jan 10, 2018

igrr merged commit 332e059 into esp8266:master Jan 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds digest authentication example. #4112

Adds digest authentication example. #4112

1995parham commented Jan 7, 2018

igrr Jan 8, 2018

igrr Jan 8, 2018

igrr Jan 8, 2018

igrr Jan 8, 2018

igrr Jan 8, 2018

igrr Jan 8, 2018

1995parham Jan 8, 2018

igrr Jan 8, 2018

igrr Jan 8, 2018

igrr Jan 8, 2018

1995parham Jan 8, 2018

igrr Jan 8, 2018

1995parham Jan 8, 2018

igrr Jan 8, 2018

igrr Jan 8, 2018

igrr Jan 8, 2018

igrr Jan 8, 2018

igrr Jan 8, 2018

1995parham Jan 8, 2018

igrr Jan 8, 2018

igrr Jan 8, 2018

1995parham commented Jan 8, 2018 •

edited

Loading

igrr commented Jan 9, 2018

1995parham commented Jan 9, 2018

igrr Jan 10, 2018

devyte Jan 10, 2018

igrr Jan 10, 2018 •

edited

Loading

1995parham Jan 10, 2018

devyte Jan 10, 2018

1995parham Jan 10, 2018

igrr Jan 10, 2018

1995parham Jan 10, 2018

1995parham commented Jan 10, 2018



		// http.begin("http://admin:[email protected]/api/state?power=1");
		http.begin("http://192.168.100.1/api/state?power=0");

Adds digest authentication example. #4112

Adds digest authentication example. #4112

Conversation

1995parham commented Jan 7, 2018

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

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

1995parham commented Jan 8, 2018 • edited Loading

igrr commented Jan 9, 2018

1995parham commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igrr Jan 10, 2018 • edited Loading

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

1995parham commented Jan 10, 2018

1995parham commented Jan 8, 2018 •

edited

Loading

igrr Jan 10, 2018 •

edited

Loading