client-side gss-tsig packet

[David Disseldorp]

Hi Dimitris,

I haven't gone through your changes in details, but it does look like a
reasonable start. A few comments below...

On Thu, 20 Jul 2017 21:30:37 +0300, Dimitris Gravanis via samba-technical wrote:

> Hi,
> 
> I followed Stefan's steps below:
> 
> Basically the signature generation needs to use the original
> dns packet and:
> - remove the bytes from dns_tsig_record
> - count down the arcount field in the buffer
> - construct a blob using dns_fake_tsig_rec and append that
> - pass the constructed buffer to gensec_sign_packet()
> - rebuild the dns packet buffer with the signature (MAC) field of
>    dns_tsig_record filled with the signature from gensec_sign_packet().
> 
> I came up with what you can see in the attached files client_crypto.c 
> and libcli_crypto.h.

 40 static WERROR dns_empty_tsig(TALLOC_CTX *mem_ctx,

Does WERROR or NTSTATUS make more sense here? Which error codes are used
on the wire for this specific protocol? It normally makes sense to use them.

 41                                         struct dns_res_rec *orig_record,
 42                                         struct dns_res_rec *empty_record)
 43 {
 44         /* see /libprc/idl/dns.idl for PIDL tsig definition */
 45         empty_record->name = talloc_strdup(mem_ctx, orig_record->name);
 46         empty_record->rr_type = orig_record->rr_type;
 47         empty_record->rr_class = orig_record->rr_class;
 48         empty_record->ttl = orig_record->ttl;
 49         empty_record->length = orig_record->length;
 50 
 51         /* tsig rdata field in the new record */
 52         empty_record->rdata.tsig_record.algorithm_name = talloc_strdup(mem_ctx, NULL);
 53         empty_record->rdata.tsig_record.time_prefix = 0;
 54         empty_record->rdata.tsig_record.time = 0;
 55         empty_record->rdata.tsig_record.fudge = 0;
	...
	might as well just use memset or similar here, rather than
	zeroing each member....
 63         return WERR_OK;
 64 }


 66 /* identify tkey in record */
 67 struct dns_client_tkey *dns_find_tkey(struct dns_client_tkey_store *store,                 
 68                                       const char *name)                                    
 69 { 

This cache looks fragile, and prone to overflows / use-after-frees etc.
IMO you'd be better off starting with a plain linked-list.

> I think I've used tsocket, tevent, talloc and gensec correctly, based 
> everything on PIDL, dns_server/dns_crypto.c, as well as all pre-existing 
> work in libcli/dns and every relevant lib in Samba that I could find 
> (the online guide 
> <https://www.samba.org/samba/docs/man/Samba-Developers-Guide/internals.html#id2557664> 
> for internals helped a lot as well).
> 
> I'll be writing a test to check my code, but I'd need your feedback 
> first, in case there's something immensely wrong that has to be fixed 
> right away.

Please go proceed with your test code. Having something to play with
and compare with the protocol docs will make this a lot nicer to
review :)

Thanks for the update!

Cheers, David