Re: [RFC-v3 07/12] iscsi-target: Add CHAP Authentication support using libcrypto

On Thu, 17 Mar 2011, Nicholas A. Bellinger wrote:

From: Nicholas Bellinger <redacted>

Minor nitpicks below.

"??" ?

+int chap_string_to_hex(unsigned char *dst, unsigned char *src, int len)
+{
+	int i = 0, j = 0;
+
+	for (i = 0; i < len; i += 2)

Initializing 'i' to zero twice is a little needless.

+void chap_binaryhex_to_asciihex(char *dst, char *src, int src_len)
+{
+	int i;
+
+	for (i = 0; i < src_len; i++)

Do we still support compiler versions that don't understand 'for-scope'?

for (int i = 0; ...

+void chap_set_random(char *data, int length)
+{
+	long r;
+	unsigned n;
+
+	while (length > 0) {
+

Pointless newline IMHO.

+		get_random_bytes(&r, sizeof(long));
+		r = r ^ (r >> 8);
+		r = r ^ (r >> 4);
+		n = r & 0x7;
+
+		get_random_bytes(&r, sizeof(long));
+		r = r ^ (r >> 8);
+		r = r ^ (r >> 5);
+		n = (n << 3) | (r & 0x7);
+
+		get_random_bytes(&r, sizeof(long));
+		r = r ^ (r >> 8);
+		r = r ^ (r >> 5);
+		n = (n << 2) | (r & 0x3);
+
+		*data++ = n;
+		 length--;

Mixing space and tab in indentation here.

+int chap_gen_challenge(
+	struct iscsi_conn *conn,
+	int caller,
+	char *C_str,
+	unsigned int *C_len)
+{
+	unsigned char challenge_asciihex[CHAP_CHALLENGE_LENGTH * 2 + 1];
+	struct iscsi_chap *chap = (struct iscsi_chap *) conn->auth_protocol;
+
+	memset(challenge_asciihex, 0, CHAP_CHALLENGE_LENGTH * 2 + 1);
+
+	chap_set_random(chap->challenge, CHAP_CHALLENGE_LENGTH);
+	chap_binaryhex_to_asciihex(challenge_asciihex, chap->challenge,
+					CHAP_CHALLENGE_LENGTH);
+	/*
+	 * Set CHAP_C, and copy the generated challenge into C_str.
+	 */
+	*C_len += sprintf(C_str + *C_len, "CHAP_C=0x%s", challenge_asciihex);
+	*C_len += 1;
+
+	PRINT("[%s] Sending CHAP_C=0x%s\n\n", (caller) ? "server" : "client",
+			challenge_asciihex);
+	return 0;

You only ever return '0', so why couldn't this function just return 
'void'?  - sorry, didn't bother to actually check how its used :-/

+int chap_server_compute_md5(
+	struct iscsi_conn *conn,
+	struct iscsi_node_auth *auth,
+	char *NR_in_ptr,
+	char *NR_out_ptr,
+	unsigned int *NR_out_len)
+{
+	char *endptr;
+	unsigned char id, digest[MD5_SIGNATURE_SIZE];
+	unsigned char type, response[MD5_SIGNATURE_SIZE * 2 + 2];
+	unsigned char identifier[10], *challenge, *challenge_binhex;

If you changed the above line to 

	unsigned char identifier[10], *challenge;
	unsigned char *challenge_binhex = 0;

+	unsigned char client_digest[MD5_SIGNATURE_SIZE];
+	unsigned char server_digest[MD5_SIGNATURE_SIZE];
+	unsigned char chap_n[MAX_CHAP_N_SIZE], chap_r[MAX_RESPONSE_LENGTH];
+	struct iscsi_chap *chap = (struct iscsi_chap *) conn->auth_protocol;
+	struct crypto_hash *tfm;
+	struct hash_desc desc;
+	struct scatterlist sg;
+	int auth_ret = -1, ret, challenge_len;
+
+	memset(identifier, 0, 10);
+	memset(chap_n, 0, MAX_CHAP_N_SIZE);
+	memset(chap_r, 0, MAX_RESPONSE_LENGTH);
+	memset(digest, 0, MD5_SIGNATURE_SIZE);
+	memset(response, 0, MD5_SIGNATURE_SIZE * 2 + 2);
+	memset(client_digest, 0, MD5_SIGNATURE_SIZE);
+	memset(server_digest, 0, MD5_SIGNATURE_SIZE);
+
+	challenge = kzalloc(CHAP_CHALLENGE_STR_LEN, GFP_KERNEL);
+	if (!(challenge)) {
+		printk(KERN_ERR "Unable to allocate challenge buffer\n");
+		return -1;

this could become "goto out;"

+	}
+
+	challenge_binhex = kzalloc(CHAP_CHALLENGE_STR_LEN, GFP_KERNEL);
+	if (!(challenge_binhex)) {
+		printk(KERN_ERR "Unable to allocate challenge_binhex buffer\n");
+		kfree(challenge);
+		return -1;

and here the kfree() call could go away and the return be converted to 
"goto out;".
kfree(0) is a no-op.

...

+	 * One way authentication has succeeded, return now if mutual
+	 * authentication is not enabled.
+	 */
+	if (!auth->authenticate_target) {
+		kfree(challenge);
+		kfree(challenge_binhex);
+		return 0;

and here the two kfree() calls would also go away and "return 0" would be 
replaced with 

	auth_ret = 0;
	goto out;

...

+out:
+	kfree(challenge);
+	kfree(challenge_binhex);
+	return auth_ret;
+}

That would nicely consolidate *all* exits from the function in one
place.

+
+int chap_got_response(
+	struct iscsi_conn *conn,
+	struct iscsi_node_auth *auth,
+	char *NR_in_ptr,
+	char *NR_out_ptr,
+	unsigned int *NR_out_len)
+{
+	struct iscsi_chap *chap = (struct iscsi_chap *) conn->auth_protocol;
+
+	switch (chap->digest_type) {
+	case CHAP_DIGEST_MD5:
+		if (chap_server_compute_md5(conn, auth, NR_in_ptr,
+				NR_out_ptr, NR_out_len) < 0)
+			return -1;
+		break;
+	default:
+		printk(KERN_ERR "Unknown CHAP digest type %d!\n",
+				chap->digest_type);
+		return -1;
+	}
+
+	return 0;

You could kill the 'return 0' and replace the 'break' above with 'return 
0'. No difference in behaviour, but you'd save a few lines and it would be 
just as readable IMHO.


-- 
Jesper Juhl [off-list ref]            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.