Re: [patch]: negotiate-auth

On Wed, Jun 14, 2006 at 01:49:40AM +0200, Karel Zak wrote:
> 
>  Hi guys,

Hi,

> 
>  here is a patch with negotiate-auth support based on GSSAPI. It's
>  very useful feature for intranet web sites where is possible
>  authenticate users by Kerberos. (This feature is already implemented
>  in firefox and libcurl.)
> 
>     Karel
> 
> 
> 

[snip]

> +http_negotiate_get(struct uri *uri, int *isnew, int alloc)
> +{
> +	struct negotiate *neg;
> +	
> +	foreach (neg, negotiate_list) {	
> +		if (compare_uri(neg->uri, uri, URI_HTTP_REFERRER_HOST))
> +			return neg;
> +	}
> +	if (!alloc)	
> +		return NULL;
> +	if (isnew)
> +		*isnew = 1;
> +
> +	if (!(neg = mem_calloc(1, sizeof(*neg))))
> +		return NULL;
> +		
> +	memset(neg, 0, sizeof(*neg));
> +	neg->uri = get_uri_reference(uri);	
> +	

this should be rewritten (see doc/hacking.txt)

neg = mem_calloc(1, sizeof(*neg));
if (!neg)
	return NULL;

neg->uri = get_uri_reference(uri); 

Since mem_calloc() is used, memset() is superfluous.

> +	return neg;
> +}
> +
> +static void
> +http_negotiate_save(struct negotiate *neg)
> +{
> +	add_to_list(negotiate_list, neg);
> +}
> +
> +static void 
> +http_negotiate_cleanup(struct negotiate *neg, int full)
> +{
> +	OM_uint32 minor_status;
> +
> +	if (neg->context != GSS_C_NO_CONTEXT)
> +		gss_delete_sec_context(&minor_status, &neg->context, GSS_C_NO_BUFFER);
> +
> +	if (neg->output_token.length != 0)
> +		gss_release_buffer(&minor_status, &neg->output_token);
> +
> +	if (full && neg->server_name)
> +		gss_release_name(&minor_status, &neg->server_name);
> +
> +	if (full && neg->input_token.length != 0) {
> +		/* allocated by mem_free().. so beter not use gss_release_buffer() */
> +		mem_free(neg->input_token.value);
> +		neg->input_token.length = 0;
> +	}
> +	
> +	if (full)
> +		memset(neg, 0, sizeof(*neg));
> +}

What about:

	if (full) {
		if (neg->server_name)
			gss_release_name(&minor_status, &neg->server_name);

		if (neg->input_token.length != 0) {
			/* allocated by mem_free().. so beter not use gss_release_buffer() */
			mem_free(neg->input_token.value);
			neg->input_token.length = 0;
		}
	
		memset(neg, 0, sizeof(*neg));
	}


I perhaps missed smt though.

> +
> +static int
> +http_negotiate_get_name(struct connection *conn, struct negotiate *neg)
> +{
> +	OM_uint32 major_status, minor_status;
> +	gss_buffer_desc token = GSS_C_EMPTY_BUFFER;
> +	char name[2048];
> +	const char* service;
> +	struct uri *uri = conn->proxied_uri;
> +
> +	/* GSSAPI implementation by Globus (known as GSI) requires the name to be
> +	of form "<service>/<fqdn>" instead of <service>@<fqdn> (ie. slash instead
> +	of at-sign). Also GSI servers are often identified as 'host' not 'khttp'.
> +	Change following lines if you want to use GSI */
> +
> +	/* IIS uses the <service>@<fqdn> form but uses 'http' as the service name */
> +
> +	if (neg->type == PROTOCOL_HTTP_GSSNEG)
> +		service = "KHTTP";
> +	else
> +		service = "HTTP";
> +
> +	token.length = strlen(service) + 1 + uri->hostlen + 1;
> +	if (token.length + 1 > sizeof(name))
> +		return -1;
> +
> +	snprintf(name, token.length, "%s@%*s", service, uri->hostlen, uri->host);
> +
> +	token.value = (void *) name;
> +	major_status = gss_import_name(&minor_status,
> +			 &token,
> +			 GSS_C_NT_HOSTBASED_SERVICE,
> +			 &neg->server_name);
> +
> +	return GSS_ERROR(major_status) ? -1 : 0;
> +}
> +
> +#define GSSNEG_LEN	sizeof("GSS-Negotiate")
> +#define NEG_LEN		sizeof("Negotiate")
> +
> +static int
> +http_negotiate_parse_data(unsigned char *data, int type, 
> +			gss_buffer_desc *token)
> +{
> +	int len = 0;
> +	unsigned char *end;
> +
> +	if (data==NULL || *data=='\0')
> +		return 0;
> +	
> +	data += type==PROTOCOL_HTTP_GSSNEG ? GSSNEG_LEN : NEG_LEN;
> +	
> +	while(*data && isspace((int)*data)) 
> +		data++;
> +	
> +	if (*data=='\0' || *data==ASCII_CR || *data==ASCII_LF)
> +		return 0;	/* no data */
> +	
> +	end = data;
> +	while (isalnum((int) *end) || *end=='=')
> +		end++;

Please add spaces around == operator.

> +
> +	/* Ignore line if we encountered an unexpected char. */
> +	if (*end != ASCII_CR && *end != ASCII_LF)
> +		return 0;
> +
> +	len = end - data;
> +	
> +	if (!len)
> +		return 0;
> +	
> +	token->value = (void *) base64_decode_bin(data, len, &token->length);
> +	
> +	if (!token->value)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static int
> +http_negotiate_create_context(struct negotiate *neg)
> +{
> +	OM_uint32 major_status, minor_status;
> +
> +	major_status = gss_init_sec_context(&minor_status,
> +					    GSS_C_NO_CREDENTIAL,
> +					    &neg->context,
> +					    neg->server_name,
> +					    GSS_C_NO_OID,
> +					    GSS_C_DELEG_FLAG,
> +					    0,
> +					    GSS_C_NO_CHANNEL_BINDINGS,
> +					    &neg->input_token,
> +					    NULL,
> +					    &neg->output_token,
> +					    NULL,
> +					    NULL);
> +	neg->status = major_status;
> +
> +	if (GSS_ERROR(major_status)) 
> +		return -1;
> +	if (neg->output_token.length == 0) 
> +		return -1;
> +
> +	return 0;
> +}
> +
> +/*
> + * Register new negotiate-auth request
> + *
> + * It's possible that server sends to client input token (at least
> + * libcurl supports it) in WWW-Authenticate header, but ususaly 
> + * is this input token undefined.
> + */
> +int 
> +http_negotiate_input(struct connection *conn, struct uri *uri, 
> +					int type, unsigned char *data)
> +{
> +	struct negotiate *neg;
> +	int ret = 0, isnew = 0;
> +
> +	neg = http_negotiate_get(uri, &isnew, 1);
> +	
> +	if (neg->context) {
> +		if (type != PROTOCOL_HTTP_GSSNEG) 
> +			return -1;
> +	}
> +	neg->type = type;
> +
> +	if (neg->context && neg->status == GSS_S_COMPLETE) {
> +		/* We finished succesfully our part of authentication, but server
> +		 * rejected it (since we're again here). Exit with an error since we
> +		 * can't invent anything better */
> +		http_negotiate_cleanup(neg, 1);
> +		return -1;
> +	}
> +	if (neg->server_name == NULL && http_negotiate_get_name(conn, neg) < 0)
> +		return -1;
> +	if (data && http_negotiate_parse_data(data, type, &neg->input_token)) 
> +		return -1;
> +	if ((ret=http_negotiate_create_context(neg)) == 0 && isnew)
> +		http_negotiate_save(neg);


Please write:
	ret = http_negotiate_create_context(neg);
	if (ret == 0 && isnew)
		http_negotiate_save(neg);


> +
> +	return ret;
> +}
> +
> +/*
> + * Fill output token to "Authorization: Negotiate <token>".
> + */
> +int
> +http_negotiate_output(struct uri *uri, struct string *header)
> +{
> +	struct negotiate *neg;
> +	char *encoded = NULL;
> +	int len = 0;
> +
> +	if (!(neg = http_negotiate_get(uri, NULL, 0)))
> +		return -1;

Same as above.

> +	
> +	if (neg->output_token.length==0) {

' == '

> +		if (http_negotiate_create_context(neg) < 0) {		
> +			/* full cleanup on error and ask for 
> +			   new WWW-Authenticate from server */
> +			http_negotiate_cleanup(neg, 1);
> +			return -1;
> +		}
> +	}
> +	
> +	encoded = base64_encode_bin((unsigned char *) neg->output_token.value, 
> +				neg->output_token.length, &len);
> +
> +	if (encoded==NULL || len==0)
> +	    return -1;

' == '

> +
> +	add_to_string(header, "Authorization: ");
> +	add_to_string(header, neg->type==PROTOCOL_HTTP_GSSNEG ? 
> +				"GSS-Negotiate " : "Negotiate ");
> +	add_to_string(header, encoded);
> +	add_crlf_to_string(header);
> +	
> +	http_negotiate_cleanup(neg, 0);
> +
> +	mem_free(encoded);
> +	
> +	return 0;
> +}
> +
> diff --git a/src/protocol/http/http_negotiate.h b/src/protocol/http/http_negotiate.h
> new file mode 100644
> index 0000000..cfd240f
> --- /dev/null
> +++ b/src/protocol/http/http_negotiate.h
> @@ -0,0 +1,16 @@
> +
> +#ifndef EL__PROTOCOL_HTTP_HTTP_NEGOTIATE_H
> +#define EL__PROTOCOL_HTTP_HTTP_NEGOTIATE_H
> +
> +#define PROTOCOL_HTTP_GSSNEG	1
> +#define PROTOCOL_HTTP_NEG	2
> +
> +
> +int http_negotiate_input(struct connection *conn, struct uri *uri, 
> +			int type, unsigned char *data);
> +
> +int http_negotiate_output(struct uri *uri, struct string *header);
> +
> +
> +#endif /* EL_PROTOCOL_HTTP_HTTP_NEGOTIATE_H */
> +
> diff --git a/src/util/base64.c b/src/util/base64.c
> index 8a3918e..7f4e74e 100644
> --- a/src/util/base64.c
> +++ b/src/util/base64.c
> @@ -17,14 +17,21 @@ static unsigned char base64_chars[] = "A
>  unsigned char *
>  base64_encode(register unsigned char *in)
>  {
> +	assert(in && *in);
> +	if_assert_failed return NULL;
> +	
> +	return base64_encode_bin((char *) in, strlen(in), NULL);


Why this type casting ?

> +}
> +
> +unsigned char *
> +base64_encode_bin(register unsigned char *in, int inlen, int *outlen)
> +{
>  	unsigned char *out;
>  	unsigned char *outstr;
> -	int inlen;
>  
>  	assert(in && *in);
>  	if_assert_failed return NULL;
>  
> -	inlen = strlen(in);
>  	out = outstr = mem_alloc((inlen / 3) * 4 + 4 + 1);
>  	if (!out) return NULL;
>  
> @@ -49,16 +56,29 @@ base64_encode(register unsigned char *in
>  	}
>  	*out = 0;
>  
> +	if (outlen)
> +		*outlen = out-outstr;
> +
>  	return outstr;
>  }
>  
> -/* Base64 decoding is used only with the CONFIG_FORMHIST feature, so i'll #ifdef it */
> -#ifdef CONFIG_FORMHIST
> +/* Base64 decoding is used only with the CONFIG_FORMHIST or CONFIG_GSSAPI 
> +   feature, so i'll #ifdef it */
> +#if  defined(CONFIG_FORMHIST) || defined(CONFIG_GSSAPI)
> +
> +unsigned char *
> +base64_decode(register unsigned char *in) 
> +{
> +	assert(in && *in);
> +	if_assert_failed return NULL;
> +
> +	return base64_decode_bin(in, strlen(in), NULL);
> +}
>  
>  /* base64_decode:  @in string to decode
>   *		   returns the string decoded (must be freed by the caller) */
>  unsigned char *
> -base64_decode(register unsigned char *in)
> +base64_decode_bin(register unsigned char *in, int inlen, int *outlen)
>  {
>  	static unsigned char is_base64_char[256]; /* static to force initialization at zero */
>  	static unsigned char decode[256];
> @@ -71,7 +91,7 @@ base64_decode(register unsigned char *in
>  	assert(in && *in);
>  	if_assert_failed return NULL;
>  
> -	outstr = out = mem_alloc(strlen(in) / 4 * 3 + 1);
> +	outstr = out = mem_alloc(inlen / 4 * 3 + 1);
>  	if (!outstr) return NULL;
>  
>  	if (!once) {
> @@ -123,6 +143,10 @@ base64_decode(register unsigned char *in
>  	}
>  
>  	*out = 0;
> +
> +	if (outlen)
> +		*outlen = out-outstr;
> +
>  	return outstr;
>  
>  decode_error:
> diff --git a/src/util/base64.h b/src/util/base64.h
> index cb5cd73..2bdf0e4 100644
> --- a/src/util/base64.h
> +++ b/src/util/base64.h
> @@ -4,4 +4,7 @@ #define EL__UTIL_BASE64_H
>  unsigned char *base64_encode(unsigned char *);
>  unsigned char *base64_decode(unsigned char *);
>  
> +unsigned char *base64_encode_bin(unsigned char *, int, int *);
> +unsigned char *base64_decode_bin(unsigned char *, int, int *);
> +
>  #endif
> _______________________________________________
> elinks-dev mailing list
> [email protected]
> http://linuxfromscratch.org/mailman/listinfo/elinks-dev
> 



This patch looks great, apart from the ELinks style conformity POV ;)

Have a serious look at doc/hacking.txt, fix the patch accordingly, and
resubmit it.

Regards,

-- 

Zas
_______________________________________________
elinks-dev mailing list
[email protected]
http://linuxfromscratch.org/mailman/listinfo/elinks-dev